Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2431223003:
[DevTools]: Require explicit connection (Closed)
|
Created:
4 years, 2 months ago by eostroukhov Modified:
4 years, 1 month ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools]: Require explicit connection
Instead of immediately connecting to discovered remote targets, a
connection node is now shown. User is now required to explicitly connect
to a target. It is also possible to disconnect from such targets.
BUG=650398
Committed: https://crrev.com/8815c41f286b76846313c9b1955507c7f5709fbf
Cr-Commit-Position: refs/heads/master@{#429934}
Patch Set 1 #Patch Set 2 : Redone on top of new API. #Patch Set 3 : Fixed Node.JS connection title #
Total comments: 15
Patch Set 4 : Reworked - again #
Total comments: 35
Patch Set 5 : [DevTools]: Require explicit connection #Patch Set 6 : Now threads view shows "subtargets" and not the "targets" #Patch Set 7 : Code review comments were addressed #
Total comments: 8
Patch Set 8 : Updated & reformatted. #Patch Set 9 : Tests pass now. #
Total comments: 28
Patch Set 10 : Comments were addressed #
Total comments: 10
Patch Set 11 : Review comments were addressed. #Patch Set 12 : Addressed one last comment #
Messages
Total messages: 66 (42 generated)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eostroukhov@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
Could you please add a screenshot here or (better) to the bug? Also, I've just removed RemoteLocationManager, so you'll have to rebaseline. Functionality moved to SubTargetsManager.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
I've redid the CL on top of new API - please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed Node.JS connection title
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I update the bug with the screenshots. I've shown the functionality to UX before sending out the codereview.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:167: targetConnection && targetConnection._attached(target); Why do we need this? https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:206: targetConnection && targetConnection._detached(); style: let's follow the surrounding code and do proper if statement. https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:228: var connection = this._pendingConnections.get(targetInfo.id); How come we could have one? https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:231: connection = new WebInspector.PendingConnection(targetInfo, this.target().targetAgent()); this._agent https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:410: var promise = new Promise((resolve, reject) => { The following pattern is used heavily in inspector: var fulfill; var promise = new Promise(f => fulfill = f); ... fulfill(value); https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:234: if (!createOnly) Let's avoid this boolean at all costs. https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js (right): https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:147: var hasThreads = WebInspector.targetManager.targets(WebInspector.Target.Capability.JS).length > 1 || WebInspector.targetConnectionManager.connections().length > 0; I think this is complicated enough to justify static method ThreadSidebarPane.multipleThreadsPresent(). https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:30: WebInspector.targetConnectionManager.addEventListener(WebInspector.TargetConnectionManager.Events.ConnectionAdded, this._connectionAdded, this); Why can't we talk directly to SubTargetsManager, which already has the pending connections? https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:47: var listItem = (connection && this._connectionToListItem.get(connection)) || this._createListItem(); I'd suggest to remove connections from the collection, so that we never have both target and pending connection for it.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Reworked once more, please take a look. https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:167: targetConnection && targetConnection._attached(target); On 2016/10/26 22:14:06, dgozman wrote: > Why do we need this? For targets, that can be disconnected from, mapping to a "connection" object is needed. This way UI knows to show the "Disconnect" button - and also avoids reshuffling the list of targets when connect/disconnect happens (target node replaces the connection node or vice versa). https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:206: targetConnection && targetConnection._detached(); On 2016/10/26 22:14:06, dgozman wrote: > style: let's follow the surrounding code and do proper if statement. Done. https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:228: var connection = this._pendingConnections.get(targetInfo.id); On 2016/10/26 22:14:06, dgozman wrote: > How come we could have one? Speculatively, the code supports a case when attachedToTarget comes without prior targetCreated notification. This is mostly a vestige from the previous code that I can cleanup. https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:231: connection = new WebInspector.PendingConnection(targetInfo, this.target().targetAgent()); On 2016/10/26 22:14:06, dgozman wrote: > this._agent Done. https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:410: var promise = new Promise((resolve, reject) => { On 2016/10/26 22:14:06, dgozman wrote: > The following pattern is used heavily in inspector: > > var fulfill; > var promise = new Promise(f => fulfill = f); > ... > fulfill(value); Done (TIL - parentheses are optional for arrow functions :) ) https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:47: var listItem = (connection && this._connectionToListItem.get(connection)) || this._createListItem(); On 2016/10/26 22:14:06, dgozman wrote: > I'd suggest to remove connections from the collection, so that we never have > both target and pending connection for it. Then target will need "disconnect" and "isDisconnectable" methods, I do not like it :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:38: SubTargetConnectionCreated: Symbol("SubTargetCreated"), PendingConnectionCreated https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:225: var connection = this._pendingConnections.get(targetInfo.id); console.assert(!this._pendingConnections.has(targetInfo.id)) https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:231: return connection; You never use return value, which is good :-) https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:249: targetConnections: function() pendingConnections https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:251: return Array.from(this._pendingConnections.values()); .valuesArray() https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:383: return this._targetId; Just use ._targetId directly in this file. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:396: if (this.type !== "page" && this.type !== "iframe" && this.type !== "node") { This has changed recently, please rebase. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:454: this._connectPromise = new Promise(resolve => this._attachedCallback = resolve) If you don't follow the pattern I described, then you have to do all the work in promiseBody(resolve) function. This means removing .then here and putting attachToTarget call inside of promise body. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:469: notifyAttached: function(target) JSDoc please. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:111: WebInspector.targetManager.addModelListener(WebInspector.SubTargetsManager, WebInspector.SubTargetsManager.Events.SubTargetConnectionCreated, this._connectionAdded, this) missing semicolon https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:150: for (var target of WebInspector.targetManager.targets(WebInspector.Target.Capability.Target)) {} around body https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:158: var hasThreads = WebInspector.targetManager.targets(WebInspector.Target.Capability.JS).length > 1 || this._hasPendingConnections(); I still believe that hasTreads logic belongs to ThreadsSidebarPane static method. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:35: for (var target of WebInspector.targetManager.targets(WebInspector.Target.Capability.Target)) {} around body https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:52: for (var conn of this._connectionToListItem.keys()) { no abbreviations https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:54: continue missing semicolon https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:210: this._connectionToListItem.delete(connection) missing semicolon... https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/UIList.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/UIList.js:93: this.actionElement = null; _actionElement
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review. I'e reddid the code, please take another look. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:38: SubTargetConnectionCreated: Symbol("SubTargetCreated"), On 2016/10/28 18:31:35, dgozman wrote: > PendingConnectionCreated I removed the word "connection" - it is too ambiguous in our context. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:225: var connection = this._pendingConnections.get(targetInfo.id); On 2016/10/28 18:31:35, dgozman wrote: > console.assert(!this._pendingConnections.has(targetInfo.id)) Why not "console.assert(!connection)"? https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:231: return connection; On 2016/10/28 18:31:35, dgozman wrote: > You never use return value, which is good :-) Fixed https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:249: targetConnections: function() On 2016/10/28 18:31:35, dgozman wrote: > pendingConnections subTargets? https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:251: return Array.from(this._pendingConnections.values()); On 2016/10/28 18:31:35, dgozman wrote: > .valuesArray() Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:383: return this._targetId; On 2016/10/28 18:31:35, dgozman wrote: > Just use ._targetId directly in this file. Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:396: if (this.type !== "page" && this.type !== "iframe" && this.type !== "node") { On 2016/10/28 18:31:35, dgozman wrote: > This has changed recently, please rebase. Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:454: this._connectPromise = new Promise(resolve => this._attachedCallback = resolve) On 2016/10/28 18:31:35, dgozman wrote: > If you don't follow the pattern I described, then you have to do all the work in > promiseBody(resolve) function. This means removing .then here and putting > attachToTarget call inside of promise body. Not sure I understood correctly, but I restructured code a bit. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:469: notifyAttached: function(target) On 2016/10/28 18:31:35, dgozman wrote: > JSDoc please. Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:111: WebInspector.targetManager.addModelListener(WebInspector.SubTargetsManager, WebInspector.SubTargetsManager.Events.SubTargetConnectionCreated, this._connectionAdded, this) On 2016/10/28 18:31:35, dgozman wrote: > missing semicolon Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:150: for (var target of WebInspector.targetManager.targets(WebInspector.Target.Capability.Target)) On 2016/10/28 18:31:36, dgozman wrote: > {} around body Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:158: var hasThreads = WebInspector.targetManager.targets(WebInspector.Target.Capability.JS).length > 1 || this._hasPendingConnections(); On 2016/10/28 18:31:35, dgozman wrote: > I still believe that hasTreads logic belongs to ThreadsSidebarPane static > method. Agreed. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:35: for (var target of WebInspector.targetManager.targets(WebInspector.Target.Capability.Target)) On 2016/10/28 18:31:36, dgozman wrote: > {} around body Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:52: for (var conn of this._connectionToListItem.keys()) { On 2016/10/28 18:31:36, dgozman wrote: > no abbreviations Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:54: continue On 2016/10/28 18:31:36, dgozman wrote: > missing semicolon Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:210: this._connectionToListItem.delete(connection) On 2016/10/28 18:31:36, dgozman wrote: > missing semicolon... Done. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/UIList.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/UIList.js:93: this.actionElement = null; On 2016/10/28 18:31:36, dgozman wrote: > _actionElement Done.
https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (left): https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:73: this._agent.detachFromTarget(connection._targetId); I think it was important to send this to the agent. https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:479: isRemote: function() canConnect? https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:258: _setupConnectionItem: function(item, subtarget) If we'd have pending.isConnected(), we can merge this method with previous, and maybe more to simplify this file. https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:354: isRemote: function() Already passing false in constructor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please take a look. There is still one failing test I am tracking - but I do not anticipate many changes to the code. https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:225: var connection = this._pendingConnections.get(targetInfo.id); On 2016/11/01 20:28:14, eostroukhov wrote: > On 2016/10/28 18:31:35, dgozman wrote: > > console.assert(!this._pendingConnections.has(targetInfo.id)) > > Why not "console.assert(!connection)"? I had to rollback the change. As we discussed before, in some cases attached arrives before the create - what I do is init a subtarget in that case. Then create will be called with the subtarget already in the map. https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (left): https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:73: this._agent.detachFromTarget(connection._targetId); On 2016/11/01 22:07:39, dgozman wrote: > I think it was important to send this to the agent. It does so in the _detachedFromTarget https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:479: isRemote: function() On 2016/11/01 22:07:39, dgozman wrote: > canConnect? Done. https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:258: _setupConnectionItem: function(item, subtarget) On 2016/11/01 22:07:39, dgozman wrote: > If we'd have pending.isConnected(), we can merge this method with previous, and > maybe more to simplify this file. Done. https://codereview.chromium.org/2431223003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:354: isRemote: function() On 2016/11/01 22:07:39, dgozman wrote: > Already passing false in constructor. Done.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Tests pass (at least on my machine)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Mostly style and naming comments. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:22: this._subtargets = new Map(); _pendingTargets https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:235: subTargets() { pendingTargets https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:380: WebInspector.PendingSubTarget = class { PendingTarget https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:409: { { on the same line now. Doesn't it complain? https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:15: this._subtargetToListItem = new Map(); pendingToListItem https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:17: this._targetToSubtarget = new Map(); targetToPending https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:19: this._mainTargetConnection = null; _mainPendingTarget https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:52: static isShown() { nit: shouldBeShown is correct, while isShown is not always true :-) https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:137: if (this._mainTargetConnection) Should not ever be a case, if you null-out mainTargetConnection in targetRemoved. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:261: _setupConnectionItem(item, subtarget) { nit: no connection anymore https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:341: return this._target; 2 spaces https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:349: return this._target.name(); ditto https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/UIList.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/UIList.js:230: Promise.resolve(handler(event)).then(() => link.disabled = false); Why Promise.resolve?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the review. I addressed the comments, please take another look. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:22: this._subtargets = new Map(); On 2016/11/02 23:20:39, dgozman wrote: > _pendingTargets Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:235: subTargets() { On 2016/11/02 23:20:39, dgozman wrote: > pendingTargets Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:380: WebInspector.PendingSubTarget = class { On 2016/11/02 23:20:39, dgozman wrote: > PendingTarget Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:409: { On 2016/11/02 23:20:39, dgozman wrote: > { on the same line now. Doesn't it complain? Linter seemed happy... Fixed! https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:15: this._subtargetToListItem = new Map(); On 2016/11/02 23:20:40, dgozman wrote: > pendingToListItem Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:17: this._targetToSubtarget = new Map(); On 2016/11/02 23:20:40, dgozman wrote: > targetToPending Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:19: this._mainTargetConnection = null; On 2016/11/02 23:20:40, dgozman wrote: > _mainPendingTarget "_mainTargetPending"? Still atrocious - but I don't think we ever have main target that is not actually connected. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:52: static isShown() { On 2016/11/02 23:20:40, dgozman wrote: > nit: shouldBeShown is correct, while isShown is not always true :-) Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:137: if (this._mainTargetConnection) On 2016/11/02 23:20:40, dgozman wrote: > Should not ever be a case, if you null-out mainTargetConnection in > targetRemoved. Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:261: _setupConnectionItem(item, subtarget) { On 2016/11/02 23:20:40, dgozman wrote: > nit: no connection anymore Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:341: return this._target; On 2016/11/02 23:20:40, dgozman wrote: > 2 spaces Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:349: return this._target.name(); On 2016/11/02 23:20:40, dgozman wrote: > ditto Done. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/UIList.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/UIList.js:230: Promise.resolve(handler(event)).then(() => link.disabled = false); On 2016/11/02 23:20:40, dgozman wrote: > Why Promise.resolve? I don't want to force people who use the UIList to always return value or to return a promise. With resolve, 'then' will be called no matter what the callback returns.
Thank you for the review. I addressed the comments, please take another look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with comments https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/UIList.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/UIList.js:230: Promise.resolve(handler(event)).then(() => link.disabled = false); On 2016/11/03 17:25:52, eostroukhov wrote: > On 2016/11/02 23:20:40, dgozman wrote: > > Why Promise.resolve? > > I don't want to force people who use the UIList to always return value or to > return a promise. With resolve, 'then' will be called no matter what the > callback returns. In this case you must mark handler as returning promise, .then() on that promise and actually return promise in clients. Let's not do ambiguous apis. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:259: SubTargetAdded: Symbol('SubTargetAdded'), PendingTarget{Added,Removed,Attached,Detached} https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:418: { { on the same line everywhere please. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:425: connect() Let's call these attach/detach to be consistent. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:155: this._targetRemoved(this._mainTargetPending); this._mainTargetPending = null; https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:190: _titleForSubtarget(pendingTarget) { _titleForPending
Thank you for the review, I addressed the comments. https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/UIList.js (right): https://codereview.chromium.org/2431223003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/UIList.js:230: Promise.resolve(handler(event)).then(() => link.disabled = false); On 2016/11/03 21:21:30, dgozman wrote: > On 2016/11/03 17:25:52, eostroukhov wrote: > > On 2016/11/02 23:20:40, dgozman wrote: > > > Why Promise.resolve? > > > > I don't want to force people who use the UIList to always return value or to > > return a promise. With resolve, 'then' will be called no matter what the > > callback returns. > > In this case you must mark handler as returning promise, .then() on that promise > and actually return promise in clients. Let's not do ambiguous apis. Done. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js (right): https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:259: SubTargetAdded: Symbol('SubTargetAdded'), On 2016/11/03 21:21:30, dgozman wrote: > PendingTarget{Added,Removed,Attached,Detached} Done. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:418: { On 2016/11/03 21:21:30, dgozman wrote: > { on the same line everywhere please. Done. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/SubTargetsManager.js:425: connect() On 2016/11/03 21:21:30, dgozman wrote: > Let's call these attach/detach to be consistent. Done. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js (right): https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:155: this._targetRemoved(this._mainTargetPending); On 2016/11/03 21:21:30, dgozman wrote: > this._mainTargetPending = null; Thanks! Missed that one. https://codereview.chromium.org/2431223003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sources/ThreadsSidebarPane.js:190: _titleForSubtarget(pendingTarget) { On 2016/11/03 21:21:30, dgozman wrote: > _titleForPending Done.
The CQ bit was checked by eostroukhov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2431223003/#ps220001 (title: "Addressed one last comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by eostroukhov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eostroukhov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools]: Require explicit connection Instead of immediately connecting to discovered remote targets, a connection node is now shown. User is now required to explicitly connect to a target. It is also possible to disconnect from such targets. BUG=650398 ========== to ========== [DevTools]: Require explicit connection Instead of immediately connecting to discovered remote targets, a connection node is now shown. User is now required to explicitly connect to a target. It is also possible to disconnect from such targets. BUG=650398 Committed: https://crrev.com/8815c41f286b76846313c9b1955507c7f5709fbf Cr-Commit-Position: refs/heads/master@{#429934} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/8815c41f286b76846313c9b1955507c7f5709fbf Cr-Commit-Position: refs/heads/master@{#429934} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
