|
|
Created:
3 years, 11 months ago by alex clarke (OOO till 29th) Modified:
3 years, 10 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. |
DescriptionAdds DOM.getFlatDocument which returns an array of nodes
We need this because some web pages have very deep dom trees which
causes the JSON parser to CHECK (there's currently a limit of 200
nodes).
BUG=546953
Review-Url: https://codereview.chromium.org/2633343003
Cr-Commit-Position: refs/heads/master@{#447844}
Committed: https://chromium.googlesource.com/chromium/src/+/ea635266172221937f38f16417c0b622a5bdef97
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressed some of the revire comments (just want to see if the test works cross platform now). #Patch Set 3 : Try to share code #
Total comments: 2
Patch Set 4 : Removed visitor #
Total comments: 2
Patch Set 5 : new-style #Patch Set 6 : Fix layout test #
Total comments: 10
Patch Set 7 : Changes for Pavel #Patch Set 8 : Fix headless browser test #Messages
Total messages: 53 (33 generated)
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
alexclarke@chromium.org changed reviewers: + 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...
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_...)
https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getFlatDocument-expected.txt (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getFlatDocument-expected.txt:414: "documentURL": "file:///usr/local/google/code/clankium/src/third_party/WebKit/LayoutTests/inspector-protocol/dom/resources/simple-iframe.html", Do we need to strip these out? https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:512: int sanitizedDepth = depth.fromMaybe(2); The comment says the default is 1. Should this be the given depth + 1 or something? https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1724: int id = bind(node, nodesMap); Are we missing a termination condition on |depth| or did I misread?
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1728: switch (node->getNodeType()) { Can we extract some (all) code here to be reused with buildObjectForNode? https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/browser_protocol.json:2148: "name": "getFlatDocument", getFlattenedDocument
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_...)
PTAL https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getFlatDocument-expected.txt (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-getFlatDocument-expected.txt:414: "documentURL": "file:///usr/local/google/code/clankium/src/third_party/WebKit/LayoutTests/inspector-protocol/dom/resources/simple-iframe.html", On 2017/01/17 17:57:43, Sami wrote: > Do we need to strip these out? Done. https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:512: int sanitizedDepth = depth.fromMaybe(2); On 2017/01/17 17:57:43, Sami wrote: > The comment says the default is 1. Should this be the given depth + 1 or > something? Just copying the above - I assume that's right? https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1724: int id = bind(node, nodesMap); On 2017/01/17 17:57:43, Sami wrote: > Are we missing a termination condition on |depth| or did I misread? Acknowledged. https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1728: switch (node->getNodeType()) { On 2017/01/17 18:34:58, pfeldman wrote: > Can we extract some (all) code here to be reused with buildObjectForNode? Done. https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/browser_protocol.json:2148: "name": "getFlatDocument", On 2017/01/17 18:34:58, pfeldman wrote: > getFlattenedDocument 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...
Thanks! Non-owner lgtm. https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:512: int sanitizedDepth = depth.fromMaybe(2); On 2017/01/18 16:31:08, alex clarke wrote: > On 2017/01/17 17:57:43, Sami wrote: > > The comment says the default is 1. Should this be the given depth + 1 or > > something? > > Just copying the above - I assume that's right? Ah, I see. A little surprising to me at least :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2633343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1616: std::unique_ptr<protocol::DOM::Node> InspectorDOMAgent::buildObjectForNode( Could we simplify things via adding an optional Vector<std::unique_ptr<protocol::DOM::Node>>* flattenResult = nullptr parameter here? When we run buildObjectForNode with that array as a parameter, it would be storing results in a flat array instead of value->setChildren-ing it. That was we simply affect the transport without introducing new visitors / abstractions.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2633343003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1616: std::unique_ptr<protocol::DOM::Node> InspectorDOMAgent::buildObjectForNode( On 2017/01/18 22:48:20, pfeldman wrote: > Could we simplify things via adding an optional > > Vector<std::unique_ptr<protocol::DOM::Node>>* flattenResult = nullptr > > parameter here? > > When we run buildObjectForNode with that array as a parameter, it would be > storing results in a flat array instead of value->setChildren-ing it. That was > we simply affect the transport without introducing new visitors / abstractions. Done.
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.
https://codereview.chromium.org/2633343003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1815: children->addItem(minimalCopy(flattenResult->get(nextIndex))); i don't follow the minimalCopy bit. I was hoping that we could deliver results either as a hierarchy or as a flat list of nodes with parent ids (similarly to the setChildNodes notification), but not both.
https://codereview.chromium.org/2633343003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1815: children->addItem(minimalCopy(flattenResult->get(nextIndex))); On 2017/01/19 21:25:59, pfeldman wrote: > i don't follow the minimalCopy bit. I was hoping that we could deliver results > either as a hierarchy or as a flat list of nodes with parent ids (similarly to > the setChildNodes notification), but not both. The reason I added this is to disambiguate the various types of sub node: children, iframes, templates etc... If there's another way you'd prefer I'm happy to do that. E.g. we could do something like this: { "id": "ParentRelationship", "type": "string", "enum": [ "child", "contentDocument", "shadowRoot", "templateContent", "pseudoElement", "importedDocument", }, { "id": "FlatNode", "type": "object", "properties": [ { "name": "node", "$ref": "Node"}, { "parentNodeId": "node", "type": "integer"}, { "parentRelationship": "node", "$ref": "ParentRelationship"}, }, { "name": "getFlattenedDocument", "parameters": [ { "name": "depth", "type": "integer", "optional": true, "description": "The maximum depth at which children should be retrieved, defaults to 1. Use -1 for the entire subtree or provide an integer larger than 0.", "experimental": true }, { "name": "pierce", "type": "boolean", "optional": true, "description": "Whether or not iframes and shadow roots should be traversed when returning the subtree (default is false).", "experimental": true } ], "returns": [ { "name": "nodes", "type": "array", "items": { "$ref": "FlatNode" }, "description": "Resulting node." } ], "description": "Returns the root DOM node (and optionally the subtree) to the caller." },
> The reason I added this is to disambiguate the various types of sub node: > children, iframes, templates etc... > > If there's another way you'd prefer I'm happy to do that. Ah. I actually don't think it is necessary. We could still report contentDocument, shadowRoot, templatecontent, pseudoElement and importentDocument inline as a part of the node, as node properties. We would only flatten children and that should simplify the code a great deal.
On 2017/01/20 20:27:16, pfeldman wrote: > > The reason I added this is to disambiguate the various types of sub node: > > children, iframes, templates etc... > > > > If there's another way you'd prefer I'm happy to do that. > > Ah. I actually don't think it is necessary. We could still report > contentDocument, shadowRoot, templatecontent, pseudoElement and > importentDocument inline as a part of the node, as node properties. We would > only flatten children and that should simplify the code a great deal. Whoops I meant to reply to this. How do we know the parent ID for each child?
> Whoops I meant to reply to this. How do we know the parent ID for each child? I might be missing some edge case, but if we are only flattening primary DOM hierarchy, wouldn't it be the immediate node parent that is always non-null?
On 2017/01/26 18:15:13, pfeldman wrote: > > Whoops I meant to reply to this. How do we know the parent ID for each child? > > I might be missing some edge case, but if we are only flattening primary DOM > hierarchy, wouldn't it be the immediate node parent that is always non-null? Would returning an array of these do what you want? { "id": "FlatNode", "type": "object", "properties": [ { "name": "node", "$ref": "Node"}, { "parentNodeId": "node", "type": "integer"}, },
> { > "id": "FlatNode", > "type": "object", > "properties": [ > { "name": "node", "$ref": "Node"}, > { "parentNodeId": "node", "type": "integer"}, > }, Yes, that was what I had in mind. We could actually be a tiny bit more brave and add parentId into the Node itself as an optional parameter. Leaving it up to you.
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 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.
On 2017/01/30 18:46:34, pfeldman wrote: > > { > > "id": "FlatNode", > > "type": "object", > > "properties": [ > > { "name": "node", "$ref": "Node"}, > > { "parentNodeId": "node", "type": "integer"}, > > }, > > Yes, that was what I had in mind. We could actually be a tiny bit more brave and > add parentId into the Node itself as an optional parameter. Leaving it up to > you. Done, PTAL :)
lgtm % comments. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:502: if (!enabled()) You can return error if not enabled. We auto-enable in getDocument for backwards compat only, while this method is new, so you don't need to worry about it. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:510: int sanitizedDepth = depth.fromMaybe(2); This could be -1 by default, that is how we expect everyone (you) to consume it anyways. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1749: buildObjectForNode(firstChild, 0, pierce, nodesMap, flattenResult); Create childNode unconditionally, set its parent id unconditionally... https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1751: flattenResult->addItem(std::move(childNode)); ... then either populate flatten or add into collection. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1767: std::unique_ptr<protocol::DOM::Node> childNode = ditto
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 alexclarke@chromium.org
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/2633343003/#ps120001 (title: "Changes for Pavel")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right): https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:502: if (!enabled()) On 2017/02/02 01:24:19, pfeldman wrote: > You can return error if not enabled. We auto-enable in getDocument for backwards > compat only, while this method is new, so you don't need to worry about it. Done. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:510: int sanitizedDepth = depth.fromMaybe(2); On 2017/02/02 01:24:19, pfeldman wrote: > This could be -1 by default, that is how we expect everyone (you) to consume it > anyways. Done. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1749: buildObjectForNode(firstChild, 0, pierce, nodesMap, flattenResult); On 2017/02/02 01:24:19, pfeldman wrote: > Create childNode unconditionally, set its parent id unconditionally... Done. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1751: flattenResult->addItem(std::move(childNode)); On 2017/02/02 01:24:19, pfeldman wrote: > ... then either populate flatten or add into collection. Done. https://codereview.chromium.org/2633343003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:1767: std::unique_ptr<protocol::DOM::Node> childNode = On 2017/02/02 01:24:19, pfeldman wrote: > ditto Done.
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/2633343003/#ps140001 (title: "Fix headless browser test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486064144571010, "parent_rev": "f1238c589d9d22d51fe24c0744e28389e594267f", "commit_rev": "ea635266172221937f38f16417c0b622a5bdef97"}
Message was sent while issue was closed.
Description was changed from ========== Adds DOM.getFlatDocument which returns an array of nodes We need this because some web pages have very deep dom trees which causes the JSON parser to CHECK (there's currently a limit of 200 nodes). BUG=546953 ========== to ========== Adds DOM.getFlatDocument which returns an array of nodes We need this because some web pages have very deep dom trees which causes the JSON parser to CHECK (there's currently a limit of 200 nodes). BUG=546953 Review-Url: https://codereview.chromium.org/2633343003 Cr-Commit-Position: refs/heads/master@{#447844} Committed: https://chromium.googlesource.com/chromium/src/+/ea635266172221937f38f16417c0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/ea635266172221937f38f16417c0... |