Closed Bug 1367225 Opened 7 years ago Closed 7 years ago

stylo: Animation-only restyle seems to be broken

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1367225 - Make replace_rules returning boolean.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1367225 - Don't process style attribute changes in animation-only restyle.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
While I was checking bug 1361938, I found a couple of problem in animation-only restyle.

1) RESTYLE_STYLE_ATTRIBUTE seems to be also processed in animation-only restyle
2) RESTYLE_SMIL, RESTYLE_CSS_TRANSITIONS and RESTYLE_CSS_ANIMATIONS look exclusive, we should use interscts() instead of contains()
3) TraversalFlags.for_animation_only() should also use intersetcs() instead of contains()

https://treeherder.mozilla.org/#/jobs?repo=try&revision=624d0982d97cafbed1fd3f1e80b299fbcb6e93dd
Comment on attachment 8870569 [details]
Bug 1367225 - Animation-only restyle should be also processed in all the kinds of traversal.

https://reviewboard.mozilla.org/r/142026/#review145694

::: servo/components/style/traversal.rs:50
(Diff revision 1)
>  }
>  
>  impl TraversalFlags {
>      /// Returns true if the traversal is for animation-only restyles.
>      pub fn for_animation_only(&self) -> bool {
> -        self.contains(ANIMATION_ONLY)
> +        self.intersects(ANIMATION_ONLY)

Hmmm... I'm very confused about this patch (and the previous ones too). Does this make a difference? 

`intersects` is defined as:

self.bits & flags.bits != 0

While `contains` is:

self.bits & flags.bits == flags.bits

For flags that are only one bit, that shouldn't make a difference, should it?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Comment on attachment 8870569 [details]
> Bug 1367225 - Animation-only restyle should be also processed in all the
> kinds of traversal.
> 
> https://reviewboard.mozilla.org/r/142026/#review145694
> 
> ::: servo/components/style/traversal.rs:50
> (Diff revision 1)
> >  }
> >  
> >  impl TraversalFlags {
> >      /// Returns true if the traversal is for animation-only restyles.
> >      pub fn for_animation_only(&self) -> bool {
> > -        self.contains(ANIMATION_ONLY)
> > +        self.intersects(ANIMATION_ONLY)
> 
> Hmmm... I'm very confused about this patch (and the previous ones too). Does
> this make a difference? 
> 
> `intersects` is defined as:
> 
> self.bits & flags.bits != 0
> 
> While `contains` is:
> 
> self.bits & flags.bits == flags.bits
> 
> For flags that are only one bit, that shouldn't make a difference, should it?

Oh, you are right. I was just referring the bitflags document. There is no difference. But it's really confusing.  I think we should use intersetcs() there just in case that if someone tries to add another flag in contains().
Comment on attachment 8870566 [details]
Bug 1367225 - Make replace_rules returning boolean.

https://reviewboard.mozilla.org/r/142020/#review145796

::: servo/components/style/matching.rs:967
(Diff revision 1)
>                                     new_values,
>                                     pseudo)
>      }
>  
>      /// Updates the rule nodes without re-running selector matching, using just
> -    /// the rule tree. Returns RulesChanged which indicates whether the rule nodes changed
> +    /// the rule tree. Returns true if an important rule was replaced.

(Nit: s/important/!important/)
Attachment #8870566 - Flags: review?(bbirtles) → review+
I don't understand the change from intersects to contains. Don't we want the more restrictive 'contains' instead?
Comment on attachment 8870567 [details]
Bug 1367225 - Don't process style attribute changes in animation-only restyle.

https://reviewboard.mozilla.org/r/142022/#review145798
Attachment #8870567 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #8)
> I don't understand the change from intersects to contains. Don't we want the
> more restrictive 'contains' instead?

Ok, I will drop them, it's confusing.
Attachment #8870568 - Attachment is obsolete: true
Attachment #8870568 - Flags: review?(bbirtles)
Attachment #8870569 - Attachment is obsolete: true
Attachment #8870569 - Flags: review?(bbirtles)
I am going to include these changes into a PR for bug 1366631 to avoid an extra PR in the servo's CI queue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef94d0ae936c0a4ad8978a093cba8e18bf93c830
Assignee: nobody → hikezoe
Priority: -- → P1
https://hg.mozilla.org/integration/autoland/rev/38e611761168
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: