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

RESOLVED FIXED in mozilla29

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla29
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=CSS])

Attachments

(6 attachments, 27 obsolete attachments)

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
(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8335079 - Attachment is patch: true
(Assignee)

Updated

4 years ago
Blocks: 931262
(Assignee)

Updated

4 years ago
Blocks: 940734
(Assignee)

Comment 1

4 years ago
Created attachment 8335105 [details] [diff] [review]
WIP 2: rebase + progress
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/.
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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?
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).
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 11

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
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.
Assignee: nobody → bgirard
Attachment #8335467 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 15

4 years ago
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.
Attachment #8335589 - Flags: review?(roc)
(Assignee)

Comment 16

4 years ago
Created attachment 8335591 [details] [diff] [review]
Part 2: Implementation

Removed printf
Attachment #8335589 - Attachment is obsolete: true
Attachment #8335589 - Flags: review?(roc)
Attachment #8335591 - Flags: review?(roc)
(Assignee)

Comment 17

4 years ago
Created attachment 8335592 [details] [diff] [review]
Part 2: Implementation
Attachment #8335591 - Attachment is obsolete: true
Attachment #8335591 - Flags: review?(roc)
Attachment #8335592 - Flags: review?(roc)
(Assignee)

Comment 18

4 years ago
(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+
(Assignee)

Comment 20

4 years ago
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
Attachment #8335592 - Flags: feedback+ → feedback-
(Assignee)

Comment 23

4 years ago
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.
Attachment #8335588 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
Created attachment 8336571 [details]
testcase
(Assignee)

Comment 25

4 years ago
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?
Attachment #8336905 - Flags: feedback?(dbaron)
(Assignee)

Comment 26

4 years ago
Created attachment 8336906 [details] [diff] [review]
Part 2: Implementation
Attachment #8335592 - Attachment is obsolete: true
Attachment #8335841 - Attachment is obsolete: true
Attachment #8336906 - Flags: review?(matt.woodrow)
(Assignee)

Comment 27

4 years ago
Created attachment 8337001 [details] [diff] [review]
Part 3: Mochitest
Attachment #8337001 - Flags: review?(matt.woodrow)

Updated

4 years ago
Blocks: 942333

Updated

4 years ago
Blocks: 942339
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 28

4 years ago
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.
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 32

4 years ago
Created attachment 8337503 [details] [diff] [review]
part 4: Keep nsDisplayTransform active
Attachment #8337322 - Attachment is obsolete: true
Attachment #8337503 - Flags: review?(matt.woodrow)
(Assignee)

Comment 33

4 years ago
Created attachment 8337511 [details] [diff] [review]
Part 2: Implementation
Attachment #8336906 - Attachment is obsolete: true
Attachment #8336906 - Flags: review?(matt.woodrow)
Attachment #8337511 - Flags: review?(matt.woodrow)
(Assignee)

Comment 34

4 years ago
Created attachment 8337516 [details] [diff] [review]
Part 3: Mochitest

Fixed up part 3. Removing will-animate makes the test fail.
Attachment #8337001 - Attachment is obsolete: true
Attachment #8337001 - Flags: review?(matt.woodrow)

Updated

4 years ago
Blocks: 943086

Updated

4 years ago
Blocks: 943089
Attachment #8337503 - Flags: review?(matt.woodrow) → review+
Attachment #8337511 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 35

4 years ago
Posted proposal to www-style. Pending moderator review (up to 2 business days):
http://lists.w3.org/Archives/Public/www-style/

Updated

3 years ago
Blocks: 945461

Updated

3 years ago
Blocks: 945464

Updated

3 years ago
Blocks: 945514

Updated

3 years ago
Blocks: 945515

Updated

3 years ago
Blocks: 945516
(Assignee)

Comment 36

3 years ago
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.
(Assignee)

Comment 37

3 years ago
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.
Attachment #8336905 - Attachment is obsolete: true
Attachment #8336905 - Flags: feedback?(dbaron)
Attachment #8342608 - Flags: review?(dbaron)
(Assignee)

Comment 38

3 years ago
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.
Attachment #8342608 - Attachment is obsolete: true
Attachment #8342608 - Flags: review?(dbaron)
(Assignee)

Comment 39

3 years ago
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.
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.
(Assignee)

Comment 41

3 years ago
(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.
Blocks: 949895
Why not update the syntax right now? It should be a simple change. will-animate -> will-change, scroll -> scroll-position.
(Assignee)

Comment 43

3 years ago
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.
(Assignee)

Comment 44

3 years ago
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)
(Assignee)

Comment 45

3 years ago
Created attachment 8347643 [details] [diff] [review]
Part 1: Add will-change CSS Property v5

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)
(Assignee)

