Last Comment Bug 940842 - Add a will-change CSS property to *hint* something will animate (proxy for layerization)
: Add a will-change CSS property to *hint* something will animate (proxy for la...
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
: mozilla29
Assigned To: Benoit Girard (:BenWa)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 975769 975789
Blocks: 950713 milk-flow 931262 940734 942333 942339 943086 943089 945461 945464 945514 945515 945516 949895 950498 will-change 964885 974125
  Show dependency treegraph
 
Reported: 2013-11-19 20:43 PST by Benoit Girard (:BenWa)
Modified: 2014-11-18 05:48 PST (History)
34 users (show)
ioana_damy: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (9.30 KB, patch)
2013-11-19 20:43 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
WIP 2: rebase + progress (14.24 KB, patch)
2013-11-19 21:40 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Add CSS Property (13.49 KB, patch)
2013-11-20 11:18 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Add CSS Property (Needs discussion) (15.24 KB, patch)
2013-11-20 14:19 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Implementation (5.37 KB, patch)
2013-11-20 14:20 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Implementation (5.38 KB, patch)
2013-11-20 14:21 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Implementation (5.29 KB, patch)
2013-11-20 14:22 PST, Benoit Girard (:BenWa)
roc: feedback-
Details | Diff | Splinter Review
Part 1: Add will-animate CSS Property (17.55 KB, patch)
2013-11-20 21:39 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
testcase (628 bytes, text/html)
2013-11-21 20:55 PST, Benoit Girard (:BenWa)
no flags Details
Part 1: Add will-animate CSS Property (17.94 KB, patch)
2013-11-22 10:20 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 2: Implementation (4.25 KB, patch)
2013-11-22 10:21 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 3: Mochitest (4.69 KB, patch)
2013-11-22 12:30 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
part 4: Keep nsDisplayTransform active (42.95 KB, patch)
2013-11-23 17:20 PST, Benoit Girard (:BenWa)
matt.woodrow: review-
Details | Diff | Splinter Review
part 4: Keep nsDisplayTransform active (651 bytes, patch)
2013-11-24 20:06 PST, Benoit Girard (:BenWa)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 2: Implementation (4.95 KB, patch)
2013-11-24 20:25 PST, Benoit Girard (:BenWa)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 3: Mochitest (4.81 KB, patch)
2013-11-24 20:43 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Add will-animate CSS Property (17.61 KB, patch)
2013-12-04 13:08 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Add will-animate CSS Property v4 (19.12 KB, patch)
2013-12-04 15:52 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Add will-animate CSS Property v4 (17.59 KB, patch)
2013-12-09 09:03 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 1: Add will-change CSS Property v5 (17.63 KB, patch)
2013-12-14 10:56 PST, Benoit Girard (:BenWa)
dbaron: review-
Details | Diff | Splinter Review
Part 2: Implementation (4.98 KB, patch)
2013-12-14 10:57 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Part 3: Mochitest (4.82 KB, patch)
2013-12-14 10:57 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Part 4: Keep nsDisplayTransform active (1.28 KB, patch)
2013-12-14 10:58 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Part 4: Keep nsDisplayTransform active (1.31 KB, patch)
2013-12-18 14:04 PST, Benoit Girard (:BenWa)
b56girard: review+
Details | Diff | Splinter Review
Part 1: Add will-change CSS Property v6 (fixed a memory leak and applied dbarons comments) (16.90 KB, patch)
2014-01-16 14:38 PST, Benoit Jacob [:bjacob] (mostly away)
dbaron: review-
Details | Diff | Splinter Review
Part 3.1: mark some mochitest failures as expected on old-layers (2.00 KB, patch)
2014-01-17 12:17 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
1/3: Add will-change CSS property (19.69 KB, patch)
2014-01-21 11:20 PST, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
2/3 - Implement will-change in layers-building code (carries r+ from mattwoodrow) (6.09 KB, patch)
2014-01-21 11:21 PST, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
Details | Diff | Splinter Review
3/3 - Add a mochitest covering repaint heuristics with will-change (5.68 KB, patch)
2014-01-21 11:27 PST, Benoit Jacob [:bjacob] (mostly away)
matt.woodrow: review+
Details | Diff | Splinter Review
1/3 - Add will-change CSS property (19.67 KB, patch)
2014-01-21 11:38 PST, Benoit Jacob [:bjacob] (mostly away)
dbaron: review-
Details | Diff | Splinter Review
Remove the scrolling part of the mochitest, is too fragile (3.03 KB, patch)
2014-01-22 10:44 PST, Benoit Jacob [:bjacob] (mostly away)
matt.woodrow: review+
b56girard: review+
Details | Diff | Splinter Review
1/3: Add will-change CSS property (18.85 KB, patch)
2014-01-28 10:55 PST, Benoit Jacob [:bjacob] (mostly away)
dbaron: review+
Details | Diff | Splinter Review
1/3: Add will-change CSS property (Final form) (18.81 KB, patch)
2014-01-29 08:56 PST, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description User image Benoit Girard (:BenWa) 2013-11-19 20:43:50 PST
Created attachment 8335079 [details] [diff] [review]
WIP

I want to investigate if and how we should expose a CSS property to force an active layer. Currently we can already do this with TranslateZ(1px). It feels more useful to add a property to hint rather then use something like TranslateZ(1px) which is a terrible proxy for many reasons. Also by using a non transform property it means that we don't have to worry about overwriting the value when something needs to actually use transform for it's real attribute.
Comment 1 User image Benoit Girard (:BenWa) 2013-11-19 21:40:47 PST
Created attachment 8335105 [details] [diff] [review]
WIP 2: rebase + progress
Comment 2 User image David Baron :dbaron: ⌚️UTC-8 2013-11-19 21:46:35 PST
Comment on attachment 8335105 [details] [diff] [review]
WIP 2: rebase + progress

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h

You only need to add keywords for the values, not the property name itself.

Also, no need for prefixed values for a prefixed property.  But I'm also not sure about the names.

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp

You shouldn't need to change nsCSSParser.cpp at all.

>diff --git a/layout/style/nsCSSPropList.h b/layout/style/nsCSSPropList.h
>+CSS_PROP_DISPLAY(
>+    -moz-layer,
>+    layer,
>+    Layer,
>+    CSS_PROPERTY_PARSE_VALUE,
>+    "layout.css.mozlayer.enabled",
>+    VARIANT_KEYWORD,
>+    kLayerKTable,
>+    offsetof(nsStyleDisplay, mForceLayer),
>+    eStyleAnimType_None)

You want VARIANT_HK (although I should at some point fix it so that's not required).  And you want  CSS_PROP_DOMPROP_PREFIXED() around the third parameter.


And you obviously don't want the abort()s in nsComputedDOMStyle.cpp.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2013-11-19 21:47:37 PST
Oh, and then you should add to layout/style/test/property_database.js and run the mochitests in layout/style/test/.
Comment 4 User image Benoit Girard (:BenWa) 2013-11-19 21:58:16 PST
Thanks for the early feedback. It certainly helps since I don't really know what I'm doing here x_X. The patch is pretty messy I didn't think anyone would really look at it yet but I appreciate.
Comment 5 User image Benoit Girard (:BenWa) 2013-11-20 11:18:33 PST
Created attachment 8335467 [details] [diff] [review]
Part 1: Add CSS Property

Ok some open questions here:
1) Do we prefix
2) Do we hide it behind a preference
3) Do we expose it for b2g and/or web content
4) Do we add a proposal (I'm not sure if this is something we really want long term to be part of the web).

Thoughts?
Comment 6 User image Matt Woodrow (:mattwoodrow) 2013-11-20 11:26:32 PST
Even for a prefixed property, I don't think we want to expose layers (an implementation detail of our rendering engine) to web content.

How about '-moz-will-animate' as a fairly general hint that this element should be assumed to be animating/moving in the near future (and include this in our layerization heuristics).
Comment 7 User image Benoit Girard (:BenWa) 2013-11-20 11:37:07 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> Even for a prefixed property, I don't think we want to expose layers (an
> implementation detail of our rendering engine) to web content.

If we go that route then we're giving apps preferential performance treatment.

> 
> How about '-moz-will-animate' as a fairly general hint that this element
> should be assumed to be animating/moving in the near future (and include
> this in our layerization heuristics).

That could be a better name for it.
Comment 8 User image Matt Woodrow (:mattwoodrow) 2013-11-20 13:37:53 PST
(In reply to Benoit Girard (:BenWa) from comment #7)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> > Even for a prefixed property, I don't think we want to expose layers (an
> > implementation detail of our rendering engine) to web content.
> 
> If we go that route then we're giving apps preferential performance
> treatment.

Sorry, this was a poor choice of words by me. I don't think we want to expose the concept of layers to *anything* in JS, regardless of the origin.

I'd prefer exposing ways that the author can signal what their intentions for the content are, and we can internally decide how to best interpret that.
Comment 9 User image Benoit Girard (:BenWa) 2013-11-20 14:09:32 PST
Renamed the title to suggest that we're looking to hint that something will animate and not infer anything about the implementation.
Comment 10 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-20 14:12:15 PST
Will this force layerization for elements that are offscreen? Because translateZ(1px) doesn't do that (nor do any of the other standard hacks).

It's unclear to me why we think we should prefix this, given our general policy is to not use prefixes.
Comment 11 User image Benoit Girard (:BenWa) 2013-11-20 14:15:19 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Will this force layerization for elements that are offscreen?

As implemented, no. Should it? I don't know, we should discuss this here. FWIW I don't need that behavior above what we currently do but it might make sense.

> It's unclear to me why we think we should prefix this, given our general
> policy is to not use prefixes.

Removing it.
Comment 12 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-20 14:16:16 PST
I think something like "will-animate: none | transform" (extensible to other property values) probably makes sense. I think this should be raised on www-style though.
Comment 13 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-20 14:17:31 PST
(In reply to Benoit Girard (:BenWa) from comment #11)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> > Will this force layerization for elements that are offscreen?
> 
> As implemented, no. Should it? I don't know, we should discuss this here.
> FWIW I don't need that behavior above what we currently do but it might make
> sense.

I don't know. Maybe our existing heuristics (or future extensions of them) are good enough.
Comment 14 User image Benoit Girard (:BenWa) 2013-11-20 14:19:22 PST
Created attachment 8335588 [details] [diff] [review]
Part 1: Add CSS Property (Needs discussion)

This patch wont land as-is. We need to agree on a name. But it works.
Comment 15 User image Benoit Girard (:BenWa) 2013-11-20 14:20:29 PST
Created attachment 8335589 [details] [diff] [review]
Part 2: Implementation

This part is ready for review although the variable name may change to something like willAnimate.
Comment 16 User image Benoit Girard (:BenWa) 2013-11-20 14:21:40 PST
Created attachment 8335591 [details] [diff] [review]
Part 2: Implementation

Removed printf
Comment 17 User image Benoit Girard (:BenWa) 2013-11-20 14:22:14 PST
Created attachment 8335592 [details] [diff] [review]
Part 2: Implementation
Comment 18 User image Benoit Girard (:BenWa) 2013-11-20 14:25:09 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> I think something like "will-animate: none | transform" (extensible to other
> property values) probably makes sense. I think this should be raised on
> www-style though.

Ok I'm starting on the rename since it's much better then what I have currently. Patch incoming.
Comment 19 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-11-20 14:32:19 PST
Comment on attachment 8335592 [details] [diff] [review]
Part 2: Implementation

Review of attachment 8335592 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2171,5 @@
>    bool isStackingContext =
>      (isPositioned && (disp->mPosition == NS_STYLE_POSITION_STICKY ||
>                        pos->mZIndex.GetUnit() == eStyleUnit_Integer)) ||
> +     isVisuallyAtomic || (aFlags & DISPLAY_CHILD_FORCE_STACKING_CONTEXT) ||
> +     suggestActiveLayer;

OK so you're saying that will-animate other than 'none' forces a stacking context. I think that's the right thing to do, but it affects rendering so this isn't just a hint and definitely needs to be documented as part of the proposal.
Comment 20 User image Benoit Girard (:BenWa) 2013-11-20 15:01:53 PST
mattwoodrow: BenWa: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2418
mattwoodrow: HasTransformStyle() should be true if will-animate: transform; is specified
Comment 21 User image Matt Woodrow (:mattwoodrow) 2013-11-20 15:04:30 PST
Assuming we're going with will-animate: <property>, then the implementation will differ per-property.

For transform:

We need to tell the style system that this property behaves as if a transform: property was set.
This function needs to return true: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#1819

Note that this will force a stacking context, and can change the rendering of the page, so needs to be documented in the proposal.

That change should get all of layout to think we have a transform, and will get an nsDisplayTransform created.

Then we also want to make that nsDisplayTransform active. nsDisplayTransform::GetLayerState calls this function - http://mxr.mozilla.org/mozilla-central/source/layout/base/ActiveLayerTracker.cpp#207 to determine this. We'd want to check if will-animate is specified for aProperty, and return true if it (maybe subject to some heuristics to avoid abuse).

I think that should be all that required, and that nsDisplayTransform::GetResultingTransformMatrix will happily compute a no-op transform in this case.
Comment 22 User image Matt Woodrow (:mattwoodrow) 2013-11-20 15:07:19 PST
For opacity:

The second half should be the same, but for the first half I think we need to change this function - http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1009

Or possiblly add nsStyleDisplay::HasOpacity() to make it closer match transform.

For scroll:

This is a bit confusing from the css side, since 'scroll' isn't a css property type, but transform and opacity are. I don't know if that matters.

Forcing active scroll layers: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1799
Comment 23 User image Benoit Girard (:BenWa) 2013-11-20 21:39:06 PST
Created attachment 8335841 [details] [diff] [review]
Part 1: Add will-animate CSS Property

I still have something wrong here because it doesn't handle having "will-animate: scroll transform". Otherwise it seems to be fine.
Comment 24 User image Benoit Girard (:BenWa) 2013-11-21 20:55:24 PST
Created attachment 8336571 [details]
testcase
Comment 25 User image Benoit Girard (:BenWa) 2013-11-22 10:20:37 PST
Created attachment 8336905 [details] [diff] [review]
Part 1: Add will-animate CSS Property

This add 'will-animate: [default | [transform | opacity | scroll]+]'. However I have a problem that it seems to parse correctly when I specify more then one identifier but it rejects the property anyways. What I am doing where here?
Comment 26 User image Benoit Girard (:BenWa) 2013-11-22 10:21:15 PST
Created attachment 8336906 [details] [diff] [review]
Part 2: Implementation
Comment 27 User image Benoit Girard (:BenWa) 2013-11-22 12:30:47 PST
Created attachment 8337001 [details] [diff] [review]
Part 3: Mochitest
Comment 28 User image Benoit Girard (:BenWa) 2013-11-23 17:20:45 PST
Created attachment 8337322 [details] [diff] [review]
part 4: Keep nsDisplayTransform active

Not sure if this is right or now but this fixes the homescreen pre-rendering being invalidated.
Comment 29 User image Matt Woodrow (:mattwoodrow) 2013-11-24 15:17:34 PST
Comment on attachment 8337322 [details] [diff] [review]
part 4: Keep nsDisplayTransform active

Review of attachment 8337322 [details] [diff] [review]:
-----------------------------------------------------------------

I think you uploaded the wrong patch!
Comment 30 User image Matt Woodrow (:mattwoodrow) 2013-11-24 15:20:03 PST
Comment on attachment 8336906 [details] [diff] [review]
Part 2: Implementation

Review of attachment 8336906 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +1015,4 @@
>             nsLayoutUtils::HasAnimationsForCompositor(mContent,
>                                                       eCSSProperty_opacity) &&
>             mContent->GetPrimaryFrame() == this);
>  }

You might also want to update the check for opacity == 0.0 at the top of BuildDisplayListForStackingContext.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1802,5 @@
>      return true;
>    }
>  
> +  const nsStyleDisplay* disp = mOuter->StyleDisplay();
> +  if (disp && disp->mWillAnimate == NS_STYLE_WILL_ANIMATE_SCROLL) {

& rather than ==.

::: layout/style/nsStyleStruct.h
@@ +1822,4 @@
>             mTransformStyle == NS_STYLE_TRANSFORM_STYLE_PRESERVE_3D ||
> +           mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN ||
> +           (mWillAnimate & NS_STYLE_WILL_ANIMATE_TRANSFORM) ||
> +           (mWillAnimate & NS_STYLE_WILL_ANIMATE_OPACITY);

I don't think we need to check NS_STYLE_WILL_ANIMATE_OPACITY here, do we?
Comment 31 User image Matt Woodrow (:mattwoodrow) 2013-11-24 15:28:52 PST
Comment on attachment 8337001 [details] [diff] [review]
Part 3: Mochitest

Review of attachment 8337001 [details] [diff] [review]:
-----------------------------------------------------------------

You could write these as reftests instead using the 'reftest-no-paint' attribute instead.

We've had quite a few problems with intermittent failures in invalidation mochitests, the reftest harness hopefully has all the corner cases worked out.

::: layout/base/tests/chrome/test_will_animate.html
@@ +13,5 @@
> +    }
> +    #checkScrollRepaint {
> +      will-animate: scroll;
> +    }
> +    #checkScrollRepaint {

Merge this with the one above?

@@ +23,5 @@
> +</head>
> +<body onload="startTest()">
> +  <div id="checkRepaint">
> +    Check repaint without will-animate
> +  </div>

Does this one actually fail? These divs don't have a size, nor do they actually paint anything for the opacity to be applied to.

I think you need to give them a width/height, and some content (that isn't just a color or image, since we'll optimize those to things that aren't painted anyway). A gradient background should work.
Comment 32 User image Benoit Girard (:BenWa) 2013-11-24 20:06:06 PST
Created attachment 8337503 [details] [diff] [review]
part 4: Keep nsDisplayTransform active
Comment 33 User image Benoit Girard (:BenWa) 2013-11-24 20:25:59 PST
Created attachment 8337511 [details] [diff] [review]
Part 2: Implementation
Comment 34 User image Benoit Girard (:BenWa) 2013-11-24 20:43:03 PST
Created attachment 8337516 [details] [diff] [review]
Part 3: Mochitest

Fixed up part 3. Removing will-animate makes the test fail.
Comment 35 User image Benoit Girard (:BenWa) 2013-11-26 12:49:50 PST
Posted proposal to www-style. Pending moderator review (up to 2 business days):
http://lists.w3.org/Archives/Public/www-style/
Comment 36 User image Benoit Girard (:BenWa) 2013-12-03 13:28:29 PST
I talked this with dbaron and we're going to discuss on dev-b2g/dev-gaia if it make sense to enable this for B2G only (with the intention that it's primarily gaia that will use it). There doesn't appear to be any precedence for exposing a CSS property only to b2g/gaia. I only intend on proceeding with this if the outcome to that discussion is favorable in addition to the patch reviews here.
Comment 37 User image Benoit Girard (:BenWa) 2013-12-04 13:08:02 PST
Created attachment 8342608 [details] [diff] [review]
Part 1: Add will-animate CSS Property

I updated the CSS parsing to reflect what is being discussed on www-style. Note that this patch currently puts will-animate behind a preference. I'd like to land it now to facilitate testing on b2g while we continue the discussion of how it will be turned on.

I'll be turning this on in a separate patch once we've reached a consensus on how to turn this on for b2g.
Comment 38 User image Benoit Girard (:BenWa) 2013-12-04 15:52:57 PST
Created attachment 8342716 [details] [diff] [review]
Part 1: Add will-animate CSS Property v4

Supports computed style now. I still maintain the bitfield to do efficient lookups.
Comment 39 User image Benoit Girard (:BenWa) 2013-12-09 09:03:06 PST
Created attachment 8344702 [details] [diff] [review]
Part 1: Add will-animate CSS Property v4

For now this just lands it behind a preference for testing.
Comment 40 User image J. Ryan Stinnett [:jryans] (use ni?) 2013-12-12 11:36:18 PST
This would also be handy to use in chrome code on desktop Firefox for the Dev Tools UI (and likely other purposes).  Maybe it can be exposed there too, unless that is somehow complex / risky.
Comment 41 User image Benoit Girard (:BenWa) 2013-12-12 14:28:09 PST
(In reply to J. Ryan Stinnett [:jryans] from comment #40)
> This would also be handy to use in chrome code on desktop Firefox for the
> Dev Tools UI (and likely other purposes).  Maybe it can be exposed there
> too, unless that is somehow complex / risky.

Please file another bug. I'd like to restrict the scope of this bug since the scope is already large enough. In your bug please describe the use case that requires this. They may be a better way.
Comment 42 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2013-12-13 04:33:33 PST
Why not update the syntax right now? It should be a simple change. will-animate -> will-change, scroll -> scroll-position.
Comment 43 User image Benoit Girard (:BenWa) 2013-12-13 08:44:30 PST
I wanted to let the discussion continue rather then updating these patches back and forth and wait for a final consensus to do a final adjustment. We may not be far from that now.
Comment 44 User image Benoit Girard (:BenWa) 2013-12-14 10:53:56 PST
Rename title to reflect updated proposal.
Comment 45 User image Benoit Girard (:BenWa) 2013-12-14 10:56:53 PST
Created attachment 8347643 [details] [diff] [review]
Part 1: Add will-change CSS Property v5

https://tbpl.mozilla.org/?tree=Try&rev=e12b74e68fc8

renamed property
Comment 46 User image Benoit Girard (:BenWa) 2013-12-14 10:57:19 PST
Created attachment 8347644 [details] [diff] [review]
Part 2: Implementation
Comment 47 User image Benoit Girard (:BenWa) 2013-12-14 10:57:40 PST
Created attachment 8347645 [details] [diff] [review]
Part 3: Mochitest
Comment 48 User image Benoit Girard (:BenWa) 2013-12-14 10:58:05 PST
Created attachment 8347646 [details] [diff] [review]
Part 4: Keep nsDisplayTransform active
Comment 49 User image Benoit Girard (:BenWa) 2013-12-14 11:43:20 PST
https://tbpl.mozilla.org/?tree=Try&rev=39da8534dcdc
Comment 50 User image David Baron :dbaron: ⌚️UTC-8 2013-12-15 08:30:34 PST
Comment on attachment 8347643 [details] [diff] [review]
Part 1: Add will-change CSS Property v5

>+    nsDependentString str(cur->mValue.GetStringBufferValue());

If you're converting it to a string class, use GetStringValue, i.e.:
  nsString str;
  cur->mValue.GetStringValue(str);

>+      if (!CheckEndProperty() || !first) {

Skip the CheckEndProperty() call here; there are other checks that will
catch garbage after the end of a property.  (Really, all uses of
CheckEndProperty are incorrect; I won't make you eliminate the other
one, though.)

You need to accept initial and inherit and unset rather than
rejecting them, but only when they're alone, of course.  You could do
this by calling ParseVariant with VARIANT_IDENTIFIER | VARIANT_INHERIT
and then if the result isn't eCSSUnit_Ident, then:
 * if first is true, breaking
 * if first is false, returning false

But I think you should reject 'default' (like 'all') rather than
accepting it as a value.  (And the same for 'unset' if it's not
supported, really.) That's because the point of rejecting these is
because they're reserved for being future keywords that are accepted by
any CSS property.

(Where's the current proposal, anyway?)


Why add the type |nsWillChange| when |const nsString| should do just
fine instead?


>+// Is support for CSS "will-change" enabled?
>+#ifdef RELEASE_BUILD
>+pref("layout.css.will-change.enabled", false);
>+#else
>+pref("layout.css.will-change.enabled", true);
>+#endif

This seems problematic for front-end developers, since they're
likely to do performance work using this feature, and it won't
necessarily be obvious that the feature doesn't exist in releases.

Maybe, instead, land as fully disabled, and then figure out the right conditions in another bug?
Comment 51 User image Benoit Girard (:BenWa) 2013-12-16 08:46:47 PST
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-5) from comment #50)
> >+// Is support for CSS "will-change" enabled?
> >+#ifdef RELEASE_BUILD
> >+pref("layout.css.will-change.enabled", false);
> >+#else
> >+pref("layout.css.will-change.enabled", true);
> >+#endif
> 
> This seems problematic for front-end developers, since they're
> likely to do performance work using this feature, and it won't
> necessarily be obvious that the feature doesn't exist in releases.
> 
> Maybe, instead, land as fully disabled, and then figure out the right
> conditions in another bug?

Alright, roc suggested this but it does make sense to avoid having a large performance difference caused by this.
Comment 52 User image Benoit Girard (:BenWa) 2013-12-18 14:04:31 PST
Created attachment 8349671 [details] [diff] [review]
Part 4: Keep nsDisplayTransform active
Comment 53 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2014-01-07 01:44:28 PST
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #50)
> This seems problematic for front-end developers, since they're
> likely to do performance work using this feature, and it won't
> necessarily be obvious that the feature doesn't exist in releases.
> 
> Maybe, instead, land as fully disabled, and then figure out the right
> conditions in another bug?

Why not just pref it on now? As far as I know there are no outstanding unresolved issues.
Comment 54 User image David Baron :dbaron: ⌚️UTC-8 2014-01-15 00:36:54 PST
Are you planning to revise part 1?  (The main reason for the review- rather than review+ with comments was because I wanted to see the revisions for what values are accepted/rejected; I should have been more explicit about that.)
Comment 55 User image Benoit Girard (:BenWa) 2014-01-16 07:48:49 PST
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #54)
> Are you planning to revise part 1?  (The main reason for the review- rather
> than review+ with comments was because I wanted to see the revisions for
> what values are accepted/rejected; I should have been more explicit about
> that.)

Yes absolutely! I was asked to prioritize bug 950488. bjacob jumped in and is already working on it.
Comment 56 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-16 08:47:05 PST
Yup, working on it. So far, I've only fixed a leak in the part 1 patch, at this line

    mWillChange.AppendElement(new nsWillChange(buffer));

(see bug 960591 for the story of how it was made possible by a clever templated footgun).
Comment 57 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-16 14:38:05 PST
Created attachment 8361335 [details] [diff] [review]
Part 1: Add will-change CSS Property v6  (fixed a memory leak and applied dbarons comments)

This intends to apply all of dbaron's review comments, aside from the pref change which I leave for you guys to discuss.

The 'spec' is this email:
http://www.marshut.com/imqykv/proposal-will-change-property-formerly-will-animate.html

There are some differences already between it and what was recommended in dbaron's review above. Decide how to resolve these differences!

This seems to work locally, no leak anymore, with will-change:transform correctly avoiding repaints, but I get mochitest failures:

 0:18.11 3 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_animate.html | will-change checkOpacityRepaint element should not have been painted - got true, expected false
 0:18.11 4 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_animate.html | will-change checkTransformRepaint element should not have been painted - got true, expected false
Comment 58 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-17 06:41:39 PST
Hah, the above mochitest failures only happen when using old-basic-layers, which is still default on desktop linux. When using new-layers/OMTC, I can't reproduce. BenWa, do we still care about these failures then?
Comment 59 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-17 06:43:29 PST
With OMTC layers, I get as expected:

 0:13.62 2 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_animate.html | element should have been painted
 0:13.64 3 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_animate.html | will-change checkOpacityRepaint element should not have been painted
 0:13.64 4 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_animate.html | will-change checkTransformRepaint element should not have been painted
 0:13.69 5 INFO TEST-KNOWN-FAIL | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_animate.html | false - will-change checkScrollRepaint element should not have been painted
Comment 60 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-17 12:17:10 PST
Created attachment 8361875 [details] [diff] [review]
Part 3.1: mark some mochitest failures as expected on old-layers

After spending some time trying to debug these failures, :BenWa and me seem to agree that they're not worth much more effort trying to fix them: it is a big priority at the moment to get rid of old-layers, and that is intended to happen soon.
Comment 61 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-17 13:11:16 PST
https://tbpl.mozilla.org/?tree=Try&rev=f648a99fdf87
Comment 62 User image David Baron :dbaron: ⌚️UTC-8 2014-01-17 15:33:52 PST
Tab started a spec draft at http://tabatkins.github.io/specs/css-will-change/ .
Comment 63 User image David Baron :dbaron: ⌚️UTC-8 2014-01-17 15:56:44 PST
Comment on attachment 8361335 [details] [diff] [review]
Part 1: Add will-change CSS Property v6  (fixed a memory leak and applied dbarons comments)

>[mq]: Part1CSSProp

Please provide a useful commit message when posting patches for review.


+    nsAutoString str;
+    cur->mValue.GetStringValue(str);
+    // NOTE: 'none' is explictly allowed for forward compatibility
+    if (str.LowerCaseEqualsLiteral("all") ||
+        str.LowerCaseEqualsLiteral("default"))
+    {
+      return false;
+    }
+
+    if (str.LowerCaseEqualsLiteral("default")) {
+      if (!first) {
+        // default must be the only property
+        return false;
+      }
+      break;
+    }

A few problems here:

 (1) the second block's if is pretty clearly unreachable given the first
 block (and I think the first block is correct, given the spec)

 (2) I don't see anything in the spec about 'all' being disallowed.  Or
 about 'none' being explicitly allowed.  That suggests there should
 be no special handling for 'all' and no comment about 'none'.

So I think this block should be reduced to just returning false for
'default'.

However, I sent a comment on the spec
http://lists.w3.org/Archives/Public/www-style/2014Jan/0236.html
which would require an additional change here; rejecting an 'auto'
value that's part of a length>1 list.

nsCSSPropList.h:

>+    VARIANT_IDENTIFIER,

please leave this field as 0 since it's unused.

nsRuleNode.cpp:

>+  const nsCSSValue* willChangeValue = aRuleData->ValueForWillChange();
>+  if (willChangeValue->GetUnit() == eCSSUnit_List ||
>+      willChangeValue->GetUnit() == eCSSUnit_ListDep) {

You need to clear display->mWillChange and zero out
display->mWillChangeBitField inside this if, but before the loop.  I'm a
little disturbed that no tests caught this omission, though it would
only show up with partial computation with a non-null aStartStruct.

>+      if (buffer.Equals(NS_LITERAL_STRING("scroll"))) {

The spec draft uses scroll-position here rather than scroll.

nsStyleConsts.h:

>+#define NS_STYLE_WILL_CHANGE_DEFAULT            0x00

Omit this, and just use literal 0 for "empty bitfield" where you need
it.

And please write the rest as (1<<0), (1<<1), etc.

property_database.js:

This might need adjustments if my proposed spec change in
http://lists.w3.org/Archives/Public/www-style/2014Jan/0236.html is
accepted.

>+              initial_values: [ "default" ],
>+               other_values: [ "scroll", "transform", "opacity", "scroll, transform", "transform, opacity", "property-that-doesnt-exist-yet" ],
>+               invalid_values: [ "default, scroll", "none", "inherit", "all", "transform scroll", ",", "trailing," ]

The initial value is 'auto'.  Listing it as 'default', which is invalid,
should have caused a pile of tests to fail.

So should listing 'none' and 'all' as invalid_values, which you don't
reject, and shouldn't reject.

The spec draft also uses scroll-position rather than scroll; please
adjust the tests.

Please also add an explicit test for the 'contents' value.



nsStyleStruct.cpp:

You should have change handling for mWillChangeBitField in
nsStyleDisplay::CalcDifference.  It's possible that doing nothing
is correct, but if it is you need to add a comment explaining why.

all.js:

Either pick on or off; don't land it conditional on RELEASE_BUILD.
Comment 64 User image David Baron :dbaron: ⌚️UTC-8 2014-01-17 15:59:18 PST
Comment on attachment 8361875 [details] [diff] [review]
Part 3.1: mark some mochitest failures as expected on old-layers

I'm not comfortable reviewing this one.
Comment 65 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-21 11:20:25 PST
Created attachment 8363122 [details] [diff] [review]
1/3: Add will-change CSS property

This intends to address all of your review comments.

In addition, I realized that test_all_shorthand.html was not testing will-change anymore since I switched the pref to false by default, because it loaded to list of CSS properties before it set the pref. This patch also fixes that.
Comment 66 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-21 11:21:20 PST
Created attachment 8363123 [details] [diff] [review]
2/3 - Implement will-change in layers-building code (carries r+ from mattwoodrow)
Comment 67 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-21 11:21:52 PST
(This new 2/3 patch folds together the old "part 2" and "part 4" patches).
Comment 68 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-21 11:27:27 PST
Created attachment 8363125 [details] [diff] [review]
3/3 - Add a mochitest covering repaint heuristics with will-change

I tried hard having a mochitest that would test heuristics on all platforms, but kept running in BasicLayers having different heuristics than the rest, where I couldn't get will-change to prevent repaints. If I understand correctly, this is because with BasicLayers, the relative benefit of having active layers is lower, so it makes sense that will-change would not have the same effect there as it has on "accelerated" layers.

Things that I tried included:
 - removing the text in the <div>'s;
 - replacing the gradient background by a solid color or by a background-image.
With no text and a solid color, I did avoid repaints on BasicLayers, but then, even without will-change nothing got repainted, so the first is() in that mochitest failed.

So I think that the present form is a reasonable compromise at this point. If we want to improve on it, we need to understand more precisely what effect will-change should have on BasicLayers.

Note that I had to move the CSS to a place where it wouldn't get parsed before we got a chance to set the pref that enables will-change. Whence the change from <style> to <script type="something_that_wont_be_parsed">.
Comment 69 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-21 11:27:54 PST
https://tbpl.mozilla.org/?tree=Try&rev=123b3885ea46
Comment 70 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-21 11:38:09 PST
Created attachment 8363129 [details] [diff] [review]
1/3 - Add will-change CSS property

Just a small style fix. See previous attachment comment for details about this patch.
Comment 71 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-22 09:30:31 PST
The only test failure on https://tbpl.mozilla.org/?tree=Try&rev=123b3885ea46 is on WinXP only, we get:

TEST-UNEXPECTED-PASS | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_will_change.html | false - will-change checkScrollRepaint element should not have been painted 

I managed to reproduce this failure on Linux by making this change:

diff --git a/layout/base/tests/chrome/test_will_change.html b/layout/base/tests/chrome/test_will_change.html
--- a/layout/base/tests/chrome/test_will_change.html
+++ b/layout/base/tests/chrome/test_will_change.html
@@ -8,18 +8,18 @@
   <script type="css_but_do_not_parse" id="csscode">
     #checkOpacityRepaint {
       will-change: opacity;
     }
     #checkTransformRepaint {
       will-change: transform;
     }
     #checkScrollRepaint {
-      width: 10px;
-      height: 10px;
+      width: 30px;
+      height: 45px;
       overflow: scroll;
       will-change: scroll-position;
     }


In other words, this test passes or fails depending on the width and height of the scrollable element, and the passing/failing values are not the same between platforms.

Here on Linux, empirically, the test gives me UNEXPECTED-PASS if the following condition is met:

  height >= 60 || (height >= 45 && width < 60)

This is independent of the contents of the div, and independent of text size (I was wondering if this was related to text size: it's not).
Comment 72 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-22 10:44:25 PST
Created attachment 8363814 [details] [diff] [review]
Remove the scrolling part of the mochitest, is too fragile

BenWa suggested just removing this fragile part of the test, given the trouble that it is giving us. If someone thinks that we really must have this covered by a test, please comment on how to test that reliably.
Comment 73 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-23 08:37:50 PST
For what it's worth, the failure investigated in comment 71 seems to be related to the painting of scrollbars... and indeed, doing some numerology, gcd(45,60)==15 and grepping for '15' through widget/gtk, we see that 15 pixels is the minimum size for NS_THEME_RESIZER in GTK:

http://hg.mozilla.org/mozilla-central/file/8dc7bf30840d/widget/gtk/nsNativeThemeGTK.cpp#l1241

This theory also explains why on Windows we already had this failure with the 10x10 pixels size: looking at nsNativeThemeWin.cpp we see that there is no minimum size for NS_THEME_RESIZER on Windows:

http://hg.mozilla.org/mozilla-central/file/8dc7bf30840d/widget/windows/nsNativeThemeWin.cpp#l2303

This doesn't explain why this fails specifically on WinXP and not on Win7/8, though. But it does seem that our failure has something to do with native theming counting as repaints.
Comment 74 User image David Baron :dbaron: ⌚️UTC-8 2014-01-24 14:19:48 PST
Comment on attachment 8363129 [details] [diff] [review]
1/3 - Add will-change CSS property

>+bool CSSParserImpl::ParseWillChange()
>+{
>+  nsCSSValue value;
>+  nsCSSValueList* cur = value.SetListValue();
>+  bool first = true;
>+  bool hasAuto = false;
>+  bool hasOtherThanAuto = false;
>+  for (;;) {
>+    if (!ParseVariant(cur->mValue, VARIANT_IDENTIFIER | VARIANT_INHERIT, nullptr)) {
>+      return false;
>+    }
>+
>+    if (cur->mValue.GetUnit() != eCSSUnit_Ident) {
>+      if (first) {
>+        break;
>+      } else {
>+        return false;
>+      }
>+    }
>+
>+    nsAutoString str;
>+    cur->mValue.GetStringValue(str);
>+    if (str.LowerCaseEqualsLiteral("all")) {
>+      return false;
>+    }
>+    if (str.LowerCaseEqualsLiteral("auto")) {
>+      hasAuto = true;
>+    } else {
>+      hasOtherThanAuto = true;
>+    }
>+
>+    if (CheckEndProperty()) {
>+      break;
>+    }
>+    if (!ExpectSymbol(',', true)) {
>+      REPORT_UNEXPECTED_TOKEN(PEExpectedComma);
>+      return false;
>+    }
>+    cur->mNext = new nsCSSValueList;
>+    cur = cur->mNext;
>+    first = false;
>+  }
>+
>+  if (hasAuto && hasOtherThanAuto) {
>+    // auto must be the only value
>+    return false;
>+  }

Per the current spec, you also need to exclude 'none' and 'all'.

It would also simplify the code if:
 * you added VARIANT_AUTO, VARIANT_NONE, and VARIANT_ALL to the variant mask
 * you explicitly reject eCSSUnit_None and eCSSUnit_All in all cases
 * you remove the hasOtherThanAuto / hasAuto business entirely, since simply adding VARIANT_AUTO to the mask and then relying on the 
    if (cur->mValue.GetUnit() != eCSSUnit_Ident) {
      if (first) {
        break;
      } else {
        return false;
      }
    }
   right below handles the parsing requirements
 * you make the nsRuleNode code expect 'auto' values to be represented as VARIANT_AUTO
Comment 75 User image David Baron :dbaron: ⌚️UTC-8 2014-01-24 14:22:31 PST
Er, you also need to include 'default'... you had ignoring default in the previous patch, but lost it in this patch.  (You have 'all' in this patch, but could do it the better way I describe above.)
Comment 76 User image David Baron :dbaron: ⌚️UTC-8 2014-01-24 14:29:49 PST
Comment on attachment 8363129 [details] [diff] [review]
1/3 - Add will-change CSS property

>+  /* Convert the nsCSSValueList into a will-change bitfield for fast lookup */
>+  const nsCSSValue* willChangeValue = aRuleData->ValueForWillChange();
>+  if (willChangeValue->GetUnit()) {

Don't put the whole thing inside this if; it doesn't match local code style.

>+    if (willChangeValue->GetUnit() == eCSSUnit_List ||
>+        willChangeValue->GetUnit() == eCSSUnit_ListDep)
>+    {
>+      display->mWillChange.Clear();
>+      display->mWillChangeBitField = 0;
>+      const nsCSSValueList* head = willChangeValue->GetListValue();
>+      while (head) {
>+        if (head->mValue.UnitHasStringValue()) {
>+          nsAutoString buffer;
>+          head->mValue.GetStringValue(buffer);
>+          if (buffer.Equals(NS_LITERAL_STRING("transform"))) {
>+            display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_TRANSFORM;
>+          }
>+          if (buffer.Equals(NS_LITERAL_STRING("opacity"))) {
>+            display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_OPACITY;
>+          }
>+          if (buffer.Equals(NS_LITERAL_STRING("scroll-position"))) {
>+            display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_SCROLL;
>+          }
>+          display->mWillChange.AppendElement(buffer);
>+        }
>+        head = head->mNext;
>+      }
>+    } else if (willChangeValue->GetUnit() == eCSSUnit_Inherit) {
>+      display->mWillChange = parentDisplay->mWillChange;
>+      display->mWillChangeBitField = parentDisplay->mWillChangeBitField;
>+    }

You also need to handle eCSSUnit_Initial and eCSSUnit_Unset (and, with the changes I suggest above, eCSSUnit_auto), with:

      display->mWillChange.Clear();
      display->mWillChangeBitField = 0;

(Sorry I missed that last time... though tests should have caught it.  If they didn't, I'd like to know why.)
Comment 77 User image David Baron :dbaron: ⌚️UTC-8 2014-01-24 14:39:28 PST
Comment on attachment 8363129 [details] [diff] [review]
1/3 - Add will-change CSS property

>+if (SpecialPowers.getBoolPref("layout.css.will-change.enabled")) {
>+	gCSSProperties["will-change"] = {
>+		domProp: "willChange",
>+		inherited: false,
>+		type: CSS_TYPE_LONGHAND,
>+		initial_values: [ "auto" ],
>+		other_values: [ "none", "all", "scroll-position", "transform", "opacity", "scroll-position, transform", "transform, opacity", "property-that-doesnt-exist-yet" ],
>+		invalid_values: [ "default", "auto, scroll-position", "scroll-position, auto", "inherit", "transform scroll-position", ",", "trailing," ]
>+	};
>+}
>+

Please add the test for the "contents" value that I mentioned in comment 63.

Please don't list "inherit" as an invalid value.  (I'm pretty sure you still haven't actually run most of the tests with the property enabled.)

>diff --git a/layout/style/test/test_all_shorthand.html b/layout/style/test/test_all_shorthand.html
>--- a/layout/style/test/test_all_shorthand.html
>+++ b/layout/style/test/test_all_shorthand.html

Please don't touch this file.  The prefs it currently touches are actually related to the feature it's testing.

Instead:
 * please address comment 74, comment 75, comment 76, and this comment
 * please locally run the mochitests in layout/style with the pref enabled and see if they pass (I think it's clear you haven't done this yet), and fix them if they don't
 * please do a try push with both the pref as you plan to commit it and one with the pref enabled
 * request review again, and please don't add other changes to the patch other than what's needed for the above
Comment 78 User image David Baron :dbaron: ⌚️UTC-8 2014-01-24 14:43:24 PST
Oh, and nsComputedDOMStyle::DoGetWillChange will need to produce 'auto' when it has an empty list.
Comment 79 User image David Baron :dbaron: ⌚️UTC-8 2014-01-24 16:24:24 PST
Also, it seems like implementing this part of the spec:

----
	If any non-initial value of a property would create a stacking context on the element, specifying that property in will-change must create a stacking context on the element.

If a non-initial value of a property would cause rendering differences on the element (such as using a different anti-aliasing strategy for text), the user agent should use that alternate rendering when the property is specified in will-change, to avoid sudden rendering differences when the property is eventually changed. 
----

requires that you track quite a few more CSS properties than you're currently tracking.  I suspect it means you'll want to track enough properties that you'll want to add bits in nsCSSPropList.h to say which properties trigger stacking contexts.  I'd prefer fixing that in a followup patch, but it should happen before the property gets enabled.
Comment 80 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-27 17:37:13 PST
Thanks for all the help. New try pushes, will ask for review if they're green:

Without will-change enabled:
https://tbpl.mozilla.org/?tree=Try&rev=4295a6158a54

With will-change enabled:
https://tbpl.mozilla.org/?tree=Try&rev=05ec83472bd4
Comment 81 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-28 07:18:51 PST
Another round. Things that were still wrong in the previous try pushes:
 - I hadn't yet moved "none" and "all" from other_values to invalid_values;
 - ParseWillChange was always returning a list; if I understand correctly, it shouldn't have in the 'inherit' and 'auto' cases, which are their own units separate from 'list'.
 - ComputeDisplayData wanted me to set canStoreInRuleTree = false; in the inherit case, as an assertion kindly told me.

Without will-change enabled:
https://tbpl.mozilla.org/?tree=Try&rev=fad12da167eb

With will-change enabled:
https://tbpl.mozilla.org/?tree=Try&rev=4a4d24c24588
Comment 82 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-28 10:52:12 PST
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #79)
> Also, it seems like implementing this part of the spec:
> 
> ----
> 	If any non-initial value of a property would create a stacking context on
> the element, specifying that property in will-change must create a stacking
> context on the element.
> 
> If a non-initial value of a property would cause rendering differences on
> the element (such as using a different anti-aliasing strategy for text), the
> user agent should use that alternate rendering when the property is
> specified in will-change, to avoid sudden rendering differences when the
> property is eventually changed. 
> ----
> 
> requires that you track quite a few more CSS properties than you're
> currently tracking.  I suspect it means you'll want to track enough
> properties that you'll want to add bits in nsCSSPropList.h to say which
> properties trigger stacking contexts.  I'd prefer fixing that in a followup
> patch, but it should happen before the property gets enabled.

I just filed bug 964885 about this.
Comment 83 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-28 10:55:16 PST
Created attachment 8366769 [details] [diff] [review]
1/3: Add will-change CSS property

Try results are green --- including the try push with will-change enabled.

In addition to addressing your review comments, there is a couple of other changes described in comment 81 needed to pass tests.

You guessed right about this not having passed tests before with will-change enabled. There were about 6 different mochitests failing in the previous try pushes further above.
Comment 84 User image David Baron :dbaron: ⌚️UTC-8 2014-01-28 14:07:12 PST
Comment on attachment 8366769 [details] [diff] [review]
1/3: Add will-change CSS property

nsCSSParser.cpp:
>+      const uint32_t variantMask = VARIANT_IDENTIFIER |
>+                                   VARIANT_INHERIT |
>+                                   VARIANT_NONE |
>+                                   VARIANT_ALL |
>+                                   VARIANT_AUTO;

This needs 2 spaces *less* indentation.

>+        if (cur->mValue.GetUnit() == eCSSUnit_Auto) {
>+          value.SetAutoValue();
>+        } else if (cur->mValue.GetUnit() == eCSSUnit_Inherit) {
>+          value.SetInheritValue();
>+        }

You actually need this for all the cases that go through
this path, and you should just change it to:

  value = cur->mValue;
  cur = nullptr; // the above assignment to value deleted cur

>+    nsAutoString str;
>+    cur->mValue.GetStringValue(str);
>+    if (str.LowerCaseEqualsLiteral("default")) {

nsString rather than nsAutoString, since nsCSSValue strings always
have a shared buffer so there's no need for a stack buffer.

nsRuleNode.cpp:
>+    const nsCSSValueList* head = willChangeValue->GetListValue();
>+    while (head) {
>+      if (head->mValue.UnitHasStringValue()) {
>+        nsAutoString buffer;
>+        head->mValue.GetStringValue(buffer);
>+        if (buffer.Equals(NS_LITERAL_STRING("transform"))) {
>+          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_TRANSFORM;
>+        }
>+        if (buffer.Equals(NS_LITERAL_STRING("opacity"))) {
>+          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_OPACITY;
>+        }
>+        if (buffer.Equals(NS_LITERAL_STRING("scroll-position"))) {
>+          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_SCROLL;
>+        }
>+        display->mWillChange.AppendElement(buffer);
>+      }
>+      head = head->mNext;
>+    }

Don't use |head| for a variable that iterates over the whole list,
and use a for loop rather than a while loop, to yield:

  for (const nsCSSValueList* item = willChangeValue->GetListValue();
       item; item = item->mNext) {
  }

Also, replace all 3 occurences of Equals(NS_LITERAL_STRING("x"))
with EqualsLiteral("x").

nsStyleStruct.h:
>+  uint8_t mWillChangeBitField; // see nsStyleConsts.h. Stores a bitfield

One more space of indent for the comment.  And also begin the comment
with "[reset]".

r=dbaron with that
Comment 85 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-29 05:34:43 PST
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #84)
> You actually need this for all the cases that go through
> this path, and you should just change it to:
> 
>   value = cur->mValue;
>   cur = nullptr; // the above assignment to value deleted cur

I had tried this, tried again to make sure, but this crashes with value.mUnit==0xa5a5a5a5a5... this is because by the time we assign cur->mValue.mUnit into value.mUnit, cur->mValue has already been deleted (this is a use-after-free). Here is a stack to where cur->mValue.mUnit gets set to 0xa5a5a5a5:

(gdb) bt 16
#0  __memset_sse2 () at ../sysdeps/x86_64/multiarch/../memset.S:335
#1  0x000000000042d70f in arena_dalloc_small (arena=0x7ffff7ecc040, chunk=0x7fffcb100000, ptr=0x7fffcb1e8790, 
    mapelm=0x7fffcb1015e0) at /hack/mozilla-central/memory/mozjemalloc/jemalloc.c:4452
#2  0x00000000004267e8 in arena_dalloc (ptr=0x7fffcb1e8790, offset=952208)
    at /hack/mozilla-central/memory/mozjemalloc/jemalloc.c:4583
#3  0x000000000042636d in free (ptr=0x7fffcb1e8790) at /hack/mozilla-central/memory/mozjemalloc/jemalloc.c:6493
#4  0x00007ffff7fe6115 in moz_free (ptr=0x7fffcb1e8790) at /hack/mozilla-central/memory/mozalloc/mozalloc.cpp:46
#5  0x00007ffff1d5c05f in operator delete (ptr=<optimized out>, ptr=<optimized out>)
    at ../../dist/include/mozilla/mozalloc.h:225
#6  nsCSSValueList_heap::Release (this=0x7fffcb1e8790) at /hack/mozilla-central/layout/style/nsCSSValue.h:756
#7  0x00007ffff1d37b03 in nsCSSValue::DoReset (this=0x7fffffff72c8)
    at /hack/mozilla-central/layout/style/nsCSSValue.cpp:334
#8  0x00007ffff0209c89 in nsCSSValue::Reset (this=0x7fffffff72c8) at ../../dist/include/nsCSSValue.h:531
#9  0x00007ffff1d23d5b in nsCSSValue::operator= (this=0x7fffffff72c8, aCopy=...)
    at /hack/mozilla-central/layout/style/nsCSSValue.cpp:189
#10 0x00007ffff1ce53fd in (anonymous namespace)::CSSParserImpl::ParseWillChange (this=0x7fffd8e48000)
    at /hack/mozilla-central/layout/style/nsCSSParser.cpp:11482
#11 0x00007ffff1cd5938 in (anonymous namespace)::CSSParserImpl::ParsePropertyByFunction (this=0x7fffd8e48000, 
    aPropID=eCSSProperty_will_change) at /hack/mozilla-central/layout/style/nsCSSParser.cpp:7834
#12 0x00007ffff1cd4370 in (anonymous namespace)::CSSParserImpl::ParseProperty (this=0x7fffd8e48000, aPropID=
    eCSSProperty_will_change) at /hack/mozilla-central/layout/style/nsCSSParser.cpp:7501
#13 0x00007ffff1cd1bf4 in (anonymous namespace)::CSSParserImpl::ParseProperty (this=0x7fffd8e48000, aPropID=
    eCSSProperty_will_change, aPropValue=..., aSheetURI=0x7fffc9674100, aBaseURI=0x7fffc9674100, 
    aSheetPrincipal=0x7fffc9671dc0, aDeclaration=0x7fffc96a06a0, aChanged=0x7fffffff7b57, aIsImportant=false, 
    aIsSVGMode=false) at /hack/mozilla-central/layout/style/nsCSSParser.cpp:1311
#14 0x00007ffff1cd18f0 in nsCSSParser::ParseProperty (this=0x7fffffff7b58, aPropID=eCSSProperty_will_change, 
    aPropValue=..., aSheetURI=0x7fffc9674100, aBaseURI=0x7fffc9674100, aSheetPrincipal=0x7fffc9671dc0, 
    aDeclaration=0x7fffc96a06a0, aChanged=0x7fffffff7b57, aIsImportant=false, aIsSVGMode=false)
    at /hack/mozilla-central/layout/style/nsCSSParser.cpp:12907
#15 0x00007ffff1d4f84f in nsDOMCSSDeclaration::ParsePropertyValue (this=0x7fffc9e37318, aPropID=
    eCSSProperty_will_change, aPropValue=..., aIsImportant=false)
    at /hack/mozilla-central/layout/style/nsDOMCSSDeclaration.cpp:331


If I understand correctly, the problem is that 'cur' is an entry in the linked list that 'value' currently is, and where we assign cur->mValue to 'value', this first resets 'value', which destroys the existing linked list, in particular destroying 'cur->mValue'.
Comment 86 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-29 05:53:22 PST
I am fixing this by having ParseWillChange work on a local nsCSSValue until it knows that that value should be appended to the list, rather than working right away on a list entry only to realize later that after all it wants to return a non-list.

I suppose that this should be non-controversial... rather than re-requesting review on the entire patch, I'll just check with you before landing this. Here's the new ParseWillChange:

bool CSSParserImpl::ParseWillChange()
{
  nsCSSValue listValue;
  nsCSSValueList* currentListValue = listValue.SetListValue();
  bool first = true;
  for (;;) {
    const uint32_t variantMask = VARIANT_IDENTIFIER |
                                 VARIANT_INHERIT |
                                 VARIANT_NONE |
                                 VARIANT_ALL |
                                 VARIANT_AUTO;
    nsCSSValue value;
    if (!ParseVariant(value, variantMask, nullptr)) {
      return false;
    }

    if (value.GetUnit() == eCSSUnit_None ||
        value.GetUnit() == eCSSUnit_All)
    {
      return false;
    }

    if (value.GetUnit() != eCSSUnit_Ident) {
      if (first) {
        AppendValue(eCSSProperty_will_change, value);
        return true;
      } else {
        return false;
      }
    }

    nsAutoString str;
    value.GetStringValue(str);
    if (str.LowerCaseEqualsLiteral("default")) {
      return false;
    }

    currentListValue->mValue = value;

    if (CheckEndProperty()) {
      break;
    }
    if (!ExpectSymbol(',', true)) {
      REPORT_UNEXPECTED_TOKEN(PEExpectedComma);
      return false;
    }
    currentListValue->mNext = new nsCSSValueList;
    currentListValue = currentListValue->mNext;
    first = false;
  }

  AppendValue(eCSSProperty_will_change, listValue);
  return true;
}
Comment 87 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-29 05:54:17 PST
Oh, and I am getting to your other review comments, including the strings comment on that function.
Comment 88 User image Benoit Jacob [:bjacob] (mostly away) 2014-01-29 08:56:26 PST
Created attachment 8367378 [details] [diff] [review]
1/3: Add will-change CSS property (Final form)
Comment 89 User image David Baron :dbaron: ⌚️UTC-8 2014-01-29 11:50:00 PST
Yes, that's fine.  Sorry for getting that wrong.
Comment 91 User image Benoit Girard (:BenWa) 2014-01-29 12:59:16 PST
Congrats. Thanks for the help bjacob!
Comment 93 User image Ivan Enderlin 2014-09-09 08:03:09 PDT
What is the state of this CSS property? Is it accepted by the CSS WG? I didn't find any documentations on MDN also.
Comment 94 User image Cameron McCormack (:heycam) (away 25 Feb–5 Mar) 2014-09-09 14:59:57 PDT
There is a specification http://dev.w3.org/csswg/css-will-change/ which was published in April as a First Public Working Draft.
Comment 95 User image Jean-Yves Perrier [:teoli] 2014-11-18 05:48:10 PST
Thanks to a lot of people (thank you all!) we now have
https://developer.mozilla.org/en-US/docs/Web/CSS/will-change
and a note in (note for activation will be added via another ddn)
https://developer.mozilla.org/en-US/Firefox/Releases/29#CSS

Note You need to log in before you can comment on or make changes to this bug.