Closed Bug 1353202 Opened 7 years ago Closed 7 years ago

stylo: Implement accumulate on Servo AnimationValue

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: hiro, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 24 obsolete files)

Servo PR #17001
41 bytes, text/x-github-pull-request
Details | Review
Bug 1353202 - Add nsInterfaceHashtable to opaque types
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1353202 - Add support for iteration composite modes
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1353202 - Use assert_matrix_equal to compare matrices in iterationComposite.html test
59 bytes, text/x-review-board-request
hiro
: review+
Details 
+++ This bug was initially created as a clone of Bug #1329878 +++

This is for *accumulative' animations.  I just noticed that we need to extend RGBA struct in servo (precisely it's in cssparser) to represent 255 over each component values just like we did in bug 1295107.
Depends on: 1356941
No longer depends on: 1329878
In trying to determine the right API for the accumulate method, I decided to investigate how SMIL uses
accumulate.

For accumulation SMIL does:

  if (!IsToAnimation() && GetAccumulate() && mRepeatIteration) {
    const nsSMILValue& lastValue = aValues[aValues.Length() - 1];

    // If the target attribute type doesn't support addition, Add will
    // fail and we leave aResult untouched.
    aResult.Add(lastValue, mRepeatIteration);
  }

Here, aResult.Add is basically:

  return mType->Add(*this, aValueToAdd, aCount);

i.e.

  nsSMILCSSValueType::Add(aResult, aLastValue, aRepeatIteration)

which ends up calling:

  return StyleAnimationValue::Add(property,
                                  destWrapper->mCSSValue.mGecko,
                                  valueToAdd->mGecko, aCount)
         ? NS_OK
         : NS_ERROR_FAILURE;

i.e. roughly

  StyleAnimationValue::Add(property, aResult, aValueToAdd, aRepeatIteration)

and *that* becomes:

  return AddWeighted(aProperty, 1.0, aDest, aCount, aValueToAdd, aDest);

i.e.

  AddWeighted(property, 1.0, aResult, aRepeatIteration, aValueToAdd, aResult);

Now, the curious thing is that we're using this for *accumulation*. Normally we have a different API for that:
StyleAnimationValue::Accumulate which does special things for:

  * eUnit_Filter,
  * eUnit_Shadow,
  * eUnit_Color,
  * eUnit_Transform

Of those, we only animate eUnit_Color from SMIL. If we were to call StyleAnimationValue::Accumulate, we would
end up doing:

  RGBAColorData color1 = ExtractColor(result);
  RGBAColorData color2 = ExtractColor(aA);
  result.mValue.mCSSValue->SetRGBAColorValue(
    AddWeightedColors(1.0, color1, aCount, color2));

But instead we end up calling AddWeighted directly which does:

  RGBAColorData color1 = ExtractColor(aValue1);
  RGBAColorData color2 = ExtractColor(aValue2);
  auto resultColor = MakeUnique<nsCSSValue>();
  resultColor->SetColorValue(
    AddWeightedColorsAndClamp(aCoeff1, color1, aCoeff2, color2));
  aResultValue.SetAndAdoptCSSValueValue(resultColor.release(), eUnit_Color);

So it would seem the difference is whether we call AddWeightedColors or AddWeightedColorsAndClamp.

However, if we look at AddWeightedColorsAndClamp it is just:

  return aCoeff2 == 0.0
    ? DiluteColor(aValue1, aCoeff1)
    : AddWeightedColors(aCoeff1, aValue1, aCoeff2, aValue2).ToColor();

And since aCoeff2 is aRepeatIteration and we explicitly zero-check mRepeatIteration before doing any of this,
I think these basically end up doing the same thing.

Ideally, I'd like to make SMIL call the corresponding |accumulate| and |add| methods rather than just always
calling Add. The reason is that if we eventually get SMIL to animate 'filter' or 'transform' or 'text-shadow',
the behavior will differ.

