Implement will-change-based creation of layers, stacking contexts, and containing blocks

Implement will-change-based creation of layers, stacking contexts, and containing blocks

This uses an element's will-change value to determine whether 
it should be composited, create a stacking context, or become
a containing block. will-change is currently behind a runtime flag.

Intent-to-implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/M62y2nKZ-gE

Spec: http://tabatkins.github.io/specs/css-will-change/

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

[Ian Vollick, 2014-02-26 16:21:12]

https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:175: case CSSPropertyZIndex:
z-index only creates a sc if the element is also positioned.

I wonder if we need to change the logic so that
will-change: z-index ==> sc if positioned or will-change: position,
and
will-change: position ==> sc if z-index != auto or will-change: z-index.

Chatting offline with Ali about this seemed to suggest some ambiguity in the
spec. Will need to follow up with Tab.

[ajuma, 2014-02-26 17:00:30]

https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:175: case CSSPropertyZIndex:
On 2014/02/26 16:21:12, Ian Vollick wrote:
> z-index only creates a sc if the element is also positioned.
> 
> I wonder if we need to change the logic so that
> will-change: z-index ==> sc if positioned or will-change: position,
> and
> will-change: position ==> sc if z-index != auto or will-change: z-index.
> 
> Chatting offline with Ali about this seemed to suggest some ambiguity in the
> spec. Will need to follow up with Tab.

I took a look at what Mozilla's implementation does
(https://bugzilla.mozilla.org/attachment.cgi?id=8378525&action=diff), and it
turns out that they unconditionally create stacking contexts for will-change:
z-index and will-change: position. This seems reasonable to me (given that I
don't expect these particular values of will-change to be widely used, keeping
the implementation simple makes sense), so I've updated this CL so that we also
unconditionally create a stacking context for will-change: position. I'll follow
up with Tab about getting the spec updated to clarify this.

[Ian Vollick, 2014-02-26 18:30:25]

On 2014/02/26 17:00:30, ajuma wrote:
>
https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
> Source/core/css/resolver/StyleAdjuster.cpp:175: case CSSPropertyZIndex:
> On 2014/02/26 16:21:12, Ian Vollick wrote:
> > z-index only creates a sc if the element is also positioned.
> > 
> > I wonder if we need to change the logic so that
> > will-change: z-index ==> sc if positioned or will-change: position,
> > and
> > will-change: position ==> sc if z-index != auto or will-change: z-index.
> > 
> > Chatting offline with Ali about this seemed to suggest some ambiguity in the
> > spec. Will need to follow up with Tab.
> 
> I took a look at what Mozilla's implementation does
> (https://bugzilla.mozilla.org/attachment.cgi?id=8378525&action=diff), and it
> turns out that they unconditionally create stacking contexts for will-change:
> z-index and will-change: position. This seems reasonable to me (given that I
> don't expect these particular values of will-change to be widely used, keeping
> the implementation simple makes sense), so I've updated this CL so that we
also
> unconditionally create a stacking context for will-change: position. I'll
follow
> up with Tab about getting the spec updated to clarify this.

Ah, cool. LGTM in that case. (Nice tests, BTW)

[Ian Vollick, 2014-02-26 18:30:28]

On 2014/02/26 17:00:30, ajuma wrote:
>
https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
> File Source/core/css/resolver/StyleAdjuster.cpp (right):
> 
>
https://codereview.chromium.org/175263002/diff/220001/Source/core/css/resolve...
> Source/core/css/resolver/StyleAdjuster.cpp:175: case CSSPropertyZIndex:
> On 2014/02/26 16:21:12, Ian Vollick wrote:
> > z-index only creates a sc if the element is also positioned.
> > 
> > I wonder if we need to change the logic so that
> > will-change: z-index ==> sc if positioned or will-change: position,
> > and
> > will-change: position ==> sc if z-index != auto or will-change: z-index.
> > 
> > Chatting offline with Ali about this seemed to suggest some ambiguity in the
> > spec. Will need to follow up with Tab.
> 
> I took a look at what Mozilla's implementation does
> (https://bugzilla.mozilla.org/attachment.cgi?id=8378525&action=diff), and it
> turns out that they unconditionally create stacking contexts for will-change:
> z-index and will-change: position. This seems reasonable to me (given that I
> don't expect these particular values of will-change to be widely used, keeping
> the implementation simple makes sense), so I've updated this CL so that we
also
> unconditionally create a stacking context for will-change: position. I'll
follow
> up with Tab about getting the spec updated to clarify this.

Ah, cool. LGTM in that case. (Nice tests, BTW)

[ajuma, 2014-02-26 18:36:57]

+jamesr for Source/platform

[jamesr, 2014-02-26 23:56:21]

/platform/ lgtm, but the style change seems a bit iffy and should be looked at
by a style expert

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:176: case CSSPropertyPosition:
this is supposed to be a set of all CSS properties that can in any value create
a stacking context? it seems very fragile to keep this set here in just a switch
- are you sure this has all cases?

[ajuma, 2014-02-27 00:05:46]

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:176: case CSSPropertyPosition:
On 2014/02/26 23:56:22, jamesr wrote:
> this is supposed to be a set of all CSS properties that can in any value
create
> a stacking context? it seems very fragile to keep this set here in just a
switch
> - are you sure this has all cases?

Yes, this is supposed to be all CSS properties that have some value that creates
a stacking context. The list here corresponds to the conditions under which
adjustRenderStyle calls setZIndex(0) (see lines 295-309 below), so should be the
complete list.

[ajuma, 2014-02-27 19:40:24]

esprehn, PTAL at the style change (see jamesr's comments above).

[esprehn, 2014-02-27 20:58:30]

Sadly the style change is correct unless you want to write a unit test. Ideally
we'd ASSERT that setting any of those properties also triggers a non auto
z-index.

btw I don't think your description is correct, do you create containing blocks
for fixed position things?

https://codereview.chromium.org/175263002/diff/240001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/containing-block-added.html (right):

https://codereview.chromium.org/175263002/diff/240001/LayoutTests/compositing...
LayoutTests/compositing/will-change/containing-block-added.html:3: <head>
We often leave off the <html>, <head> and <body> elements since it makes the
test simpler.

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:176: case CSSPropertyPosition:
This just begs for a unit test, but I suppose it'll be hard to do.

[ajuma, 2014-02-27 21:56:23]

On 2014/02/27 20:58:30, esprehn wrote:
 > btw I don't think your description is correct, do you create containing
blocks
> for fixed position things?

Yes, this happens indirectly as a result of the other change in
StyleAdjuster.cpp (that sets an identity transform when will-change:transform is
set but no transform is set). See also the containing-block-* layout tests added
here that check for containing block creation.

[esprehn, 2014-02-27 22:05:27]

lgtm, note that we need to get a spec change for this. The alternative is to add
some hacks to hide it from script that's crazy.

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/175263002/diff/240001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:312: // including the creation of a
containing block for fixed position descendants.
This is observable from script. If you do getComputedStyle(e).transform you'll
see the identity transform you've added. That's fine, but we need to make the
spec text say something like  "The transform of the element computes to the
identity if you used will-animate: transform"

[ajuma, 2014-02-27 22:19:46]

On 2014/02/27 22:05:27, esprehn wrote:
> lgtm, note that we need to get a spec change for this. The alternative is to
add
> some hacks to hide it from script that's crazy.

Thanks, I'll follow-up with getting the spec updated.

[ajuma, 2014-02-27 22:56:10]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-02-27 22:56:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/175263002/240001

[commit-bot: I haz the power, 2014-02-28 01:07:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/175263002/240001

[commit-bot: I haz the power, 2014-02-28 03:16:59]

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

[commit-bot: I haz the power, 2014-02-28 03:17:00]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[ajuma, 2014-02-28 14:03:51]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-02-28 14:04:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/175263002/240001

[commit-bot: I haz the power, 2014-02-28 15:05:51]

Change committed as 168134