Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(461)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2951913002: [DevTools] Support multiple sessions in Target domain (Closed)

Created:
3 years, 6 months ago by dgozman
Modified:
3 years, 5 months ago
Reviewers:
caseq
CC:
chromium-reviews, caseq+blink_chromium.org, jam, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, darin-cc_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[DevTools] Support multiple sessions in Target domain BUG=590878

Patch Set 1 #

Patch Set 2 : works #

Patch Set 3 : rebased, works now #

Patch Set 4 : simplify #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+306 lines, -239 lines) Patch
M content/browser/devtools/devtools_agent_host_impl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/devtools/devtools_agent_host_impl.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/devtools/protocol/devtools_protocol_browsertest.cc View 1 5 chunks +12 lines, -7 lines 0 comments Download
M content/browser/devtools/protocol/target_handler.h View 1 2 6 chunks +12 lines, -14 lines 1 comment Download
M content/browser/devtools/protocol/target_handler.cc View 1 2 10 chunks +106 lines, -64 lines 2 comments Download
M headless/lib/headless_devtools_client_browsertest.cc View 1 5 chunks +34 lines, -34 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/service-workers/service-worker-agents-expected.txt View 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-evaluate-in-worker-while-pause-in-page.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-pause-dedicated-worker.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-pause-dedicated-worker-loop.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-setTimeout-sourceUrl-dedicated-worker-loop.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-step-into-dedicated-worker.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/debugger-terminate-dedicated-worker-while-paused.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/debugger/suspend-setTimeout-on-pause-in-dedicated-worker.html View 1 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/resources/inspector-protocol-test.js View 1 2 6 chunks +10 lines, -15 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/worker/exception-from-worker-contains-stack.js View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/inspector-protocol/worker/worker-console.js View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.h View 3 chunks +6 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp View 8 chunks +54 lines, -39 lines 3 comments Download
M third_party/WebKit/Source/core/inspector/browser_protocol.json View 1 2 4 chunks +14 lines, -9 lines 1 comment Download
M third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js View 1 2 3 6 chunks +20 lines, -21 lines 1 comment Download

Messages

Total messages: 11 (9 generated)
dgozman
Could you please take a look?
3 years, 5 months ago (2017-06-26 22:25:45 UTC) #6
caseq
3 years, 5 months ago (2017-06-27 18:29:30 UTC) #11 
lgtm

https://codereview.chromium.org/2951913002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/target_handler.cc (right):

https://codereview.chromium.org/2951913002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/target_handler.cc:102: DevToolsAgentHost*
agent_host,
pass as scoped_refptr<> to indicate we keep it.

https://codereview.chromium.org/2951913002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/target_handler.cc:131:
handler_->frontend_->DetachedFromTarget(id_);
Most of the logic in this methods looks like it could be in TargetHandler. Can
you extract a method there?

https://codereview.chromium.org/2951913002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/target_handler.h (right):

https://codereview.chromium.org/2951913002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/target_handler.h:109: HostsMap
auto_attached_hosts_;
Does this have to be a map -- i.e. can it be a set<DevToolsAgentHost> instead?

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp (right):

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp:106: auto it =
protocol_to_session_id_.find(session_id);
This now looks confusing -- should we call the parameter protocol_id? Should we
call int identifiers differently?

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp:110:
DCHECK(proxy);
No need for a DCHECK() in a case like this -- you'll crash soon anyway.

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorWorkerAgent.cpp:206: int
session_id = ++last_session_id_;
This looks more line proxy_id to me -- the word session is a bit overloaded.

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3659: { "name":
"sessionId", "$ref": "TargetID" }
s/TargetID/SessionID/

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2951913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:460: for (var
sessionId of Array.from(this._childConnections.keys()))
drop Array.from()

Powered by Google App Engine
This is Rietveld 408576698