Clean up the way layout tests force elements to opt in/out of composited scrolling.

Clean up the way layout tests force elements to opt in/out of composited scrolling.

Currently, we're toggling a global setting, but we should really be affecting
the opt in decision for individual elements. This patch cleans that up.

This is part of a larger plan to finish the opt-in checks. Here's the aggregate patch (it's getting stale, so YMMV): https://codereview.chromium.org/14741004/

The reason this patch is necessary, is that in subsequent patches, we will be reducing the number of times we update composited scrolling state; explicitly forcing a particular layer (and only one layer) to opt in or out will allow us to guarantee that the layer's state (in particular, its z-order lists) will be updated, even if we've optimized/reduced the frequency with which we update the composited scrolling state.

R=hartmanng@chromium.org, jchaffraix@chromium.org
BUG=None

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151433

[hartmanng, 2013-05-02 15:29:35]

lgtm

[Julien - ping for review, 2013-05-06 22:56:58]

Just one big issue (about who enabled accelerated overflow scrolling). The rest
looks fine.

https://codereview.chromium.org/14858004/diff/2001/LayoutTests/compositing/ov...
File LayoutTests/compositing/overflow/build-paint-order-lists.html (right):

https://codereview.chromium.org/14858004/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/build-paint-order-lists.html:106:
window.internals.setNeedsCompositedScrolling(container,
AFAICT no one is calling setAcceleratedCompositingForOverflowScrollEnable to
ensure that the feature is enabled now.

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:2063: 
ASSERT_NOT_REACHED()

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.h:1141: ForceNeedsCompositedScrollingMode
m_forceNeedsCompositedScrolling;
By mixing 2 different bitfield types here, you *will* make RenderLayer bigger on
Windows by preventing any bit packing.

We don't track memory consumption on RenderLayer as we do for other objects but
it would still be nice if you moved that after the bitfields (ideally, we should
move everything to unsigned to ensure proper padding)

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
Source/core/testing/Internals.h:232: };
I don't think we need this enum as you don't poke into the |unsigned| returned
from JavaScript.

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
Source/core/testing/Internals.idl:182: const unsigned short
ForceCompositedScrollingOff = 2;
These values need to be in sync with RenderLayer so it would need a comment.

[Ian Vollick, 2013-05-08 13:10:58]

Thanks for the review! Comments have been addressed, PTAL.

https://codereview.chromium.org/14858004/diff/2001/LayoutTests/compositing/ov...
File LayoutTests/compositing/overflow/build-paint-order-lists.html (right):

https://codereview.chromium.org/14858004/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/build-paint-order-lists.html:106:
window.internals.setNeedsCompositedScrolling(container,
On 2013/05/06 22:56:59, Julien Chaffraix wrote:
> AFAICT no one is calling setAcceleratedCompositingForOverflowScrollEnable to
> ensure that the feature is enabled now.
I think that the machinery coincidentally gets kicked into motion with this
setNeedsCompositedScrolling call (the setting is mostly used to determine if we
should waste resources computing the opt-in state) but it's very brittle. If any
bit of new logic gets added that looks at the setting, this test would break.
I've updated the test to turn the setting on.

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:2063: 
On 2013/05/06 22:56:59, Julien Chaffraix wrote:
> ASSERT_NOT_REACHED()

Done.

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.h:1141: ForceNeedsCompositedScrollingMode
m_forceNeedsCompositedScrolling;
On 2013/05/06 22:56:59, Julien Chaffraix wrote:
> By mixing 2 different bitfield types here, you *will* make RenderLayer bigger
on
> Windows by preventing any bit packing.
> 
> We don't track memory consumption on RenderLayer as we do for other objects
but
> it would still be nice if you moved that after the bitfields (ideally, we
should
> move everything to unsigned to ensure proper padding)
Thank you for catching this! Noted and fixed.

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
File Source/core/testing/Internals.h (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
Source/core/testing/Internals.h:232: };
On 2013/05/06 22:56:59, Julien Chaffraix wrote:
> I don't think we need this enum as you don't poke into the |unsigned| returned
> from JavaScript.
I didn't think so either, but it seems to be used when setting up the v8
bindings. It looks like it tries to find some c++ entity with the same name as
the constants in the .idl file to bind to.

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/14858004/diff/2001/Source/core/testing/Intern...
Source/core/testing/Internals.idl:182: const unsigned short
ForceCompositedScrollingOff = 2;
On 2013/05/06 22:56:59, Julien Chaffraix wrote:
> These values need to be in sync with RenderLayer so it would need a comment.

Done.

[Julien - ping for review, 2013-05-09 22:34:34]

Sorry, I should have caught that in a previous review: you can make the Settings
(setAcceleratedCompositingForOverflowScrollEnabled) an enum fairly easily. I am
concerned about having the information about whether we enable
accelerated-scrolling now spread in 2 places (RenderLayer and Settings).

https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2113: m_forceNeedsCompositedScrolling =
mode;
Shouldn't we check:

if (mode == m_forceNeedsCompositedScrolling)
    return;

https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:833: ForceCompositedScrollingOff
Maybe better names for the 2 arguments:
* CompositedScrollingAlwaysEnabled
* CompositedScrollingAlwaysDisabled

[Ian Vollick, 2013-05-10 15:40:00]

On 2013/05/09 22:34:34, Julien Chaffraix wrote:
> Sorry, I should have caught that in a previous review: you can make the
Settings
> (setAcceleratedCompositingForOverflowScrollEnabled) an enum fairly easily. I
am
> concerned about having the information about whether we enable
> accelerated-scrolling now spread in 2 places (RenderLayer and Settings).
I think we do need both since they mean different things. The value in Settings
enables or disables the feature globally, but the setting in RenderLayer lets us
force a particular layer to opt in/or out. In
build-paint-order-list-where-opt-in-decisions-can-each-other.html, we want to
keep the global setting enabled, but force one particular layer to opt in or out
(and see how the other layer responds), but we wouldn't have this control with
one, global setting.
> 
>
https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:2113: m_forceNeedsCompositedScrolling =
> mode;
> Shouldn't we check:
> 
> if (mode == m_forceNeedsCompositedScrolling)
>     return;
Done.

> 
>
https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.h (right):
> 
>
https://codereview.chromium.org/14858004/diff/14001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.h:833: ForceCompositedScrollingOff
> Maybe better names for the 2 arguments:
> * CompositedScrollingAlwaysEnabled
> * CompositedScrollingAlwaysDisabled
I've updated the names, but I'm wondering if the renaming suggestion was linked
to the idea of making setAcceleratedCompositingForOverflowScrollEnabled take a
global tri-state? If we keep the ability to force opt in/out for a particular
layer, do the old names make more sense?

[Ian Vollick, 2013-05-10 15:47:08]

Sorry for the spam; I noticed something that breaks my previous argument.

On 2013/05/10 15:40:00, vollick wrote:
> On 2013/05/09 22:34:34, Julien Chaffraix wrote:
> > Sorry, I should have caught that in a previous review: you can make the
> Settings
> > (setAcceleratedCompositingForOverflowScrollEnabled) an enum fairly easily. I
> am
> > concerned about having the information about whether we enable
> > accelerated-scrolling now spread in 2 places (RenderLayer and Settings).
> I think we do need both since they mean different things. The value in
Settings
> enables or disables the feature globally, but the setting in RenderLayer lets
us
> force a particular layer to opt in/or out. In
> build-paint-order-list-where-opt-in-decisions-can-each-other.html, we want to
> keep the global setting enabled, but force one particular layer to opt in or
out
> (and see how the other layer responds), but we wouldn't have this control with
> one, global setting.

Actually, it looks like the paint order list building code ignores the global
setting, so having a global tri-state could work. I'll try this now.

[Ian Vollick, 2013-05-10 19:20:47]

On 2013/05/10 15:47:08, vollick wrote:
> Sorry for the spam; I noticed something that breaks my previous argument.
> 
> On 2013/05/10 15:40:00, vollick wrote:
> > On 2013/05/09 22:34:34, Julien Chaffraix wrote:
> > > Sorry, I should have caught that in a previous review: you can make the
> > Settings
> > > (setAcceleratedCompositingForOverflowScrollEnabled) an enum fairly easily.
I
> > am
> > > concerned about having the information about whether we enable
> > > accelerated-scrolling now spread in 2 places (RenderLayer and Settings).
> > I think we do need both since they mean different things. The value in
> Settings
> > enables or disables the feature globally, but the setting in RenderLayer
lets
> us
> > force a particular layer to opt in/or out. In
> > build-paint-order-list-where-opt-in-decisions-can-each-other.html, we want
to
> > keep the global setting enabled, but force one particular layer to opt in or
> out
> > (and see how the other layer responds), but we wouldn't have this control
with
> > one, global setting.
> 
> Actually, it looks like the paint order list building code ignores the global
> setting, so having a global tri-state could work. I'll try this now.

The global tri-state does work. I've confirmed that all subsequent patches in
the series will work with the change, too. This makes the CL much bigger, but
most of the changes are trivial. Does this look better?

[Julien - ping for review, 2013-05-11 01:05:52]

https://codereview.chromium.org/14858004/diff/23001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/14858004/diff/23001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebSettingsImpl.cpp:459:
AcceleratedCompositingForOverflowScrollEnabledMode mode = enabled
Probably worth a FIXME as we will want to propagate the value to about://flags.

https://codereview.chromium.org/14858004/diff/23001/Source/core/page/Settings.in
File Source/core/page/Settings.in (left):

https://codereview.chromium.org/14858004/diff/23001/Source/core/page/Settings...
Source/core/page/Settings.in:71: acceleratedCompositingForOverflowScrollEnabled
initial=false
We should be able to keep this Setting by switching to an enum, see
editingBehaviorType above.

https://codereview.chromium.org/14858004/diff/23001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14858004/diff/23001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:849:
AcceleratedCompositingForOverflowScrollEnabledMode
acceleratedCompositingForOverflowScrollEnabled() const;
It's annoying to have to #include Settings here and not only in the cpp file.
Can't this function still returns a boolean?

https://codereview.chromium.org/14858004/diff/23001/Source/core/testing/Inter...
File Source/core/testing/InternalSettings.idl (right):

https://codereview.chromium.org/14858004/diff/23001/Source/core/testing/Inter...
Source/core/testing/InternalSettings.idl:56: const unsigned short
AcceleratedCompositingForOverflowScrollDisabled = 0;
Shouldn't they be kept in sync with
AcceleratedCompositingForOverflowScrollEnabledMode ?

[Ian Vollick, 2013-05-11 01:32:59]

https://codereview.chromium.org/14858004/diff/23001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebSettingsImpl.cpp (right):

https://codereview.chromium.org/14858004/diff/23001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebSettingsImpl.cpp:459:
AcceleratedCompositingForOverflowScrollEnabledMode mode = enabled
On 2013/05/11 01:05:52, Julien Chaffraix wrote:
> Probably worth a FIXME as we will want to propagate the value to
about://flags.

Done.

https://codereview.chromium.org/14858004/diff/23001/Source/core/page/Settings.in
File Source/core/page/Settings.in (left):

https://codereview.chromium.org/14858004/diff/23001/Source/core/page/Settings...
Source/core/page/Settings.in:71: acceleratedCompositingForOverflowScrollEnabled
initial=false
On 2013/05/11 01:05:52, Julien Chaffraix wrote:
> We should be able to keep this Setting by switching to an enum, see
> editingBehaviorType above.

Done.

https://codereview.chromium.org/14858004/diff/23001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/14858004/diff/23001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:849:
AcceleratedCompositingForOverflowScrollEnabledMode
acceleratedCompositingForOverflowScrollEnabled() const;
On 2013/05/11 01:05:52, Julien Chaffraix wrote:
> It's annoying to have to #include Settings here and not only in the cpp file.
> Can't this function still returns a boolean?

Done.

https://codereview.chromium.org/14858004/diff/23001/Source/core/testing/Inter...
File Source/core/testing/InternalSettings.idl (right):

https://codereview.chromium.org/14858004/diff/23001/Source/core/testing/Inter...
Source/core/testing/InternalSettings.idl:56: const unsigned short
AcceleratedCompositingForOverflowScrollDisabled = 0;
On 2013/05/11 01:05:52, Julien Chaffraix wrote:
> Shouldn't they be kept in sync with
> AcceleratedCompositingForOverflowScrollEnabledMode ?

Yes, they should. Added a comment.

[Julien - ping for review, 2013-05-13 20:48:56]

LGTM but don't land without fixing the comments.

https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:94:
window.internals.settings.AcceleratedCompositingForOverflowScrollAlwaysOn);
This is actually a change in behavior in the test as
setAcceleratedCompositingForOverflowScrollEnabled(true) maps to
setAcceleratedCompositingForOverflowScrollEnabledMode(AcceleratedCompositingForOverflowScrollEnabled).

I would like an explanation in the change description and possibly the test as
to why this is required.

https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html
(right):

https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:129:
window.internals.settings.AcceleratedCompositingForOverflowScrollAlwaysOn);
Same comment.

https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
File LayoutTests/compositing/overflow/build-paint-order-lists.html (right):

https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/build-paint-order-lists.html:111:
window.internals.settings.AcceleratedCompositingForOverflowScrollAlwaysOn);
Same comment.

https://codereview.chromium.org/14858004/diff/34006/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/14858004/diff/34006/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:487: &&
renderer()->frame()->page()->settings()
I don't think this is right: the page can be NULL, not the Setings themselves
(their are an OwnPtr populated inside Page constructor).

Why you don't just use Document::settings as it would avoid the need for such
custom code?

Settings* settings = renderer()->document()->settings();
return settings && (settings->acceleratedCompositingForOverflowScrollEnabled()
!= AcceleratedCompositingForOverflowScrollDisabled);

https://codereview.chromium.org/14858004/diff/34006/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2061: if (renderer()->frame() &&
renderer()->frame()->page()->settings())
Same comment here about NULL-checking settings.

[Ian Vollick, 2013-05-20 19:59:08]

Hi, Julien. I'm sorry to ask for a re-review, but I've had to revert to the
previous approach (forcing individual RenderLayers to opt-in or out). I've put
an explanation below. PTAL

As I was preparing to polish up and land this patch, I saw that a test was
crashing after the rebase. hartmanng did some investigation for me while I was
on vacation. He noticed that when the next test starts, we immediately reset the
internal settings, but the page from the last test hasn't yet torn down. The
consequence is that we immediately disable comp-scroll, so RenderLayer's on the
old page that used to be stacking containers abruptly cease to be, causing
asserts to fail. So it seemed to us that ::needsCompositedScrolling's return
value must depend solely on state owned by that layer, otherwise asserts will
trip as soon as we switch tests. I then tried using the global tristate in
::updateNeedsCompositedScrolling, but that was also unsuccessful -- updating the
global tristate doesn't force ::updateNeedsCompositedScrolling to be fired.

In the end, I thought that the best option was to have both a global setting
(for enabling/disabling the feature altogether) and a per-layer flag (for
forcing a particular layer to opt in or out). When we update the per-layer flag,
it's very simple and straightforward to update the relevant state.

On 2013/05/13 20:48:56, Julien Chaffraix wrote:
> LGTM but don't land without fixing the comments.
> 
>
https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
> File
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
> (right):
> 
>
https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:94:
> window.internals.settings.AcceleratedCompositingForOverflowScrollAlwaysOn);
> This is actually a change in behavior in the test as
> setAcceleratedCompositingForOverflowScrollEnabled(true) maps to
>
setAcceleratedCompositingForOverflowScrollEnabledMode(AcceleratedCompositingForOverflowScrollEnabled).
> 
> I would like an explanation in the change description and possibly the test as
> to why this is required.
This is no longer applicable, I think, but you make a good point about
justifying the change to the tests. I've updated the description to explain why
this change will be necessary.
> 
>
https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
> File
>
LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html
> (right):
> 
>
https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
>
LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:129:
> window.internals.settings.AcceleratedCompositingForOverflowScrollAlwaysOn);
> Same comment.
> 
>
https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
> File LayoutTests/compositing/overflow/build-paint-order-lists.html (right):
> 
>
https://codereview.chromium.org/14858004/diff/34006/LayoutTests/compositing/o...
> LayoutTests/compositing/overflow/build-paint-order-lists.html:111:
> window.internals.settings.AcceleratedCompositingForOverflowScrollAlwaysOn);
> Same comment.
> 
>
https://codereview.chromium.org/14858004/diff/34006/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/14858004/diff/34006/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:487: &&
> renderer()->frame()->page()->settings()
> I don't think this is right: the page can be NULL, not the Setings themselves
> (their are an OwnPtr populated inside Page constructor).
> 
> Why you don't just use Document::settings as it would avoid the need for such
> custom code?
Done.
> 
> Settings* settings = renderer()->document()->settings();
> return settings && (settings->acceleratedCompositingForOverflowScrollEnabled()
> != AcceleratedCompositingForOverflowScrollDisabled);
> 
>
https://codereview.chromium.org/14858004/diff/34006/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayer.cpp:2061: if (renderer()->frame() &&
> renderer()->frame()->page()->settings())
> Same comment here about NULL-checking settings.

[Julien - ping for review, 2013-05-29 19:26:02]

Sad that we can't use a tri-state so lgtm

[commit-bot: I haz the power, 2013-05-29 20:38:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/14858004/47001

[commit-bot: I haz the power, 2013-05-30 03:26:34]

Change committed as 151433