compositor-worker: plumb element id and mutable properties

compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/933f496ec9750765871f21d74a3c48eb23c5eb86
Cr-Commit-Position: refs/heads/master@{#363737}

[Ian Vollick, 2015-11-08 02:08:58]

vollick@chromium.org changed reviewers:
+ danakj@chromium.org, esprehn@chromium.org

[danakj, 2015-11-09 19:20:15]

Description was changed from

==========
compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2015-11-09 19:20:15]

danakj@chromium.org changed reviewers:
+ enne@chromium.org
- danakj@chromium.org

[danakj, 2015-11-09 19:20:29]

-dana +enne

[enne (OOO), 2015-11-10 00:11:41]

https://codereview.chromium.org/1405993008/diff/20001/cc/animation/mutable_pr...
File cc/animation/mutable_properties.h (right):

https://codereview.chromium.org/1405993008/diff/20001/cc/animation/mutable_pr...
cc/animation/mutable_properties.h:10: enum MutableProperty {
Kind of sad this has to be duplicated in platform.  Wasn't there some precedence
for just including enums across repo boundaries?

https://codereview.chromium.org/1405993008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:199:
style.setWillChangeProperties(properties);
This is what guarantees that having a compositor proxy creates a graphics layer?

[Ian Vollick, 2015-11-10 20:30:14]

https://codereview.chromium.org/1405993008/diff/20001/cc/animation/mutable_pr...
File cc/animation/mutable_properties.h (right):

https://codereview.chromium.org/1405993008/diff/20001/cc/animation/mutable_pr...
cc/animation/mutable_properties.h:10: enum MutableProperty {
On 2015/11/10 00:11:41, enne wrote:
> Kind of sad this has to be duplicated in platform.  Wasn't there some
precedence
> for just including enums across repo boundaries?

Agree that this is sad. I couldn't find a nice way to include a common header in
both core/ and cc/, but this will hopefully change in response to esprehn's
ideas. I've added a TODO and added some missing static asserts for now.

https://codereview.chromium.org/1405993008/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:199:
style.setWillChangeProperties(properties);
On 2015/11/10 00:11:41, enne wrote:
> This is what guarantees that having a compositor proxy creates a graphics
layer?

Yep, that's right.

[enne (OOO), 2015-11-10 20:34:25]

Thanks!

cc lgtm

[Ian Vollick, 2015-11-10 22:27:07]

vollick@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Ian Vollick, 2015-11-10 22:27:37]

On 2015/11/10 20:34:25, enne wrote:
> Thanks!
> 
> cc lgtm

thanks :)

esprehn, ping.

+chrishtr in case you're free to check out this plumbing.

[esprehn, 2015-11-11 01:08:31]

Yeah I'm looking.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2015-11-11 01:16:09]

Yeah I'm looking.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ian Vollick, 2015-11-11 01:36:33]

https://codereview.chromium.org/1405993008/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:199:
style.setWillChangeProperties(properties);
trchen@ just pointed out that this is wrong. It will cause an unnecessary
stacking context for scrollers.

I was going to write a follow up CL so that main can keep track of proxied
properties, but I'd be happy to roll it into this CL.

[trchen, 2015-11-11 22:50:36]

On 2015/11/11 01:36:33, vollick wrote:
>
https://codereview.chromium.org/1405993008/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/1405993008/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:199:
> style.setWillChangeProperties(properties);
> trchen@ just pointed out that this is wrong. It will cause an unnecessary
> stacking context for scrollers.
> 
> I was going to write a follow up CL so that main can keep track of proxied
> properties, but I'd be happy to roll it into this CL.

By the way what are you going to do with stuffs that has display:none ancestors?
(I know, it is a boring corner case.) Although we don't need to do any real
animation but you probably still want to send back property mutations from the
worker.

Another corner case I just think of is that inline elements may be composited,
but they don't respect transforms. Are you going to implement the same checks as
StyleAdjuster in the proxy to make sure inapplicable properties forced to
auto/none?

