Closed Bug 1361938 Opened 7 years ago Closed 7 years ago

stylo: We don't update direction or play-state immediately in test_animations_omta.html

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: boris, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1361938 - Enable test_animations_omta_start.html.
59 bytes, text/x-review-board-request
boris
: review+
Details
Bug 1361938 - Introduce a dirty flag that represents DeclarationBlock has been modified but not restyled.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1361938 - Set the dirty flag of DeclarationBlock when the DeclarationBlock is modified and clear the flag when it's resyled.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
There are some test failures in test_animations_omta.html [1] while applying a new animation direction or animation play state. I dumped the log and it seems we didn't call AddAnimationsAndTransitionsToLayer() for these cases, so the animation-direction or animation-play-state on the compositor thread wasn't updated to the new value.

[1] http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/layout/style/test/test_animations_omta.html#1008-1011
Priority: -- → P2
One thing I am concerned is that we really flush styles by getBoundingClientRect() [1] on stylo when we are in test refresh mode.

[1] https://hg.mozilla.org/mozilla-central/file/4a6a71f4aa22/testing/mochitest/tests/SimpleTest/paint_listener.js#l53
Blocks: stylo-style-mochitest
Does this still fail?
Blocks: 1366657
Bumping up priority since this currently will cause mismatching of failure numbers for test_animations_omta.html.

Boris, I think this is more important than bug 1361663. As for bug 1361663, I think we can just tune tolerance value for now.
Priority: P2 → P1
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Bumping up priority since this currently will cause mismatching of failure
> numbers for test_animations_omta.html.
> 
> Boris, I think this is more important than bug 1361663. As for bug 1361663,
> I think we can just tune tolerance value for now.

OK, I see. I still don't know the root cause of this bug. Need more time to debug it.
In the tests of animation-direction, it seems this bug only happens when "we advance time and change the direction together" [1] because we pass incorrect direction into compositor (e.g. still pass alternative-reverse, after we change the direction to normal).

[1] http://searchfox.org/mozilla-central/source/layout/style/test/test_animations_omta.html#1020-1021,1037-1038,1054-1055,1071-1072,1088-1089
(In reply to Boris Chiou [:boris] from comment #5)
> In the tests of animation-direction, it seems this bug only happens when "we
> advance time and change the direction together" [1] because we pass
> incorrect direction into compositor (e.g. still pass alternative-reverse,
> after we change the direction to normal).

Nice find! So, we are re-sending the target animation to compositor when we call waitForPaintsFlushed() without updating the animation style?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Boris Chiou [:boris] from comment #5)
> > In the tests of animation-direction, it seems this bug only happens when "we
> > advance time and change the direction together" [1] because we pass
> > incorrect direction into compositor (e.g. still pass alternative-reverse,
> > after we change the direction to normal).
> 
> Nice find! So, we are re-sending the target animation to compositor when we
> call waitForPaintsFlushed() without updating the animation style?

Yes. Looks like we get the old compositorAnimations from EffectCompositor::GetAnimationsForCompositor() in AddAnimationsAndTransitionsToLayer() at these moments. Need to check what happened.
(In reply to Boris Chiou [:boris] from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Boris Chiou [:boris] from comment #5)
> > > In the tests of animation-direction, it seems this bug only happens when "we
> > > advance time and change the direction together" [1] because we pass
> > > incorrect direction into compositor (e.g. still pass alternative-reverse,
> > > after we change the direction to normal).
> > 
> > Nice find! So, we are re-sending the target animation to compositor when we
> > call waitForPaintsFlushed() without updating the animation style?
> 
> Yes. Looks like we get the old compositorAnimations from
> EffectCompositor::GetAnimationsForCompositor() in
> AddAnimationsAndTransitionsToLayer() at these moments. Need to check what
> happened.

At these moments, not only main thread but also compositor thread are using the "old direction". However, we indeed set the correct direction on normal restyle.
Looks like we didn't call UpdateOldAnimationPropertiesWithNew() after changing the direction to "Normal" in the normal restyle. Why does this only happen after we advance the clock?
OK. Let me explain the possible root cause:

After advancing 2000ms and change animationDirection to "Normal", we trigger an animation restyle, which includes:
a. doing interpolation
b. setting the new animation direction  (Is this a bug?),
   so the animation direction becomes "Normal", from "Alternative-Reverse".

After this animation-only restyle, we trigger a normal restyle, which including:
c. setting the new animation direction, and we notice that there is no change because "Normal" == "Normal",
   so we don't update css animation properties.

That is why we didn't update animation direction value in KeyframeEffect at the moment.

e.g.
For animation 10s linear, opacity [0, 1]:

1) When changing the animation direction to "Alternate_reverse" without advancing 2000ms:

GECKO(78969) | ----- normal traversal ------
GECKO(78969) | set_animation_direction: Alternate_reverse
GECKO(78969) | call UpdateOldAnimationPropertiesWithNew
GECKO(78969) | ----- normal traversal finish ------

GECKO(78969) | ----- animation-only traversal ------
GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 1 => Opacity(1)
GECKO(78969) | set_animation_direction: Alternate_reverse
GECKO(78969) | ----- animation-only traversal finish ------
GECKO(78969) | ----- normal traversal ------
GECKO(78969) | ----- normal traversal finish ------


2) When changing the animation direction to "Normal" after advancing 2000ms:

GECKO(78969) | ==== advance 2000 and change to "Normal" ====
GECKO(78969) | ----- animation-only traversal ------
GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)   // we still use Alternate_reverse
GECKO(78969) | set_animation_direction: Normal
GECKO(78969) | ----- animation-only traversal finish ------
GECKO(78969) | ----- normal traversal ------
GECKO(78969) | set_animation_direction: Normal       // This bug. In normal traversal, we thought there is no change, so
                                                     // didn't call UpdateOldAnimationPropertiesWithNew()
GECKO(78969) | ----- normal traversal finish ------

GECKO(78969) | ----- animation-only traversal ------
GECKO(78969) | specified timing direction: 3   // still Alternative-Reverse
GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)
GECKO(78969) | set_animation_direction: Normal
GECKO(78969) | ----- animation-only traversal finish ------
GECKO(78969) | ----- normal traversal ------
GECKO(78969) | ----- normal traversal finish ------


Why is there an animation-only restyle after advancing 2000ms? When we change the style, we only trigger a normal restyle
in other cases. This animation-only restyle happens only after advancing 2000ms. (And looks like it is not a pending play/pause animation?)
(In reply to Boris Chiou [:boris] from comment #11)
> 2) When changing the animation direction to "Normal" after advancing 2000ms:
> 
> GECKO(78969) | ==== advance 2000 and change to "Normal" ====
> GECKO(78969) | ----- animation-only traversal ------
> GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)  
> // we still use Alternate_reverse
> GECKO(78969) | set_animation_direction: Normal
> GECKO(78969) | ----- animation-only traversal finish ------
> GECKO(78969) | ----- normal traversal ------
> GECKO(78969) | set_animation_direction: Normal       // This bug. In normal traversal, we thought there is no change, so
>                                                      // didn't call UpdateOldAnimationPropertiesWithNew()
> GECKO(78969) | ----- normal traversal finish ------

Hiro, is the updating of direction/playback-state by set_animation_${ident} [1] in animation-only traversal expected?

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/gecko.mako.rs#1967
Flags: needinfo?(hikezoe)
(In reply to Boris Chiou [:boris] from comment #11)
> OK. Let me explain the possible root cause:
> 
> After advancing 2000ms and change animationDirection to "Normal", we trigger
> an animation restyle, which includes:
> a. doing interpolation
> b. setting the new animation direction  (Is this a bug?),

When you say "setting the new animation direction", do you mean on the CSSAnimation object? I guess not, right?
(In reply to Boris Chiou [:boris] from comment #12)
> (In reply to Boris Chiou [:boris] from comment #11)
> > 2) When changing the animation direction to "Normal" after advancing 2000ms:
> > 
> > GECKO(78969) | ==== advance 2000 and change to "Normal" ====
> > GECKO(78969) | ----- animation-only traversal ------
> > GECKO(78969) | interpolate Opacity(0) + Opacity(1) at 0.8 => Opacity(0.8)  
> > // we still use Alternate_reverse
> > GECKO(78969) | set_animation_direction: Normal
> > GECKO(78969) | ----- animation-only traversal finish ------
> > GECKO(78969) | ----- normal traversal ------
> > GECKO(78969) | set_animation_direction: Normal       // This bug. In normal traversal, we thought there is no change, so
> >                                                      // didn't call UpdateOldAnimationPropertiesWithNew()
> > GECKO(78969) | ----- normal traversal finish ------
> 
> Hiro, is the updating of direction/playback-state by set_animation_${ident}
> [1] in animation-only traversal expected?

Definitely not!  Leaving NI? since this seems a bug in animation-only restyle.
(In reply to Brian Birtles (:birtles) from comment #13)
> (In reply to Boris Chiou [:boris] from comment #11)
> > OK. Let me explain the possible root cause:
> > 
> > After advancing 2000ms and change animationDirection to "Normal", we trigger
> > an animation restyle, which includes:
> > a. doing interpolation
> > b. setting the new animation direction  (Is this a bug?),
> 
> When you say "setting the new animation direction", do you mean on the
> CSSAnimation object? I guess not, right?

Sorry, I mean set_animation_{$ident}() function call [1] (i.e. gecko style struct).

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/gecko.mako.rs#1967
(In reply to Boris Chiou [:boris] from comment #15)
> > When you say "setting the new animation direction", do you mean on the
> > CSSAnimation object? I guess not, right?
> 
> Sorry, I mean set_animation_{$ident}() function call [1] (i.e. gecko style
> struct).
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/
> gecko.mako.rs#1967

Not sure how many values have this problem. All I know is: 'Direction', 'FillMode', 'PlayState' [2].

[2] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/gecko.mako.rs#2511-2516
(In reply to Boris Chiou [:boris] from comment #16)
> Not sure how many values have this problem. All I know is: 'Direction',
> 'FillMode', 'PlayState' [2].
> 
> [2]
> http://searchfox.org/mozilla-central/rev/
> 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/servo/components/style/properties/
> gecko.mako.rs#2511-2516

In test_animations_omta.html, we use a different way to test fill mode, and it seems we don't combine the style change and advance clock to test fill mode, so now it looks good. Anyway, I think if there is any, after we fix the bug of direction and play-state, fill mode would be also fixed.
(In reply to Boris Chiou [:boris] from comment #12)
> Hiro, is the updating of direction/playback-state by set_animation_${ident}
> [1] in animation-only traversal expected?

The setter was called in cascading process. What I don't still understand is why the new value appeared there.
I will investigate it further.

Filed a couple of issues in animation-only restyle I found as another bug (bug 1367225).
Assignee: nobody → hikezoe
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Boris Chiou [:boris] from comment #12)
> > Hiro, is the updating of direction/playback-state by set_animation_${ident}
> > [1] in animation-only traversal expected?
> 
> The setter was called in cascading process. What I don't still understand is
> why the new value appeared there.
> I will investigate it further.

Just a quick update.
The new value has been already set in primary rules before we replace animation rules in replace_rules(). So, where?
I had been totally blinding about the fact that a PropertyDeclarationBlock is stored in StyleSource which is a member of RuleNode.
In my understandings, when we call setProperty (e.g. div.style.animationDirection = 'reverse'), the property value is stored in the PropetyDeclarationBlock in the StyleSource, and after that, it is set to gecko's style struct in the cascading process.  So if we have both of RESTYLE_STYLE_ATTRIBUTE and RESTYLE_CSS_ANIMATIONS hints, the PropertyDeclarationBlock which was set by setProperty() is also set to the gecko's struct during animation-only restyle unfortunately.

The only solution what I can think of is to skip the cascading process that if we have both of restyle hints, that means the cascading process is deferred to the time when we process RESTYLE_STYLE_ATTRIBUTE in normal traversal.

Here is a try with the solution:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af12659b21fc3ea1fbf67b9199894d6cfb1657b0

It seems to work well.
Oops, I forgot to add Cameron in CC.
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

https://reviewboard.mozilla.org/r/142936/#review146654

::: layout/style/test/mochitest.ini:62
(Diff revision 1)
>  [test_animations_omta.html]
> -skip-if = stylo # crashing bug 1367670, bug 1361938, bug 1361663

I will drop this change once bug 1367670 is fixed.
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

https://reviewboard.mozilla.org/r/142936/#review146718

Clearing review request since some tests failed on a try based on today's autoland.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bc7537770d49db5df59767585e858c6e1044d4a
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

https://reviewboard.mozilla.org/r/143554/#review147334

::: commit-message-44873:8
(Diff revision 1)
> +This test had been passed accidentaly.
> +In the test initially a transition (transform: none to translateX(300px)) is
> +created.  After the transition was sent to the compositor, secondary transition
> +tries to be created, the after-change style of the secondary transitions is
> +translateX(0px).  As per the transition spec, the before-change style of the
> +secondary should be translateX(300px), but on current implementation it's

I thought the before-change style should be the previous style change event, *except with* any styles derived from declarative animations such as CSS Transitions, CSS Animations, and SMIL Animations updated to the current time. In other words, it should be the current computed values including up-to-date animation rules?

If this test is failed on non-e10s, it's ok to skip it. Thanks for checking this.
Attachment #8872032 - Flags: review?(boris.chiou) → review+
(In reply to Boris Chiou [:boris] from comment #29)
> I thought the before-change style should be the previous style change event,
> *except with* any styles derived from declarative animations such as CSS
> Transitions, CSS Animations, and SMIL Animations updated to the current
> time. In other words, it should be the current computed values including
> up-to-date animation rules?

That's correct.
(In reply to Boris Chiou [:boris] from comment #29)
> Comment on attachment 8872032 [details]
> Bug 1361938 - Disable test_transitions_replacement_on_busy_frame.html.
> 
> https://reviewboard.mozilla.org/r/143554/#review147334
> 
> ::: commit-message-44873:8
> (Diff revision 1)
> > +This test had been passed accidentaly.
> > +In the test initially a transition (transform: none to translateX(300px)) is
> > +created.  After the transition was sent to the compositor, secondary transition
> > +tries to be created, the after-change style of the secondary transitions is
> > +translateX(0px).  As per the transition spec, the before-change style of the
> > +secondary should be translateX(300px), but on current implementation it's
> 
> I thought the before-change style should be the previous style change event,
> *except with* any styles derived from declarative animations such as CSS
> Transitions, CSS Animations, and SMIL Animations updated to the current
> time. In other words, it should be the current computed values including
> up-to-date animation rules?

Oh, you are right. I was totally misreading the spec.
I have still no idea why the before-change style sometimes becomes translateX(0px) there...
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

Clearing r+ since this patch is not a right thing to do.
Attachment #8872032 - Flags: review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)

> I have still no idea why the before-change style sometimes becomes
> translateX(0px) there...

OK.  I think I understand the reason now.

From the spec;
before-change style (snip), 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*

Without attachment 8872031 [details], both of non-animating and animating values are the values updated to the *current time*.
Whereas with attachment 8872031 [details], both of non-animating and animating values are the values at the time of previous style change event.
We need a way to cascade only animation values, this way seems pretty tricky to me.
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

Clearing review request.
Attachment #8871475 - Flags: review?(cam)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> Without attachment 8872031 [details], both of non-animating and animating
> values are the values updated to the *current time*.
> Whereas with attachment 8872031 [details], both of non-animating and
> animating values are the values at the time of previous style change event.

Correction:
with attachment 8872031 [details], non-animating values are the values at he time of previous style change event, animating values are the values at the last animation-only restyle (generally it's the previous frame, but in case of properties that can be run on the compositor, it's the values when the animation was sent to the compositor).
I think I found a proper solution. The solution is just to clone old DeclarationBlock in nsDOMCSSDeclaration::ModifyDeclaration() to avoid the DeclarationBlock is used in element's rule nodes immediately.

Here is a try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6d7200bf5b820e8650a6c5ea2dc17e17ba30149
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

https://reviewboard.mozilla.org/r/143554/#review147442

Honestely I haven't carefully look into the reason why this test passes with the first patch.
Boris any ideas?
Attachment #8872032 - Flags: review?(hikezoe)
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

MozReview cheated me.
Attachment #8872032 - Flags: review?(hikezoe) → review?(boris.chiou)
Blocks: stylo-css-animations
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

https://reviewboard.mozilla.org/r/143554/#review147494

Great!!
Attachment #8872032 - Flags: review?(boris.chiou) → review+
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

https://reviewboard.mozilla.org/r/143554/#review147442

> Honestely I haven't carefully look into the reason why this test passes with the first patch.

We disable this test case without e10s because we get the incorrect before change of the child element.
e.g. The expected transition: opacity: [0.4, 1.0] (because the opacity of parent element is 0.4.)
     The actual result: opacity: [1.0, 1.0], so there is not transition, and the test is failed.
     
Looks like the 1st patch makes sure we get the correct before-change style with/without e10s. In other words, when we inline-change the style of child element to opacity(1.0), we still keep its before-change style (inherit from parent, opacity(0.4)), so the transition starts! And we pass the test!

If everything looks good, you can close Bug 1362292. :)
Comment on attachment 8872031 [details]
Bug 1361938 - Don't process visited rules during animation-only restyle.

https://reviewboard.mozilla.org/r/143552/#review147658

This seems fine, but do you also want to skip the visited cacade in `cascade_primary_and_pseudos` as well?
Attachment #8872031 - Flags: review?(jryans) → review+
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

https://reviewboard.mozilla.org/r/142936/#review147758

::: layout/style/nsDOMCSSDeclaration.cpp:323
(Diff revision 3)
>    // rule (see stack in bug 209575).
>    mozAutoDocConditionalContentUpdateBatch autoUpdate(DocToUpdate(), true);
> -  RefPtr<DeclarationBlock> decl = olddecl->EnsureMutable();
> +  RefPtr<DeclarationBlock> decl;
> +  if (olddecl->IsServo()) {
> +    // In stylo, the old DeclaraionBlock is stored in element's rule node tree
> +    // directly, to avoid that new values replace the DeclaraionBlock in the

s/to avoid that new values replace/to avoid new values replacing/

s/DeclarionBlock/DeclarationBlock/

::: layout/style/nsDOMCSSDeclaration.cpp:326
(Diff revision 3)
> +  if (olddecl->IsServo()) {
> +    // In stylo, the old DeclaraionBlock is stored in element's rule node tree
> +    // directly, to avoid that new values replace the DeclaraionBlock in the
> +    // tree directly, we need to copy the old one here. As a result the new
> +    // value does not replace rule node tree until traversal happens.
> +    decl = olddecl->Clone();

What happens when you do multiple setProperty() calls before restyling?  Do we keep cloning the declaration?  If so, I think we should somehow prevent that.
Ah, right. We need some kind of dirty flag there, it requires more works.
Attachment #8871475 - Flags: review?(cam)
Comment on attachment 8872582 [details]
Bug 1361938 - Introduce a dirty flag that represents DeclarationBlock has been modified but not restyled.

https://reviewboard.mozilla.org/r/144112/#review147810

::: commit-message-34ac1:1
(Diff revision 1)
> +Bug 1361938 - Introduce a dirty flag that represents DelecratonBlock has been modified but not restyled. r?heycam

DeclarationBlock

::: layout/style/DeclarationBlock.h:76
(Diff revision 1)
> +  bool IsDirty() const {
> +    return mIsDirty;
> +  }

Feel free to put this all on one line like the other methods. :-)
Attachment #8872582 - Flags: review?(cam) → review+
Comment on attachment 8872583 [details]
Bug 1361938 - Set the dirty flag of DeclarationBlock when the DeclarationBlock is modified and clear the flag when it's resyled.

https://reviewboard.mozilla.org/r/144114/#review147812

::: layout/style/nsDOMCSSAttrDeclaration.cpp:76
(Diff revision 1)
>  
>  nsresult
>  nsDOMCSSAttributeDeclaration::SetCSSDeclaration(DeclarationBlock* aDecl)
>  {
>    NS_ASSERTION(mElement, "Must have Element to set the declaration!");
> +  aDecl->SetDitry();

SetDirty

::: servo/components/style/dom.rs:333
(Diff revision 1)
> +    fn unset_dirty_style_attribute(&self) {
> +    }

Maybe add a comment in here saying we don't need to do anything here, because Servo .....
Attachment #8872583 - Flags: review?(cam) → review+
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

https://reviewboard.mozilla.org/r/142936/#review147814

::: layout/style/nsDOMCSSDeclaration.cpp:322
(Diff revision 4)
>    // between when we mutate the declaration and when we set the new
>    // rule (see stack in bug 209575).
>    mozAutoDocConditionalContentUpdateBatch autoUpdate(DocToUpdate(), true);
> -  RefPtr<DeclarationBlock> decl = olddecl->EnsureMutable();
> +  RefPtr<DeclarationBlock> decl;
> +  if (olddecl->IsServo() && !olddecl->IsDirty()) {
> +    // In stylo, the old DeclaraionBlock is stored in element's rule node tree

DeclarationBlock
Attachment #8871475 - Flags: review?(cam) → review+
Comment on attachment 8872582 [details]
Bug 1361938 - Introduce a dirty flag that represents DeclarationBlock has been modified but not restyled.

https://reviewboard.mozilla.org/r/144112/#review148120

::: layout/style/DeclarationBlock.h:34
(Diff revision 1)
>  } // namespace css
>  
>  class DeclarationBlock
>  {
>  protected:
> -  explicit DeclarationBlock(StyleBackendType aType)
> +  DeclarationBlock(StyleBackendType aType)

Doesn't this still need to be `explicit`?
Comment on attachment 8872583 [details]
Bug 1361938 - Set the dirty flag of DeclarationBlock when the DeclarationBlock is modified and clear the flag when it's resyled.

https://reviewboard.mozilla.org/r/144114/#review148126

::: layout/style/ServoBindings.cpp:417
(Diff revision 1)
>  }
>  
> +void
> +Gecko_UnsetDirtyStyleAttr(RawGeckoElementBorrowed aElement)
> +{
> +  DeclarationBlock* decl = aElement->GetInlineStyleDeclaration();

Can we really do this from worker threads? How do we guarantee that it's unique? Style attributes are shared in Gecko when possible.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #58)
> Comment on attachment 8872582 [details]
> Bug 1361938 - Introduce a dirty flag that represents DelecratonBlock has
> been modified but not restyled.
> 
> https://reviewboard.mozilla.org/r/144112/#review148120
> 
> ::: layout/style/DeclarationBlock.h:34
> (Diff revision 1)
> >  } // namespace css
> >  
> >  class DeclarationBlock
> >  {
> >  protected:
> > -  explicit DeclarationBlock(StyleBackendType aType)
> > +  DeclarationBlock(StyleBackendType aType)
> 
> Doesn't this still need to be `explicit`?

Good catch! I accidentally removed it.
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

https://reviewboard.mozilla.org/r/142936/#review148128

::: layout/style/nsDOMCSSDeclaration.cpp:323
(Diff revision 4)
>    // rule (see stack in bug 209575).
>    mozAutoDocConditionalContentUpdateBatch autoUpdate(DocToUpdate(), true);
> -  RefPtr<DeclarationBlock> decl = olddecl->EnsureMutable();
> +  RefPtr<DeclarationBlock> decl;
> +  if (olddecl->IsServo() && !olddecl->IsDirty()) {
> +    // In stylo, the old DeclaraionBlock is stored in element's rule node tree
> +    // directly, to avoid new values replacing the DeclarationBlock in the tree

Can we perhaps try to reduce this to elements that have animations? Perhaps marking it as immutable when doing animation stuff? This will create a lot of extra rule nodes I fear.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #59)
> Comment on attachment 8872583 [details]
> Bug 1361938 - Set the dirty flag of DeclarationBlock when the
> DeclarationBlock is modified and clear the flag when it's resyled.
> 
> https://reviewboard.mozilla.org/r/144114/#review148126
> 
> ::: layout/style/ServoBindings.cpp:417
> (Diff revision 1)
> >  }
> >  
> > +void
> > +Gecko_UnsetDirtyStyleAttr(RawGeckoElementBorrowed aElement)
> > +{
> > +  DeclarationBlock* decl = aElement->GetInlineStyleDeclaration();
> 
> Can we really do this from worker threads? How do we guarantee that it's
> unique? Style attributes are shared in Gecko when possible.

Thanks, I will check our implementation.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #61)
> Comment on attachment 8871475 [details]
> Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.
> 
> https://reviewboard.mozilla.org/r/142936/#review148128
> 
> ::: layout/style/nsDOMCSSDeclaration.cpp:323
> (Diff revision 4)
> >    // rule (see stack in bug 209575).
> >    mozAutoDocConditionalContentUpdateBatch autoUpdate(DocToUpdate(), true);
> > -  RefPtr<DeclarationBlock> decl = olddecl->EnsureMutable();
> > +  RefPtr<DeclarationBlock> decl;
> > +  if (olddecl->IsServo() && !olddecl->IsDirty()) {
> > +    // In stylo, the old DeclaraionBlock is stored in element's rule node tree
> > +    // directly, to avoid new values replacing the DeclarationBlock in the tree
> 
> Can we perhaps try to reduce this to elements that have animations? Perhaps
> marking it as immutable when doing animation stuff? This will create a lot
> of extra rule nodes I fear.

Unfortunately we can't since transition may start on an element which has no animations.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #59)
> > Comment on attachment 8872583 [details]
> > Bug 1361938 - Set the dirty flag of DeclarationBlock when the
> > DeclarationBlock is modified and clear the flag when it's resyled.
> > 
> > https://reviewboard.mozilla.org/r/144114/#review148126
> > 
> > ::: layout/style/ServoBindings.cpp:417
> > (Diff revision 1)
> > >  }
> > >  
> > > +void
> > > +Gecko_UnsetDirtyStyleAttr(RawGeckoElementBorrowed aElement)
> > > +{
> > > +  DeclarationBlock* decl = aElement->GetInlineStyleDeclaration();
> > 
> > Can we really do this from worker threads? How do we guarantee that it's
> > unique? Style attributes are shared in Gecko when possible.
> 
> Thanks, I will check our implementation.

Ah, I remembered the code I read last night. sharing is managed by MiscContainer::Evict() and Cache()? Need to check it.
It seems to me that once style attribute sharing starts working (bug 1368399) this bug will be solved?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #65)
> It seems to me that once style attribute sharing starts working (bug
> 1368399) this bug will be solved?

Maybe not. The setup what I did in this bug should be handled apart from mImmutable flag. 
So, a remaining concern is whether we touch sharing style attribute during parallel traversal. I don't think so for now since the attribute is cloned in this bug.:-)
Attachment #8872031 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3bf3996068d
Introduce a dirty flag that represents DeclarationBlock has been modified but not restyled. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2585796d4280
Set the dirty flag of DeclarationBlock when the DeclarationBlock is modified and clear the flag when it's resyled. r=heycam
https://hg.mozilla.org/integration/autoland/rev/e6ba19be442f
Do not reuse DeclarationBlock when modifying inline style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/26a25602688a
Enable test_animations_omta_start.html. r=boris
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc70d2e36fac
Mark Gecko_UnsetDirtyStyleAttr as ignoreContens for now. r=me DONTBUILD
Thank you philor!

After some more thought about the call of SetDirty() during traversal, it seems to me that it's not thread safe at all.
I will file a follow-up bug to make the call thread safe.
Depends on: 1368922
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: