Closed Bug 1390352 Opened 7 years ago Closed 7 years ago

stylo: Various SMIL mochitests fail due to animation of 'clip' property

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #18210
41 bytes, text/x-github-pull-request
Details | Review 
e.g.

TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation end) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (after animation end) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation end) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (after animation end) - testcase specified to have no effect - got "rect(auto, auto, auto, auto)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "rect(1px, 2px, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "rect(1px, 2px, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(1px, 2px, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(1px, 2px, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation end) - testcase specified to have no effect - got "rect(1px, 2px, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (after animation end) - testcase specified to have no effect - got "rect(1px, 2px, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "rect(1px, auto, 3px, 4px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation mid) - testcase specified to have no effect - got "rect(6px, auto, 18px, 24px)", expected "auto"
TEST-UNEXPECTED-FAIL | dom/smil/test/test_smilCSSFromBy.xhtml | clip: shouldn't be affected by animation (at animation begin) - testcase specified to have no effect - got "rect(1px, auto, 3px, 4px)", expected "auto"

Similarly in:
* dom/smil/test/test_smilCSSPaced.xhtml
* dom/smil/test/test_smilMappedAttrFromBy.xhtml
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Failed test is as follow[1] :
    new AnimTestcaseFromBy("auto",
                           "rect(auto, auto, auto, auto)", { noEffect: 1 }),
    new AnimTestcaseFromBy("rect(auto, auto, auto, auto)",
                           "rect(auto, auto, auto, auto)", { noEffect: 1 }),
    new AnimTestcaseFromBy("rect(1px, 2px, 3px, 4px)", "auto", { noEffect: 1 }),
    new AnimTestcaseFromBy("auto", "rect(1px, 2px, 3px, 4px)", { noEffect: 1 }),
    new AnimTestcaseFromBy("rect(1px, auto, 3px, 4px)",
                           "rect(10px, auto, 30px, 40px)", { noEffect: 1 }),

[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/smil/test/db_smilCSSFromBy.js#107-112,115-116

It seems to me that servo fail to interpolate between 'auto' and rect value, and fail to interpolate value which rect has 'auto' value. However it will success to interpolate 'rect(1px, 2px, 3px, auto)' to 'rect(10px, 20px, 30px, 40px)'.

I think that to interpolate clip property has 2 problems:

1) Servo will interpolate clip property between ClipRect and Auto
 If we will animate 'rect(1px, 2px, 3px, 4px)' to 'auto', servo will interpolate as discrete since Servo define this type as ClipRectOrAuto(i.e. Either<ClipRect, Auto>).  We should not interpolate this type. [2]

[2] https://lists.w3.org/Archives/Public/public-fx/2013OctDec/0056.html

 We had better to use enum for this property value instead of Either<>.

2) Servo will interpolate clip property rect value which has auto value
 An each value of ClipRect defined as Option<Au>, so servo can't interpolate between None and Some(Au). However servo will interpolate between None and None, as result interpolating the clip property which rect value has auto value will success. We should not interpolate in this case.

 We need to check that rect has an auto value when interpolating this property. if rect has auto value, servo should stop interpolate immediately.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> However servo will interpolate between None and
> None, as result interpolating the clip property which rect value has auto
> value will success. We should not interpolate in this case.

I have a patch on try for this part which seems ok so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3471604790d63052806895bbf2e3b85a686f58ff
(In reply to Brian Birtles (:birtles) from comment #2)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> > However servo will interpolate between None and
> > None, as result interpolating the clip property which rect value has auto
> > value will success. We should not interpolate in this case.
> 
> I have a patch on try for this part which seems ok so far:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3471604790d63052806895bbf2e3b85a686f58ff

Thank you!
I'm going to add new enum for this type instead of Either<>. An interpolating of this enum will throw Err(()) when value is None. 
Maybe, this behavior will same to your patch on try.

https://hg.mozilla.org/try/rev/dc5d97efbfb682205a5d50dfb1056eb4dd83a275#l2.60
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ab56739e5957b85d4caf139893db439c0a35e8
Oh sorry, no I was thinking of the 'none' paint type. It's completely different :)
Ah,, comment 1 is wrong.
Gecko stop to interpolate if sum of coeff is not 1.0 when rect's offset unit is 'Auto'[1].

[1] http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/layout/style/StyleAnimationValue.cpp#3130-3134

So fix plan of 2) which I mentioned on comment 1 is not correct. If servo will stop interpolating this value which has auto offset value, we make regression of bug 1363639. (i.e. we should interpolate between 'rect(1px, 2px, auto, 4px)' and 'rect(3px, 4px, auto, 6px)')

I think servo need to be same implementation with gecko for fixing this bug.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> I think servo need to be same implementation with gecko for fixing this bug.

Implemented it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b6d15027bc85592557691bb5f6e9269591933c5
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> Ah,, comment 1 is wrong.
> Gecko stop to interpolate if sum of coeff is not 1.0 when rect's offset unit
> is 'Auto'[1].
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 5ce320a16f6998dc2b36e30458131d029d107077/layout/style/StyleAnimationValue.
> cpp#3130-3134
> 
> So fix plan of 2) which I mentioned on comment 1 is not correct. If servo
> will stop interpolating this value which has auto offset value, we make
> regression of bug 1363639. (i.e. we should interpolate between 'rect(1px,
> 2px, auto, 4px)' and 'rect(3px, 4px, auto, 6px)')
> 
> I think servo need to be same implementation with gecko for fixing this bug.

Could you please elaborate the implementation?

It seems to me that your fix in the try breaks iteration composite

https://hg.mozilla.org/try/rev/69d8bc00070d3baafea81295aea95175a772bdb2#l1.16
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5)
> > Ah,, comment 1 is wrong.
> > Gecko stop to interpolate if sum of coeff is not 1.0 when rect's offset unit
> > is 'Auto'[1].
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > 5ce320a16f6998dc2b36e30458131d029d107077/layout/style/StyleAnimationValue.
> > cpp#3130-3134
> > 
> > So fix plan of 2) which I mentioned on comment 1 is not correct. If servo
> > will stop interpolating this value which has auto offset value, we make
> > regression of bug 1363639. (i.e. we should interpolate between 'rect(1px,
> > 2px, auto, 4px)' and 'rect(3px, 4px, auto, 6px)')
> > 
> > I think servo need to be same implementation with gecko for fixing this bug.
> 
> Could you please elaborate the implementation?
> 
> It seems to me that your fix in the try breaks iteration composite
> 
> https://hg.mozilla.org/try/rev/69d8bc00070d3baafea81295aea95175a772bdb2#l1.16

Oh sorry.
I forgot remove this macro code. This is unnecessary since this patch check same condition when interpolating Rect's value.[1] 
[1] https://hg.mozilla.org/try/rev/69d8bc00070d3baafea81295aea95175a772bdb2#l4.26

Thanks.
Priority: -- → P2
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

Clear review request flag.
Sorry this patch is wrong the comment and format. I'll update this soon.
Attachment #8898451 - Flags: review?(bbirtles)
Note to self:
Failed this paced test..?
http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/smil/test/db_smilCSSPaced.js#202

Servo will return 0.0 value when fail calculating the squared distance[1]. However gecko implementation expected that return bool value as failure of computing distance.[2]

[1] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/servo/components/style_derive/compute_squared_distance.rs#63
[2] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/layout/style/StyleAnimationValue.cpp#1723

I'm not sure that why geckolib eat this failure. However I think we need to care this error for smil paced animation.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #12)
> Note to self:
> Failed this paced test..?
> http://searchfox.org/mozilla-central/rev/
> e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/smil/test/db_smilCSSPaced.js#202
> 
> Servo will return 0.0 value when fail calculating the squared distance[1].
> However gecko implementation expected that return bool value as failure of
> computing distance.[2]
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> b258e6864ee3e809d40982bc5d0d5aff66a20780/servo/components/style_derive/
> compute_squared_distance.rs#63
> [2]
> http://searchfox.org/mozilla-central/rev/
> b258e6864ee3e809d40982bc5d0d5aff66a20780/layout/style/StyleAnimationValue.
> cpp#1723
> 
> I'm not sure that why geckolib eat this failure. However I think we need to
> care this error for smil paced animation.

I decided that I will fix this test failure bug in this bug.
Servo should return computing distance result as bool, and move the calculating distance to the out parameter.
Gecko side should treat the result of `Servo_AnimationValues_ComputeDistance()`, if return value is false, `nsSMILCSSValue::ComputeDistanceForServoe()` return NS_ERROR_FAILURE[1].  Gecko will use this result value for paced animation interpolation[2].

[1] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/dom/smil/nsSMILCSSValueType.cpp#458
[2] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/dom/smil/nsSMILAnimationFunction.cpp#417


Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a51442f3510c8df0e266ae450951dcc5a400f4b
Comment on attachment 8899322 [details]
Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.

https://reviewboard.mozilla.org/r/170534/#review175716

Where is the change for the binding function?
Comment on attachment 8899323 [details]
Bug 1390352 - Add check the return value of Servo_AnimationValues_ComputeDistance.

https://reviewboard.mozilla.org/r/170536/#review175722

::: dom/smil/nsSMILCSSValueType.cpp:463
(Diff revision 1)
> -    double distance = Servo_AnimationValues_ComputeDistance(*fromValue, *toValue);
> +    double distance = 0.0;
> +    if (!Servo_AnimationValues_ComputeDistance(*fromValue,
> +                                               *toValue,
> +                                               &distance)) {
> +      return NS_ERROR_FAILURE;
> +    }

Nit: Put a blank line here.

::: layout/style/StyleAnimationValue.cpp:5404
(Diff revision 1)
>  
> -  if (mServo) {
> -    return Servo_AnimationValues_ComputeDistance(mServo, aOther.mServo);
> -  }
>  
> -  double distance = 0.0;
> +  double distance= 0.0;

Nit: Put a space before '='.
Comment on attachment 8899322 [details]
Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.

https://reviewboard.mozilla.org/r/170534/#review175718

This patch needs to be unified with the third patch during review.
r=me with unifying them.

::: commit-message-f07e2:1
(Diff revision 1)
> +Bug 1390352 - Add the result of computing distance for Servo_AnimationValues_ComputeDistance. r?hiro

Make Servo_AnimationValues_ComputeDistance return false instead of 0.0 when the function fails to distinguish its failure.

::: commit-message-f07e2:3
(Diff revision 1)
> +Bug 1390352 - Add the result of computing distance for Servo_AnimationValues_ComputeDistance. r?hiro
> +
> +We need to check the result of computing distance in order to check

This comment is really confusing. 'the result of computing distance' is the distance value, right? it's a double whatever the function returns. So this sentence does not make sense to me.

::: commit-message-f07e2:4
(Diff revision 1)
> +Bug 1390352 - Add the result of computing distance for Servo_AnimationValues_ComputeDistance. r?hiro
> +
> +We need to check the result of computing distance in order to check
> +whether we support the specified paced aniamtion value.

Nit: animation.
Attachment #8899322 - Flags: review?(hikezoe) → review+
Comment on attachment 8899322 [details]
Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.

https://reviewboard.mozilla.org/r/170534/#review175736

As per discussion with Boris, we should return a negative value for the failure case here.
Comment on attachment 8899322 [details]
Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.

https://reviewboard.mozilla.org/r/170534/#review175736

Thanks hiro.
I addressed this patch using by negative return value.
Comment on attachment 8899323 [details]
Bug 1390352 - Add check the return value of Servo_AnimationValues_ComputeDistance.

https://reviewboard.mozilla.org/r/170536/#review175766

Please integrate this into the second patch.
Attachment #8899323 - Attachment is obsolete: true
Attachment #8899323 - Flags: review?(hikezoe)
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176152

