Closed Bug 1352067 Opened 7 years ago Closed 7 years ago

stylo: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive

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: boris)

Details

Attachments

(3 files)

Bug 1352067 - Part 1: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive.
59 bytes, text/x-review-board-request
birtles
: review+
emilio
: review+
Details
Bug 1352067 - Part 2: Avoid passing any nullptr into Servo_AnimationValue_DeepEqual.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #16202
41 bytes, text/x-github-pull-request
Details | Review 
We will obsolete StyleAnimationValue in the future, and we can treat AnimationValue as a wrapper of RawServoAnimationValue to hide the FFIs. However, we still need both types now, i.e. StyleAnimationValue and RawServoAnimationValue, so it's better to make sure they are mutual exclusive in AnimationValue. One of the solutions is to add some assertions, according to Bug 1343753 comment 17.

Besides, I think those FFIs might do many things and it seems those methods are not critical, so let's move them into cpp files, so we can remove some dependencies to avoid re-compiler so many files if someone needs revise ServoBindings.h (Bug 1340033).
Comment on attachment 8852996 [details]
Bug 1352067 - Part 1: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive.

https://reviewboard.mozilla.org/r/125076/#review127806

Gecko side seems fine to me, except for a couple of nits mentioned below.

::: commit-message-03d60:1
(Diff revision 1)
> +Bug 1352067 - stylo: Make sure AnimationValue::{mGecko|mServo} are mutual exclusive.

Nit: s/mutual/mutually/

Also, you should move most of the explanation from this bug's comment 0 into this commit message. That is, in the commit message, you should explain what the change is, why you're making this change, why it is correct etc.

::: layout/style/StyleAnimationValue.h:573
(Diff revision 1)
>    }
>  };
>  
>  struct AnimationValue
>  {
> +  // mGecko and mServo are mutual exclusive, one of them must be null.

s/mutual/mutually/

Nit: It's not clear if "mGecko and mServo are mutual exclusive" and "one of them must be null" are saying the same thing or not. If you think the first part is not sufficiently clear, then perhaps you could make it obvious that the second part of clarifying the first, e.g.

  "mGecko and mServo are mutually exclusive: only one or the other should ever be set."

::: layout/style/StyleAnimationValue.h:575
(Diff revision 1)
>  
>  struct AnimationValue
>  {
> +  // mGecko and mServo are mutual exclusive, one of them must be null.
> +  // FIXME: After obsoleting StyleAnimationValue, we should remove mGecko, and
> +  // make AnimationValue as a wrapper of RawServoAnimationValue to hide these

Nit: s/as a wrapper/a wrapper/
Attachment #8852996 - Flags: review?(bbirtles) → review+
In missing keyframes case, neither mServo nor mGecko is null?
Oh, nvm. This value is composed value.
No. We are using AnimationValue in AnimationPropertySegment.
Summary: stylo: Make sure AnimationValue::{mGecko|mServo} are mutual exclusive → stylo: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> In missing keyframes case, neither mServo nor mGecko is null?

I think we should try to keep "only one of them is non-null for AnimationValue" in different cases, so we can make sure we only use RawServoAnimationValue for animations on the main thread.
Comment on attachment 8852996 [details]
Bug 1352067 - Part 1: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive.

https://reviewboard.mozilla.org/r/125076/#review127852

::: layout/style/StyleAnimationValue.cpp:5205
(Diff revision 2)
> +  if (mServo) {
> +    return Servo_AnimationValue_DeepEqual(mServo, aOther.mServo);
> +  }

Oho.  I just realize we need to fix this for missing keyframe.
Boris, would you mind fixing it in this bug?

Actually this operator fails when mServo is nullptr and aOther.mServo is not nullptr.
Comment on attachment 8852996 [details]
Bug 1352067 - Part 1: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive.

https://reviewboard.mozilla.org/r/125076/#review127852

> Oho.  I just realize we need to fix this for missing keyframe.
> Boris, would you mind fixing it in this bug?
> 
> Actually this operator fails when mServo is nullptr and aOther.mServo is not nullptr.

Sure. Thanks for finding this bug! I'm working on this now.
Comment on attachment 8853224 [details]
Bug 1352067 - Part 2: Avoid passing any nullptr into Servo_AnimationValue_DeepEqual.

https://reviewboard.mozilla.org/r/125290/#review127872

::: layout/style/StyleAnimationValue.cpp:5205
(Diff revision 1)
>    if (mServo) {
> -    return Servo_AnimationValue_DeepEqual(mServo, aOther.mServo);
> +    // If one of the value is nullptr, Servo_AnimationValue_DeepEqual will
> +    // trigger a debug assertion.
> +    return !!aOther.mServo &&
> +           Servo_AnimationValue_DeepEqual(mServo, aOther.mServo);

This looks still wrong....
We'd want to get false in the case where mServo is null and aOther.mServo isn't null.
Comment on attachment 8853224 [details]
Bug 1352067 - Part 2: Avoid passing any nullptr into Servo_AnimationValue_DeepEqual.

https://reviewboard.mozilla.org/r/125290/#review127882
Attachment #8853224 - Flags: review?(hikezoe) → review+
Comment on attachment 8852996 [details]
Bug 1352067 - Part 1: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive.

https://reviewboard.mozilla.org/r/125076/#review127974

Looks ok to me, thanks :)
Attachment #8852996 - Flags: review?(emilio+bugs) → review+
Attached file Servo PR, #16202 — 

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d6ec43bbab7
Part 1: Make sure AnimationValue::{mGecko|mServo} are mutually exclusive. r=birtles,emilio
https://hg.mozilla.org/integration/autoland/rev/c22f3585e9f7
Part 2: Avoid passing any nullptr into Servo_AnimationValue_DeepEqual. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0d6ec43bbab7
https://hg.mozilla.org/mozilla-central/rev/c22f3585e9f7
Status: NEW → RESOLVED
Closed: 7 years 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: