[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

Introduces a new DOMSnapshot.getSnapshot command, which returns a
flattened dom tree together with layout objects and computed styles.

By combining all trees in one command's result, we avoid the potential
for inconsistencies between them.

The patch also removes the unused DomTreeExtractor wrapper in
headless/. In a subsequent patch, we'll further remove the now obsolete
FlatDomTreeExtractor and CSS.getLayoutTreeAndStyles.

BUG=546953
Review-Url: https://codereview.chromium.org/2882193002
Cr-Commit-Position: refs/heads/master@{#478513}
Committed: https://chromium.googlesource.com/chromium/src/+/5a101abbe0fd58cfa3fdadfeacde86c8dd108ec7

[Eric Seckler, 2017-05-15 10:08:11]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 10:08:24]

Dry run: CQ is trying da patch.

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

[Eric Seckler, 2017-05-15 10:10:42]

eseckler@chromium.org changed reviewers:
+ alexclarke@chromium.org, pfeldman@chromium.org

[Eric Seckler, 2017-05-15 10:10:43]

I'm also happy to later remove the getFlattenedDocument and
getLayoutTreeAndStyles commands from the public DevTools API. WDYT?

Thanks :)

[alex clarke (OOO till 29th), 2017-05-15 10:18:36]

non owner LGTM

[commit-bot: I haz the power, 2017-05-15 11:20:02]

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

[commit-bot: I haz the power, 2017-05-15 11:20:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-05-15 11:27:25]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 11:27:39]

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-05-15 13:11:36]

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

[commit-bot: I haz the power, 2017-05-15 13:11:38]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-05-15 18:42:18]

https://codereview.chromium.org/2882193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2882193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2305: Response
InspectorCSSAgent::getLayoutTreeAndStyles(
Do we still need it?

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

https://codereview.chromium.org/2882193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3293: { "name":
"domNodes", "type": "array", "items": { "$ref": "DOM.Node" }, "description":
"The nodes in the DOM tree." },
When I was suggesting a separate domain, I meant that it would help isolating
the data structures as well. I.e. you don't need nodeId because it has front-end
specific semantics, you can drop other node properties you don't need, you could
inline layout and css info the way you needed.

If we are still using all the data structures from DOM and CSS, we can just put
this method into DOM domain, but it would need to push nodes to the front-end to
maintain present contracts.

[Eric Seckler, 2017-05-17 10:44:31]

Thanks, Pavel!

https://codereview.chromium.org/2882193002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2882193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2305: Response
InspectorCSSAgent::getLayoutTreeAndStyles(
On 2017/05/15 18:42:18, pfeldman wrote:
> Do we still need it?

Not really. I was going to remove it and move the implementation to the
DOMSnapshotAgent if feasible, but only after we update headless clients
including the FlatDomTreeExtractor to stop using this.

We can do this in this patch, too, once we agree on the approach here :)

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

https://codereview.chromium.org/2882193002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3293: { "name":
"domNodes", "type": "array", "items": { "$ref": "DOM.Node" }, "description":
"The nodes in the DOM tree." },
On 2017/05/15 18:42:18, pfeldman wrote:
> When I was suggesting a separate domain, I meant that it would help isolating
> the data structures as well. I.e. you don't need nodeId because it has
front-end
> specific semantics, you can drop other node properties you don't need, you
could
> inline layout and css info the way you needed.
Ah, I see. I think the only thing that's not really necessary for headless
snapshots are the devtools node ids. We'd like to keep the other fields of the
DOM/CSS domain types and we wouldn't inline css/layout info either. So this
would amount to duplicating the types into the new domain, which is a lot of
work without much benefit (other than removing devtools ids).

What may be more useful (if necessary in the future) would be wrapping some of
the DOM/CSS types to add snapshot/headless-specific fields to them that are
irrelevant for the frontend.

> If we are still using all the data structures from DOM and CSS, we can just
put
> this method into DOM domain, but it would need to push nodes to the front-end
to
> maintain present contracts.
I see your point about pushing the nodes. Would moving this command to the DOM
domain avoid the need for additional pushing, as we also don't do that for
DOM.getFlattenedDocument? It's not useful for us if we get all nodes twice, once
via the DOM domain events and once in the snapshot.

I'd be happy to (optionally) piggyback the css/layout info in
DOM.getFlattenedDocument instead, but that would introduce a dependency from the
DOM domain to the CSS domain types - thus creating a cyclic dependency between
DOM and CSS, which seems unsupported.

[pfeldman, 2017-05-18 20:12:27]

> Ah, I see. I think the only thing that's not really necessary for headless
> snapshots are the devtools node ids. We'd like to keep the other fields of the
> DOM/CSS domain types and we wouldn't inline css/layout info either. So this
> would amount to duplicating the types into the new domain, which is a lot of
> work without much benefit (other than removing devtools ids).

So you are suggesting to (ab)use Node definition and introduce a class of
non-pushed nodes that have nodeId set to -1. That would probably work, but in
this case you can simply keep everything in the CSS domain as it is now... I
don't see why we would need a new domain in this case.

[Eric Seckler, 2017-05-19 15:57:16]

On 2017/05/18 20:12:27, pfeldman wrote:
> > Ah, I see. I think the only thing that's not really necessary for headless
> > snapshots are the devtools node ids. We'd like to keep the other fields of
the
> > DOM/CSS domain types and we wouldn't inline css/layout info either. So this
> > would amount to duplicating the types into the new domain, which is a lot of
> > work without much benefit (other than removing devtools ids).
> 
> So you are suggesting to (ab)use Node definition and introduce a class of
> non-pushed nodes that have nodeId set to -1. That would probably work, but in
> this case you can simply keep everything in the CSS domain as it is now... I
> don't see why we would need a new domain in this case.

Ideally, I think, we'd put the snapshot command in the DOM domain (where we
don't violate any nodeId contracts), so that we can return dom nodes with
nodeIds. But that's not possible because of cyclic domain dependencies, AFAICT.

If we are to put the command in the CSS domain and set nodeId to -1, would you
recommend that we replace DOM.Node.parentId by DOM.Node.parentBackendId to
support a flattened tree without nodeIds?

[pfeldman, 2017-05-19 23:21:12]

> 
> Ideally, I think, we'd put the snapshot command in the DOM domain (where we
> don't violate any nodeId contracts), so that we can return dom nodes with
> nodeIds. But that's not possible because of cyclic domain dependencies,
AFAICT.
> 
> If we are to put the command in the CSS domain and set nodeId to -1, would you
> recommend that we replace DOM.Node.parentId by DOM.Node.parentBackendId to
> support a flattened tree without nodeIds?

Ah, you are asking all the right and tough questions. Put it into CSS and
maintain nodeId contract? I.e. pretty much what we have?

[Eric Seckler, 2017-05-22 10:26:05]

On 2017/05/19 23:21:12, pfeldman wrote:
> Ah, you are asking all the right and tough questions. Put it into CSS and
> maintain nodeId contract? I.e. pretty much what we have?

:) Sounds fine, PTAL. I added a comment about this to the domNodes result, let
me know if you think we need to do more.

Alex, PTAL at headless/ changes.

Thank you!

[Eric Seckler, 2017-05-22 10:27:58]

Description was changed from

==========
[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

The new getSnapshot command avoids the potential for inconsistencies
between DOM.get(Flattened)Document and CSS.getLayoutTreeAndStyles by
combining the two commands in one. The new command lives in its own
domain, as it needs to depend on types from both the CSS and DOM domain.

BUG=546953
==========

to

==========
[devtools] Add CSS.getDocumentSnapshot for dom+layout+style snapshots.

The new getDocumentSnapshot command avoids the potential for
inconsistencies between DOM.get(Flattened)Document and
CSS.getLayoutTreeAndStyles by combining the two commands in one.

The command lives in the CSS domain as it depends on types from both
the CSS and DOM domains.

BUG=546953
==========

[Eric Seckler, 2017-05-22 10:28:25]

Description was changed from

==========
[devtools] Add CSS.getDocumentSnapshot for dom+layout+style snapshots.

The new getDocumentSnapshot command avoids the potential for
inconsistencies between DOM.get(Flattened)Document and
CSS.getLayoutTreeAndStyles by combining the two commands in one.

The command lives in the CSS domain as it depends on types from both
the CSS and DOM domains.

BUG=546953
==========

to

==========
[devtools] Add CSS.getDocumentSnapshot for dom+layout+style snapshots.

The new getDocumentSnapshot command avoids the potential for
inconsistencies between DOM.get(Flattened)Document and
CSS.getLayoutTreeAndStyles by combining the two commands in one.

The command lives in the CSS domain as it depends on types from both
the CSS and DOM domains.

The patch also updates headless/ wrappers for dom tree extraction.

BUG=546953
==========

[Eric Seckler, 2017-05-22 10:29:02]

Description was changed from

==========
[devtools] Add CSS.getDocumentSnapshot for dom+layout+style snapshots.

The new getDocumentSnapshot command avoids the potential for
inconsistencies between DOM.get(Flattened)Document and
CSS.getLayoutTreeAndStyles by combining the two commands in one.

The command lives in the CSS domain as it depends on types from both
the CSS and DOM domains.

The patch also updates headless/ wrappers for dom tree extraction.

BUG=546953
==========

to

==========
[devtools] Add CSS.getDocumentSnapshot for dom+layout+style snapshots.

The new getDocumentSnapshot command avoids the potential for
inconsistencies between DOM.get(Flattened)Document and
CSS.getLayoutTreeAndStyles by combining the two commands in one.

The command lives in the CSS domain as it depends on types from both
the CSS and DOM domains.

The patch also updates headless/ wrappers for dom tree extraction and
removes the old, now unused, getLayoutTreeAndStyles command.

BUG=546953
==========

[alex clarke (OOO till 29th), 2017-05-22 13:27:42]

Still LGTM

[Eric Seckler, 2017-05-25 08:03:04]

Pavel, friendly ping :)

[pfeldman, 2017-05-31 17:56:37]

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

https://codereview.chromium.org/2882193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3002: { "name":
"nodeId", "$ref": "DOM.NodeId", "description": "The id of the related DOM node
in the domNodes array returned by getDocumentSnapshot." },
It can't be of type NodeId - NodeId is a node handle type which it no longer is.
It is now the "index" in the domNodes array.

https://codereview.chromium.org/2882193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3217: { "name":
"domDepth", "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." },
it is called depth everywhere else.

https://codereview.chromium.org/2882193002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3221: { "name":
"domNodes", "type": "array", "items": { "$ref": "DOM.Node" }, "description":
"The nodes in the DOM tree. These nodes will also be considered as known to the
DOM domain client in the future." },
I don't think it makes much sense - we should not combine the "inspection" and
"scripting/automation" semantics here. It is unlikely that the snapshot client
wants to receive all the node-related notifications post the snapshot retrieval
and you are forcing this.

[Eric Seckler, 2017-06-06 15:01:27]

Based on our offline chat, I moved the snapshot command back into the
DOMSnapshot domain and added custom node types there, avoiding any node IDs and
purely using indexes instead.

Some things that should probably be mentioned/discussed:

1) I got rid of a number of properties from the DOM.Node type. I think we'll be
better off adding things back if we need them later.

2) The Node types still uses explicit (and largely optional) result object
fields for properties of the different node types. An alternative would be to
use a single "auxiliary properties" field with a list of name/value string
pairs. Advantage being that it would be more easily extensible, disadvantage
being that available properties would less well documented / defined. I'd lean
towards keeping the explicit fields, WDYT?