With that in mind, I *think* the following API will work:

    /// Returns the result of [accumulating][animation-accumulation] |other| onto this value
    /// |count| times.
    /// [Accumulates][animation-accumulation] this value onto itself (|count| - 1) times then
    /// accumulates |other| onto the result.
    ///
    /// If |count| is zero, the result will be |other|.
    ///
    /// [animation-accumulation]: https://w3c.github.io/web-animations/#animation-accumulation
    fn accumulate(&self, other: &Self, count: u64) -> Result<Self, ()> {
        match count {
            0 => Ok(other),
            _ => self.add_weighted(other, count as f64, 1.0)
        }
    }

The only real difference from StyleAnimationValue::Accumulate is that it is fallible (which, for the time
being, is necessary for easy integration with SMIL).
Assignee: nobody → bbirtles
Blocks: 1355349
Status: NEW → ASSIGNED
Depends on: 1364795
Priority: P3 → P1
Manish or Emilio, I wonder if you can help me understand the rust way of doing this.

Basically, I am trying to build up end points (from, to) for interpolating between, where 99% of the time we
just want to use the end points as passed-in. Sometimes, however, we want to build up a different value.

Previously we had this arrangement for getting the end points:

    // Declare for making derefenced raw pointer alive outside the if block.
    let raw_from_value;
    let from_value = if !segment.mFromValue.mServo.mRawPtr.is_null() {
        raw_from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
        AnimationValue::as_arc(&raw_from_value).as_ref()
    } else {
        underlying_value.as_ref().unwrap()
    };

    // (the exact same pattern for the 'to' value)

Then, when I added support for additive animation, I made this:

    // Temporaries used in the following if-block whose lifetimes we need to prolong.
    let raw_from_value;
    let from_composite_result;
    let from_value = if !segment.mFromValue.mServo.mRawPtr.is_null() {
        raw_from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
        match segment.mFromComposite {
            CompositeOperation::Add => {
                let value_to_composite = AnimationValue::as_arc(&raw_from_value).as_ref();
                from_composite_result = underlying_value.as_ref().unwrap().add(value_to_composite);
                from_composite_result.as_ref().unwrap_or(value_to_composite)
            }
            _ => { AnimationValue::as_arc(&raw_from_value) }
        }
    } else {
        underlying_value.as_ref().unwrap()
    };;

    // (and again, the same pattern for the 'from' value)

Basically, the arrangement is:

  1. If the value in segment.mFromValue.mServo.mRawPtr is null, we want to use the underlying
     value.

  2. Otherwise, if the have a composite mode of "add", we want to add to the value from
     segment.mFromValue... to the underlying value and use that.
     The add method (implemented on AnimationValue) has the following signature:
         fn add(&self, other: &Self) -> Result<Self, ()>;
     However, if add() errors, we want to just use the segment.mFromValue... as-is.

  3. Otherwise, we want to want to just use the segment.mFromValue... as-is.

This is pretty ugly, especially since it it repeated twice. I wanted to factor out a closure for this but wasn't
sure how to do that with the deferred initialization that this relies on. I asked on #rust-beginners but no one
responded so I just landed the repeated code block.

Now, in this bug, the condition gets more complicated still.

Firstly, I want to handle the Accumulate composite mode, so basically:

    // Temporaries used in the following if block whose lifetimes we need to prolong
    let raw_from_value;
    let from_composite_result;
    let from_value = if !segment.mFromValue.mServo.mRawPtr.is_null() {
        raw_from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
        match segment.mFromComposite {
            CompositeOperation::Add => {
                let value_to_composite = AnimationValue::as_arc(&raw_from_value).as_ref();
                from_composite_result = underlying_value.as_ref().unwrap().add(value_to_composite);
                from_composite_result.as_ref().unwrap_or(value_to_composite)
            },
            CompositeOperation::Accumulate => {
                let value_to_composite = AnimationValue::as_arc(&raw_from_value).as_ref();
                from_composite_result =
                    underlying_value.as_ref().unwrap().accumulate(value_to_composite, 1);
                from_composite_result.as_ref().unwrap_or(value_to_composite)
            },
            _ => { AnimationValue::as_arc(&raw_from_value) }
        }
    } else {
        underlying_value.as_ref().unwrap()
    };

    // (and again, the same pattern for the 'from' value)

This is a big block of code, repeated twice. If it's not possible to factor into a closure, perhaps we can use
a macro? However, the trouble is I *also* want to *sometimes* further modify the from_value (or to_value) to
apply the "iteration composite" mode like so (after making from_value/to_value 'mut'):

    // Apply iteration composite behavior
    if iteration_composite == IterationCompositeOperation::Accumulate &&
       computed_timing.mCurrentIteration > 0 {

        let raw_last_value;
        let last_value = if !last_segment.mToValue.mServo.mRawPtr.is_null() {
            raw_last_value = unsafe { &*last_segment.mToValue.mServo.mRawPtr };
            AnimationValue::as_arc(&raw_last_value).as_ref()
        } else {
            underlying_value.as_ref().unwrap()
        };

        from_iteration_composite_result =
            last_value.accumulate(from_value, computed_timing.mCurrentIteration);
        from_value = from_iteration_composite_result.as_ref().unwrap_or(from_value);

        to_iteration_composite_result =
            last_value.accumulate(to_value, computed_timing.mCurrentIteration);
        to_value = to_iteration_composite_result.as_ref().unwrap_or(to_value);
    }

Now, this relies on two *more* temporaries: from_iteration_composite_result / to_iteration_composite_result.
I tried re-using from_composite_result / to_composite_result but rust complained when I tried to assign to
them because they were being borrowed by from_value / to_value (from the previous block).

Furthermore, these temporaries need to be put *before* the declaration of from_value / to_value so that they
are destroyed *after* them at the end of the function.

So this is all super messy and I'm definitely doing it wrong. I'm just wondering what the rust way  of doing
this is.

Bear in mind that 99% of the time we won't use the underlying value, OR do addition / accumulation, OR do
iteration composition -- we'll just use the from/to values as-is so we'd rather not clone them if we don't
need to.
Flags: needinfo?(manishearth)
Flags: needinfo?(emilio+bugs)
(In reply to Brian Birtles (:birtles) from comment #2)
>         match segment.mFromComposite {
>             CompositeOperation::Add => {
>                 let value_to_composite =
> AnimationValue::as_arc(&raw_from_value).as_ref();
>                 from_composite_result =
> underlying_value.as_ref().unwrap().add(value_to_composite);
>                 from_composite_result.as_ref().unwrap_or(value_to_composite)
>             },
>             CompositeOperation::Accumulate => {
>                 let value_to_composite =
> AnimationValue::as_arc(&raw_from_value).as_ref();
>                 from_composite_result =
>                    
> underlying_value.as_ref().unwrap().accumulate(value_to_composite, 1);
>                 from_composite_result.as_ref().unwrap_or(value_to_composite)
>             },
>             _ => { AnimationValue::as_arc(&raw_from_value) }
>         }

This part can be written:

        match segment.mFromComposite {
            CompositeOperation::Replace => AnimationValue::as_arc(&raw_from_value),
            operation => {
                let value_to_composite = AnimationValue::as_arc(&raw_from_value).as_ref();
                from_composite_result = if operation == CompositeOperation::Add {
                    underlying_value.as_ref().unwrap().add(value_to_composite)
                } else {
                    underlying_value.as_ref().unwrap().accumlate(value_to_composite, 1)
                };
                from_composite_result.as_ref().unwrap_or(value_to_composite)
            },
        }

Very little bit shorter.
With these two patches the relevant tests should pass except:

* We don't support mismatched transform lists yet
* iterationComposite.html needs to be adjusted to allow some tolerance in the values it accepts (like the other accumulate
  tests do)
* The test in iterationComposite.html for "iterationComposite of transform from none to translate" seems completely bogus to me.
  That probably needs to be changed and Gecko fixed to match.
* stroke-dasharray probably should be additive and simply made non-additive for SMIL only
* Obviously we don't do animation of filter lists or a number of discrete properties so those tests don't pass
Slight simplification.
Attachment #8869349 - Attachment is obsolete: true
I think at this stage mutating Options would be better than using pinned temporaries.

Could you link me to the code in context? Preferably something I can pull and hack on.
Flags: needinfo?(manishearth)
Attachment #8869356 - Attachment is obsolete: true
Sorry for the lag in replying here.

So basically, you can't factor it out because you need to lengthen the lifetime, right? Given that lifetime is completely fake, perhaps it makes sense to just use a pointer, and assert it's not null?
Flags: needinfo?(emilio+bugs)
Attachment #8869309 - Attachment is obsolete: true
Attachment #8869310 - Attachment is obsolete: true
(In reply to Manish Goregaokar [:manishearth] from comment #9)
> I think at this stage mutating Options would be better than using pinned
> temporaries.
> 
> Could you link me to the code in context? Preferably something I can pull
> and hack on.

Sure. I've just published the patches here. It's the last patch in the series that needs tweaking. You should just be able to pull:

  hg pull -r 92872e07d225fb6ed947bc0e9c4b484faa3a30cb https://reviewboard-hg.mozilla.org/gecko

(Or whatever the git equivalent of that is.)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Sorry for the lag in replying here.
> 
> So basically, you can't factor it out because you need to lengthen the
> lifetime, right? Given that lifetime is completely fake, perhaps it makes
> sense to just use a pointer, and assert it's not null?

I think so, but I haven't tried yet. I'll try Manish's Option suggestion first, I think.
Attachment #8869883 - Flags: review?(hikezoe)
Attachment #8869884 - Flags: review?(hikezoe)
Attachment #8869885 - Flags: review?(hikezoe)
Attachment #8869886 - Flags: review?(hikezoe)
Attachment #8869886 - Flags: review?(hikezoe)
Comment on attachment 8869883 [details]
Bug 1353202 - Define accumulate method on Animatable trait

https://reviewboard.mozilla.org/r/141418/#review144956
Attachment #8869883 - Flags: review?(hikezoe) → review+
Comment on attachment 8869885 [details]
Bug 1353202 - Fix typo in comment in Servo_AnimationCompose

https://reviewboard.mozilla.org/r/141422/#review144960
Attachment #8869885 - Flags: review?(hikezoe) → review+
Comment on attachment 8869884 [details]
Bug 1353202 - Support accumulation of transform lists

https://reviewboard.mozilla.org/r/141420/#review144964

The logic looks good to me.
One thing I am worring about is that reference of numeric literals is allowed in future Rust.  I saw this PR. https://github.com/servo/servo/pull/16939

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2315
(Diff revision 1)
> +            let mut clamped_w = self.quaternion.3.min(1.0);
> +            clamped_w = clamped_w.max(-1.0);

You can write this as below I think:

let clamped_w = self.quaternion.3.min(1.0).max(-1.0);

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2319
(Diff revision 1)
> +
> +            let mut clamped_w = self.quaternion.3.min(1.0);
> +            clamped_w = clamped_w.max(-1.0);
> +
> +            // Determine the scale factor.
> +            let mut theta = clamped_w.acos() as f32;

Is the f32 cast needed?

Rust have acos() for f32 [1], it the cast is necessary here, it means clamped_w is f64?  I don't quite understand how Rust treats this case.

[1] https://doc.rust-lang.org/std/primitive.f32.html#method.acos

Oh, I just realized that gecko's implementation uses double for the theta and scale, we should use float there for consistency (given that we are using float for quaternion).
Attachment #8869884 - Flags: review?(hikezoe) → review+
Comment on attachment 8869884 [details]
Bug 1353202 - Support accumulation of transform lists

https://reviewboard.mozilla.org/r/141420/#review144972

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1724
(Diff revision 1)
>                      result.push(TransformOperation::Translate(ix, iy, iz));
>                  }
>                  (&TransformOperation::Scale(fx, fy, fz),
>                   &TransformOperation::Scale(tx, ty, tz)) => {
> -                    let ix = fx.add_weighted(&tx, self_portion, other_portion).unwrap();
> -                    let iy = fy.add_weighted(&ty, self_portion, other_portion).unwrap();
> +                    let ix = add_weighted_with_initial_val(&fx, &tx, self_portion,
> +                                                           other_portion, &1.0).unwrap();

Manish, it this 'reference of numeric literals' OK?
Manish: ^
Flags: needinfo?(manishearth)
Yeah, we definitely don't *need* to use references to literals it's just the when I went to write the signature for add_weighted_with_initial_val it seemed inconsistent to use references for the self/other values, but not the initial values.

  fn add_weighted_with_initial_val<T: Animatable>(a: &T,
                                                  b: &T,
                                                  a_portion: f64,
                                                  b_portion: f64,
                                                  initial_val: &T) -> Result<T, ()>
Attachment #8869883 - Attachment is obsolete: true
Attachment #8869885 - Attachment is obsolete: true
Attachment #8869887 - Attachment is obsolete: true
Looks like MozReview got confused again :(
Attachment #8869886 - Flags: review?(hikezoe)
Attachment #8869931 - Flags: review?(manishearth)
Attachment #8869931 - Flags: review?(hikezoe)
Comment on attachment 8869932 [details]
Bug 1353202 - Add support for iteration composite modes

https://reviewboard.mozilla.org/r/141492/#review145010

::: servo/ports/geckolib/glue.rs:448
(Diff revision 1)
> +                Some(endpoint) => last_value.accumulate(&endpoint, count)
> +                                            .ok()
> +                                            .or(Some(endpoint)),

I clearly have no idea about rust. I wanted to make the last line here just '.or(composited_value)' but then rust complained that I'm borrowing composited_value.
Attachment #8869932 - Flags: review?(hikezoe)
Attachment #8869933 - Flags: review?(hikezoe)
Comment on attachment 8869933 [details]
Bug 1353202 - Use assert_matrix_equal to compare matrices in iterationComposite.html test

https://reviewboard.mozilla.org/r/141494/#review145016
Attachment #8869933 - Flags: review?(hikezoe) → review+
Comment on attachment 8869886 [details]
Bug 1353202 - Apply accumulate composite mode when compositing animations

https://reviewboard.mozilla.org/r/141424/#review145032
Attachment #8869886 - Flags: review?(hikezoe) → review+
Comment on attachment 8869931 [details]
Bug 1353202 - Rework Servo_AnimationCompose to rely less on pinned temporaries.

https://reviewboard.mozilla.org/r/141490/#review145046

::: servo/ports/geckolib/glue.rs:393
(Diff revision 1)
> +                        assert!(need_underlying_value,
> +                                "Should have detected we need an underlying value");

We scarely use assert! in servo/components/style code, I don't know the reason but we should use debug_assert! here and elsewhere.

::: servo/ports/geckolib/glue.rs:405
(Diff revision 1)
> +                        underlying_value.as_ref().unwrap().accumulate(keyframe_value, 1).ok()
> +                    },
> +                    _ => None,
> -            }
> +                }
> -            _ => { AnimationValue::as_arc(&raw_to_value) }
> +            },
> +            _ => {

This should be None?

::: servo/ports/geckolib/glue.rs:408
(Diff revision 1)
> -            }
> +                }
> -            _ => { AnimationValue::as_arc(&raw_to_value) }
> +            },
> +            _ => {
> +                assert!(need_underlying_value,
> +                        "Should have detected we need an underlying value");
> +                underlying_value.as_ref().cloned()

Can't we simply do clone() here?

::: servo/ports/geckolib/glue.rs:421
(Diff revision 1)
> +    let from_value = composited_from_value.as_ref().unwrap_or(keyframe_from_value.unwrap());
> +    let to_value   = composited_to_value.as_ref().unwrap_or(keyframe_to_value.unwrap());

Note to myself.
Initially I was wondering why composite_endpoint returns keyframe_from_value when composite: 'replace', after reading the next patch, I understand the reason.  composited_from_value and composited_to_value might be modified if there is interationComposite.
Attachment #8869931 - Flags: review?(hikezoe) → review+
Comment on attachment 8869932 [details]
Bug 1353202 - Add support for iteration composite modes

https://reviewboard.mozilla.org/r/141492/#review145060

::: servo/ports/geckolib/glue.rs:438
(Diff revision 1)
> +        let raw_last_value;
> +        let last_value = if !last_segment.mToValue.mServo.mRawPtr.is_null() {
> +            raw_last_value = unsafe { &*last_segment.mToValue.mServo.mRawPtr };
> +            AnimationValue::as_arc(&raw_last_value).as_ref()
> +        } else {
> +            assert!(need_underlying_value, "Should have detected we need an underlying value");

Nit: debug_assert!

::: servo/ports/geckolib/glue.rs:448
(Diff revision 1)
> +                Some(endpoint) => last_value.accumulate(&endpoint, count)
> +                                            .ok()
> +                                            .or(Some(endpoint)),

This noticed me that accumulate() is not infallible yet?

::: servo/ports/geckolib/glue.rs:451
(Diff revision 1)
> +            let count = computed_timing.mCurrentIteration;
> +            match composited_value {
> +                Some(endpoint) => last_value.accumulate(&endpoint, count)
> +                                            .ok()
> +                                            .or(Some(endpoint)),
> +                _ => last_value.accumulate(keyframe_value.unwrap(), count)

Nit: None => ...,
Comment on attachment 8869884 [details]
Bug 1353202 - Support accumulation of transform lists

https://reviewboard.mozilla.org/r/141420/#review144972

> Manish, it this 'reference of numeric literals' OK?

Yes, this is fine. Rust will prolong the lifetime of the temporary there.
Flags: needinfo?(manishearth)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> Comment on attachment 8869931 [details]
> Bug 1353202 - Rework Servo_AnimationCompose to rely less on pinned
> temporaries.
> 
> https://reviewboard.mozilla.org/r/141490/#review145046
> 
> ::: servo/ports/geckolib/glue.rs:393
> (Diff revision 1)
> > +                        assert!(need_underlying_value,
> > +                                "Should have detected we need an underlying value");
> 
> We scarely use assert! in servo/components/style code, I don't know the
> reason but we should use debug_assert! here and elsewhere.

Oh, I didn't know that. I just figured that if that assertion fails, the next line will panic anyway, and I'd rather we panic on this line instead so we know what the real problem is. I'll change it to debug_assert though.

> ::: servo/ports/geckolib/glue.rs:405
> (Diff revision 1)
> > +                        underlying_value.as_ref().unwrap().accumulate(keyframe_value, 1).ok()
> > +                    },
> > +                    _ => None,
> > -            }
> > +                }
> > -            _ => { AnimationValue::as_arc(&raw_to_value) }
> > +            },
> > +            _ => {
> 
> This should be None?

Oh, is this a stylistic thing? Better to be explicit? Makes sense.

> ::: servo/ports/geckolib/glue.rs:408
> (Diff revision 1)
> > -            }
> > +                }
> > -            _ => { AnimationValue::as_arc(&raw_to_value) }
> > +            },
> > +            _ => {
> > +                assert!(need_underlying_value,
> > +                        "Should have detected we need an underlying value");
> > +                underlying_value.as_ref().cloned()
> 
> Can't we simply do clone() here?

Yes we can!

> ::: servo/ports/geckolib/glue.rs:421
> (Diff revision 1)
> > +    let from_value = composited_from_value.as_ref().unwrap_or(keyframe_from_value.unwrap());
> > +    let to_value   = composited_to_value.as_ref().unwrap_or(keyframe_to_value.unwrap());
> 
> Note to myself.
> Initially I was wondering why composite_endpoint returns keyframe_from_value
> when composite: 'replace', after reading the next patch, I understand the
> reason.  composited_from_value and composited_to_value might be modified if
> there is interationComposite.

Yeah, sorry about this. I originally wrote this all as one patch and then split it up so the intermediate state probably doesn't make a lot of sense.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> ::: servo/ports/geckolib/glue.rs:448
> (Diff revision 1)
> > +                Some(endpoint) => last_value.accumulate(&endpoint, count)
> > +                                            .ok()
> > +                                            .or(Some(endpoint)),
> 
> This noticed me that accumulate() is not infallible yet?

No, not yet. That's deliberate because I think SMIL needs to detect when addition fails or not. In future I would like to make all these methods infallible (including add_weighted) but I think that's better done as a separate bug.
Comment on attachment 8869932 [details]
Bug 1353202 - Add support for iteration composite modes

https://reviewboard.mozilla.org/r/141492/#review145310
Attachment #8869932 - Flags: review?(hikezoe) → review+
Attachment #8869929 - Attachment is obsolete: true
Attachment #8869930 - Attachment is obsolete: true
Attachment #8869932 - Attachment is obsolete: true
Attachment #8869933 - Attachment is obsolete: true
Attachment #8869931 - Flags: review?(manishearth)
Attachment #8870282 - Attachment is obsolete: true
Attachment #8870283 - Attachment is obsolete: true
Attachment #8870284 - Attachment is obsolete: true
Attachment #8870281 - Flags: review?(hikezoe)
Comment on attachment 8870281 [details]
Bug 1353202 - Support additive animation of font-stretch

https://reviewboard.mozilla.org/r/141738/#review145356

Oh nice! I wan't aware of the test case. And this way looks pretty nice!
Attachment #8870281 - Flags: review?(hikezoe) → review+
Attachment #8870280 - Attachment is obsolete: true
Attachment #8869884 - Attachment is obsolete: true
Attachment #8870281 - Attachment is obsolete: true
Attachment #8870286 - Attachment is obsolete: true
Attachment #8869886 - Attachment is obsolete: true
Attachment #8869931 - Attachment is obsolete: true
Attachment #8870287 - Attachment is obsolete: true
Attachment #8870288 - Attachment is obsolete: true
Attachment #8870303 - Flags: review?(hikezoe)
Attachment #8870304 - Flags: review?(hikezoe)
Sorry, MozReview seemed to lose track of the reviews. I'm not going to land this until tomorrow but would you mind re-adding r+ so I can push with autoland?
Comment on attachment 8870303 [details]
Bug 1353202 - Add support for iteration composite modes

https://reviewboard.mozilla.org/r/141764/#review145368
Attachment #8870303 - Flags: review?(hikezoe) → review+
Comment on attachment 8870304 [details]
Bug 1353202 - Use assert_matrix_equal to compare matrices in iterationComposite.html test

https://reviewboard.mozilla.org/r/141766/#review145370
Attachment #8870304 - Flags: review?(hikezoe) → review+
Attachment #8870303 - Attachment is obsolete: true
Attachment #8870304 - Attachment is obsolete: true
I'm sorry -- I need you to re-add the r+ marks because MozReview forgot them again :(

I keep needing to rebase this because no one has updated the bindings in a while. Also, I decided to stick with mercurial queues for this bug but that means MozReview has trouble matching up the patches.

If this happens again, I'll just push to autoland.
Attachment #8870651 - Flags: review?(bbirtles)
Attachment #8870653 - Flags: review?(hikezoe)
Attachment #8870652 - Flags: review?(hikezoe)
Comment on attachment 8870651 [details]
Bug 1353202 - Add nsInterfaceHashtable to opaque types

https://reviewboard.mozilla.org/r/142120/#review145780
Attachment #8870651 - Flags: review?(bbirtles) → review+
Comment on attachment 8870652 [details]
Bug 1353202 - Add support for iteration composite modes

https://reviewboard.mozilla.org/r/142122/#review145782

No worries. I am good at pushing buttons!
Attachment #8870652 - Flags: review?(hikezoe) → review+
Comment on attachment 8870653 [details]
Bug 1353202 - Use assert_matrix_equal to compare matrices in iterationComposite.html test

https://reviewboard.mozilla.org/r/142124/#review145784
Attachment #8870653 - Flags: review?(hikezoe) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b5cd52e9374
Add nsInterfaceHashtable to opaque types r=birtles
https://hg.mozilla.org/integration/autoland/rev/b20c45a2031a
Add support for iteration composite modes r=hiro
https://hg.mozilla.org/integration/autoland/rev/6b57c5487b60
Use assert_matrix_equal to compare matrices in iterationComposite.html test r=hiro
https://hg.mozilla.org/mozilla-central/rev/5b5cd52e9374
https://hg.mozilla.org/mozilla-central/rev/b20c45a2031a
https://hg.mozilla.org/mozilla-central/rev/6b57c5487b60
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
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: