Devtools: Switch JS execution context to match inspected node

Devtools: Switch JS execution context to match inspected node

Each time a new element is inspected, the JS context is switched to match
the inspected frame.

BUG=469618
Committed: https://crrev.com/886819ce7a81bf35682dc209094afbad37bced1f
Cr-Commit-Position: refs/heads/master@{#377509}

[samli, 2016-02-03 20:03:00]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2016-02-04 00:16:50]

On 2016/02/03 20:03:00, samli wrote:

This needs a test.

[pfeldman, 2016-02-04 00:18:53]

https://codereview.chromium.org/1668603003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/components/ExecutionContextModel.js
(right):

https://codereview.chromium.org/1668603003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/components/ExecutionContextModel.js:157:
domModel.addEventListener(WebInspector.DOMModel.Events.NodeInspected,
this._nodeInspected, this);
This code should move to the UI level in order to track manual node selection in
the elements panel.

[samli, 2016-02-05 19:43:16]

ptal

https://codereview.chromium.org/1668603003/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/components/ExecutionContextModel.js
(right):

https://codereview.chromium.org/1668603003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/components/ExecutionContextModel.js:157:
domModel.addEventListener(WebInspector.DOMModel.Events.NodeInspected,
this._nodeInspected, this);
On 2016/02/04 at 00:18:53, pfeldman wrote:
> This code should move to the UI level in order to track manual node selection
in the elements panel.

Done

[pfeldman, 2016-02-06 02:59:05]

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:392: if
(context.frameId == selectedNode.frameId() || (!selectedNode.frameId() &&
!frame.parentFrame))
- What if there is no frame?
- In most of the cases you would not need the frame, why fetching it?
- Computing frameId() twice in a loop gives MxN complexity twice - you should
compute it once before the loop.
- Every node should have frameId, did not get the parentFrame bit.

[samli, 2016-02-08 03:53:50]

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:392: if
(context.frameId == selectedNode.frameId() || (!selectedNode.frameId() &&
!frame.parentFrame))
On 2016/02/06 at 02:59:05, pfeldman wrote:
> - What if there is no frame?
> - In most of the cases you would not need the frame, why fetching it?
> - Computing frameId() twice in a loop gives MxN complexity twice - you should
compute it once before the loop.
> - Every node should have frameId, did not get the parentFrame bit.

Appears nodes in the top frame don't get a frameId at all. Hence I used the top
frame for those cases. If that's not meant to be the case, where should that get
fixed (looks like root doesn't get the frame id)?

^ is the reason I'm fetching the frame, to figure out if its the top frame.

[pfeldman, 2016-02-11 21:47:34]

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:392: if
(context.frameId == selectedNode.frameId() || (!selectedNode.frameId() &&
!frame.parentFrame))
nodes don't get frameids, only document nodes do. if the top document does not
have one - it needs to be fixed in the sdk.

[samli, 2016-02-15 00:40:13]

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/1668603003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:392: if
(context.frameId == selectedNode.frameId() || (!selectedNode.frameId() &&
!frame.parentFrame))
On 2016/02/11 at 21:47:34, pfeldman wrote:
> nodes don't get frameids, only document nodes do. if the top document does not
have one - it needs to be fixed in the sdk.

Ok done. PTAL, I'm not sure this is the best way to go about it. I suspect it
was regressed at some point with some top node not being sent across protocol
anymore.

[pfeldman, 2016-02-17 01:18:46]

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1525: if
(node->parentNode() && node->parentNode()->isDocumentNode() &&
!node->parentNode()->parentNode()) {
the last check should not be necessary.

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:392: if
(context.frameId == selectedNode.frameId())
Could we not compute this in the loop?

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:393:
WebInspector.context.setFlavor(WebInspector.ExecutionContext, context);
should we break this n^2 loop sooner?

[samli, 2016-02-18 02:14:36]

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1525: if
(node->parentNode() && node->parentNode()->isDocumentNode() &&
!node->parentNode()->parentNode()) {
On 2016/02/17 at 01:18:46, pfeldman wrote:
> the last check should not be necessary.

Done.

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js
(right):

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:392: if
(context.frameId == selectedNode.frameId())
On 2016/02/17 at 01:18:46, pfeldman wrote:
> Could we not compute this in the loop?

Yes, done, sorry.

https://codereview.chromium.org/1668603003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/elements/ElementsPanel.js:393:
WebInspector.context.setFlavor(WebInspector.ExecutionContext, context);
On 2016/02/17 at 01:18:46, pfeldman wrote:
> should we break this n^2 loop sooner?

Done.

[samli, 2016-02-21 23:52:14]

ping

[pfeldman, 2016-02-22 19:47:11]

lgtm

[samli, 2016-02-22 22:35:46]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-02-22 22:38:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668603003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668603003/60001

[commit-bot: I haz the power, 2016-02-22 23:54:09]

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

[commit-bot: I haz the power, 2016-02-22 23:54:10]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[samli, 2016-02-25 00:21:20]

The CQ bit was checked by samli@chromium.org

[samli, 2016-02-25 00:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1668603003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-02-25 00:22:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668603003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668603003/80001

[commit-bot: I haz the power, 2016-02-25 01:49:42]

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

[commit-bot: I haz the power, 2016-02-25 01:49:43]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[samli, 2016-02-25 04:36:43]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2016-02-25 04:38:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1668603003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1668603003/80001

[commit-bot: I haz the power, 2016-02-25 04:47:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-25 04:48:16]

Description was changed from

==========
Devtools: Switch JS execution context to match inspected node

Each time a new element is inspected, the JS context is switched to match
the inspected frame.

BUG=469618
==========

to

==========
Devtools: Switch JS execution context to match inspected node

Each time a new element is inspected, the JS context is switched to match
the inspected frame.

BUG=469618

Committed: https://crrev.com/886819ce7a81bf35682dc209094afbad37bced1f
Cr-Commit-Position: refs/heads/master@{#377509}
==========

[commit-bot: I haz the power, 2016-02-25 04:48:17]

Patchset 5 (id:??) landed as
https://crrev.com/886819ce7a81bf35682dc209094afbad37bced1f
Cr-Commit-Position: refs/heads/master@{#377509}