Closed
Bug 1353202
Opened 8 years ago
Closed 7 years ago
stylo: Implement accumulate on Servo AnimationValue
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hiro, Assigned: birtles)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 24 obsolete files)
+++ 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.
Reporter | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Comment 2•7 years ago
|
||
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)
Reporter | ||
Comment 3•7 years ago
|
||
(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.
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
This might be better done as a separate bug. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaf880a447b0c34cfa93ff68430d1c9eeb39be53
Assignee | ||
Comment 8•7 years ago
|
||
Slight simplification.
Attachment #8869349 -
Attachment is obsolete: true
Comment 9•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8869356 -
Attachment is obsolete: true
Comment 10•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8869309 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869310 -
Attachment is obsolete: true
Assignee | ||
Comment 16•7 years ago
|
||
(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.)
Assignee | ||
Comment 17•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8869883 -
Flags: review?(hikezoe)
Attachment #8869884 -
Flags: review?(hikezoe)
Attachment #8869885 -
Flags: review?(hikezoe)
Attachment #8869886 -
Flags: review?(hikezoe)
Assignee | ||
Updated•7 years ago
|
Attachment #8869886 -
Flags: review?(hikezoe)
Reporter | ||
Comment 18•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 19•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 20•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-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?
Assignee | ||
Comment 23•7 years ago
|
||
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, ()>
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8869883 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869885 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869887 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869886 -
Flags: review?(hikezoe)
Attachment #8869931 -
Flags: review?(manishearth)
Attachment #8869931 -
Flags: review?(hikezoe)
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8869932 -
Flags: review?(hikezoe)
Attachment #8869933 -
Flags: review?(hikezoe)
Reporter | ||
Comment 33•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 34•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 35•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 36•7 years ago
|
||
mozreview-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 37•7 years ago
|
||
mozreview-review-reply |
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.
Updated•7 years ago
|
Flags: needinfo?(manishearth)
Assignee | ||
Comment 38•7 years ago
|
||
(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.
Assignee | ||
Comment 39•7 years ago
|
||
(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.
Reporter | ||
Comment 40•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8869929 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869930 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869932 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869933 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869931 -
Flags: review?(manishearth)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8870282 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870283 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870284 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870281 -
Flags: review?(hikezoe)
Reporter | ||
Comment 55•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8870280 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869884 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870281 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870286 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869886 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8869931 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870287 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870288 -
Attachment is obsolete: true
Assignee | ||
Comment 58•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8870303 -
Flags: review?(hikezoe)
Attachment #8870304 -
Flags: review?(hikezoe)
Assignee | ||
Comment 59•7 years ago
|
||
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?
Reporter | ||
Comment 60•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 61•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8870303 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8870304 -
Attachment is obsolete: true
Assignee | ||
Comment 66•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8870651 -
Flags: review?(bbirtles)
Attachment #8870653 -
Flags: review?(hikezoe)
Assignee | ||
Updated•7 years ago
|
Attachment #8870652 -
Flags: review?(hikezoe)
Assignee | ||
Comment 67•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 68•7 years ago
|
||
mozreview-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+
Reporter | ||
Comment 69•7 years ago
|
||
mozreview-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+
Comment 70•7 years ago
|
||
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
Comment 71•7 years ago
|
||
bugherder |
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.
Description
•