Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties from
the results.

BUG=546953
Committed: https://crrev.com/6d803545166f33cac42f4e2a74658cf834d09b5c
Cr-Commit-Position: refs/heads/master@{#428178}

[alex clarke (OOO till 29th), 2016-10-26 14:46:55]

Description was changed from

==========
Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties.

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

to

==========
Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties from
the results.

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

[alex clarke (OOO till 29th), 2016-10-26 14:46:55]

alexclarke@chromium.org changed reviewers:
+ dgozman@chromium.org, skyostil@google.com

[alex clarke (OOO till 29th), 2016-10-26 14:48:32]

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

[alex clarke (OOO till 29th), 2016-10-26 14:48:48]

PTAL :)

[commit-bot: I haz the power, 2016-10-26 14:48:51]

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, 2016-10-26 17:08:33]

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

[commit-bot: I haz the power, 2016-10-26 17:08:34]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-10-26 17:15:23]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2016-10-26 17:15:23]

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/css-getLayoutTreeNodes.html
(right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-getLayoutTreeNodes.html:15:
InspectorTest.sendCommandOrDie("DOM.getDocument",  {"depth": -1}, onDocument);
nit: extra space before {"depth"...

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2379:
style.append("dummy value");
Would it be hard to fix this in WTF::HashMap? (I wonder if it's by design or
not). Alternatively could we avoid inserting when we have nothing to insert?

[dgozman, 2016-10-26 17:24:39]

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2379:
style.append("dummy value");
On 2016/10/26 17:15:23, Sami wrote:
> Would it be hard to fix this in WTF::HashMap? (I wonder if it's by design or
> not). Alternatively could we avoid inserting when we have nothing to insert?

There should be a possibility with some magic boolean flags combination on the
traits, but I cannot advise which one...

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2383: continue;
If you skip values in resulting array, how does client match the values to the
properties they requested?

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2443: int nodeId
= m_domAgent->boundNodeId(node);
This requires the node to be already pushed to the client. Is that what you
want?

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2914: { "name":
"nodeId", "$ref": "DOM.BackendNodeId", "description": "The id of the related DOM
node matching one from DOM.GetDocument." },
I think the $ref should be changed as well.

[alex clarke (OOO till 29th), 2016-10-26 21:18:49]

I'll send another patch set tomorrow but a couple of quick replies for now.

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2379:
style.append("dummy value");
On 2016/10/26 17:15:23, Sami wrote:
> Would it be hard to fix this in WTF::HashMap? (I wonder if it's by design or
> not). Alternatively could we avoid inserting when we have nothing to insert?

Possibly, I'm not sure how though.  Honestly WTF::HashMap is a nightmare to use,
I spent an hour trying to avoid the ugly dummy value with no success :(

It would be trivial to fix this with std::unordered_map...

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2383: continue;
On 2016/10/26 17:24:39, dgozman wrote:
> If you skip values in resulting array, how does client match the values to the
> properties they requested?

We send the key as well as the value, see:

styleProperties->addItem(protocol::CSS::CSSComputedStyleProperty::create()
 .setName(cssPropertyWhitelist[i - 1].first)
 .setValue(style[i])
.build());

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2443: int nodeId
= m_domAgent->boundNodeId(node);
On 2016/10/26 17:24:39, dgozman wrote:
> This requires the node to be already pushed to the client. Is that what you
> want?

What I want to do is join the results of DOM.getDocument (with depth of -1) and
CSS.getLayoutTreeAndStyles 
With that in mind this seems ok.

Still I don't really mind how we do that, and if theres a better way I'd be
happy to explore it.

[alex clarke (OOO till 29th), 2016-10-27 10:51:03]

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

[alex clarke (OOO till 29th), 2016-10-27 10:51:12]

PTAL

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/css-getLayoutTreeNodes.html
(right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-getLayoutTreeNodes.html:15:
InspectorTest.sendCommandOrDie("DOM.getDocument",  {"depth": -1}, onDocument);
On 2016/10/26 17:15:23, Sami wrote:
> nit: extra space before {"depth"...

Done.

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455613002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2914: { "name":
"nodeId", "$ref": "DOM.BackendNodeId", "description": "The id of the related DOM
node matching one from DOM.GetDocument." },
On 2016/10/26 17:24:39, dgozman wrote:
> I think the $ref should be changed as well.

Done.

[commit-bot: I haz the power, 2016-10-27 10:51:31]

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, 2016-10-27 11:59:01]

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

[commit-bot: I haz the power, 2016-10-27 11:59:02]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[alex clarke (OOO till 29th), 2016-10-27 15:06:18]

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

[commit-bot: I haz the power, 2016-10-27 15:06:40]

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, 2016-10-27 16:54:47]

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

[commit-bot: I haz the power, 2016-10-27 16:54:48]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-10-27 19:16:05]

lgtm

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

https://codereview.chromium.org/2455613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2399:
.setName(cssPropertyWhitelist[i].first)
Aren't these two out of sync, since we skip items in |style|, but don't skip in
|cssPropertyWhitelist| ?

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

https://codereview.chromium.org/2455613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3135:
"description": "For the main document and any content documents, return the
LayoutTreeNodes and a whitelisted subset of the computed style.",
Let's mention that this only works for pushed nodes, and mention getDocument
with depth=-1 as a way to push everything.

[alex clarke (OOO till 29th), 2016-10-27 20:51:20]

Thanks for the review!

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

https://codereview.chromium.org/2455613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2399:
.setName(cssPropertyWhitelist[i].first)
On 2016/10/27 19:16:05, dgozman wrote:
> Aren't these two out of sync, since we skip items in |style|, but don't skip
in
> |cssPropertyWhitelist| ?

Uh wow yes! Fixed now.

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

https://codereview.chromium.org/2455613002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3135:
"description": "For the main document and any content documents, return the
LayoutTreeNodes and a whitelisted subset of the computed style.",
On 2016/10/27 19:16:05, dgozman wrote:
> Let's mention that this only works for pushed nodes, and mention getDocument
> with depth=-1 as a way to push everything.

Done.

[alex clarke (OOO till 29th), 2016-10-27 20:51:36]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-10-27 20:51:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2455613002/#ps60001
(title: "Fix skew between property names and values when emepty ones are
skipped.")

[commit-bot: I haz the power, 2016-10-27 20:52: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, 2016-10-27 22:53:13]

Description was changed from

==========
Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties from
the results.

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

to

==========
Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties from
the results.

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

[commit-bot: I haz the power, 2016-10-27 22:53:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-27 22:55:38]

Description was changed from

==========
Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties from
the results.

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

to

==========
Fix index used to join results of DOM.getDocument and CSS.getLayoutTreeAndStyles

I had thought the backend node id matched the output of DOM.getDocument, but it
doesn't.

As an added bonus I've omitted empty css style properties from
the results.

BUG=546953

Committed: https://crrev.com/6d803545166f33cac42f4e2a74658cf834d09b5c
Cr-Commit-Position: refs/heads/master@{#428178}
==========

[commit-bot: I haz the power, 2016-10-27 22:55:39]

Patchset 4 (id:??) landed as
https://crrev.com/6d803545166f33cac42f4e2a74658cf834d09b5c
Cr-Commit-Position: refs/heads/master@{#428178}