3) The new command uses a flat-tree traversal, which essentially abstracts away
any shadow DOM / distributed nodes into a flattened DOM tree. I believe this
flattened tree is probably what most clients are interested in (this way, they
don't need to match distributed nodes with slots themselves).

4) I'm removing CSS.getLayoutTreeAndStyles in this patch, but I think we might
have to split that out and do this in a subsequent patch.

[Eric Seckler, 2017-06-06 21:51:30]

eseckler@chromium.org changed reviewers:
+ dgozman@chromium.org

[Eric Seckler, 2017-06-06 21:51:31]

+dgozman - thanks! :)

[pfeldman, 2017-06-06 22:54:29]

I like this a lot. Some comments below, but overall looks very good.

https://codereview.chromium.org/2882193002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/schema_handler.cc (right):

https://codereview.chromium.org/2882193002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/schema_handler.cc:25: static const char*
kDomains[] = {
Wondering if git cl format is cool with this :)

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom-snapshot/dom-snapshot-getSnapshot.html
(right):

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom-snapshot/dom-snapshot-getSnapshot.html:17:
InspectorTest.sendCommandOrDie("DOMSnapshot.getSnapshot",
{"computedStyleWhitelist": whitelist, "depth": -1, "pierce": true}, onSnapshot);
It's a new world!

async function test() {
  await InspectorTest.sendCommand("DOM.enable", {});
  var response = await InspectorTest.sendCommandOrDie("", {});
  ...
}

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp
(right):

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:155: if
(node_value.length() > kMaxTextSize)
They want to get a snapshot, give them a snapshot? Don't trim, go lossless.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:251: //
If node has an only text child, add it anyway.
You don't need this logic, it was purely for inspection.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:283:
depth--;
Pseudos should not be depth-aware, they should not have children.

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

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3312: { "name":
"childNodeIndexes", "type": "array", "items": { "type": "integer" }, "optional":
true, "description": "The indexes of the node's child nodes in the
<code>domNodes</code> array returned by <code>getSnapshot</code>, if any." },
It is tempting to use backendNodeId here - it is stable Node id that is
associated with the node. Either use those in place of indexes or at least
include them for nodes. That would be your interop path into the DOM/CSS world.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3314: { "name":
"documentURL", "type": "string", "optional": true, "description": "Document URL
that <code>Document</code> or <code>FrameOwner</code> node points to." },
Everything rare that is not essential to any node and has named string value:
extract into array of auxProperties of type "Attribute" or better yet have it
renamed into "NameValue" and reuse for attributes and computed styles.

That would be baseURL, publicId, systemId.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3324: { "name":
"isSVG", "type": "boolean", "optional": true, "description": "Whether the node
is SVG." },
Let's save at least on this one:)

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3345: { "name":
"properties", "type": "array", "items": { "$ref": "CSS.CSSComputedStyleProperty"
} }
Rename and reuse your "Attribute" to make your domain almost self-contained?

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3364: { "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." },
Drop depth and pierce, always imply -1, true?

[Eric Seckler, 2017-06-07 10:23:37]

Thanks! PTAL. Alex, probably makes sense for you to take another peek, too.

FYI, I reverted the removal of getLayoutTreeAndStyles - will do that separately
afterwards.

https://codereview.chromium.org/2882193002/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/schema_handler.cc (right):

https://codereview.chromium.org/2882193002/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/schema_handler.cc:25: static const char*
kDomains[] = {
On 2017/06/06 22:54:29, pfeldman wrote:
> Wondering if git cl format is cool with this :)

Oh, this was done by "git cl format" (wasn't me :)). We can probably trick it
not to reformat this if you don't like it, though.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom-snapshot/dom-snapshot-getSnapshot.html
(right):

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom-snapshot/dom-snapshot-getSnapshot.html:17:
InspectorTest.sendCommandOrDie("DOMSnapshot.getSnapshot",
{"computedStyleWhitelist": whitelist, "depth": -1, "pierce": true}, onSnapshot);
On 2017/06/06 22:54:29, pfeldman wrote:
> It's a new world!
> 
> async function test() {
>   await InspectorTest.sendCommand("DOM.enable", {});
>   var response = await InspectorTest.sendCommandOrDie("", {});
>   ...
> }

Done, also removed unnecessary DOM.enable :)

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom-snapshot/dom-snapshot-getSnapshot.html:17:
InspectorTest.sendCommandOrDie("DOMSnapshot.getSnapshot",
{"computedStyleWhitelist": whitelist, "depth": -1, "pierce": true}, onSnapshot);
On 2017/06/06 22:54:29, pfeldman wrote:
> It's a new world!
> 
> async function test() {
>   await InspectorTest.sendCommand("DOM.enable", {});
>   var response = await InspectorTest.sendCommandOrDie("", {});
>   ...
> }

Done.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp
(right):

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:155: if
(node_value.length() > kMaxTextSize)
On 2017/06/06 22:54:29, pfeldman wrote:
> They want to get a snapshot, give them a snapshot? Don't trim, go lossless.

Ah, I thought this might be b/c of a protocol limitation. Got rid of it :)

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:251: //
If node has an only text child, add it anyway.
On 2017/06/06 22:54:29, pfeldman wrote:
> You don't need this logic, it was purely for inspection.

Removed.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:283:
depth--;
On 2017/06/06 22:54:29, pfeldman wrote:
> Pseudos should not be depth-aware, they should not have children.

Depth is gone everywhere now.

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

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3312: { "name":
"childNodeIndexes", "type": "array", "items": { "type": "integer" }, "optional":
true, "description": "The indexes of the node's child nodes in the
<code>domNodes</code> array returned by <code>getSnapshot</code>, if any." },
On 2017/06/06 22:54:29, pfeldman wrote:
> It is tempting to use backendNodeId here - it is stable Node id that is
> associated with the node. Either use those in place of indexes or at least
> include them for nodes. That would be your interop path into the DOM/CSS
world.

Added as a field. We'd prefer to continue referencing nodes by their indexes
here though.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3314: { "name":
"documentURL", "type": "string", "optional": true, "description": "Document URL
that <code>Document</code> or <code>FrameOwner</code> node points to." },
On 2017/06/06 22:54:29, pfeldman wrote:
> Everything rare that is not essential to any node and has named string value:
> extract into array of auxProperties of type "Attribute" or better yet have it
> renamed into "NameValue" and reuse for attributes and computed styles.
> 
> That would be baseURL, publicId, systemId.

Chatted to Alex about this, too. We'd like to keep the supported auxProperties
explicit + documented in the protocol. I moved everything that only exists for
particular node types into an additional AuxiliaryProperties type to clean this
up a little.

An alternative might be a list of key->string items, where key is of an enum
type (which we can use to document what keys are supported). Not sure if that
would make any practical difference though.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3324: { "name":
"isSVG", "type": "boolean", "optional": true, "description": "Whether the node
is SVG." },
On 2017/06/06 22:54:29, pfeldman wrote:
> Let's save at least on this one:)

Got rid of it for now. If we need it later, we can add it back.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3345: { "name":
"properties", "type": "array", "items": { "$ref": "CSS.CSSComputedStyleProperty"
} }
On 2017/06/06 22:54:29, pfeldman wrote:
> Rename and reuse your "Attribute" to make your domain almost self-contained?

Done.

https://codereview.chromium.org/2882193002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3364: { "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." },
On 2017/06/06 22:54:29, pfeldman wrote:
> Drop depth and pierce, always imply -1, true?

Done.

[Eric Seckler, 2017-06-07 10:23:47]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 10:24:01]

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-06-07 10:26:13]

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

[commit-bot: I haz the power, 2017-06-07 10:26:14]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-06-07 10:27:32]

Description was changed from

==========
[devtools] Add CSS.getDocumentSnapshot for dom+layout+style snapshots.

The new getDocumentSnapshot command avoids the potential for
inconsistencies between DOM.get(Flattened)Document and
CSS.getLayoutTreeAndStyles by combining the two commands in one.

The command lives in the CSS domain as it depends on types from both
the CSS and DOM domains.

The patch also updates headless/ wrappers for dom tree extraction and
removes the old, now unused, getLayoutTreeAndStyles command.

BUG=546953
==========

to

==========
[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

This introduces a new DOMSnapshot.getSnapshot command, which returns a
flattened dom tree together with layout objects and computed styles.

By combining all trees in one command's result, we avoid the potential
for inconsistencies between them.

The patch also removes the unused DomTreeExtractor wrapper in
headless/. In a subsequent patch, we'll further remove the now obsolete
FlatDomTreeExtractor and CSS.getLayoutTreeAndStyles.

BUG=546953
==========

[Eric Seckler, 2017-06-07 10:27:46]

Description was changed from

==========
[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

This introduces a new DOMSnapshot.getSnapshot command, which returns a
flattened dom tree together with layout objects and computed styles.

By combining all trees in one command's result, we avoid the potential
for inconsistencies between them.

The patch also removes the unused DomTreeExtractor wrapper in
headless/. In a subsequent patch, we'll further remove the now obsolete
FlatDomTreeExtractor and CSS.getLayoutTreeAndStyles.

BUG=546953
==========

to

==========
[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

Introduces a new DOMSnapshot.getSnapshot command, which returns a
flattened dom tree together with layout objects and computed styles.

By combining all trees in one command's result, we avoid the potential
for inconsistencies between them.

The patch also removes the unused DomTreeExtractor wrapper in
headless/. In a subsequent patch, we'll further remove the now obsolete
FlatDomTreeExtractor and CSS.getLayoutTreeAndStyles.

BUG=546953
==========

[Eric Seckler, 2017-06-07 10:31:51]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 10:32:09]

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-06-07 10:41:00]

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

[commit-bot: I haz the power, 2017-06-07 10:41:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Eric Seckler, 2017-06-07 10:58:16]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 10:58:27]

Dry run: CQ is trying da patch.

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

[alex clarke (OOO till 29th), 2017-06-07 12:56:44]

Still looks fine functionality wise although I didn't do a detailed review.

https://codereview.chromium.org/2882193002/diff/140001/headless/lib/headless_...
File headless/lib/headless_devtools_client_browsertest.cc (right):

https://codereview.chromium.org/2882193002/diff/140001/headless/lib/headless_...
headless/lib/headless_devtools_client_browsertest.cc:1328: const
std::vector<std::string> expected_dom_nodes = {
Bikeshed (maybe best as a todo): Formatting this string is a right pain ;)  I
wonder if it's worth moving this into a file and storing it in json array
format?

It's relatively simple to to include a file, see the .grd file change in
https://codereview.chromium.org/2902583002/

https://codereview.chromium.org/2882193002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp
(right):

https://codereview.chromium.org/2882193002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:52:
return node->getAuxProperties(nullptr);
I wonder if the compiler is smart enough to compile the following to the same
code?

node->getAuxProperties(protocol::DOMSnapshot::AuxiliaryProperties::create().build())

[Eric Seckler, 2017-06-07 13:05:17]

thanks!

https://codereview.chromium.org/2882193002/diff/140001/headless/lib/headless_...
File headless/lib/headless_devtools_client_browsertest.cc (right):

https://codereview.chromium.org/2882193002/diff/140001/headless/lib/headless_...
headless/lib/headless_devtools_client_browsertest.cc:1328: const
std::vector<std::string> expected_dom_nodes = {
On 2017/06/07 12:56:44, alex clarke wrote:
> Bikeshed (maybe best as a todo): Formatting this string is a right pain ;)  I
> wonder if it's worth moving this into a file and storing it in json array
> format?
> 
> It's relatively simple to to include a file, see the .grd file change in
> https://codereview.chromium.org/2902583002/

ah, I was just printf-ing the result_jsons to create this list of strings -
wasn't too painful. Added a todo to extract all these strings into expectation
files.

https://codereview.chromium.org/2882193002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp
(right):

https://codereview.chromium.org/2882193002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorDOMSnapshotAgent.cpp:52:
return node->getAuxProperties(nullptr);
On 2017/06/07 12:56:44, alex clarke wrote:
> I wonder if the compiler is smart enough to compile the following to the same
> code?
> 
>
node->getAuxProperties(protocol::DOMSnapshot::AuxiliaryProperties::create().build())

Probably not, but even if it would, that doesn't actually result in the same
logic - getAuxProperties() doesn't set the default value in the object if it
didn't have a value. It just returns it. So you'd still have to call
setAuxProperties() afterwards.

[pfeldman, 2017-06-07 21:33:35]

devtools lgtm

https://codereview.chromium.org/2882193002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2882193002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3356: "id":
"AuxiliaryProperties",
There is no point in the nesting, my proposal was to make rare data reflective,
a key-value pairs. If you feel strongly that you want publicId/systemId
information typed, just inline it all it back.

[Eric Seckler, 2017-06-09 13:01:38]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 13:01:49]

Dry run: CQ is trying da patch.

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

[Eric Seckler, 2017-06-09 13:01:54]

Thanks, Pavel! :)

https://codereview.chromium.org/2882193002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2882193002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3356: "id":
"AuxiliaryProperties",
On 2017/06/07 21:33:35, pfeldman wrote:
> There is no point in the nesting, my proposal was to make rare data
reflective,
> a key-value pairs. If you feel strongly that you want publicId/systemId
> information typed, just inline it all it back.

It's more about the supported keys, rather than the types of values. I prefer to
have the key typed out explicitly, since that'll lead to compile errors in
C++/Closure clients if they change. Moved fields back into the node type.

[commit-bot: I haz the power, 2017-06-09 15:02:23]

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

[commit-bot: I haz the power, 2017-06-09 15:02:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-06-09 15:11:55]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-06-09 15:11:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2882193002/#ps180001
(title: "move aux properties back to node type")

[commit-bot: I haz the power, 2017-06-09 15:12:16]

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-06-09 17:19:45]

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

[commit-bot: I haz the power, 2017-06-09 17:19:47]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Seckler, 2017-06-10 09:02:44]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2017-06-10 09:02:50]

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-06-10 09:46:10]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1497085364529710,
"parent_rev": "632b145c0e44fc890662f41a8638d7bcb4a203bf", "commit_rev":
"5a101abbe0fd58cfa3fdadfeacde86c8dd108ec7"}

