Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2373023002:
Make DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries (Closed)
|
Created:
4 years, 2 months ago by alex clarke (OOO till 29th) Modified:
4 years, 2 months 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, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries
This means you can easily capture all the DOM nodes on a page with iframes with a single command,
previously multiple calls to DOM.getChildNodes were required.
BUG=546953
Committed: https://crrev.com/86b48bfeabfd2b8876efcd18f2f018c5d9a3e53a
Cr-Commit-Position: refs/heads/master@{#425063}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix indent #Patch Set 3 : Added a requestFrames parameter to fix the broken test #
Total comments: 2
Patch Set 4 : Export it under contentDocument #Patch Set 5 : Per IM conversation, let DOM.getDocument optionally return the sub tree as well. #
Total comments: 4
Patch Set 6 : Plumbed it instead #Patch Set 7 : Fix layout test #
Total comments: 4
Patch Set 8 : Nits #Messages
Total messages: 52 (34 generated)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
alexclarke@chromium.org changed reviewers: + pfeldman@chromium.org, skyostil@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nice, lgtm! https://codereview.chromium.org/2373023002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-requestChildNodes.html (right): https://codereview.chromium.org/2373023002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-requestChildNodes.html:13: if (messageObject.hasOwnProperty("error")) Is this block indented right?
https://codereview.chromium.org/2373023002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-requestChildNodes.html (right): https://codereview.chromium.org/2373023002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-requestChildNodes.html:13: if (messageObject.hasOwnProperty("error")) On 2016/09/27 13:50:58, Sami wrote: > Is this block indented right? Done.
The CQ bit was checked by alexclarke@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: 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 alexclarke@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...
Description was changed from ========== Make DOM.getChildNodes with a depth of -1 pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ========== to ========== Add a param to make DOM.getChildNode pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ==========
https://codereview.chromium.org/2373023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1622: children->addItem(buildObjectForNode(iframeDocument, depth, nodesMap)); Those are not the children, you should report iframe's document under contentDocument instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2373023002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1622: children->addItem(buildObjectForNode(iframeDocument, depth, nodesMap)); On 2016/09/27 18:37:41, pfeldman wrote: > Those are not the children, you should report iframe's document under > contentDocument instead. Done.
The CQ bit was checked by alexclarke@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.
ping :)
Pavel: Is this the right change or would adding optional depth & traverseFrames params to getDocument be better? That way we could get everything in one go.
Description was changed from ========== Add a param to make DOM.getChildNode pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ========== to ========== Make DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes with a single command, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ==========
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL
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.
alexclarke@chromium.org changed reviewers: + dgozman@chromium.org
Looks like Pavel is busy, Dmitry would you mind taking a look?
https://codereview.chromium.org/2373023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:524: m_traverseFrames = traverseFrames.fromMaybe(false); You should plumb it, it is sessional, not a property of the agent. https://codereview.chromium.org/2373023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1625: buildObjectForNode(doc, m_traverseFrames ? -1 : 0, nodesMap)); You should still respect the depth here.
The CQ bit was checked by alexclarke@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...
PTAL https://codereview.chromium.org/2373023002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:524: m_traverseFrames = traverseFrames.fromMaybe(false); On 2016/10/07 19:35:12, pfeldman wrote: > You should plumb it, it is sessional, not a property of the agent. Done. https://codereview.chromium.org/2373023002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1625: buildObjectForNode(doc, m_traverseFrames ? -1 : 0, nodesMap)); On 2016/10/07 19:35:12, pfeldman wrote: > You should still respect the depth here. Done.
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 alexclarke@chromium.org to run a CQ dry run
ping :)
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.
lgtm % comment https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-request-child-nodes-traverse-frames.html (right): https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-request-child-nodes-traverse-frames.html:50: messageObject.params.nodes[0].children[0].children[0].children[0].frameId = "???"; Could you replace frameIds recursively in the response and just dump it to keep the test sane? https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:713: // Cached results may not be valid if |traverseFrames| is true. childrenRequested is about children only, frames hierarchy is orthogonal. You requested children here, the bit gets set.
https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-request-child-nodes-traverse-frames.html (right): https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-request-child-nodes-traverse-frames.html:50: messageObject.params.nodes[0].children[0].children[0].children[0].frameId = "???"; On 2016/10/12 18:45:26, pfeldman wrote: > Could you replace frameIds recursively in the response and just dump it to keep > the test sane? Done. https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:713: // Cached results may not be valid if |traverseFrames| is true. On 2016/10/12 18:45:26, pfeldman wrote: > childrenRequested is about children only, frames hierarchy is orthogonal. You > requested children here, the bit gets set. OK we can remove this. You need to be careful when calling DOM.getDocument and DOM.requestChildNodes because the traverseFrames flag must match or you won't necessarily get the nodes you expected. Thinking about it - it's probably OK you need the traverseFrames flag to match.
https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-request-child-nodes-traverse-frames.html (right): https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-request-child-nodes-traverse-frames.html:50: messageObject.params.nodes[0].children[0].children[0].children[0].frameId = "???"; On 2016/10/12 18:45:26, pfeldman wrote: > Could you replace frameIds recursively in the response and just dump it to keep > the test sane? Done. https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2373023002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:713: // Cached results may not be valid if |traverseFrames| is true. On 2016/10/12 18:45:26, pfeldman wrote: > childrenRequested is about children only, frames hierarchy is orthogonal. You > requested children here, the bit gets set. OK we can remove this. You need to be careful when calling DOM.getDocument and DOM.requestChildNodes because the traverseFrames flag must match or you won't necessarily get the nodes you expected. Thinking about it - it's probably OK you need the traverseFrames flag to match.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2373023002/#ps140001 (title: "Nits")
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.
Description was changed from ========== Make DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes with a single command, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ========== to ========== Make DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes with a single command, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Make DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes with a single command, previously multiple calls to DOM.getChildNodes were required. BUG=546953 ========== to ========== Make DOM.getChildNodes & DOM.getDocument optionally pierce iframe boundaries This means you can easily capture all the DOM nodes on a page with iframes with a single command, previously multiple calls to DOM.getChildNodes were required. BUG=546953 Committed: https://crrev.com/86b48bfeabfd2b8876efcd18f2f018c5d9a3e53a Cr-Commit-Position: refs/heads/master@{#425063} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/86b48bfeabfd2b8876efcd18f2f018c5d9a3e53a Cr-Commit-Position: refs/heads/master@{#425063} |
