1315874 - SMIL animations trigger CSS transitions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Mantaroh Yoshinaga[:mantaroh]]

Spec is:
'... except with any styles derived from declarative animations such as CSS Transitions, CSS Animations ([CSS3-ANIMATIONS]), and SMIL Animations ([SMIL-ANIMATION], [SVG11]) updated to the current time. ' [1]

[1] https://drafts.csswg.org/css-transitions-1/#before-change-style

So I think that we should not create new transition when changing style due to SMIL.
However, after release Firefox 41, we create new transition in above condition.

For example, following example, we expect blue box.
----------------------------------------------------------
<svg>
 <style>
 rect#rec {
   fill: red;
   transition: fill 1s;
 }
 </style>
 <rect width="50" height="50" id="rec">
   <animate attributeName="fill" to="blue" dur="1s" fill="freeze"/>
 </rect>
</svg>

(https://people-mozilla.org/~mantaroh/svg/transition_svg.svg)
----------------------------------------------------------

The result of each version is as follow:
Before Firefox 40	: Work correctly.
Firefox 46 ～ 48		: Repeat the color between red and blue.
After Firefox 49	: Display red box. (and create the new transition after SMIL is finished.)

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Sorry I wrote wrong comment.
The result of above example is as follow:

Before bug 1171966 :Work correctly.
After  bug 1171966 : Repeat the color between red and blue.
After  bug 1209405 : Display red box.

Perhaps, I think this bug contain two problems.
1) Transition will start when working reflow of SMIL.
2) SMIL's fill mode doesn't work.

I tried revert change of bug 1171996, then I confirmed that it work correctly.

Comment 2

[Andrew Overholt [:overholt]]

Brian, it seems bug 1171966 caused this. Can you take a quick look, please?

Comment 3

[Brian Birtles (:birtles)]

Mantaroh will look into this in for the next release.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Currently, we will create CSS-Transition always due to restyle from SMIL.
So Perhaps we will need to ignore creating the CSS-Transition when restyling from SMIL.

Local patch will fix this problem, I'm going to investigate impact of this patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f239f1ee6ead5de9dfc1c9c252f22530e8947ab

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: (WIP)Ignore the step of creating the transition when restyling with SVG Animations.

We can remove the step of creating the transition when restyling with SVG Animations.
Currently, we've run this step always when restyling with SVG Animations.
As result, many transitions are created when restyling with SVG.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fce41069250d1e043527a40a357fb59e7a64bb99

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Ignore the step of creating the transition when restyling of SVG Animations.

Hi Brian,

I think that this phenomenon is simillar to bug 1062106.
I wonder if you can help me understanding the this problem.

If we use attributeType="CSS", this phenomenon is occurred.
In this case, |mInAnimationOnlyStyleUpdate| is false when restyling. [1]
So transition will create when restyling of SVG Animation.

I think that we will need to drop creating transition when restyle hint is StyleAttribute_Animations.

I hope that you can point out where my above understanding is wrong.

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/style/nsTransitionManager.cpp#420

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8817354 [details] [diff] [review]
Ignore the step of creating the transition when restyling of SVG Animations.

Makes sense to me.

The test might need some work though:
* The duration is too short
* I'm not sure what getAnimations() is supposed to be doing there

I assume the test fails without the code changes in this patch?

dbaron should probably review this once you've tidied up the test.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to fix in 50.1.0 release

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1315874 part 1 - Add mochitest for SMIL with CSS-Transition.

Review commit: https://reviewboard.mozilla.org/r/98718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98718/

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

Currently RestyleManager will skip creating transition when restyling of animation only update. However SMIL will update style with eRestyle_StyleAttribute and eRestyle_StyleAttribute_Animations.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/Element.cpp#1987

In this patch, RestyleManager will skip creating transition when including eRestyle_StyleAttribute_Animations hint.

Review commit: https://reviewboard.mozilla.org/r/98722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98722/

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles, away 13-16 Dec) from comment #7)
> Comment on attachment 8817354 [details] [diff] [review]
> dbaron should probably review this once you've tidied up the test.

Thanks, Brian,
I've fixed previous patch.
I think that we can use mochitest instead reftests using by getAnimations function.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2452d82add1482cd0f5b86cfc67296811e5302ef

dbaron,
Transition will create due to SMIL restyling.
I think that this phenomenon occur calling Element::SetSMILOverrideStyleDeclaration. 

We will skip creating transition when restyling of animation only update[1]. However SMIL will update style with eResytle_StyleAttribute and eRestyle_StyleAttribute_Animations.[2]

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/layout/style/nsTransitionManager.cpp#420
[2] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/dom/base/Element.cpp#1987

This patch will ignore creating transition when style hint is eRestyle_StyleAttribute_Animations.
Could you please review this patch?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Looks like a pretty simple patch with a test included. Seems worth keeping on the radar for possible uplift to 51 if it lands soon.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8818779 [details]
Bug 1315874 part 1 - Add mochitest for SMIL with CSS-Transition.

https://reviewboard.mozilla.org/r/98718/#review103112

::: dom/smil/test/test_smilWithTransition.html:22
(Diff revision 1)
> +  // Animation doesn't start until onload
> +  SimpleTest.waitForExplicitFinish();
> +  window.addEventListener("DOMContentLoaded", runTests, false);

Seems like you should have a test assertion here that getComputedStyle(rect).fill is red, both (a) to flush style and (b) to ensure that there's actually the style change you expect.

::: dom/smil/test/test_smilWithTransition.html:31
(Diff revision 1)
> +  function runTests() {
> +    var rect = document.getElementById("rect");
> +    requestAnimationFrame(function(time) {
> +      var anim = document.getAnimations()[0];
> +      todo(!anim, "CSS-Transition shouldn't created by restyling of SMIL.");
> +      SimpleTest.finish();

And also maybe worth asserting here that the rect is green.

(Also, is this guaranteed to be after onload?  Maybe the event listener should be for load rather than DOMContentLoaded?)

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

https://reviewboard.mozilla.org/r/98722/#review103116

::: layout/base/RestyleManager.cpp:2968
(Diff revision 1)
> +      if (!(aRestyleHint & eRestyle_StyleAttribute_Animations)) {
> +        changedStyle = RestyleManager::TryInitiatingTransition(mPresContext,

So this isn't really an appropriate fix, because multiple style changes can be coalesced together.  So if you want to fix this bug at this level, your fix will either incorrectly suppress transitions that it should not (as this patch does) or fail to suppress transitions that it should.

I previously addressed this sort of thing by having separate animation and non-animation phases of restyling.  Those were removed in bug 960465, in favor of an UpdateOnlyAnimationStyles mechanism.

Patch 5 in bug 960465 (along with other parts of the bug) should have prevented this bug from existing.  Is it not working correctly?

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Thanks dbaron,

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #14)
> I previously addressed this sort of thing by having separate animation and
> non-animation phases of restyling. Those were removed in bug 960465, in
> favor of an UpdateOnlyAnimationStyles mechanism.
> 
> Patch 5 in bug 960465 (along with other parts of the bug) should have
> prevented this bug from existing. Is it not working correctly?

In Element::SetSMILOverrideStyleDeclaration, we will add pending restyle task to PendingTracker with eRestyle_StyleAttribute_Animations when compoing attribute. [1][2] 
Then, this restyle task will process immediately. [2][3]

[1] https://hg.mozilla.org/mozilla-central/annotate/2977ca1224525680cbfb5c3ce3018818b6dfd8f2/dom/base/Element.cpp#l1983
[2] https://dxr.mozilla.org/mozilla-central/rev/2977ca1224525680cbfb5c3ce3018818b6dfd8f2/dom/smil/nsSMILCompositor.cpp#84
[3] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/dom/smil/nsSMILCSSProperty.cpp#105

Currently, In UpdateOnlyAnimationStyles, we will check nsSMILAnimationController::mightHavePendingStyleUpdates.[4] This flag will updated from composed attribute result when last process of DoSample. [5]

[4] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/layout/base/RestyleManager.cpp#911
[5] https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/dom/smil/nsSMILAnimationController.cpp#455

As mentioned before, restyle will process immediately when composing attribute, so nsSMILAnimationController::mightHavePendingStyleUpdates doesn't update yet..

Perhaps, we will need to check that current phase is DoSample or not when add restyle task to PendingTrakcer.

WIP patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac79b5cb47b13a1235951cba76d62aa23818eca4

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

I changed the approach of this.

nsSMILCSSProperty::GetBaseValue will cause the restyling. So we will notify to the TransitionManager directly in this case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6ac7c0c92aa4025f4da4af0edd55d0f9d985c93

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8357a3def2befa6faee924b922552c92f7c3fe39

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8818779 [details]
Bug 1315874 part 1 - Add mochitest for SMIL with CSS-Transition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98718/diff/1-2/

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98722/diff/1-2/

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

https://reviewboard.mozilla.org/r/98722/#review108058

::: dom/smil/nsSMILCSSProperty.cpp:106
(Diff revision 2)
> +  // In GetCSSComputedValue(), we will update style. So we need to prevent
> +  // creating a CSS Transition.
> +  SetAnimationOnlyStyleUpdateToTransitionManager();
>    bool didGetComputedVal = GetCSSComputedValue(mElement, mPropID,
>                                                   computedStyleVal);
> +  UnsetAnimationOnlyStyleUpdateToTransitionManager();

I don't see why this is safe to do.  Can't there be other changes buffered that this will flush, that should start transitions?

::: dom/smil/nsSMILCSSProperty.cpp:286
(Diff revision 2)
> +void
> +nsSMILCSSProperty::SetAnimationOnlyStyleUpdateToTransitionManager() const
> +{
> +  nsTransitionManager* manager = GetTransitionManager();
> +  if (manager) {
> +    manager->SetInAnimationOnlyStyleUpdate(true);
> +  }
> +}

If you were to continue with this approach (which I'm skeptical of), you'd want:

 (a) to rename these methods (remove the "ToTransitionManager", although if it stayed it should have been "OnTransitionManager")
 
 (b) to have callers use them through an RAII helper class
 
 (c) probably to add more assertions to SetInAnimationOnlyStyleUpdate to assert that it's always flipping the value between true and false, and never leaving it the same

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8818780 [details]
Bug 1315874 part 2 - Skip creating CSS-Transition when SMIL restyling.

https://reviewboard.mozilla.org/r/98722/#review108058

Thanks dbaron,

I have several questions about this.

> I don't see why this is safe to do.  Can't there be other changes buffered that this will flush, that should start transitions?

I think that GetCSSComputedValue will call FlushPendingNotification finally, and we will need to prevent start transition when composing SMIL attributes.

Here's what I gather and I hope you can point out where I'm wrong:
 a. GetCSSComputed will call the nsComputedDOMStyle::UpdateCurrentStyleSourceｓ with needLayoutFlash flag is false.[1]
 b. Near the above this line, ValueFromString will call the nsComputedDOMStyle::GetStyleContextForElement[2].
 c. a) and b) will cause to call the FlushPendingNotification.
 d. In the result of a)-c), Compositing of SMIL Attribute will occur the style flush.

[1] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/layout/style/nsComputedDOMStyle.cpp#975
[2] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/dom/smil/nsSMILCSSValueType.cpp#364

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

None of that contradicts anything I said.  The problem is that the flush will flush all pending changes, some of which might be for other things that should start transitions.

It's also far from ideal to insert additional flushes in our own code.  I'm somewhat unhappy to see that SMIL code is triggering flushes at all.

Comment 23

[Brian Birtles (:birtles)]

I had a quick look into what we need to do here. I think we want to align our base value handling with what we
do in dom/animation since that should mean:

a) More code reuse / less SMIL-specific hooks into the style system we need to add to Stylo
b) It's easier to migrate SMIL to re-use other animation machinery in future

To recap what we do for base values in dom/animation we resolve base styles using
nsStyleSet::ResolveStyleByRemovingAnimation which we call in the following situations:

 i. When the style context on the target element changes. Specifically, from nsStyleSet::GetContext where we
    call EffectCompositor::UpdateEffectProperties.
ii. When an animation is created or modified in such a way that might cause us to start needing base values.
    Specifically, from KeyframeEffect::SetTarget and KeyframeEffect::SetComposite.

That is, we don't do it on each tick, but only when we detect we might need to.

