Add compositor element id for scroll nodes in SPv2.

Add compositor element id for scroll nodes in SPv2.

- Add compositor element id field to ScrollPaintPropertyNode.
- Generate compositor element id for ScrollPaintPropertyNodes when
  created/updated.
- Copy element id into cc::ScrollNode when generating cc property trees.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[wkorman, 2017-01-19 23:38:39]

Description was changed from

==========
Add compositor element id for scroll nodes in SPv2.

- Add compositor element id field to ScrollPaintPropertyNode.
- Generate compositor element id for ScrollPaintPropertyNodes when
  created/updated.
- Copy element id into cc::ScrollNode when generating cc property trees.

BUG=674317
==========

to

==========
Add compositor element id for scroll nodes in SPv2.

- Add compositor element id field to ScrollPaintPropertyNode.
- Generate compositor element id for ScrollPaintPropertyNodes when
  created/updated.
- Copy element id into cc::ScrollNode when generating cc property trees.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2017-01-19 23:43:09]

wkorman@chromium.org changed reviewers:
+ ajuma@chromium.org, pdr@chromium.org

[wkorman, 2017-01-19 23:43:10]

Will add unit tests following initial feedback to confirm approach looks
reasonable.

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:89: static
CompositorElementId createDomNodeBasedCompositorElementId(
Moved here without changes so that it's in scope for new call from static method
below.

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:108: //
TODO(wkorman): Should we generate compositor element id conditionally on
Seeking feedback here. The general things I'm uncertain of though of course I
did research briefly:

- am I missing any ScrollPaintPropertyNode create/update calls?
- am I building the compositor element id from the correct layout object?
- are we assured that these are only created for composited scrolling cases? If
not, what should we be checking?

If we get either of these wrong we can correct in subsequent change but worth
considering now.

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:805: //
TODO(wkorman): Should we do this conditionally on something that
And seeking feedback here.

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
(right):

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:137:
m_propertyTrees.element_id_to_scroll_node_index.clear();
This (and below) could be done in a separate change but I figured I'd throw it
in here as well since pdr@ will need it in short order. I have a separate change
where I'm doing this (and setting in field and map as below) for
transform/effect nodes.

[wkorman, 2017-01-20 01:57:34]

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
(right):

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp:271:
compositorNode.element_id = scrollNode->compositorElementId();
Probably should wrap this in a block to make sure compositor element id exists
as I did in http://crrev.com/2642183003

[pdr., 2017-01-20 06:26:33]

I actually think this looks good as-is with the question comments removed. LGTM,
but please wait for Ali's review as well.

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:108: //
TODO(wkorman): Should we generate compositor element id conditionally on
On 2017/01/19 at 23:43:10, wkorman wrote:
> Seeking feedback here. The general things I'm uncertain of though of course I
did research briefly:
> 
> - am I missing any ScrollPaintPropertyNode create/update calls?

Nope, you got the two.

> - am I building the compositor element id from the correct layout object?

This is the non-RLS codepath so it's for FrameView scrolling. I'm really not
sure what Element a FrameView would have.

> - are we assured that these are only created for composited scrolling cases?
If not, what should we be checking?

These are currently created for composited and non-composited scrollers, and
will be in the future too. With SPV2's design, we won't know until
PaintArtifactCompositor which scrollers actually end up being composited. Is
there any harm in setting this id if it isn't used?

> 
> If we get either of these wrong we can correct in subsequent change but worth
considering now.

+1

[ajuma, 2017-01-20 14:41:57]

lgtm

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:108: //
TODO(wkorman): Should we generate compositor element id conditionally on
On 2017/01/20 06:26:32, pdr. wrote:
> On 2017/01/19 at 23:43:10, wkorman wrote:
> > Seeking feedback here. The general things I'm uncertain of though of course
I
> did research briefly:
> > 
> > - am I missing any ScrollPaintPropertyNode create/update calls?
> 
> Nope, you got the two.
> 
> > - am I building the compositor element id from the correct layout object?
> 
> This is the non-RLS codepath so it's for FrameView scrolling. I'm really not
> sure what Element a FrameView would have.
> 
> > - are we assured that these are only created for composited scrolling cases?
> If not, what should we be checking?
> 
> These are currently created for composited and non-composited scrollers, and
> will be in the future too. With SPV2's design, we won't know until
> PaintArtifactCompositor which scrollers actually end up being composited. Is
> there any harm in setting this id if it isn't used?

The only downside I can think of is that each DOMNodeId we generate gets
inserted into a hash map (see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/DOMNo...),
so that hash map gets a bit bigger. So just something to keep in mind as a
possible performance issue we might want to look into later.

https://codereview.chromium.org/2638333007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:805: //
TODO(wkorman): Should we do this conditionally on something that
On 2017/01/19 23:43:10, wkorman wrote:
> And seeking feedback here.

See comment above, but I think this is fine for now.

[wkorman, 2017-01-20 21:31:50]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-01-20 21:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2638333007/#ps40001
(title: "Remove unused includes.")

[commit-bot: I haz the power, 2017-01-20 21:32:57]

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-01-20 22:58:22]

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

[commit-bot: I haz the power, 2017-01-20 22:58:24]

Failed to apply patch for
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp:23
  error:
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp:
patch does not apply

Patch:      
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
Index:
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
diff --git
a/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
b/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
index
f9b87e54f16540fc3598fa1179526c2f265f9e09..dd8b2f73c1d3ac52dbea4273423e75bb802c8252
100644
---
a/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
+++
b/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
@@ -23,12 +23,14 @@ String ScrollPaintPropertyNode::toString() const {
   return String::format(
       "parent=%p scrollOffsetTranslation=%s clip=%s bounds=%s "
       "userScrollableHorizontal=%s"
-      " userScrollableVertical=%s mainThreadScrollingReasons=%s",
+      " userScrollableVertical=%s mainThreadScrollingReasons=%s"
+      "compositorElementId=(%d, %d)",
       m_parent.get(), scrollOffset.toString().ascii().data(),
       m_clip.toString().ascii().data(), m_bounds.toString().ascii().data(),
       m_userScrollableHorizontal ? "yes" : "no",
       m_userScrollableVertical ? "yes" : "no",
-      mainThreadScrollingReasonsAsText.c_str());
+      mainThreadScrollingReasonsAsText.c_str(),
m_compositorElementId.primaryId,
+      m_compositorElementId.secondaryId);
 }
 
 }  // namespace blink

[wkorman, 2017-01-20 23:34:44]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-01-20 23:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2638333007/#ps60001
(title: "Sync to head.")

[commit-bot: I haz the power, 2017-01-20 23:34:59]

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-01-21 01:26:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484955284332230,
"parent_rev": "8d43296f5c9c5c7f5f362ea599881fbd28df4898", "commit_rev":
"cfdf2d92414d581e8fdc5fdc04d825c668113fcc"}

[commit-bot: I haz the power, 2017-01-21 01:27:20]

Description was changed from

==========
Add compositor element id for scroll nodes in SPv2.

- Add compositor element id field to ScrollPaintPropertyNode.
- Generate compositor element id for ScrollPaintPropertyNodes when
  created/updated.
- Copy element id into cc::ScrollNode when generating cc property trees.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add compositor element id for scroll nodes in SPv2.

- Add compositor element id field to ScrollPaintPropertyNode.
- Generate compositor element id for ScrollPaintPropertyNodes when
  created/updated.
- Copy element id into cc::ScrollNode when generating cc property trees.

BUG=674317
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-01-21 01:27:21]

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