::: commit-message-93238:1
(Diff revision 2)
> +Bug 1390352 - Make clip animatable between auto value and others. r?manishearth
> +
> +This pach make clip property animatable between following values:
> + 1) interpolate between 'auto' and rect value.
> +
> + 2) interpolate between rect which contain 'auto' value, and those 'auto' value
> +     coresponded other rect's offset value.
> +
> +    i.e. We can interpolate following value which between
> +         'rect(1px, auto, 2px, 3px)' and 'rect(10px, auto, 12px, 13px)'
> +         However we can't interpolate between 'rect(1px, 2px, 3px, 4px)' and
> +         'rect(auto, 12px, 13px, 14px)' since corresponded offset value doesn't
> +         same to 'auto'.

It this comment up-to-date?

If I understand correctly, what this patch does it allow *interpolating* rect(1px, 1px, 1px, auto) with rect(2px, 2px, 2px, auto) but not *adding* or *accumulating* them.

Is that right?

::: servo/components/style/values/computed/mod.rs:493
(Diff revision 2)
> +            (None, None) => {
> +                // Interpolating between two auto values makes sense;
> +                // adding in other ratios does not.
> +                if (self_portion + other_portion) != 1.0 {
> +                    return false;
> +                }
> +                true
> +            },

Couldn't this just be:

  (None, None) => self_portion + other_portion == 1.0

::: servo/components/style/values/specified/mod.rs:626
(Diff revision 2)
> +// rect(...) | auto
> +#[allow(missing_docs)]
> +#[derive(Clone, Debug, HasViewportPercentage, PartialEq, ToCss)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
>  /// rect(...) | auto
> -pub type ClipRectOrAuto = Either<ClipRect, Auto>;
> +pub enum ClipRectOrAuto {
> +    ClipRect(ClipRect),
> +    Auto(Auto),
> +}

Do we actually need this extra type?

(I'm not sure because the commit message isn't clear, but I thought the main point of this patch was the extra call to can_interpolate_to in add_weighted for ClipRect.)
Attachment #8899322 - Attachment is obsolete: true
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176152

Thank you for reviewing this!

> It this comment up-to-date?
> 
> If I understand correctly, what this patch does it allow *interpolating* rect(1px, 1px, 1px, auto) with rect(2px, 2px, 2px, auto) but not *adding* or *accumulating* them.
> 
> Is that right?

As you mentioned in a comment, my understanding was wrong, So I updated message. (I misused the `interpolate` word.)

> Do we actually need this extra type?
> 
> (I'm not sure because the commit message isn't clear, but I thought the main point of this patch was the extra call to can_interpolate_to in add_weighted for ClipRect.)

Oh,, Sorry. This structure is acutually not necessary.
I thought that we should remove Either code from this, but it is not needed for fixing this bug.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #32)
> Created attachment 8899693 [details]
> Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative
> value instead of 0.0 when the function fails to distinguish its failure.
> 
> We need to check the return value of Servo_AnimationValues_ComputeDistance
> in order to check whether we support the specified paced animation value.
> 
> Current servo returns 0.0 when failed computing distance, so servo doesn't
> distinguish its failure. This patch makes
> Servo_AnimationValue_ComputeDistance
> return a negative value when the function fails.
> 
> Review commit: https://reviewboard.mozilla.org/r/171008/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/171008/

I deleted the previous review by mistake. I updated the patch again.
Comment on attachment 8899693 [details]
Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure.

https://reviewboard.mozilla.org/r/171008/#review176184

::: commit-message-8ed3c:3
(Diff revision 1)
> +Bug 1390352 - Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure. r=hiro
> +
> +We need to check the return value of Servo_AnimationValues_ComputeDistance

Nit: What we need to do here is to know whether the function fails or not. So 'check the return value' is inacculate, 'check whether the function fails or not'?
Attachment #8899693 - Flags: review?(hikezoe) → review+
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176178

Thanks this looks a lot better. r=me with the following comments addressed