Adapting a similar approach in SMIL woukd mean we basically want to update base values in the following
situations:

 α. When an new SVG Animation element is created. We might refine this further to new animation elements that
    have additive="add" or use to-animation (i.e. basically when its animation function returns true from
    WillReplace()).
 β. When the additive attribute changes or when we become a to-animation (e.g. the from attribute is dropped).

There are a few tricky bits here. We *could* try to detect changes to the animation sandwich and not update
base values when additive animations build on top of non-additive ones but I suspect that's too complex.

We could also limit this to when we're animating a CSS property but base values for attribute animations can
be dependent on context too. For example, length attributes uses nsSVGLength2 whose GetBaseValue method
returns a normalized value that is dependent on the current font size.

All that might mean we regress a few tests due to things like bug 1254424 (i.e. failing to respond to changes
to the style of ancestors) but that will simply mean SMIL behaves more like CSS animations/transitions so it
probably acceptable as temporary regression.

If there are more significant regressions we might need to do some special case handling so that we do less
aggressive caching of non-CSS base values (like we currently do).

As for how to store this, the natural place to use is nsSMILAnimationController::mLastCompositorTable.
Basically we have the following arrangement:

  nsSMILAnimationController::mLastCompositorTable - Store cached base values from the last sample. When we
    sample again, we copy the cached values across to the new compositor.
  nsSMILAnimationController::mAnimationElementTable - Stores the actual animation elements (e.g. <animate>) we
    need to sample.

The tricky party here is that mLastCompositor is a hashtable keyed-off a triple of (target element, attribute
name, attribute namespace).

This works fine for β above since animation elements only animate a single attribute so we can easily find the
corresponding compositor in mLastCompositorTable and update it.

The hard part is α since in that case we want to update the base values in mLastCompositorTable for
a particular *target element*. We *could* do that by iterating through |mAnimationElementTable| and skipping
any elements that don't target the target element. For the remaining elements, we can easily find their
corresponding compositor mLastCompositorTable and update them. The problem with that, however, is if we need
to restyle 100 elements each with their own animation element, we basically visit 100 animation elements 100
times, i.e. it's an O(n^2) operation.

Instead, I think we might want to just update all compositors at once whenever we decide we need to update one
of them. I think we could probably arrange that by using the animation generation on the restyle manager and
store the latest animation generation we updated the base styles for on the nsSMILAnimationController. I'm not
sure quite how that would work for regular attributes whose base values are context-sensitive, however.

They're my initial thoughts anyway. I think something like that would at least prevent us from flushing style
when we go to get the base style. I still need to work out if we can get rid of the other flush in DoSample.

Comment 24

[Brian Birtles (:birtles)]

Marking this P1 since I think we should do this as part of getting SMIL to work with Stylo.

Comment 25

[Brian Birtles (:birtles)]

Wow, that's funny. I looked into why we Flush during DoSample and it appears to be that we added it in bug 592477 because the flush in GetBaseValue was sometimes causing us to crash (see bug 592477 comment 25)!

So in theory, at least, if we remove the flush from GetBaseValue we might be able to remove the flush from DoSample.

Comment 26

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
> (https://people-mozilla.org/~mantaroh/svg/transition_svg.svg)
> ----------------------------------------------------------
> 
> The result of each version is as follow:
> Before Firefox 40	: Work correctly.
> Firefox 46 ～ 48		: Repeat the color between red and blue.
> After Firefox 49	: Display red box. (and create the new transition after
> SMIL is finished.)

Sorry, my people.mozilla account has changed.
I confirmed using the following example to find regression version range:
https://people-mozilla.org/~myoshinaga/svg/bug1315874/test-fill-withTransition.html

Comment 27

[Brian Birtles (:birtles)]

Attachment: WIP patch

I had a go at fixing this. For some reason I tied skipping flushing styles to using the sort of out-of-band
updating base values that we use in dom/animation but I'm not completely sure they are both necessary.

This WIP patch mostly works but fails six tests that I'm aware of. There are two root causes for those
failures:

a) We fail to update base values when we update mapped attributes on the target element. This should be easy
   to fix. It basically just involves adding another method to nsSMILAnimationController that will update
   a single property if there is an existing cached base value (much like the current UpdateBaseValues but for
   a single value).

b) We fail to update base values when an SVG animatable type (e.g. SVGLength) is updated. We could fairly
   easily make all these types do the same as (a) but that sounds like a lot of code so I suspect we should
   actually handle base styles and base attribute values differently. It's not too costly to fetch these base
   values on each tick (which is what we currently do) so we should just do that. We still need to cache them
   (so we can compare and see if the base value has changed and we need to recomposite) but we don't need to
   do the caching out-of-band.

Stealing this for now as discussed with Mantaroh.

Comment 28

[Brian Birtles (:birtles)]

I made a start on this but there are two problems with my approach:

1) I tried to make updating the base style out-of-band (i.e. not during DoSample). As mentioned in comment 27,
   I'm not sure why I conflated updating the base style out-of-band and not flushing. DoSample doesn't run as
   part of restyling so, provided we are careful not to flush, we should be able to get a style context
   without triggering the sort of nasty recursive issues we hit when adding base styles to dom/animation. The
   out-of-band logic is fairly solid but since we don't resolve base values for attribute animations
   out-of-band, adding this logic means we end up with two code paths which complicates the code.

2) I tried to use mLastCompositorTable to store the base styles. This *almost* works. The trouble is
   animations that start in the future. In that case when we go to build up the current compositor table we
   don't add those animation functions (since they're not *active*) and as a result we can't easily determine
   if we should create a new compositor for storing the base value from mLastCompositorTable or not.

   We could work around this by adding a flag to nsSMILCompositor and simply set that if we have any animation
   elements targetting the element that are additive (even if they're not active). That would probably work
   without adding too much more complexity but I'm afraid we'd end up preserving that table forever. That's
   because unlike CSS/Web Animations which are run-once, SMIL animations can be scheduled to re-run many times
   so it's hard to determine when they'd no longer need that base style.

With that said, I'm going to try and just resolve the base styles as part of ComposeAttribute using some version of nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation/NoFlush and see how that goes.

I'd still like to post the patches for this first approach since, apart from (2) above, I think they're reasonably solid and we may decide to retry that approach. However Mercurial has been "bundling" the changes for about 2 hours now and now it's just sitting there doing nothing so I'll have to wait and see if it ever succeeds in pushing them to my user repo.

Comment 29

[Brian Birtles (:birtles)]

Previous approach finally pushed to user repo: https://hg.mozilla.org/users/bbirtles_mozilla.com/main/log/59c166953b5b

Comment 30

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Drop a few remaining references to attributeType;

These are left over from bug 1062106.

Review commit: https://reviewboard.mozilla.org/r/125894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125894/

Comment 31

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Sort includes in nsSMILAnimationController.cpp;

We will add to these includes later in this patch series so we sort them now so
it's clear where to add to.

Review commit: https://reviewboard.mozilla.org/r/125896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125896/

Comment 32

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

Review commit: https://reviewboard.mozilla.org/r/125898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125898/

Comment 33

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Use UniquePtr for handling heap-allocated nsISMILAttr objects;

Later in this patch series we will add another call site for CreateSMILAttr so
this seems like a good opportunity to convert this to UniquePtr which should be
safer than passing back raw pointers the caller needs to free.

I've also replaced a few uses of () with {} to make these lines unambiguous
(i.e. avoid "C++'s most vexing parse").

Review commit: https://reviewboard.mozilla.org/r/125900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125900/

Comment 34

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

nsSMILCompositor::mCachedBaseValue is an nsAutoPtr<nsSMILValue> that we allocate
on the heap. It looks like we did that back in bug 533291 presumably because it
makes transferring these cached values between nsSMILCompositor objects cheaper.

One drawback of this, however, is that mCachedBaseValue has two null states:
the mCachedBaseValue pointer can be null, and the pointed-to nsSMILValue can be
a null value (i.e. IsNull() returns true).

Now that we have move ctors and operators defined for nsSMILValue we can
transfer these objects between compositors cheaply without requiring the object
to be allocated as separate heap object. This patch makes mCachedBaseValue just
a regular nsSMILValue class member (i.e. drops the nsAutoPtr).

There's a subtle difference in behavior with regards to the first sample.
Previously we would compare the (initially) null mCachedBaseValue pointer with
the passed-in nsSMILValue and set mForceCompositing to true. With this patch,
however, we will only set mForceCompositing to true if the passed-in
mCachedBaseValue is not null.

I believe this is correct, however, since if we don't call GetBaseValue in
ComposeAttribute we should not be setting mForceCompositing to true (something
else should ensure that gets set to true), and if we do call GetBaseValue the
result should not be a null nsSMILValue (except in some OOM cases where we don't
really care if we miss a sample). This patch adds an assertion to check that
GetBaseValue does, in fact, return a non-null value. (I checked the code and
this appears to be the case. Even in error cases we typically return an empty
nsSMILValue of a non-null type. For example, the early return in
nsSMILCSSProperty::GetBaseValue() does this.)

Review commit: https://reviewboard.mozilla.org/r/125902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125902/

Comment 35

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Factor out nsSMILCompositor::IsCSSPropertyAnimation helper method;

In a later patch in this series we willl create a method to check if we're
likely to need to resolve base styles or not, and for that we need to extract
the check for whether or not we're animating a CSS property.

This method doesn't necessarily need to be public, but since it's a static
method there doesn't seem to be any harm in making it so.

Review commit: https://reviewboard.mozilla.org/r/125904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125904/

Comment 36

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Add check for attribute namespace ID when decided if it should animate as a CSS property or not;

This seems like an existing bug. If the content specifies
attributeType="yer:opacity", we should not try to animate it as a CSS property.
This patch adds a namespace check before we try to animate as a CSS property.

Review commit: https://reviewboard.mozilla.org/r/125906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125906/

Comment 37

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

In a subsequent patch we will extract a StyleAnimationValue from an
nsStyleContext. Rather than serializing it and then re-parsing it into
a StyleAnimationValue, this patch adds a method to directly accept
a StyleAnimationValue since that is what nsSMILCSSValueType stores internally
anyway.

Review commit: https://reviewboard.mozilla.org/r/125908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125908/

Comment 38

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Add MightNeedBaseStyle helper method;

We will use this in a subsequent patch in this series to determine if we need to
fetch a style context at the beginning of ComposeAttribute (when we create the
nsISMILAttr) or not.

Review commit: https://reviewboard.mozilla.org/r/125910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125910/

Comment 39

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

There are currently no call sites of GetStyleContextForElementWithoutAnimation
but there are a couple of places (including this patch series) where we would
like to use such a method, but without the flushing behavior.

This patch drops the flushing from GetStyleContextForElementWithoutAnimation and
renames it to GetNonAnimationStyleContextNoFlush. It also makes a few other
minor tweaks:

* Adds the aStyleType parameter for consistency with
  GetStyleContextForElementNoFlush and GetStyleContextForElement
* Moves the implementation of this method and GetStyleContextForElementNoFlush
  to the header file since they simply wrap DoGetStyleContextForElementNoFlush.
* Changes the order of these declarations so that the more specialized method
  comes later.
* Drops the comment describing the method since both the method name and the
  inline definition should now make it obvious what it is doing.

(Actually making this method not return animation styles when it re-uses an
existing style context will happen in the next patch.)

Review commit: https://reviewboard.mozilla.org/r/125912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125912/

Comment 40

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

Review commit: https://reviewboard.mozilla.org/r/125914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125914/

Comment 41

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

In ensuring that this method does *not* re-use the existing style context if
aStyleType != eAll, I found it easy to miss this check in the multi-condition if
station the way some, but not all, of the conditions are on one line. I think
this would be a little easier to read if this multi-line condition used one line
per condition.

Review commit: https://reviewboard.mozilla.org/r/125916/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125916/

Comment 42

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

We will use this in the next patch in this series to fetch the base style.

(I tried simply passing this as a parameter to GetBaseStyle but it proved to be
more complicated since all the overrides of GetBaseStyle would need to be
updated to take a parameter that would be irrelevant to most of them.)

Review commit: https://reviewboard.mozilla.org/r/125918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125918/

Comment 43

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

Review commit: https://reviewboard.mozilla.org/r/125920/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125920/

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8853864 [details]
Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

This has already been reviewed but MozReview is confused.

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 8853863 [details]
Bug 1315874 - Sort includes in nsSMILAnimationController.cpp;

https://reviewboard.mozilla.org/r/125896/#review128688

r=me

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8853865 [details]
Bug 1315874 - Use UniquePtr for handling heap-allocated nsISMILAttr objects;

https://reviewboard.mozilla.org/r/125900/#review128692

r=me

::: dom/smil/nsSMILCompositor.h:76
(Diff revision 1)
>   private:
> -  // Create a nsISMILAttr for my target, on the heap.  Caller is responsible
> -  // for deallocating the returned object.
> +  // Create a nsISMILAttr for my target, on the heap.
> +  UniquePtr<nsISMILAttr> CreateSMILAttr();
> -  nsISMILAttr* CreateSMILAttr();

s/UniquePtr/mozilla::UniquePtr/

::: dom/smil/nsSMILCompositor.cpp:59
(Diff revision 1)
>    if (!mKey.mElement)
>      return;
>  
>    // FIRST: Get the nsISMILAttr (to grab base value from, and to eventually
>    // give animated value to)
> -  nsAutoPtr<nsISMILAttr> smilAttr(CreateSMILAttr());
> +  UniquePtr<nsISMILAttr> smilAttr{CreateSMILAttr()};

If you like, I'd suggest the standard assignment-as-constructor syntax here -- I believe it's equivalent, and I find it more readable:
  UniquePtr<nsISMILAttr> smilAttr = CreateSMILAttr();

...but I suppose this is fine too.

(If you do change this to use assignment syntax, be sure to update the extended commit message, where you have a note about {}.)

::: dom/smil/nsSMILCompositor.cpp:116
(Diff revision 1)
>  nsSMILCompositor::ClearAnimationEffects()
>  {
>    if (!mKey.mElement || !mKey.mAttributeName)
>      return;
>  
> -  nsAutoPtr<nsISMILAttr> smilAttr(CreateSMILAttr());
> +  UniquePtr<nsISMILAttr> smilAttr{CreateSMILAttr()};

Here as well, I'd lean towards assignment (if you do want to change this), but not a big deal.

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8853866 [details]
Bug 1315874 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue;

https://reviewboard.mozilla.org/r/125902/#review128708

r=me with one suggested comment nit.  (Thanks for the nice explanatory extended-commit-message here, BTW!)

::: dom/smil/nsSMILCompositor.h:100
(Diff revision 1)
>    // Cached base value, so we can detect & force-recompose when it changes
> -  // from one sample to the next. (nsSMILAnimationController copies this
> +  // from one sample to the next. (nsSMILAnimationController moves this
>    // forward from the previous sample's compositor.)

Might be good to mention "StealCachedBaseValue" in the comment here, since that's what actually does the Move()'ing of this value.

e.g. at the end of the parenthetical, maybe add:
 ", via StealCachedBaseValue()."

Comment 48

[Daniel Holbert [:dholbert]]

Comment on attachment 8853867 [details]
Bug 1315874 - Factor out nsSMILCompositor::IsCSSPropertyAnimation helper method;

https://reviewboard.mozilla.org/r/125904/#review128712

r- since this probably merits another look after the feedback has been addressed.

::: dom/smil/nsSMILCompositor.h:75
(Diff revision 1)
> +  // Returns true if, given an attributeName and corresponding property ID,
> +  // an animation targetting |aTargetElement| will animate the corresponding CSS
> +  // property rather than an attribute.
> +  //

Two things:

(1) s/targetting/targeting/  (i.e. drop the double-"t")

(The two-t version is perhaps an obsolete Britishism, according to https://en.wiktionary.org/wiki/targetting

Also, http://www.future-perfect.co.uk/grammar-tip/is-it-targetted-or-targeted/ is a UK source which agrees on the single-"t" form, though it focuses primarily on "-ed" rather than "-ing".)


(2) The relationship between the attribute name and property ID is confusing here, particularly in the case where the attribute *has* no "corresponding property" -- in that case, "the corresponding property ID" is a meaningless concept, so it's unclear what aPropID is expected to contain/represent then.  How about just focus on the attribute-name arg in the first sentence/paragraph here, and then add an afterthought to mention the expectation/optimization about "aPropID"?

::: dom/smil/nsSMILCompositor.cpp:127
(Diff revision 1)
> +nsSMILCompositor::IsCSSPropertyAnimation(nsIAtom* aAttributeName,
> +                                         nsCSSPropertyID aPropID,
> +                                         mozilla::dom::Element* aTargetElement)
> +{
> +  MOZ_ASSERT(aAttributeName);
> +  MOZ_ASSERT(aTargetElement);

You should add an assertion about the expected relationship between aAttributeName and aPropID here.  (a debug-only call to LookupProperty, I think.)  This will help reinforce/document/verify that callers should pass these args in a consistent/coherent manner.

::: dom/smil/nsSMILCompositor.cpp:143
(Diff revision 1)
> +  bool animateAsAttr = (aAttributeName == nsGkAtoms::width ||
> +                        aAttributeName == nsGkAtoms::height) &&
> +                        aTargetElement->GetNameSpaceID() == kNameSpaceID_SVG &&
> +                        !aTargetElement->IsAttributeMapped(aAttributeName);
> +
> +  return !animateAsAttr;

This inverted-logic with the local var ("return !animateAsAttr") is kinda confusing.  Let's just rework the logic slightly to replace the confusing single-usage local-var with an early-return.  (This wasn't as straightforward in the old place where this code lived, but it's quite easy in this new refactored home.)

Specifically: let's replace the code starting at "bool animateAsAttr" with something like the following:
=====
  if (animating width or height on non-root element) {
    return false;
  }

  // Otherwise, we should treat this as an animation of the
  // corresponding CSS property.
  return true;
=====

Comment 49

[Daniel Holbert [:dholbert]]

Comment on attachment 8853868 [details]
Bug 1315874 - Add check for attribute namespace ID when decided if it should animate as a CSS property or not;

https://reviewboard.mozilla.org/r/125906/#review128720

::: commit-message-d6d29:1
(Diff revision 1)
> +Bug 1315874 - Add check for attribute namespace ID when decided if it should animate as a CSS property or not; r=dholbert

s/when decided/when deciding/

::: dom/smil/nsSMILCompositor.cpp:161
(Diff revision 1)
>    nsCSSPropertyID propID =
>      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
>                                 CSSEnabledState::eForAllContent);
>  
> -  if (IsCSSPropertyAnimation(mKey.mAttributeName, propID, mKey.mElement)) {
> +  if (IsCSSPropertyAnimation(mKey.mAttributeNamespaceID, mKey.mAttributeName,

So now we'll be calling nsCSSProps::LookupProperty on the attribute-name, and then later (inside of this helper function) we check the namespace.

This feels a little backwards. Shouldn't we be checking the namespace ID *before* we bother calling nsCSSProps::LookupProperty? If there's a namespace, then clearly it's not a valid CSS property and there are no circumstances under which we should care about what nsCSSProps::LookupProperty says, I think.

So from a cart-before-the-horse perspective, as well as from an efficiency perspective, I tend to think the (fast) namespace check should happen first & should prevent us from ever calling nsCSSProps::LookupProperty.

Comment 50

[Daniel Holbert [:dholbert]]

Comment on attachment 8853867 [details]
Bug 1315874 - Factor out nsSMILCompositor::IsCSSPropertyAnimation helper method;

https://reviewboard.mozilla.org/r/125904/#review128730

::: dom/smil/nsSMILCompositor.cpp:160
(Diff revision 1)
>  {
>    nsCSSPropertyID propID =
>      nsCSSProps::LookupProperty(nsDependentAtomString(mKey.mAttributeName),
>                                 CSSEnabledState::eForAllContent);
> -  if (nsSMILCSSProperty::IsPropertyAnimatable(propID)) {
> -    // If we are animating the 'width' or 'height' of an outer SVG
> +
> +  if (IsCSSPropertyAnimation(mKey.mAttributeName, propID, mKey.mElement)) {

OK, I have an idea to restructure this which I think would help sort out some of my comments here & for the next patch.

How about:
 (1) we make the new helper-method *perform the propID lookup itself*, and then *return the property ID as its actual return type*. (rather than a boolean value)
 (2) In cases where your current method should return false, it should return the sentinel value eCSSProperty_UNKNOWN.  So notably, width/height would usually return eCSSProperty_UNKNOWN from this method (except for on the root element, in which case they'd return the expected property ID)
 (3) The helper-method can do the namespace lookup (like you've got it doing in your next patch), BUT that should happen before it calls nsCSSProps::LookupProperty.
 (4) It should also just be a normal method (not a static method), since AFAICT all of the callers pass in the same member-data, which a legit method could simply read directly.

This removes some awkwardness that the current setup is running into, I think.  What do you think?

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

https://reviewboard.mozilla.org/r/125910/#review128726

Not r+'ing this one, since I think it'll change soon, per comment 50.

Minor comment nit, though:

::: dom/smil/nsSMILCompositor.h:92
(Diff revision 1)
>   private:
>    // Create a nsISMILAttr for my target, on the heap.
>    UniquePtr<nsISMILAttr> CreateSMILAttr();
>  
> +  // Returns true if we might need to refer to base styles (i.e. we are
> +  // targetting a style property and have one or more animation functions that

s/targetting/targeting/

s/style property/CSS property/

Comment 52

[Brian Birtles (:birtles)]

Thanks Daniel. I'll go and rework that awkwardness. It mostly is just left over from the previous attempt I made at this bug where we really did want those methods to be public static and really did want to pass in that property separately. Now that I've simplified the patch series, what you suggest seems much better.

Comment 53

[Brian Birtles (:birtles)]

Tidy up patches have been split off into bug 1353208.

Comment 54

[Brian Birtles (:birtles)]

Comment on attachment 8853864 [details]
Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125898/diff/1-2/

Comment 55

[Brian Birtles (:birtles)]

Comment on attachment 8853869 [details]
Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125908/diff/1-2/

Comment 56

[Brian Birtles (:birtles)]

Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125910/diff/1-2/

Comment 57

[Brian Birtles (:birtles)]

Comment on attachment 8853871 [details]
Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125912/diff/1-2/

Comment 58

[Brian Birtles (:birtles)]

Comment on attachment 8853872 [details]
Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125914/diff/1-2/

Comment 59

[Brian Birtles (:birtles)]

Comment on attachment 8853873 [details]
Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125916/diff/1-2/

Comment 60

[Brian Birtles (:birtles)]

Comment on attachment 8853874 [details]
Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125918/diff/1-2/

Comment 61

[Brian Birtles (:birtles)]

Comment on attachment 8853875 [details]
Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125920/diff/1-2/

Comment 62

[Brian Birtles (:birtles)]

Comment on attachment 8853864 [details]
Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

(This patch has already been reviewed.)

Comment 63

[Daniel Holbert [:dholbert]]

Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

https://reviewboard.mozilla.org/r/125910/#review128988

Comment 64

[Daniel Holbert [:dholbert]]

Comment on attachment 8853869 [details]
Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

https://reviewboard.mozilla.org/r/125908/#review128992

::: dom/smil/nsSMILCSSValueType.cpp:428
(Diff revision 2)
> +  nsAutoString valueAsString;
> +  DebugOnly<bool> uncomputeResult =
> +    StyleAnimationValue::UncomputeValue(aPropID, aValue, valueAsString);
> +  // |aValue| is typically created using
> +  // StyleAnimationValue::ExtractComputedValue so we should be able to
> +  // serialize it.
> +  MOZ_ASSERT(uncomputeResult,
> +             "Should be able to serialize provided StyleAnimationValue");
> +
> +  nsIDocument* doc = aTargetElement->GetUncomposedDoc();
> +  if (doc && !nsStyleUtil::CSPAllowsInlineStyle(nullptr,
> +                                                doc->NodePrincipal(),
> +                                                doc->GetDocumentURI(),
> +                                                0, valueAsString, nullptr)) {
> +    return result;

I initially assumed this patch was meant to be "shortcut" to avoid serialization/deserialization. But it seems we're still stringifying here, so it doesn't seem like we save much after all. :(

HOWEVER, do we really have to stringify? It looks like it's only for the CSPAllowsInlineStyle call, and it's only there "for reporting violations":
https://dxr.mozilla.org/mozilla-central/rev/b5d8b27a753725c1de41ffae2e338798f3b5cacd/layout/style/nsStyleUtil.h#184-185

And I kinda doubt that an intermediate CSS value would be very useful for CSP-violation-error reporting anyway. (depending on how this is used).

Could we just skip the stringification here, and instead pass in something like NS_LITERAL_STRING("[SVG animation of CSS]")?

Comment 65

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #64)
> Comment on attachment 8853869 [details]
> Bug 1315874 - Add a method to nsSMILCSSValueType that takes a
> StyleAnimationValue;
> I initially assumed this patch was meant to be "shortcut" to avoid
> serialization/deserialization. But it seems we're still stringifying here,
> so it doesn't seem like we save much after all. :(

We still save the parse/style-resolution/conversion steps which I think is the more significant part here.

> HOWEVER, do we really have to stringify? It looks like it's only for the
> CSPAllowsInlineStyle call, and it's only there "for reporting violations":
> https://dxr.mozilla.org/mozilla-central/rev/
> b5d8b27a753725c1de41ffae2e338798f3b5cacd/layout/style/nsStyleUtil.h#184-185
> 
> And I kinda doubt that an intermediate CSS value would be very useful for
> CSP-violation-error reporting anyway. (depending on how this is used).
> 
> Could we just skip the stringification here, and instead pass in something
> like NS_LITERAL_STRING("[SVG animation of CSS]")?

Sure, sounds good to me!

Comment 66

[Brian Birtles (:birtles)]

Comment on attachment 8853864 [details]
Bug 1315874 - Add test that SMIL does not trigger CSS Transitions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125898/diff/2-3/

Comment 67

[Brian Birtles (:birtles)]

Comment on attachment 8853869 [details]
Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125908/diff/2-3/

Comment 68

[Brian Birtles (:birtles)]

Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125910/diff/2-3/

Comment 69

[Brian Birtles (:birtles)]

Comment on attachment 8853871 [details]
Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125912/diff/2-3/

Comment 70

[Brian Birtles (:birtles)]

Comment on attachment 8853872 [details]
Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125914/diff/2-3/

Comment 71

[Brian Birtles (:birtles)]

Comment on attachment 8853873 [details]
Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125916/diff/2-3/

Comment 72

[Brian Birtles (:birtles)]

Comment on attachment 8853874 [details]
Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125918/diff/2-3/

Comment 73

[Brian Birtles (:birtles)]

Comment on attachment 8853875 [details]
Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125920/diff/2-3/

Comment 74

[Cameron McCormack (:heycam)]

Comment on attachment 8853871 [details]
Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

https://reviewboard.mozilla.org/r/125912/#review129206

::: layout/style/nsComputedDOMStyle.h:111
(Diff revision 3)
> +                                              aStyleType,
> +                                              eWithAnimation);
> +  }
>  
>    static already_AddRefed<nsStyleContext>
> -  GetStyleContextForElementNoFlush(mozilla::dom::Element* aElement,
> +  GetNonAnimationStyleContextNoFlush(mozilla::dom::Element* aElement,

All the other GetStyleContext* functions have "ForElement" in the name, so it's a shame this one is inconsistent.  Can you make this one consistent, either by adding "ForElement" or (and I think I prefer this) removing "ForElement" from the names of the others?

Comment 75

[Cameron McCormack (:heycam)]

Comment on attachment 8853872 [details]
Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

https://reviewboard.mozilla.org/r/125914/#review129212

::: layout/style/nsComputedDOMStyle.cpp:616
(Diff revision 3)
> +        // The existing style context may have animation styles so check if we
> +        // need to remove them.
> +        if (aAnimationFlag == eWithoutAnimation) {
> +          nsPresContext* presContext = presShell->GetPresContext();
> +          MOZ_ASSERT(!presContext || presContext->StyleSet()->IsGecko(),
> +                     "Stylo: Need ResolveStyleByRemovingAnimation for stylo");

Nit: we tend to use lowercase "stylo:" prefixes.

::: layout/style/nsComputedDOMStyle.cpp:617
(Diff revision 3)
> +        // need to remove them.
> +        if (aAnimationFlag == eWithoutAnimation) {
> +          nsPresContext* presContext = presShell->GetPresContext();
> +          MOZ_ASSERT(!presContext || presContext->StyleSet()->IsGecko(),
> +                     "Stylo: Need ResolveStyleByRemovingAnimation for stylo");
> +          if (presContext && presContext->StyleSet()->IsGecko()) {

I'm not sure whether it's possible for an element to have a frame (and thus an existing style context) if the pres shell has no pres context.  It looks like we have assertions in PresShell::Init that the pres context is non null, and in non-DEBUG builds we'll just return early.  Notwithstanding the fact that there's an existing null check of the pres context later in the function.

But if this case can come up, I wonder whether it makes more sense to return nullptr than return the frame's existing styles, which might have animations in them anyway.

Comment 76

[Cameron McCormack (:heycam)]

Comment on attachment 8853873 [details]
Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

https://reviewboard.mozilla.org/r/125916/#review129220

Comment 77

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) (away Apr 1-4) from comment #74)
> Comment on attachment 8853871 [details]
> Bug 1315874 - Make GetStyleContextForElementWithoutAnimation not flush;
> 
> https://reviewboard.mozilla.org/r/125912/#review129206
> 
> ::: layout/style/nsComputedDOMStyle.h:111
> (Diff revision 3)
> > +                                              aStyleType,
> > +                                              eWithAnimation);
> > +  }
> >  
> >    static already_AddRefed<nsStyleContext>
> > -  GetStyleContextForElementNoFlush(mozilla::dom::Element* aElement,
> > +  GetNonAnimationStyleContextNoFlush(mozilla::dom::Element* aElement,
> 
> All the other GetStyleContext* functions have "ForElement" in the name, so
> it's a shame this one is inconsistent.  Can you make this one consistent,
> either by adding "ForElement" or (and I think I prefer this) removing
> "ForElement" from the names of the others?

I've added the ForElement but the name is now really long: GetNonAnimationStyleContextForElementNoFlush

Doing s/NonAnimation/Unanimated/, i.e. GetUnanimationStyleContextForElementNoFlush would save 2 characters and seems more readable to me. What do you think?

Comment 78

[Cameron McCormack (:heycam)]

Comment on attachment 8853874 [details]
Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

https://reviewboard.mozilla.org/r/125918/#review129242

Comment 79

[Cameron McCormack (:heycam)]

Comment on attachment 8853875 [details]
Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

https://reviewboard.mozilla.org/r/125920/#review129244

Comment 80

[Brian Birtles (:birtles)]

Attachment: Bug 1315874 - Drop 'ForElement' from GetStyleContextForElementXXX methods;

In the next patch we want to add a method called
GetUnanimatedStyleContextForElementNoFlush but that's much too long. Instead it
seems better to just drop 'ForElement' from all these methods since it should be
fairly obvious we are getting the style context for an element given that the
first argument is an element.

Review commit: https://reviewboard.mozilla.org/r/126670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126670/

Comment 81

[Brian Birtles (:birtles)]

Comment on attachment 8853870 [details]
Bug 1315874 - Add MightNeedBaseStyle helper method;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125910/diff/3-4/

Comment 82

[Brian Birtles (:birtles)]

Comment on attachment 8853871 [details]
Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125912/diff/3-4/

Comment 83

[Brian Birtles (:birtles)]

Comment on attachment 8853872 [details]
Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125914/diff/3-4/

Comment 84

[Brian Birtles (:birtles)]

Comment on attachment 8853873 [details]
Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125916/diff/3-4/

Comment 85

[Brian Birtles (:birtles)]

Comment on attachment 8853874 [details]
Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125918/diff/3-4/

Comment 86

[Brian Birtles (:birtles)]

Comment on attachment 8853875 [details]
Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125920/diff/3-4/

Comment 87

[Brian Birtles (:birtles)]

Comment on attachment 8854711 [details]
Bug 1315874 - Drop 'ForElement' from GetStyleContextForElementXXX methods;

https://reviewboard.mozilla.org/r/126670/#review129260

::: layout/style/nsComputedDOMStyle.h:87
(Diff revision 1)
>    static already_AddRefed<nsStyleContext>
> -  GetStyleContextForElement(mozilla::dom::Element* aElement, nsIAtom* aPseudo,
> +  GetStyleContext(mozilla::dom::Element* aElement, nsIAtom* aPseudo,
>                              nsIPresShell* aPresShell,
>                              StyleType aStyleType = eAll);
>  

Oops, looks like I forgot to fix the indentation here.

Comment 88

[Brian Birtles (:birtles)]

Comment on attachment 8854711 [details]
Bug 1315874 - Drop 'ForElement' from GetStyleContextForElementXXX methods;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126670/diff/1-2/

Comment 89

[Brian Birtles (:birtles)]

Comment on attachment 8853871 [details]
Bug 1315874 - Make GetStyleContextWithoutAnimation not flush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125912/diff/4-5/

Comment 90

[Brian Birtles (:birtles)]

Comment on attachment 8853872 [details]
Bug 1315874 - Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125914/diff/4-5/

Comment 91

[Brian Birtles (:birtles)]

Comment on attachment 8853873 [details]
Bug 1315874 - Tweak some line wrapping in DoGetStyleContextNoFlush;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125916/diff/4-5/

Comment 92

[Brian Birtles (:birtles)]

Comment on attachment 8853874 [details]
Bug 1315874 - Pass a base style context to nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125918/diff/4-5/

Comment 93

[Brian Birtles (:birtles)]

Comment on attachment 8853875 [details]
Bug 1315874 - Use base style context to resolve base values in nsSMILCSSProperty;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125920/diff/4-5/

Comment 94

[Cameron McCormack (:heycam)]

Comment on attachment 8854711 [details]
Bug 1315874 - Drop 'ForElement' from GetStyleContextForElementXXX methods;

https://reviewboard.mozilla.org/r/126670/#review129272

Comment 95

[Daniel Holbert [:dholbert]]

Comment on attachment 8853869 [details]
Bug 1315874 - Add a method to nsSMILCSSValueType that takes a StyleAnimationValue;

https://reviewboard.mozilla.org/r/125908/#review129600

::: dom/smil/nsSMILCSSValueType.cpp:429
(Diff revisions 2 - 3)
> +  // We'd like to avoid serializing |aValue| if possible, and since the
> +  // string passed to CSPAllowsInlineStyle is only used for reporting violations
> +  // and an intermediate CSS value is not likely to be particularly useful
> +  // in that case, we just use a generic placeholder string instead.
> +  static const nsLiteralString kPlaceholderText =
> +    NS_LITERAL_STRING("[SVG animation of CSS]");

(On top of what you're saying here: I suspect that if the CSP would block this style, then it would have already blocked the animation earlier, e.g. in our first entry point at the string-accepting ValueFromString method.  So we would never even get here in that scenario, probably.  So, I don't expect this string will ever actually be used, in practice.  If that turns out to be wrong, we might want to consider making this something localized/etc [or falling back to just serializing the actual CSS like you originally did] -- but for now I'm not concerned about this being used particularly often.)

Comment 96

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d2b2c67b7f
Add test that SMIL does not trigger CSS Transitions; r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f233c0466f6
Add a method to nsSMILCSSValueType that takes a StyleAnimationValue; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf852fc7100f
Add MightNeedBaseStyle helper method; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b653d2bd81e
Drop 'ForElement' from GetStyleContextForElementXXX methods; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/db648d833b24
Make GetStyleContextWithoutAnimation not flush; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa276da14f2
Drop animation styles if we re-use an existing style context in DoGetStyleContextNoFlush for non-animation style; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/62092e494572
Tweak some line wrapping in DoGetStyleContextNoFlush; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d766681e346e
Pass a base style context to nsSMILCSSProperty; r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/42be15d3ed1b
Use base style context to resolve base values in nsSMILCSSProperty; r=heycam

Comment 97

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09d2b2c67b7f
https://hg.mozilla.org/mozilla-central/rev/1f233c0466f6
https://hg.mozilla.org/mozilla-central/rev/cf852fc7100f
https://hg.mozilla.org/mozilla-central/rev/5b653d2bd81e
https://hg.mozilla.org/mozilla-central/rev/db648d833b24
https://hg.mozilla.org/mozilla-central/rev/ffa276da14f2
https://hg.mozilla.org/mozilla-central/rev/62092e494572
https://hg.mozilla.org/mozilla-central/rev/d766681e346e
https://hg.mozilla.org/mozilla-central/rev/42be15d3ed1b

Comment 98

[Ryan VanderMeulen [:RyanVM]]

I assume we're letting this ride the trains? :)

Comment 99

[Brian Birtles (:birtles)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #98)
> I assume we're letting this ride the trains? :)

Yes, this doesn't seem to be particularly critical.