Store compositor element id in paint properties for animated objects.

Store compositor element id in paint properties for animated objects.

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

Committed: https://crrev.com/8a4ccdb5a2bef0e128a64cea83fa9de3daf785a8
Cr-Commit-Position: refs/heads/master@{#441255}

[wkorman, 2016-12-28 19:22:03]

Description was changed from

==========
Store compositor element id in paint properties for animated objects.

BUG=674258,674317
==========

to

==========
Store compositor element id in paint properties for animated objects.

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

[wkorman, 2016-12-28 19:22:38]

wkorman@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[wkorman, 2016-12-28 19:22:45]

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

[commit-bot: I haz the power, 2016-12-28 19:22:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-12-28 19:33:43]

In what cases will an object change its compositor element id?
Do we need extra setNeedsPaintPropertyUpdate() call in the cases?
Do we need to check under-invalidation of paint properties for compositor
element id?

[wkorman, 2016-12-28 20:47:22]

Update -- I am going to rework this patch per discussion with chrishtr@ just now
to store the compositor element id, potentially in duplicate, on the paint
property tree nodes rather than ObjectPaintProperties. I would have discovered
this need in my next patch, when I would have gone to use the element id in
PaintArtifactCompositor and found that we don't have access to OPP there. :)

Chris convinced me that, given we're storing direct compositing reason on the
individual paint property nodes, storing the compositor element id there as well
makes sense. If an element has say both a transform and effect paint property
node, each will have the same element id, but that's deemed ok. It's essentially
two ints in size.

On 2016/12/28 at 19:33:43, wangxianzhu wrote:
> In what cases will an object change its compositor element id?

Currently the compositor element id is treated as an opaque id that the
animation subsystem ends up using cc-side as essentially a layer id. The cc
animation code handles transform, opacity, filter, and scroll by doing a
compositor element id to layer id lookup, and then uses the cc layer id to look
up the transform/effect/scroll cc property tree node.

Per notes on http://crbug.com/674258 and discussion with chrishtr@ in person
just now, we think we can rework this for both SPv1 and SPv2 so that we build,
for cc use, a map from compositor element id straight to the
transform/effect/scroll nodes. For SPv2 this will happen in
PaintArtifactCompositor. For SPv1 we can do it in cc's PropertyTreeBuilder.

I don't currently believe that an object has to change its compositor element id
once created. It is based on the DOM node associated with the Element that is
being animated. I am however still learning the animation subsystem, so I may
have missed some situation. In an earlier version of this patch I had added
logic to clear out the compositor element id if we cease to animate, but I
removed that as I think it's not harmful to leave it, and saves a bit of
potential work if something that was once animated animates again later.

> Do we need extra setNeedsPaintPropertyUpdate() call in the cases?

PaintLayer::styleDidChange() calls setNeedsPaintPropertyUpdate() unconditionally
currently, which we believe covers what is needed for animations:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

Again, we may have missed something, open to thoughts or we can add as we
discover more.

> Do we need to check under-invalidation of paint properties for compositor
element id?

Hmm. Do you mean, add logic specifically to catch cases where we might have one
of the animation types (opacity, effect, transform) and we have a direct
compositing reason noted for animation purposes but somehow end up with no
compositor element id on the OPP? Or, please explain what you have in mind
further.

I need to update this patch per above in any case, so I will start on that while
we continue discussion.

[commit-bot: I haz the power, 2016-12-28 21:29:57]

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

[commit-bot: I haz the power, 2016-12-28 21:29:58]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-12-29 06:01:29]

On 2016/12/28 20:47:22, wkorman wrote:
> > Do we need to check under-invalidation of paint properties for compositor
> element id?
> 
> Hmm. Do you mean, add logic specifically to catch cases where we might have
one
> of the animation types (opacity, effect, transform) and we have a direct
> compositing reason noted for animation purposes but somehow end up with no
> compositor element id on the OPP? Or, please explain what you have in mind
> further.

I mean to catch the case that an object would change its compositor element id
but didn't get setNeedsPaintPropertyUpdate. We check other paint properties in
FindPropertiesNeedUpdate. I think this will be achieved when you add the field
in paint property nodes and update the == operators of them.

[wkorman, 2016-12-29 20:22:54]

wkorman@chromium.org changed reviewers:
+ chrishtr@chromium.org
- wangxianzhu@chromium.org

[wkorman, 2016-12-29 20:22:55]

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380: if
(style.hasCurrentTransformAnimation()) {
To call out, I made a judgement call here (and same for effect node) wherein:
- we only generate an element id when we have an animation
- if the animation stops, we remove the element id from the node

Both are debatable. I figure it's easy to change later if we decide/discover
we'd like to do so. I'm not currently aware of a reason why as written is
unsuitable. Iteration expected down the road, as it were.

[wkorman, 2016-12-29 20:23:22]

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

[commit-bot: I haz the power, 2016-12-29 20:23:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-12-29 20:40:37]

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

[Xianzhu, 2016-12-29 20:40:38]

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380: if
(style.hasCurrentTransformAnimation()) {
On 2016/12/29 20:22:54, wkorman wrote:
> To call out, I made a judgement call here (and same for effect node) wherein:
> - we only generate an element id when we have an animation
> - if the animation stops, we remove the element id from the node
> 

Is there any extra cost for a paint property node having compositorElementId? If
not, can we just always set compositorElementId and never change it regardless
of animation?

> Both are debatable. I figure it's easy to change later if we decide/discover
> we'd like to do so. I'm not currently aware of a reason why as written is
> unsuitable. Iteration expected down the road, as it were.

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:429:
loadTestData("transform-animation.html");
I would prefer inlined html to separate test file because we don't need to open
another file to see what is tested, especially the file is in another directory.

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
(right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:163:
CompositorElementId m_compositorElementId;
Please also update toString().

[wkorman, 2016-12-29 20:59:15]

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380: if
(style.hasCurrentTransformAnimation()) {
On 2016/12/29 at 20:40:38, Xianzhu wrote:
> On 2016/12/29 20:22:54, wkorman wrote:
> > To call out, I made a judgement call here (and same for effect node)
wherein:
> > - we only generate an element id when we have an animation
> > - if the animation stops, we remove the element id from the node
> > 
> 
> Is there any extra cost for a paint property node having compositorElementId?
If not, can we just always set compositorElementId and never change it
regardless of animation?

Yes, we use DOMNodeIds::idForNode() which uses a WeakIdentifierMap, which in the
end
has a static int that it increments and also a map to ensure we produce the same
identifier for that node in the future. So the cost is the perf overhead of
doing
this plus memory in the map.

I wasn't involved, but chrishtr@ said there was debate in the past about whether
the overhead is material. For now I figured it was most expedient to preserve
current behavior and we could explore performance and potential code
simplification
later, but am open to changing approach, measuring now, etc.

This is what ajuma@ is referring to in http://crbug.com/674258#c1 (4). vollick@
change that added the element id to layers explicitly did so only for those
layers that had an animation due to concern re: the overhead.

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:429:
loadTestData("transform-animation.html");
On 2016/12/29 at 20:40:38, Xianzhu wrote:
> I would prefer inlined html to separate test file because we don't need to
open another file to see what is tested, especially the file is in another
directory.

But we already use similar test data fixture in other tests in this class? It
seems a huge benefit redundancy and maintenance wise. I just hadn't seen those
when I wrote the first batch of tests. I used this one file in 4 tests, for
example, and it allowed sharing the example between this test file and the
update tests.

Of course the benefit is debatable, if you feel strongly can switch back, if I
do so should I embed the full html in each test, or use a constant at least?

Ask chrishtr@ how he feels about embedded golden proto data in Maps unit tests,
as it's a similar situation. I am not actually sure which side of that debate he
falls on, would be interested to hear.

[wkorman, 2016-12-29 20:59:47]

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
(right):

https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:163:
CompositorElementId m_compositorElementId;
On 2016/12/29 at 20:40:38, Xianzhu wrote:
> Please also update toString().

Done.

[chrishtr, 2016-12-29 21:01:41]

On 2016/12/29 at 20:59:15, wkorman wrote:
>
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380: if
(style.hasCurrentTransformAnimation()) {
> On 2016/12/29 at 20:40:38, Xianzhu wrote:
> > On 2016/12/29 20:22:54, wkorman wrote:
> > > To call out, I made a judgement call here (and same for effect node)
wherein:
> > > - we only generate an element id when we have an animation
> > > - if the animation stops, we remove the element id from the node
> > > 
> > 
> > Is there any extra cost for a paint property node having
compositorElementId? If not, can we just always set compositorElementId and
never change it regardless of animation?
> 
> Yes, we use DOMNodeIds::idForNode() which uses a WeakIdentifierMap, which in
the end
> has a static int that it increments and also a map to ensure we produce the
same
> identifier for that node in the future. So the cost is the perf overhead of
doing
> this plus memory in the map.
> 
> I wasn't involved, but chrishtr@ said there was debate in the past about
whether
> the overhead is material. For now I figured it was most expedient to preserve
> current behavior and we could explore performance and potential code
simplification
> later, but am open to changing approach, measuring now, etc.
> 
> This is what ajuma@ is referring to in http://crbug.com/674258#c1 (4).
vollick@
> change that added the element id to layers explicitly did so only for those
> layers that had an animation due to concern re: the overhead.
> 
>
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):
> 
>
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:429:
loadTestData("transform-animation.html");
> On 2016/12/29 at 20:40:38, Xianzhu wrote:
> > I would prefer inlined html to separate test file because we don't need to
open another file to see what is tested, especially the file is in another
directory.
> 
> But we already use similar test data fixture in other tests in this class? It
seems a huge benefit redundancy and maintenance wise. I just hadn't seen those
when I wrote the first batch of tests. I used this one file in 4 tests, for
example, and it allowed sharing the example between this test file and the
update tests.
> 
> Of course the benefit is debatable, if you feel strongly can switch back, if I
do so should I embed the full html in each test, or use a constant at least?
> 
> Ask chrishtr@ how he feels about embedded golden proto data in Maps unit
tests, as it's a similar situation. I am not actually sure which side of that
debate he falls on, would be interested to hear.

I think inline is better, because of not having to open multiple files to figure
out what is going on.

If multiple tests use the same testcase, it starts to get more debatable - how
about a static const char* in the .cpp test file?

[wkorman, 2016-12-29 21:06:55]

On 2016/12/29 at 21:01:41, chrishtr wrote:
> On 2016/12/29 at 20:59:15, wkorman wrote:
> >
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> > 
> >
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380: if
(style.hasCurrentTransformAnimation()) {
> > On 2016/12/29 at 20:40:38, Xianzhu wrote:
> > > On 2016/12/29 20:22:54, wkorman wrote:
> > > > To call out, I made a judgement call here (and same for effect node)
wherein:
> > > > - we only generate an element id when we have an animation
> > > > - if the animation stops, we remove the element id from the node
> > > > 
> > > 
> > > Is there any extra cost for a paint property node having
compositorElementId? If not, can we just always set compositorElementId and
never change it regardless of animation?
> > 
> > Yes, we use DOMNodeIds::idForNode() which uses a WeakIdentifierMap, which in
the end
> > has a static int that it increments and also a map to ensure we produce the
same
> > identifier for that node in the future. So the cost is the perf overhead of
doing
> > this plus memory in the map.
> > 
> > I wasn't involved, but chrishtr@ said there was debate in the past about
whether
> > the overhead is material. For now I figured it was most expedient to
preserve
> > current behavior and we could explore performance and potential code
simplification
> > later, but am open to changing approach, measuring now, etc.
> > 
> > This is what ajuma@ is referring to in http://crbug.com/674258#c1 (4).
vollick@
> > change that added the element id to layers explicitly did so only for those
> > layers that had an animation due to concern re: the overhead.
> > 
> >
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):
> > 
> >
https://codereview.chromium.org/2608543002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:429:
loadTestData("transform-animation.html");
> > On 2016/12/29 at 20:40:38, Xianzhu wrote:
> > > I would prefer inlined html to separate test file because we don't need to
open another file to see what is tested, especially the file is in another
directory.
> > 
> > But we already use similar test data fixture in other tests in this class?
It seems a huge benefit redundancy and maintenance wise. I just hadn't seen
those when I wrote the first batch of tests. I used this one file in 4 tests,
for example, and it allowed sharing the example between this test file and the
update tests.
> > 
> > Of course the benefit is debatable, if you feel strongly can switch back, if
I do so should I embed the full html in each test, or use a constant at least?
> > 
> > Ask chrishtr@ how he feels about embedded golden proto data in Maps unit
tests, as it's a similar situation. I am not actually sure which side of that
debate he falls on, would be interested to hear.
> 
> I think inline is better, because of not having to open multiple files to
figure out what is going on.
> 
> If multiple tests use the same testcase, it starts to get more debatable - how
about a static const char* in the .cpp test file?

That's what I had previously (see diffs vs ToT), but the
PaintPropertyTreeUpdateTests couldn't share it without sticking it in the
header, so then it was duplicated between the two test files even though
UpdateTests are closely related and a subset. Plus we already have other tests
using the fixture data test files.

Personally I much prefer the test fixture data files, but if we'd like to move
away from them I'm happy to revise this to use const char* again, dupe across
this and UpdateTests as I did previously, and we should do similar for the other
tests separately.

[wkorman, 2016-12-29 21:13:42]

On 2016/12/29 at 21:06:55, wkorman wrote:
> Personally I much prefer the test fixture data files, but if we'd like to move
away from them I'm happy to revise this to use const char* again, dupe across
this and UpdateTests as I did previously, and we should do similar for the other
tests separately.

My primary pro-rationale:

- these are essentially integration tests, not really unit tests. Like layout
tests written in C++
- writing and formatting/maintaining html embedded in a C string is a totally
unpleasant experience
- makes it easier to share one test case across multiple test files as may be
relevant. I hope we can further factor apart the various test cases to other
sub-files in the future.

I can invent cons in addition to those folks have already mentioned, like:

- risk that what should be kept minimal/simple is easier to grow into a larger
test case through moving out of code
- risk that a large test case may be used across too many tests due to
expediency where a narrower more focused one might be better

[chrishtr, 2016-12-29 21:28:44]

On 2016/12/29 at 21:13:42, wkorman wrote:
> On 2016/12/29 at 21:06:55, wkorman wrote:
> > Personally I much prefer the test fixture data files, but if we'd like to
move away from them I'm happy to revise this to use const char* again, dupe
across this and UpdateTests as I did previously, and we should do similar for
the other tests separately.
> 
> My primary pro-rationale:
> 
> - these are essentially integration tests, not really unit tests. Like layout
tests written in C++
> - writing and formatting/maintaining html embedded in a C string is a totally
unpleasant experience
> - makes it easier to share one test case across multiple test files as may be
relevant. I hope we can further factor apart the various test cases to other
sub-files in the future.
> 
> I can invent cons in addition to those folks have already mentioned, like:
> 
> - risk that what should be kept minimal/simple is easier to grow into a larger
test case through moving out of code
> - risk that a large test case may be used across too many tests due to
expediency where a narrower more focused one might be better

Ah - if it's in two *Test.cpp files, then moving the test data to an auxiliary
file seems ok.

[Xianzhu, 2016-12-30 01:51:42]

lgtm.

I think for the two test files, the pros are bigger than the cons.

However, for test cases that C++ code has many dependencies to HTML and the HTML
test case only for one C++ test case, for example
  <div style="width: 77px; height: 88px"></div>
and C++ code checks size of the div equals to (77,88), I think inlined HTML is
better.

[wkorman, 2016-12-30 08:15:59]

On 2016/12/30 at 01:51:42, wangxianzhu wrote:
> lgtm.
> 
> I think for the two test files, the pros are bigger than the cons.
> 
> However, for test cases that C++ code has many dependencies to HTML and the
HTML test case only for one C++ test case, for example
>   <div style="width: 77px; height: 88px"></div>
> and C++ code checks size of the div equals to (77,88), I think inlined HTML is
better.

Agreed, sounds reasonable.

[wkorman, 2016-12-30 08:18:32]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-12-30 08:18:37]

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-12-30 08:20:07]

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

[commit-bot: I haz the power, 2016-12-30 08:20:09]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[wkorman, 2016-12-30 22:49:34]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-12-30 22:49:35]

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

[commit-bot: I haz the power, 2016-12-30 22:49:47]

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-12-30 22:54:55]

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

[commit-bot: I haz the power, 2016-12-30 22:54:56]

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

[wkorman, 2016-12-30 23:03:24]

chrishtr@ requesting OWNERS for platform/graphics/paint.

[wkorman, 2017-01-03 19:43:50]

wkorman@chromium.org changed reviewers:
+ pdr@chromium.org
- chrishtr@chromium.org

[wkorman, 2017-01-03 19:43:51]

+pdr for platform OWNERS

[pdr., 2017-01-03 21:29:04]

LGTM just two minor comments.

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:497:
CompositorElementId compositorElementId;
Could you do something like:
bool needsCompositorElementId = style.has...
CompositorElementId compositorElementId = needsCompositorElementId ?
createCompositorElementId(...) : 0;

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:851: void
PaintPropertyTreeBuilder::updateCompositorElementId(
The other update* functions follow a style where specific kinds of updates are
done. WDYT of just calling createCompositorElementId instead of adding this
function?

[wkorman, 2017-01-03 22:04:40]

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:497:
CompositorElementId compositorElementId;
On 2017/01/03 at 21:29:04, pdr. wrote:
> Could you do something like:
> bool needsCompositorElementId = style.has...
> CompositorElementId compositorElementId = needsCompositorElementId ?
createCompositorElementId(...) : 0;

Yeah, sort of -- CompositorElementId is a struct with two ints, so it's not just
a '0' and it seems heavyweight to jockey a pointer around. I used updateXxx()
with ref param attempting to avoid a retval copy, and followed existing method
naming pattern, but I may have misinterpreted what that pattern was going for.

Really I just wanted a method to share the redundant logic around using DOM node
id as the compositor element id. So I renamed the method to
createDomNodeBasedCompositorElementId() and had it return the generated object
rather than mutating input param. It means we'll be copying a
CompositorElementId for the retval but probably that is cheap.

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:851: void
PaintPropertyTreeBuilder::updateCompositorElementId(
On 2017/01/03 at 21:29:04, pdr. wrote:
> The other update* functions follow a style where specific kinds of updates are
done. WDYT of just calling createCompositorElementId instead of adding this
function?

See above comment.

[wkorman, 2017-01-03 22:06:30]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-01-03 22:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2608543002/#ps80001
(title: "Feedback.")

[commit-bot: I haz the power, 2017-01-03 22:07:05]

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

[wkorman, 2017-01-03 22:08:10]

The CQ bit was unchecked by wkorman@chromium.org

[wkorman, 2017-01-03 22:15:25]

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2608543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:851: void
PaintPropertyTreeBuilder::updateCompositorElementId(
On 2017/01/03 at 22:04:40, wkorman wrote:
> On 2017/01/03 at 21:29:04, pdr. wrote:
> > The other update* functions follow a style where specific kinds of updates
are done. WDYT of just calling createCompositorElementId instead of adding this
function?
> 
> See above comment.

Moved to anonymous namespace and removed method from class definition.

[wkorman, 2017-01-03 22:15:31]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-01-03 22:15:31]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2608543002/#ps100001
(title: "Hide create-dom-node method.")

[commit-bot: I haz the power, 2017-01-03 22:16:09]

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-03 23:55:36]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483481731384990,
"parent_rev": "e6828d027b6b282f6f2be3908225dae29c0174d0", "commit_rev":
"3b8d3406f9c05099b918a05408d79ac4f39c4ed3"}

[commit-bot: I haz the power, 2017-01-03 23:56:05]

Description was changed from

==========
Store compositor element id in paint properties for animated objects.

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

to

==========
Store compositor element id in paint properties for animated objects.

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

Review-Url: https://codereview.chromium.org/2608543002
==========

[commit-bot: I haz the power, 2017-01-03 23:56:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2017-01-03 23:57:41]

Description was changed from

==========
Store compositor element id in paint properties for animated objects.

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

Review-Url: https://codereview.chromium.org/2608543002
==========

to

==========
Store compositor element id in paint properties for animated objects.

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

Committed: https://crrev.com/8a4ccdb5a2bef0e128a64cea83fa9de3daf785a8
Cr-Commit-Position: refs/heads/master@{#441255}
==========

[commit-bot: I haz the power, 2017-01-03 23:57:42]

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

[loyso (OOO), 2017-01-05 03:50:43]

On 2016/12/29 at 20:59:15, wkorman wrote:
> Yes, we use DOMNodeIds::idForNode() which uses a WeakIdentifierMap, which in
the end
> has a static int that it increments and also a map to ensure we produce the
same
> identifier for that node in the future. So the cost is the perf overhead of
doing
> this plus memory in the map.
> 
> I wasn't involved, but chrishtr@ said there was debate in the past about
whether
> the overhead is material. For now I figured it was most expedient to preserve
> current behavior and we could explore performance and potential code
simplification
> later, but am open to changing approach, measuring now, etc.
esprehn@ insisted on DOMNodeids usage.
See comments in prototype CLs:
https://codereview.chromium.org/1944623002
https://codereview.chromium.org/1921503008
An alternate solution was to always assign some unique 32 bit id with optional
reverse mapping (id->blink::Element*), given that some subsystems run composited
animations
on layers which have no source blink::Node (Android Link Highlights and
Programmatic scroll animator)

[wkorman, 2017-01-05 20:28:49]

On 2017/01/05 at 03:50:43, loyso wrote:
> esprehn@ insisted on DOMNodeids usage.
> See comments in prototype CLs:
> https://codereview.chromium.org/1944623002
> https://codereview.chromium.org/1921503008
> An alternate solution was to always assign some unique 32 bit id with optional
reverse mapping (id->blink::Element*), given that some subsystems run composited
animations
> on layers which have no source blink::Node (Android Link Highlights and
Programmatic scroll animator)

Ah, thanks for the change references, I hadn't seen those. What do we use today
for CompositorElementId for link highlights and programmatic scroll? In both
cases it seems like there should be some blink::Node that I'd think we could
logically consider canonical for that case (the anchor link, or the scrollable
view, for example).