Allow DOM.getNodeForLocation to skip to nearest non-shadow ancestor

Allow DOM.getNodeForLocation to skip to nearest non-shadow ancestor

Added optional param, includeUserAgentShadowDOM, to allow
DOM.getNodeForLocation to skip to the nearest non-shadow ancestor.

BUG=669939
TEST=Run blink test with: python third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow.html

Review-Url: https://codereview.chromium.org/2763913002
Cr-Commit-Position: refs/heads/master@{#459261}
Committed: https://chromium.googlesource.com/chromium/src/+/d66ff70d9c4f98de29ec8bdb8e939241599106ea

[peleg, 2017-03-21 04:14:15]

The CQ bit was checked by peleg3@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 04:14:26]

Dry run: 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, 2017-03-21 04:14:27]

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

[commit-bot: I haz the power, 2017-03-21 04:14:28]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[peleg, 2017-03-21 04:22:34]

Description was changed from

==========
Allow DOM.getNodeForLocation to skip to nearest non-shadow ancestor

Added optional param, includeUserAgentShadowDOM, to allow
DOM.getNodeForLocation to skip to the nearest non-shadow ancestor.

BUG=669939
TEST=Run blink test with: python
third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow.html
==========

to

==========
Allow DOM.getNodeForLocation to skip to nearest non-shadow ancestor

Added optional param, includeUserAgentShadowDOM, to allow
DOM.getNodeForLocation to skip to the nearest non-shadow ancestor.

BUG=669939
TEST=Run blink test with: python
third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow.html
==========

[peleg, 2017-03-21 04:22:34]

peleg3@gmail.com changed reviewers:
+ alexclarke@chromium.org, dgozman@chromium.org, pfeldman@chromium.org

[pfeldman, 2017-03-22 01:11:03]

The code looks good, please look at the comments below.

Also, could you plumb Common.moduleSetting('showUAShadowDOM').get() into
DOMModel.nodeForLocation from the call sites and pass it into the protocol call?
 That is in devtools/front_end/sdk/DOMModel.js.

https://codereview.chromium.org/2763913002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow-expected.txt
(right):

https://codereview.chromium.org/2763913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow-expected.txt:17:
parentId : 5
You should mask id (use <number>) in case test is running in-process with
another test.

https://codereview.chromium.org/2763913002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow-expected.txt:23:
nodeId : 7
ditto

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

https://codereview.chromium.org/2763913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2529: { "name":
"includeUserAgentShadowDOM", "type": "boolean", "optional": true, "description":
"False to skip to the nearest non-UA shadow root ancestor (default: true)." }
You should make it false by default, it is fine if it is a breaking change at
this point.

[peleg, 2017-03-22 06:29:03]

peleg3@gmail.com changed reviewers:
- alexclarke@chromium.org, dgozman@chromium.org

[peleg, 2017-03-22 06:29:04]

Thanks for reviewing! Revised per your comments.

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

https://codereview.chromium.org/2763913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1833:
this._agent.getNodeForLocation(x, y, showUAShadowDOM, mycallback.bind(this));
Is this what you had in mind for using moduleSetting?

[pfeldman, 2017-03-22 17:32:46]

https://codereview.chromium.org/2763913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol-1.2.json (right):

https://codereview.chromium.org/2763913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol-1.2.json:2499: {
"name": "includeUserAgentShadowDOM", "type": "boolean", "optional": true,
"description": "False to skip to the nearest non-UA shadow root ancestor
(default: false)." }
drop this change in the 1.2 baseline file.

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

https://codereview.chromium.org/2763913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:1833:
this._agent.getNodeForLocation(x, y, showUAShadowDOM, mycallback.bind(this));
On 2017/03/22 06:29:03, peleg wrote:
> Is this what you had in mind for using moduleSetting?

I meant that you pass the boolean into DOMModel and fetch the setting on the
call site. showUAShadowDOM setting is defined in the elements module sdk does
not depend upon, so we should not use it here. We'll migrate to the scoped
settings at some point, but before that, we just need to be careful.

[peleg, 2017-03-23 02:37:40]

got it! submitted a new patch.

https://codereview.chromium.org/2763913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol-1.2.json (right):

https://codereview.chromium.org/2763913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol-1.2.json:2499: {
"name": "includeUserAgentShadowDOM", "type": "boolean", "optional": true,
"description": "False to skip to the nearest non-UA shadow root ancestor
(default: false)." }
On 2017/03/22 17:32:46, pfeldman wrote:
> drop this change in the 1.2 baseline file.

got it. what's the purpose of this file, btw? I couldn't find a ref to it
anywhere in the codebase

https://codereview.chromium.org/2763913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js
(right):

https://codereview.chromium.org/2763913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js:235:
Common.moduleSetting('showUAShadowDOM').get(), callback.bind(this));
this works, but is there anywhere else we need to declare this module's
dependency on `Common` now, or is it just alway available?

[pfeldman, 2017-03-23 17:26:03]

lgtm

https://codereview.chromium.org/2763913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js
(right):

https://codereview.chromium.org/2763913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/screencast/ScreencastView.js:235:
Common.moduleSetting('showUAShadowDOM').get(), callback.bind(this));
On 2017/03/23 02:37:40, peleg wrote:
> this works, but is there anywhere else we need to declare this module's
> dependency on `Common` now, or is it just alway available?

module dependencies are defined in module.json files, common is there. sorry, we
are not require style.

[peleg, 2017-03-23 20:32:14]

The CQ bit was checked by peleg3@gmail.com

[commit-bot: I haz the power, 2017-03-23 20:33:10]

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, 2017-03-23 20:33:11]

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

[commit-bot: I haz the power, 2017-03-23 20:33:13]

The author peleg3@gmail.com has not signed Google Contributor License Agreement.
Please visit https://cla.developers.google.com to sign and manage CLA.

[peleg, 2017-03-23 20:38:29]

The CQ bit was checked by peleg3@gmail.com

[commit-bot: I haz the power, 2017-03-23 20:39:10]

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, 2017-03-23 22:38:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490301509745450,
"parent_rev": "702e5de73fabd0464727ef90642e084b6a6ba298", "commit_rev":
"d66ff70d9c4f98de29ec8bdb8e939241599106ea"}

[commit-bot: I haz the power, 2017-03-23 22:39:23]

Description was changed from

==========
Allow DOM.getNodeForLocation to skip to nearest non-shadow ancestor

Added optional param, includeUserAgentShadowDOM, to allow
DOM.getNodeForLocation to skip to the nearest non-shadow ancestor.

BUG=669939
TEST=Run blink test with: python
third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow.html
==========

to

==========
Allow DOM.getNodeForLocation to skip to nearest non-shadow ancestor

Added optional param, includeUserAgentShadowDOM, to allow
DOM.getNodeForLocation to skip to the nearest non-shadow ancestor.

BUG=669939
TEST=Run blink test with: python
third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getNodeForLocation-skip-shadow.html

Review-Url: https://codereview.chromium.org/2763913002
Cr-Commit-Position: refs/heads/master@{#459261}
Committed:
https://chromium.googlesource.com/chromium/src/+/d66ff70d9c4f98de29ec8bdb8e93...
==========

[commit-bot: I haz the power, 2017-03-23 22:39:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d66ff70d9c4f98de29ec8bdb8e93...