[commit-bot: I haz the power, 2017-06-10 09:46:25]

Description was changed from

==========
[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

Introduces a new DOMSnapshot.getSnapshot command, which returns a
flattened dom tree together with layout objects and computed styles.

By combining all trees in one command's result, we avoid the potential
for inconsistencies between them.

The patch also removes the unused DomTreeExtractor wrapper in
headless/. In a subsequent patch, we'll further remove the now obsolete
FlatDomTreeExtractor and CSS.getLayoutTreeAndStyles.

BUG=546953
==========

to

==========
[devtools] Add DOMSnapshot domain for dom+layout+style snapshots.

Introduces a new DOMSnapshot.getSnapshot command, which returns a
flattened dom tree together with layout objects and computed styles.

By combining all trees in one command's result, we avoid the potential
for inconsistencies between them.

The patch also removes the unused DomTreeExtractor wrapper in
headless/. In a subsequent patch, we'll further remove the now obsolete
FlatDomTreeExtractor and CSS.getLayoutTreeAndStyles.

BUG=546953

Review-Url: https://codereview.chromium.org/2882193002
Cr-Commit-Position: refs/heads/master@{#478513}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a101abbe0fd58cfa3fdadfeacde...
==========

[commit-bot: I haz the power, 2017-06-10 09:46:26]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/5a101abbe0fd58cfa3fdadfeacde...