Comment 46

3 years ago
Created attachment 8347644 [details] [diff] [review]
Part 2: Implementation
Attachment #8337511 - Attachment is obsolete: true
Attachment #8347644 - Flags: review+
(Assignee)

Comment 47

3 years ago
Created attachment 8347645 [details] [diff] [review]
Part 3: Mochitest
Attachment #8337516 - Attachment is obsolete: true
Attachment #8347645 - Flags: review+
(Assignee)

Comment 48

3 years ago
Created attachment 8347646 [details] [diff] [review]
Part 4: Keep nsDisplayTransform active
Attachment #8337503 - Attachment is obsolete: true
Attachment #8347646 - Flags: review+
(Assignee)

Comment 49

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=39da8534dcdc
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-

Updated

3 years ago
Blocks: 950498
(Assignee)

Updated

3 years ago
Blocks: 950713
(Assignee)

Comment 51

3 years ago
(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.
(Assignee)

Comment 52

3 years ago
Created attachment 8349671 [details] [diff] [review]
Part 4: Keep nsDisplayTransform active
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.
Keywords: dev-doc-needed
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)
(Assignee)

Comment 55

3 years ago
(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).
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
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
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.
Attachment #8361875 - Flags: review?(dbaron)
https://tbpl.mozilla.org/?tree=Try&rev=f648a99fdf87
Tab started a spec draft at http://tabatkins.github.io/specs/css-will-change/ .
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)
(Assignee)

Updated

3 years ago
Blocks: 961871
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.
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)
Created attachment 8363123 [details] [diff] [review]
2/3 - Implement will-change in layers-building code (carries r+ from mattwoodrow)
Attachment #8363123 - Flags: review+
(This new 2/3 patch folds together the old "part 2" and "part 4" patches).
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">.
Attachment #8363125 - Flags: review?(matt.woodrow)
https://tbpl.mozilla.org/?tree=Try&rev=123b3885ea46
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.
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).
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.
Attachment #8363814 - Flags: review?(matt.woodrow)
Attachment #8363814 - Flags: review?(bgirard)
(Assignee)

Updated

3 years ago
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
Blocks: 964885
(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.
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.
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.
Created attachment 8367378 [details] [diff] [review]
1/3: Add will-change CSS property (Final form)
Attachment #8367378 - Flags: review+
Blocks: 965337
Yes, that's fine.  Sorry for getting that wrong.
Flags: needinfo?(dbaron)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c45c9d81123b                                    
https://hg.mozilla.org/integration/mozilla-inbound/rev/02f906d9f8f6                                    
https://hg.mozilla.org/integration/mozilla-inbound/rev/14139b186fba
Target Milestone: --- → mozilla29
(Assignee)

Comment 91

3 years ago
Congrats. Thanks for the help bjacob!
https://hg.mozilla.org/mozilla-central/rev/c45c9d81123b
https://hg.mozilla.org/mozilla-central/rev/02f906d9f8f6
https://hg.mozilla.org/mozilla-central/rev/14139b186fba
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [DocArea=CSS]
(Assignee)

Updated

3 years ago
Blocks: 974125

Updated

3 years ago
Depends on: 975769

Updated

3 years ago
Depends on: 975789

Updated

3 years ago
Flags: in-testsuite?

Comment 93

3 years ago
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.