Closed Bug 940842 Opened 11 years ago Closed 10 years ago

Add a will-change CSS property to *hint* something will animate (proxy for layerization)

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=CSS])

Attachments

(6 files, 27 obsolete files)

628 bytes, text/html
Details
6.09 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
5.68 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
3.03 KB, patch
mattwoodrow
: review+
BenWa
: review+
Details | Diff | Splinter Review
18.85 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
18.81 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #8335079 - Attachment is patch: true
Blocks: 931262
Blocks: 940734
Attached patch WIP 2: rebase + progress (obsolete) — Splinter Review
Attachment #8335079 - Attachment is obsolete: true
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.
Oh, and then you should add to layout/style/test/property_database.js and run the mochitests in layout/style/test/.
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.
Attached patch Part 1: Add CSS Property (obsolete) — Splinter Review
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?
Attachment #8335105 - Attachment is obsolete: true
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).
(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.
(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.
Renamed the title to suggest that we're looking to hint that something will animate and not infer anything about the implementation.
Summary: Add a CSS property to force an active layer → Add a CSS property to *hint* something will animate (proxy for layerization)
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.
(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.
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.
(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.
This patch wont land as-is. We need to agree on a name. But it works.
Assignee: nobody → bgirard
Attachment #8335467 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Part 2: Implementation (obsolete) — Splinter Review
This part is ready for review although the variable name may change to something like willAnimate.
Attachment #8335589 - Flags: review?(roc)
Attached patch Part 2: Implementation (obsolete) — Splinter Review
Removed printf
Attachment #8335589 - Attachment is obsolete: true
Attachment #8335589 - Flags: review?(roc)
Attachment #8335591 - Flags: review?(roc)
Attached patch Part 2: Implementation (obsolete) — Splinter Review
Attachment #8335591 - Attachment is obsolete: true
Attachment #8335591 - Flags: review?(roc)
Attachment #8335592 - Flags: review?(roc)
(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 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.
Attachment #8335592 - Flags: review?(roc) → feedback+
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
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.
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
I still have something wrong here because it doesn't handle having "will-animate: scroll transform". Otherwise it seems to be fine.
Attachment #8335588 - Attachment is obsolete: true
Attached file testcase
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?
Attachment #8336905 - Flags: feedback?(dbaron)
Attached patch Part 2: Implementation (obsolete) — Splinter Review
Attachment #8335592 - Attachment is obsolete: true
Attachment #8335841 - Attachment is obsolete: true
Attachment #8336906 - Flags: review?(matt.woodrow)
Attached patch Part 3: Mochitest (obsolete) — Splinter Review
Attachment #8337001 - Flags: review?(matt.woodrow)
Summary: Add a CSS property to *hint* something will animate (proxy for layerization) → Add a will-animate CSS property to *hint* something will animate (proxy for layerization)
Not sure if this is right or now but this fixes the homescreen pre-rendering being invalidated.
Attachment #8337322 - Flags: review?(matt.woodrow)
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!
Attachment #8337322 - Flags: review?(matt.woodrow) → review-
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 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.
Attachment #8337322 - Attachment is obsolete: true
Attachment #8337503 - Flags: review?(matt.woodrow)
Attached patch Part 2: Implementation (obsolete) — Splinter Review
Attachment #8336906 - Attachment is obsolete: true
Attachment #8336906 - Flags: review?(matt.woodrow)
Attachment #8337511 - Flags: review?(matt.woodrow)
Attached patch Part 3: Mochitest (obsolete) — Splinter Review
Fixed up part 3. Removing will-animate makes the test fail.
Attachment #8337001 - Attachment is obsolete: true
Attachment #8337001 - Flags: review?(matt.woodrow)
Attachment #8337503 - Flags: review?(matt.woodrow) → review+
Attachment #8337511 - Flags: review?(matt.woodrow) → review+
Posted proposal to www-style. Pending moderator review (up to 2 business days):
http://lists.w3.org/Archives/Public/www-style/
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.
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.
Attachment #8336905 - Attachment is obsolete: true
Attachment #8336905 - Flags: feedback?(dbaron)
Attachment #8342608 - Flags: review?(dbaron)
Supports computed style now. I still maintain the bitfield to do efficient lookups.
Attachment #8342608 - Attachment is obsolete: true
Attachment #8342608 - Flags: review?(dbaron)
For now this just lands it behind a preference for testing.
Attachment #8342716 - Attachment is obsolete: true
Attachment #8344702 - Flags: review?(dbaron)
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.
(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.
Why not update the syntax right now? It should be a simple change. will-animate -> will-change, scroll -> scroll-position.
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.
Rename title to reflect updated proposal.
Summary: Add a will-animate CSS property to *hint* something will animate (proxy for layerization) → Add a will-change CSS property to *hint* something will animate (proxy for layerization)
https://tbpl.mozilla.org/?tree=Try&rev=e12b74e68fc8

renamed property
Attachment #8344702 - Attachment is obsolete: true
Attachment #8344702 - Flags: review?(dbaron)
Attachment #8347643 - Flags: review?(dbaron)
Attached patch Part 2: Implementation (obsolete) — Splinter Review
Attachment #8337511 - Attachment is obsolete: true
Attachment #8347644 - Flags: review+
Attached patch Part 3: Mochitest (obsolete) — Splinter Review
Attachment #8337516 - Attachment is obsolete: true
Attachment #8347645 - Flags: review+
Attachment #8337503 - Attachment is obsolete: true
Attachment #8347646 - Flags: review+
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?
Attachment #8347643 - Flags: review?(dbaron) → review-
Blocks: 950713
(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.
Attachment #8347646 - Attachment is obsolete: true
Attachment #8349671 - Flags: review+
(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.
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.)
Flags: needinfo?(bgirard)
(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.
Flags: needinfo?(bgirard)
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).
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
Attachment #8347643 - Attachment is obsolete: true
Attachment #8361335 - Flags: review?(dbaron)
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?
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
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.
Attachment #8361875 - Flags: review?(dbaron)
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.
Attachment #8361335 - Flags: review?(dbaron) → review-
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.
Attachment #8361875 - Flags: review?(dbaron) → review?(matt.woodrow)
Blocks: will-change
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.
Attachment #8347644 - Attachment is obsolete: true
Attachment #8347645 - Attachment is obsolete: true
Attachment #8349671 - Attachment is obsolete: true
Attachment #8361335 - Attachment is obsolete: true
Attachment #8361875 - Attachment is obsolete: true
Attachment #8361875 - Flags: review?(matt.woodrow)
Attachment #8363122 - Flags: review?(dbaron)
(This new 2/3 patch folds together the old "part 2" and "part 4" patches).
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">.
Attachment #8363125 - Flags: review?(matt.woodrow)
Just a small style fix. See previous attachment comment for details about this patch.
Attachment #8363122 - Attachment is obsolete: true
Attachment #8363122 - Flags: review?(dbaron)
Attachment #8363129 - Flags: review?(dbaron)
Attachment #8363129 - Attachment description: bug-940842-part-1 → Add will-change CSS property
Attachment #8363129 - Attachment description: Add will-change CSS property → 1/3 - Add will-change CSS property
Attachment #8363125 - Flags: review?(matt.woodrow) → review+
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).
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.
Attachment #8363814 - Flags: review?(matt.woodrow)
Attachment #8363814 - Flags: review?(bgirard)
Attachment #8363814 - Flags: review?(bgirard) → review+
Attachment #8363814 - Flags: review?(matt.woodrow) → review+
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 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
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 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 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
Attachment #8363129 - Flags: review?(dbaron) → review-
Oh, and nsComputedDOMStyle::DoGetWillChange will need to produce 'auto' when it has an empty list.
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.
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
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
(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.
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.
Attachment #8363129 - Attachment is obsolete: true
Attachment #8366769 - Flags: review?(dbaron)
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
Attachment #8366769 - Flags: review?(dbaron) → review+
(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'.
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;
}
Flags: needinfo?(dbaron)
Oh, and I am getting to your other review comments, including the strings comment on that function.
Yes, that's fine.  Sorry for getting that wrong.
Flags: needinfo?(dbaron)
Congrats. Thanks for the help bjacob!
Whiteboard: [DocArea=CSS]
Blocks: 974125
Depends on: 975769
Depends on: 975789
Flags: in-testsuite?
What is the state of this CSS property? Is it accepted by the CSS WG? I didn't find any documentations on MDN also.
There is a specification http://dev.w3.org/csswg/css-will-change/ which was published in April as a First Public Working Draft.
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
You need to log in before you can comment on or make changes to this bug.