::: servo/components/style/values/computed/mod.rs:485
(Diff revision 6)
> +    /// Whether we can interpolate rect's offset.
> +    fn can_interpolate_offset_value(o1: Option<Au>,

See below, but perhaps we should call this can_animate_component?

And make the comment:

  /// Returns true if both values can be interpolated/added together using the
  /// specified ratios.

::: servo/components/style/values/computed/mod.rs:486
(Diff revision 6)
> +    fn can_interpolate_offset_value(o1: Option<Au>,
> +                                    o2: Option<Au>,

Perhaps just call |o1| and |o2|, |a| and |b| ?

::: servo/components/style/values/computed/mod.rs:488
(Diff revision 6)
> +                                    self_portion: f64,
> +                                    other_portion: f64)

self_portion and other_portion don't correspond to anything now (is o1 self or other?) So perhaps these should be renamed a_portion and b_portion.

::: servo/components/style/values/computed/mod.rs:493
(Diff revision 6)
> +                                    self_portion: f64,
> +                                    other_portion: f64)
> +                                    -> bool {
> +        match (o1, o2) {
> +            (Some(_), Some(_)) => true,
> +            (None, None) => self_portion + other_portion == 1.0,

Does this deserve a comment saying:

// We allow interpolating 'auto' with 'auto' but not adding them.

(I assume None here represents auto)

::: servo/components/style/values/computed/mod.rs:498
(Diff revision 6)
> +    /// Compare each offset value, If corresponded offset is same type(i.e. 'auto' or not),
> +    /// return true. Otherwise is false.

/// Returns true if all corresponding components in this and |other| can be
/// interpolated/added together.

::: servo/components/style/values/computed/mod.rs:500
(Diff revision 6)
> +    pub fn can_interpolate_to(&self, other: &Self, self_portion: f64, other_portion: f64)
> +                          -> bool {

Is the indentation here right? Perhaps run rustfmt over this and check what it says. I think it might do something like:

      pub fn can_interpolate_to(&self, other: &Self, self_portion: f64, other_portion: f64)
          -> bool
      {

::: servo/components/style/values/computed/mod.rs:500
(Diff revision 6)
> +    pub fn can_interpolate_to(&self, other: &Self, self_portion: f64, other_portion: f64)
> +                          -> bool {

Since this is used for more than just interpolation, but addition too, I guess this should be called can_animate_with?
Attachment #8898451 - Flags: review?(bbirtles) → review+
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176188

::: servo/components/style/values/computed/mod.rs:493
(Diff revision 6)
> +                                    self_portion: f64,
> +                                    other_portion: f64)
> +                                    -> bool {
> +        match (o1, o2) {
> +            (Some(_), Some(_)) => true,
> +            (None, None) => self_portion + other_portion == 1.0,

I guess we need some amount of tolerance here, just like what we do in MatrixDecomposed3D.add_weighted().
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176178

Thanks. Brian, hiro.
PR 18134 has been merged, and this change effect this bug.
I merged those changed, and I addressed this review point.

> self_portion and other_portion don't correspond to anything now (is o1 self or other?) So perhaps these should be renamed a_portion and b_portion.

This code is not necessary since those portion value removed by PR 18134.
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176552

::: servo/components/style/values/computed/mod.rs:488
(Diff revision 7)
>  }
>  
> +impl ClipRect {
> +    /// Returns true if both values can be interpolated/added together using the
> +    /// specified interpolation procedure.
> +    fn can_interpolate_offset_value(a: Option<Au>,

can_animate_component

(see comment 38)

::: servo/components/style/values/computed/mod.rs:503
(Diff revision 7)
> +            _ => false,
> +        }
> +    }
> +
> +    /// Return true if all corresponding components in this and |other| can be
> +    /// interpolated//added together.

Nit: just one / between interpolated and added
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review176552

Thanks,
I addressed it, and I added the code which I forgot merged.(match arm of Procedure::Interpolate).
Comment on attachment 8898451 [details]
Bug 1390352 - Skip adding/accumulating the values which corresponding rect offset is auto.

https://reviewboard.mozilla.org/r/169612/#review177142
Attachment #8898451 - Flags: review?(manishearth) → review+
Attached file Servo PR, #18210 — 
I rebased this patches and separated this patches into Servo and Gecko.
Attachment #8898451 - Attachment is obsolete: true
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f950e9aa797d
Make Servo_AnimationValues_ComputeDistance return negative value instead of 0.0 when the function fails to distinguish its failure. r=hiro
https://hg.mozilla.org/mozilla-central/rev/f950e9aa797d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: