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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: boris, Assigned: hiro)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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
(Reporter)

Updated

3 months ago
Priority: -- → P2
(Assignee)

Comment 1

3 months ago
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
(Reporter)

Updated

3 months ago
Blocks: 1321197
(Assignee)

Comment 2

2 months ago
Does this still fail?
(Assignee)

Updated

2 months ago
Blocks: 1366657
(Assignee)

Comment 3

2 months ago
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
(Reporter)

Comment 4

2 months ago
(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.
(Reporter)

Comment 5

2 months ago
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
(Assignee)

Comment 6

2 months ago
(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?
(Reporter)

Comment 7

2 months ago
(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.
(Reporter)

Comment 8

2 months ago
(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.
(Reporter)

Comment 9

2 months ago
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?
Comment hidden (typo)
(Reporter)

Comment 11

2 months ago
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?)
(Reporter)

Comment 12

2 months ago
(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
(Reporter)

Updated

2 months ago
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?
(Assignee)

Comment 14

2 months ago
(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.
(Reporter)

Comment 15

2 months ago
(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
(Reporter)

Comment 16

2 months ago
(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
(Reporter)

Comment 17

2 months ago
(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.
(Assignee)

Comment 18

2 months ago
(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)
(Assignee)

Comment 19

2 months ago
(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?
(Assignee)

Comment 20

2 months ago
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.
(Assignee)

Comment 21

2 months ago
Oops, I forgot to add Cameron in CC.
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 months ago
mozreview-review
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.
(Assignee)

Comment 24

2 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=583539063c838efcbd53eb62866e49684e399a63
(Reporter)

Comment 29

2 months ago
mozreview-review
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.
(Assignee)

Comment 31

2 months ago
(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...
(Assignee)

Comment 32

2 months ago
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+
(Assignee)

Comment 33

2 months ago
(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.
(Assignee)

Comment 34

2 months ago
Comment on attachment 8871475 [details]
Bug 1361938 - Do not reuse DeclarationBlock when modifying inline style.

Clearing review request.
Attachment #8871475 - Flags: review?(cam)
(Assignee)

Comment 35

2 months ago
(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).
(Assignee)

Comment 36

2 months ago
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
(Assignee)

Comment 37

2 months ago
Forgot to enable test_animations_omta.html on non-e10s.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0927f821c84f496c44aec574e2119cd4daf91eb9
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

2 months ago
mozreview-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.
Boris any ideas?
Attachment #8872032 - Flags: review?(hikezoe)
(Assignee)

Comment 42

2 months ago
Comment on attachment 8872032 [details]
Bug 1361938 - Enable test_animations_omta_start.html.

MozReview cheated me.
Attachment #8872032 - Flags: review?(hikezoe) → review?(boris.chiou)
(Assignee)

Updated

2 months ago
Blocks: 1288269
(Reporter)

Comment 43

2 months ago
mozreview-review
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+
(Reporter)

Comment 44

2 months ago
mozreview-review-reply
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 45

2 months ago
mozreview-review
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 46

2 months ago
mozreview-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.
(Assignee)

Comment 47

2 months ago
Ah, right. We need some kind of dirty flag there, it requires more works.
(Assignee)

Updated

2 months ago
Attachment #8871475 - Flags: review?(cam)
(Assignee)

Comment 48

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e48bbc2487a779fcf4d3e614fb94c1870352fde0
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 54

2 months ago
mozreview-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/#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 55

2 months ago
mozreview-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 56

2 months ago
mozreview-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+
(Assignee)

Comment 57

2 months ago
Thank you Cameron.  There were lots of typos..

A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62cc851322290cee38e34daf681ed54a0b93fbda

Comment 58

2 months ago
mozreview-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 59

2 months ago
mozreview-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/#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.
(Assignee)

Comment 60

2 months ago
(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 61

2 months ago
mozreview-review
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.
(Assignee)

Comment 62

2 months ago
(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.
(Assignee)

Comment 63

2 months ago
(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.
(Assignee)

Comment 64

2 months ago
(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.
(Assignee)

Comment 65

2 months ago
It seems to me that once style attribute sharing starts working (bug 1368399) this bug will be solved?
(Assignee)

Comment 66

2 months ago
(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.:-)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8872031 - Attachment is obsolete: true

Comment 71

2 months ago
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
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1368380

Comment 73

2 months ago
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc70d2e36fac
Mark Gecko_UnsetDirtyStyleAttr as ignoreContens for now. r=me DONTBUILD
(Assignee)

Comment 74

2 months ago
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.
(Assignee)

Updated

2 months ago
Depends on: 1368922

Comment 75

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3bf3996068d
https://hg.mozilla.org/mozilla-central/rev/2585796d4280
https://hg.mozilla.org/mozilla-central/rev/e6ba19be442f
https://hg.mozilla.org/mozilla-central/rev/26a25602688a
https://hg.mozilla.org/mozilla-central/rev/cc70d2e36fac
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.