[DevTools] Track URL through the target

[DevTools] Track URL through the target

BUG=624494
R=dgozman

Committed: https://crrev.com/98a9636b7c53deeb242c48968de9134f8603f378
Cr-Commit-Position: refs/heads/master@{#408267}

[eostroukhov-old, 2016-07-26 22:17:10]

The CQ bit was checked by eostroukhov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 22:17:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov-old, 2016-07-26 22:19:34]

Please take a look.

[commit-bot: I haz the power, 2016-07-27 00:21:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 00:21:39]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-07-27 17:26:06]

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(left):

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:382:
this._inspectedPageURL = frameResource.url;
Where did this go?

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:227:
this.target().setInspectedURL(frame.url);
Let's do this at the end of the method, so that event is dispatched at the same
time as before.

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:25:
InspectedURLChanged: "InspectedURLChanged",
You stopped sending this event. Remove it? Did all clients migrate to per-target
one? Maybe remove the getters as well?

[eostroukhov-old, 2016-07-27 17:44:58]

The CQ bit was checked by eostroukhov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 17:45:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov-old, 2016-07-27 17:46:59]

Thank you for the review. I've updated the CL, please take another look.

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(left):

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:382:
this._inspectedPageURL = frameResource.url;
On 2016/07/27 17:26:06, dgozman wrote:
> Where did this go?

This is ultimately only called from _processCachedResources which sets the url
already.

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js
(right):

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:227:
this.target().setInspectedURL(frame.url);
On 2016/07/27 17:26:06, dgozman wrote:
> Let's do this at the end of the method, so that event is dispatched at the
same
> time as before.

Done.

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2186753002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:25:
InspectedURLChanged: "InspectedURLChanged",
On 2016/07/27 17:26:06, dgozman wrote:
> You stopped sending this event. Remove it? Did all clients migrate to
per-target
> one? Maybe remove the getters as well?

This event is still sent out by a setter in the target. Clients still rely on
it.

[commit-bot: I haz the power, 2016-07-27 19:22:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 19:22:28]

Dry run: This issue passed the CQ dry run.

[eostroukhov-old, 2016-07-27 20:09:40]

The CQ bit was checked by eostroukhov@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 20:10:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eostroukhov-old, 2016-07-27 20:10:46]

Updated the CL as per offline discussion. Please take another look.

[dgozman, 2016-07-27 20:24:24]

lgtm with comments

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:70:
this._inspectedURL = target.inspectedURL().removeURLFragment();
if (target === WebInspector.targetManager().mainTarget())
   ....

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:602: var
target = WebInspector.context.flavor(WebInspector.ExecutionContext).target();
Use mainTarget.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:150:
var url = event.data.inspectedURL();
if (target === WebInspector.targetManager().mainTarget())

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:204: inspectedDomain:
function()
Maybe we should inline this at call sites?

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:164:
_revealIfMainTarget: function(uiSourceCode)
Let's inline this back.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:176: {
Early return if event.data is not a main target.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:177:
this._uiSourceCodeNodes.valuesArray().forEach((node) =>
this._revealIfMainTarget(node.uiSourceCode()));
Using forEach here could be slow compared to old-style for.

[eostroukhov-old, 2016-07-27 21:13:32]

Comments addressed

[eostroukhov-old, 2016-07-27 21:37:05]

Comments addressed

[eostroukhov-old, 2016-07-27 21:39:05]

Comments addressed

[eostroukhov-old, 2016-07-27 21:39:46]

Thanks for the review. I addressed the comments.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/components/DOMBreakpointsSidebarPane.js:70:
this._inspectedURL = target.inspectedURL().removeURLFragment();
On 2016/07/27 20:24:23, dgozman wrote:
> if (target === WebInspector.targetManager().mainTarget())
>    ....

Done.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js:602: var
target = WebInspector.context.flavor(WebInspector.ExecutionContext).target();
On 2016/07/27 20:24:23, dgozman wrote:
> Use mainTarget.

Done.

I just wanted to be smart here :)

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/extensions/ExtensionServer.js:150:
var url = event.data.inspectedURL();
On 2016/07/27 20:24:23, dgozman wrote:
> if (target === WebInspector.targetManager().mainTarget())

Done.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/Target.js (right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/Target.js:204: inspectedDomain:
function()
On 2016/07/27 20:24:23, dgozman wrote:
> Maybe we should inline this at call sites?

Done.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js
(right):

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:164:
_revealIfMainTarget: function(uiSourceCode)
On 2016/07/27 20:24:23, dgozman wrote:
> Let's inline this back.

Done.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:176: {
On 2016/07/27 20:24:23, dgozman wrote:
> Early return if event.data is not a main target.

Done.

https://codereview.chromium.org/2186753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/SourcesNavigator.js:177:
this._uiSourceCodeNodes.valuesArray().forEach((node) =>
this._revealIfMainTarget(node.uiSourceCode()));
On 2016/07/27 20:24:23, dgozman wrote:
> Using forEach here could be slow compared to old-style for.

Done.

[eostroukhov-old, 2016-07-27 21:39:55]

The CQ bit was checked by eostroukhov@google.com

[eostroukhov-old, 2016-07-27 21:39:56]

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/2186753002/#ps100001
(title: "Comments addressed")

[commit-bot: I haz the power, 2016-07-27 21:40:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 23:13:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-27 23:17:00]

Description was changed from

==========
[DevTools] Track URL through the target

BUG=624494
R=dgozman
==========

to

==========
[DevTools] Track URL through the target

BUG=624494
R=dgozman

Committed: https://crrev.com/98a9636b7c53deeb242c48968de9134f8603f378
Cr-Commit-Position: refs/heads/master@{#408267}
==========

[commit-bot: I haz the power, 2016-07-27 23:17:01]

Patchset 6 (id:??) landed as
https://crrev.com/98a9636b7c53deeb242c48968de9134f8603f378
Cr-Commit-Position: refs/heads/master@{#408267}