[Ian Vollick, 2015-11-13 02:54:15]

Elliott, I've added code to track which properties are proxied on the main
thread, so I think this is ready for review.

On 2015/11/11 22:50:36, trchen wrote:
> On 2015/11/11 01:36:33, vollick wrote:
> >
>
https://codereview.chromium.org/1405993008/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1405993008/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:199:
> > style.setWillChangeProperties(properties);
> > trchen@ just pointed out that this is wrong. It will cause an unnecessary
> > stacking context for scrollers.
> > 
> > I was going to write a follow up CL so that main can keep track of proxied
> > properties, but I'd be happy to roll it into this CL.
> 
> By the way what are you going to do with stuffs that has display:none
ancestors?
> (I know, it is a boring corner case.) Although we don't need to do any real
> animation but you probably still want to send back property mutations from the
> worker.

This is interesting though I think related to the overall design overall, not
this particular patch.

The design should be able to accommodate either operating on or ignoring the
display:none'd stuff, but I think we should be consistent with CSS animations.
These update the computed style and the effects aren't observable for stuff
that's display:none.

> 
> Another corner case I just think of is that inline elements may be composited,
> but they don't respect transforms. Are you going to implement the same checks
as
> StyleAdjuster in the proxy to make sure inapplicable properties forced to
> auto/none?

I was going to follow the lead of will-change and apply naively even if there
would be no effect.

[Ian Vollick, 2015-11-17 20:07:07]

esprehn, I've renamed the stuff in the blink namespace from WebMutable* to
WebCompositorMutable* as we'd discussed. PTAL.

[ajuma, 2015-11-20 21:55:26]

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

[ajuma, 2015-11-20 21:55:27]

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:191:
style.setWillChangeScrollPosition(true);
will-change:scroll-position doesn't currently create a composited layer (or do
anything really), so you'll need to change
ComputeStyle::hasWillChangeCompositedHint. Also, will-change doesn't directly
force a PaintLayer (unless the property being will-changed is one that creates a
stacking context, in which case the computed z-index gets set to 0 which forces
a PaintLayer), but I guess if the thing is really overflow:scroll you'll already
have one.

[Ian Vollick, 2015-11-23 20:25:55]

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:191:
style.setWillChangeScrollPosition(true);
On 2015/11/20 21:55:27, ajuma wrote:
> will-change:scroll-position doesn't currently create a composited layer (or do
> anything really), so you'll need to change
> ComputeStyle::hasWillChangeCompositedHint. Also, will-change doesn't directly
> force a PaintLayer (unless the property being will-changed is one that creates
a
> stacking context, in which case the computed z-index gets set to 0 which
forces
> a PaintLayer), but I guess if the thing is really overflow:scroll you'll
already
> have one.

Ah, whoops. Yes, the composited layer _doesn't_ actually depend on will-chang.
That was handled in a previous CL
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

[chrishtr, 2015-11-24 16:22:50]

https://codereview.chromium.org/1405993008/diff/240001/cc/animation/mutable_p...
File cc/animation/mutable_properties.h (right):

https://codereview.chromium.org/1405993008/diff/240001/cc/animation/mutable_p...
cc/animation/mutable_properties.h:16: kMutablePropertyTransformRelated =
kMutablePropertyTransform |
Unused?

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:211:
doWriteUint32(compositorProxy.mutableProperties());
Why mutable?

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:185: if (element
&& element->hasCompositorProxy()) {
Factor into another method, this one is getting too long.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:191:
style.setWillChangeScrollPosition(true);
On 2015/11/23 at 20:25:54, vollick wrote:
> On 2015/11/20 21:55:27, ajuma wrote:
> > will-change:scroll-position doesn't currently create a composited layer (or
do
> > anything really), so you'll need to change
> > ComputeStyle::hasWillChangeCompositedHint. Also, will-change doesn't
directly
> > force a PaintLayer (unless the property being will-changed is one that
creates a
> > stacking context, in which case the computed z-index gets set to 0 which
forces
> > a PaintLayer), but I guess if the thing is really overflow:scroll you'll
already
> > have one.
> 
> Ah, whoops. Yes, the composited layer _doesn't_ actually depend on will-chang.
That was handled in a previous CL
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

Was Ali's comment addressed? Not sure how to read your response.

Also, doesn't setting values in this function cause a developer-visible side
effect (will-change: scroll-position in ComputedStyle) even if CompositorWorker
is not web-exposed?

Please also unittest this function.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:986:
setNeedsStyleRecalc(LocalStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::CompositorProxy));
Incrementing a count can cause a style recalc? That seems unexpected from the
name.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.h:535: void
incrementProxiedPropertyCounts(uint32_t mutableProperties);
Please document what this is doing.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1532:
m_graphicsLayer->setElementId(elementId);
Can an element which is indirectly composited end up with a CW? If so you need
to add CW as a reason to prevent squashing.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:396:
if (RuntimeEnabledFeatures::compositorWorkerEnabled() && m_scrollLayer &&
!m_layoutView.document().childNeedsStyleRecalc()) {
What is actually happening? Let's squash this bug now.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebCompositorMutableProperties.h
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebCompositorMutableProperties.h:10: //
TODO(vollick): we should not need a parallel enum.
Add a note that this has to be kept in sync with the other enum.

[Ian Vollick, 2015-11-25 18:28:45]

Thanks for the review!

https://codereview.chromium.org/1405993008/diff/240001/cc/animation/mutable_p...
File cc/animation/mutable_properties.h (right):

https://codereview.chromium.org/1405993008/diff/240001/cc/animation/mutable_p...
cc/animation/mutable_properties.h:16: kMutablePropertyTransformRelated =
kMutablePropertyTransform |
On 2015/11/24 16:22:50, chrishtr wrote:
> Unused?

Yep. It gets used in a patch later in the series. I've removed it from this
patch and will add it back in the patch that actually uses it.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptValueSerializer.cpp:211:
doWriteUint32(compositorProxy.mutableProperties());
On 2015/11/24 16:22:50, chrishtr wrote:
> Why mutable?

Actually, this got missed in my rename. It should have been
compositorMutableProperties. I.e., it's compositor-mutable; a property that can
be mutated by the compositor. On the cc side we have the concept of a "mutator"
that gets a crack at updating the compositor's state each impl frame and these
are the properties that one of those mutators may update.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:185: if (element
&& element->hasCompositorProxy()) {
On 2015/11/24 16:22:50, chrishtr wrote:
> Factor into another method, this one is getting too long.

Done.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:191:
style.setWillChangeScrollPosition(true);
On 2015/11/24 16:22:50, chrishtr wrote:
> On 2015/11/23 at 20:25:54, vollick wrote:
> > On 2015/11/20 21:55:27, ajuma wrote:
> > > will-change:scroll-position doesn't currently create a composited layer
(or
> do
> > > anything really), so you'll need to change
> > > ComputeStyle::hasWillChangeCompositedHint. Also, will-change doesn't
> directly
> > > force a PaintLayer (unless the property being will-changed is one that
> creates a
> > > stacking context, in which case the computed z-index gets set to 0 which
> forces
> > > a PaintLayer), but I guess if the thing is really overflow:scroll you'll
> already
> > > have one.
> > 
> > Ah, whoops. Yes, the composited layer _doesn't_ actually depend on
will-chang.
> That was handled in a previous CL
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> 
> Was Ali's comment addressed? Not sure how to read your response.
> 
> Also, doesn't setting values in this function cause a developer-visible side
> effect (will-change: scroll-position in ComputedStyle) even if
CompositorWorker
> is not web-exposed?
> 
> Please also unittest this function.

I was trying to say that will-change does not play a role in getting proxied
elements promoted. I.e., will-change is not required for the code to work.

I'd wanted proxies to imply will change because having a proxy pointed at a
property truly does mean that you will-change it. It would be helpful for the
engine to know this in case it wanted to make optimizations.

That said, you're right that this behavior will be developer visible when we
implement snap and sticky in terms of compositor worker. Since this will-change
business is really just an optimization for when (and if) CW is web exposed,
I've removed it from this patch.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:986:
setNeedsStyleRecalc(LocalStyleChange,
StyleChangeReasonForTracing::create(StyleChangeReason::CompositorProxy));
On 2015/11/24 16:22:50, chrishtr wrote:
> Incrementing a count can cause a style recalc? That seems unexpected from the
> name.

Yes, it's like saying that you "will change" a property (although it will not
actually do this, as I've mentioned in an earlier comment) and that dirtiness
affects what gets a paint layer and what gets composited.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.h (right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.h:535: void
incrementProxiedPropertyCounts(uint32_t mutableProperties);
On 2015/11/24 16:22:50, chrishtr wrote:
> Please document what this is doing.

Done.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1532:
m_graphicsLayer->setElementId(elementId);
On 2015/11/24 16:22:50, chrishtr wrote:
> Can an element which is indirectly composited end up with a CW? If so you need
> to add CW as a reason to prevent squashing.

Nope, a CompositorProxy is a direct compositing reason.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:396:
if (RuntimeEnabledFeatures::compositorWorkerEnabled() && m_scrollLayer &&
!m_layoutView.document().childNeedsStyleRecalc()) {
On 2015/11/24 16:22:50, chrishtr wrote:
> What is actually happening? Let's squash this bug now.

This is an old, unrelated video bug
(https://code.google.com/p/chromium/issues/detail?id=372245).
LayoutTextTrackContainerElement changes style information during layout. The
consequence is that when we ask for the scrollingElement after layout, it
sometimes complains in debug mode due to an assertion about dirty style. IIUC,
fixing this properly involves getting a w3c spec change made related to video
height/width.

I've changed my approach in the latest patch set. Rather than putting the
workaround in shipping code here, I've exposed the compositor worker feature via
InternalSettings so that I can disable it only for the two video layout tests
that invoke this style-update-during-layout behavior, tests that are closely
related to the linked bug.

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebCompositorMutableProperties.h
(right):

https://codereview.chromium.org/1405993008/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebCompositorMutableProperties.h:10: //
TODO(vollick): we should not need a parallel enum.
On 2015/11/24 16:22:50, chrishtr wrote:
> Add a note that this has to be kept in sync with the other enum.

Done.

[ajuma, 2015-11-25 19:40:09]

lgtm

[Ian Vollick, 2015-12-01 22:15:25]

On 2015/11/25 19:40:09, ajuma wrote:
> lgtm

chrishtr, ping.

[chrishtr, 2015-12-01 23:17:03]

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:395:
if (RuntimeEnabledFeatures::compositorWorkerEnabled() && m_scrollLayer) {
Is there a testcase for the code here?

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:37: // OSX attaches
main frame scrollbars to the VisualViewport so the VisualViewport layers need
Use the mock theme?

[Ian Vollick, 2015-12-02 00:33:20]

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

[commit-bot: I haz the power, 2015-12-02 00:37:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405993008/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405993008/340001

[Ian Vollick, 2015-12-02 00:39:33]

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:395:
if (RuntimeEnabledFeatures::compositorWorkerEnabled() && m_scrollLayer) {
On 2015/12/01 23:17:03, chrishtr wrote:
> Is there a testcase for the code here?

Yes. In CompositorWorkerTest, I test three types of CompositorProxy.
 1. A proxy of a typical div (for opacity and transform).
 2. A proxy of an overflow:scrolling div (for scrollLeft and scrollTop), and
 3. A proxy of document.scrollingElement (for scrollLeft and scrollTop).

This code sets up the elementId and mutableProperties for the scrolling element
(#3 above) and the tests check that we're getting the right values. If you
search for rootScrollLayer in CompositorWorkerTest.cpp you'll see the relevant
lines.

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):

https://codereview.chromium.org/1405993008/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:37: // OSX attaches
main frame scrollbars to the VisualViewport so the VisualViewport layers need
On 2015/12/01 23:17:03, chrishtr wrote:
> Use the mock theme?

These lines were copied from ScrollingCoordinatorTest and aren't actually needed
here. Removed.

[chrishtr, 2015-12-02 00:49:26]

lgtm

[commit-bot: I haz the power, 2015-12-02 02:27:38]

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

[commit-bot: I haz the power, 2015-12-02 02:27:39]

Dry run: This issue passed the CQ dry run.

[Ian Vollick, 2015-12-02 02:38:39]

vollick@chromium.org changed reviewers:
+ haraken@chromium.org

[Ian Vollick, 2015-12-02 02:39:10]

On 2015/12/02 02:27:39, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

+haraken for bindings/ and web/

[esprehn, 2015-12-02 03:28:47]

esprehn@chromium.org changed reviewers:
- haraken@chromium.org

[esprehn, 2015-12-02 03:28:49]

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/track/track-delete-during-setup.html
(right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/track/track-delete-during-setup.html:10:
window.internals.settings.setCompositorWorkerEnabled(false);
sigh, this is really awful. We need to fix the busted track feature

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:23:
WebCompositorMutableProperty compositorMutableProperty;
I would just call this "property"

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:38: String
propertyName(mapping.name, mapping.length);
this mallocs a string repeatedly as you loop, don't do that. Do std::lower_bound
on the bytes of the attributeLower itself.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:91: uint32_t
compositorMutableProperties = 0;
you can name your local vars a lot shorter if you want, like this can just be
|properties|

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:983: const bool hadProxy =
hasCompositorProxy();
no const on local primitive values

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:984:
ensureElementRareData().incrementProxiedPropertyCounts(mutableProperties);
you'd normally write this the other way, you'd write:

ElementRareData& rareData = ensureElementRareData();
if (!rareData.proxiedPropertyCounts())
  setNeedsStyleRecalc(...);
rareData.incrementProxiedPropertyCounts(mutableProperties);

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:992: const bool hadProxy =
hasCompositorProxy();
remove const, but if you're decrementing you must have had a proxy to start, so
this will always be true...

Note no ASSERT is needed, elementRareData() has the assert inside it, we should
really make it return a reference. You want:

ElementRareData& rareData = *elementRareData();
rareData.decrementProxiedPropertyCounts(mutableProperties);
if (!rareData.proxiedPropertyCounts())
  setNeedsStyleRecalc(...);

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ProxiedPropertyCounts.h (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ProxiedPropertyCounts.h:14: class
ProxiedPropertyCounts final : public
NoBaseWillBeGarbageCollectedFinalized<ProxiedPropertyCounts> {
CompositorProxiedProperties or PropertySet since this is a set? again this is
too generic in name

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ProxiedPropertyCounts.h:23: bool empty()
const;
isEmpty()

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:396:
if (Element* scrollingElement = m_layoutView.document().scrollingElement()) {
why do we need special treatment for the document level scrolling element?
shouldn't this be generic to all scrolling elements?

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1388: Node*
node = layer->enclosingElement();
heh, enclosingElement needs to be renamed enclosingNode I guess? this code looks
busted since you assign to a Node*, then check isElementNode

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:72: std::string
m_baseURL;
String

[haraken, 2015-12-02 04:04:13]

bindings/ and web/ LGTM

[Ian Vollick, 2015-12-03 17:54:43]

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/track/track-delete-during-setup.html
(right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/track/track-delete-during-setup.html:10:
window.internals.settings.setCompositorWorkerEnabled(false);
On 2015/12/02 03:28:48, esprehn wrote:
> sigh, this is really awful. We need to fix the busted track feature

Agreed.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:38: String
propertyName(mapping.name, mapping.length);
On 2015/12/02 03:28:48, esprehn wrote:
> this mallocs a string repeatedly as you loop, don't do that. Do
std::lower_bound
> on the bytes of the attributeLower itself.

Done. There was a bug here (crbug.com/531577). I've added a fix to the
lower_bound version and added a test to cover it.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:91: uint32_t
compositorMutableProperties = 0;
On 2015/12/02 03:28:48, esprehn wrote:
> you can name your local vars a lot shorter if you want, like this can just be
> |properties|

Done.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:983: const bool hadProxy =
hasCompositorProxy();
On 2015/12/02 03:28:49, esprehn wrote:
> no const on local primitive values

Done.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:984:
ensureElementRareData().incrementProxiedPropertyCounts(mutableProperties);
On 2015/12/02 03:28:48, esprehn wrote:
> you'd normally write this the other way, you'd write:
> 
> ElementRareData& rareData = ensureElementRareData();
> if (!rareData.proxiedPropertyCounts())
>   setNeedsStyleRecalc(...);
> rareData.incrementProxiedPropertyCounts(mutableProperties);

That looks much better. Done.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:992: const bool hadProxy =
hasCompositorProxy();
On 2015/12/02 03:28:49, esprehn wrote:
> remove const, but if you're decrementing you must have had a proxy to start,
so
> this will always be true...
> 
> Note no ASSERT is needed, elementRareData() has the assert inside it, we
should
> really make it return a reference. You want:
> 
> ElementRareData& rareData = *elementRareData();
> rareData.decrementProxiedPropertyCounts(mutableProperties);
> if (!rareData.proxiedPropertyCounts())
>   setNeedsStyleRecalc(...);

Done.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ProxiedPropertyCounts.h (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ProxiedPropertyCounts.h:14: class
ProxiedPropertyCounts final : public
NoBaseWillBeGarbageCollectedFinalized<ProxiedPropertyCounts> {
On 2015/12/02 03:28:49, esprehn wrote:
> CompositorProxiedProperties or PropertySet since this is a set? again this is
> too generic in name

I went with CompositorProxiedPropertySet. wdyt?

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ProxiedPropertyCounts.h:23: bool empty()
const;
On 2015/12/02 03:28:49, esprehn wrote:
> isEmpty()

Done.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:396:
if (Element* scrollingElement = m_layoutView.document().scrollingElement()) {
On 2015/12/02 03:28:49, esprehn wrote:
> why do we need special treatment for the document level scrolling element?
> shouldn't this be generic to all scrolling elements?

Once we have skobes' root layer scrolling, this should be unnecessary. As it
stands, overflow scrolling and frame scrolling are treated differently today.

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1388: Node*
node = layer->enclosingElement();
On 2015/12/02 03:28:49, esprehn wrote:
> heh, enclosingElement needs to be renamed enclosingNode I guess? this code
looks
> busted since you assign to a Node*, then check isElementNode

Sounds good. Here's a cleanup cl for that:
https://codereview.chromium.org/1495053002

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:72: std::string
m_baseURL;
On 2015/12/02 03:28:49, esprehn wrote:
> String

Done.

[Ian Vollick, 2015-12-03 22:33:02]

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1388: Node*
node = layer->enclosingElement();
On 2015/12/03 17:54:43, vollick wrote:
> On 2015/12/02 03:28:49, esprehn wrote:
> > heh, enclosingElement needs to be renamed enclosingNode I guess? this code
> looks
> > busted since you assign to a Node*, then check isElementNode
> 
> Sounds good. Here's a cleanup cl for that:
> https://codereview.chromium.org/1495053002

enclosingNode been landed and I've rebased this change. It looks less silly :)

[Ian Vollick, 2015-12-04 15:29:55]

On 2015/12/03 22:33:02, vollick wrote:
>
https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):
> 
>
https://codereview.chromium.org/1405993008/diff/340001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1388: Node*
> node = layer->enclosingElement();
> On 2015/12/03 17:54:43, vollick wrote:
> > On 2015/12/02 03:28:49, esprehn wrote:
> > > heh, enclosingElement needs to be renamed enclosingNode I guess? this code
> > looks
> > > busted since you assign to a Node*, then check isElementNode
> > 
> > Sounds good. Here's a cleanup cl for that:
> > https://codereview.chromium.org/1495053002
> 
> enclosingNode been landed and I've rebased this change. It looks less silly :)

Elliott, ping.

[esprehn, 2015-12-07 20:54:23]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org

[esprehn, 2015-12-07 20:54:24]

lgtm

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html
(right):

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html:3:
-->
remove comment?

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:35: static bool
ComparePropertyName(const MutablePropertyMapping& mapping, StringImpl*
propertyLower)
comparePropertyName

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1000: if (!hasCompositorProxy())
this technically is a few extra branches and function calls, I doubt it matters.
the most optimal would be:

if (!hasRareData())
  return WebCompositorMutablePropertyNone;
if (CompositorProxiedPropertySet* set =
elementRareData()->proxiedPropertyCounts())
  return set->proxiedProperties();
return WebCompositorMutablePropertyNone;

but you don't need to do that. :)

[Ian Vollick, 2015-12-08 00:16:44]

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html
(right):

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html:3:
-->
On 2015/12/07 20:54:24, esprehn wrote:
> remove comment?

Whoops, done.

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/CompositorProxy.cpp (right):

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/CompositorProxy.cpp:35: static bool
ComparePropertyName(const MutablePropertyMapping& mapping, StringImpl*
propertyLower)
On 2015/12/07 20:54:24, esprehn wrote:
> comparePropertyName

Done.

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1405993008/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:1000: if (!hasCompositorProxy())
On 2015/12/07 20:54:24, esprehn wrote:
> this technically is a few extra branches and function calls, I doubt it
matters.
> the most optimal would be:
> 
> if (!hasRareData())
>   return WebCompositorMutablePropertyNone;
> if (CompositorProxiedPropertySet* set =
> elementRareData()->proxiedPropertyCounts())
>   return set->proxiedProperties();
> return WebCompositorMutablePropertyNone;
> 
> but you don't need to do that. :)

I did it!

[Ian Vollick, 2015-12-08 00:16:52]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-12-08 00:16:53]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
ajuma@chromium.org, haraken@chromium.org, chrishtr@chromium.org,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1405993008/#ps440001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-08 00:21:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405993008/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405993008/440001

[commit-bot: I haz the power, 2015-12-08 00:25:29]

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

[commit-bot: I haz the power, 2015-12-08 00:25:31]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ian Vollick, 2015-12-08 03:12:38]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-12-08 03:12:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, ajuma@chromium.org, esprehn@chromium.org,
chrishtr@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1405993008/#ps460001
(title: "fix rebase error.")

[commit-bot: I haz the power, 2015-12-08 03:13:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405993008/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405993008/460001

[commit-bot: I haz the power, 2015-12-08 04:22:12]

Description was changed from

==========
compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-12-08 04:22:13]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2015-12-08 04:23:28]

Description was changed from

==========
compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
compositor-worker: plumb element id and mutable properties

For proxied elements, this CL ensures that the element id and mutable
property bitfield are propagated to the compositor.

BUG=430155
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/933f496ec9750765871f21d74a3c48eb23c5eb86
Cr-Commit-Position: refs/heads/master@{#363737}
==========

[commit-bot: I haz the power, 2015-12-08 04:23:29]

Patchset 24 (id:??) landed as
https://crrev.com/933f496ec9750765871f21d74a3c48eb23c5eb86
Cr-Commit-Position: refs/heads/master@{#363737}