Closed Bug 1358966 Opened 7 years ago Closed 7 years ago

stylo: Work out what to do with SMIL animations of shorthand properties

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: birtles, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 6 obsolete files)

Bug 1358966 - Drop parent style argument from Gecko_UpdateAnimations.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Drop RawGeckoAnimationValueList.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Add a new FFI to convert PropertyDeclarationBlock into nsTArray<RefPtr<RawServoAnimationValue>>.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Split mCSSValue into a RawServoAnimationValue and a StyleAnimationValue.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Use Servo_GetAnimationValues.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Call Servo_AnimationValues_Interpolate for each sub properties of shorthand.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Make RawServoAnimationValue in ValueWrapper an array.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Compute distance for each sub properties of shorthand.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Call additive or accumulative functions for each sub properties of shorthand.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Drop a redundant include for DeclarationBlockInlines.h.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - Serialize multiple AnimationValue(s) for sub properties of a shorthand into single shorthand string.
59 bytes, text/x-review-board-request
birtles
: review+
manishearth
: review+
Details
Bug 1358966 - Enable shorthand properties for SMIL.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1358966 - A reftest for overflow shorthand animation in SMIL.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
See bug 1355348 comment 2. We should probably either deprecate this in Gecko or handle arrays of AnimationValues for the Servo code path.
Going through the sets of properties marked as animatable by SMIL in nsSMILCSSProperty::IsPropertyAnimatable we have the following set:

 font
 font-variant
 marker
 mask (depends on MOZ_ENABLE_MASK_AS_SHORTHAND)
 overflow
 text-decoration

I need to check if Chrome/Safari support animation of 'mask' and 'marker' but given that they seem useful to animate, and given the tendency of longhands to be promoted to shorthands we probably should support this.
Priority: -- → P2
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29d3007e7ed0
Mark Gecko_UnsetDirtyStyleAttr as ignoreContens for now. r=me
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05575fbbae2b
Backed out changeset 29d3007e7ed0 ("bug 1358966") for landing with the wrong bug number
Depends on: 1367293
(In reply to Brian Birtles (:birtles) from comment #1)
> Going through the sets of properties marked as animatable by SMIL in
> nsSMILCSSProperty::IsPropertyAnimatable we have the following set:
> 
>  font
>  font-variant
>  marker
>  mask (depends on MOZ_ENABLE_MASK_AS_SHORTHAND)
>  overflow
>  text-decoration

I did check all of sub properties of these shorthand, there is no non-animatable properties.  Actually there are some properties which are not animatable yet but will be animatable in bug 1353966. So yes, we can use ShorthandId.longhands_to_css() to serialize AnimationValue to string for SMIL.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41c686fa02d01e18e2664044edd5bb5424c91a10
The try should work fine but there is not test case for shorthand yet. For now we can just use overflow property for the test since other properties still includes not-animated-yet sub properties.
Some of patches (e.g. making distance value as an array and iterate over the array in several places) look awful.  I don't have any good idea to handle them handy for now, I think we should migrate SMIL onto web animation architecture right after we get ship stylo.
Comment on attachment 8874697 [details]
Bug 1358966 - A reftest for overflow shorthand animation in SMIL.

https://reviewboard.mozilla.org/r/146074/#review149948

::: commit-message-34b28:3
(Diff revision 1)
> +Bug 1358966 - A reftest for overflow shorhand animation in SMIL. r?birtles
> +
> +The marker, a red triangle. is clipped during "overflow" property

My eyes could not tell the difference a comma and a period..
Comment on attachment 8874679 [details]
Bug 1358966 - Drop parent style argument from Gecko_UpdateAnimations.

https://reviewboard.mozilla.org/r/146038/#review149988
Attachment #8874679 - Flags: review?(bbirtles) → review+
Comment on attachment 8874680 [details]
Bug 1358966 - Drop RawGeckoAnimationValueList.

https://reviewboard.mozilla.org/r/146040/#review149990
Attachment #8874680 - Flags: review?(bbirtles) → review+
Comment on attachment 8874682 [details]
Bug 1358966 - Add a new FFI to convert PropertyDeclarationBlock into nsTArray<RefPtr<RawServoAnimationValue>>.

https://reviewboard.mozilla.org/r/146044/#review149992

::: layout/style/ServoBindingList.h:233
(Diff revision 1)
>                     RawGeckoElementBorrowed,
>                     RawGeckoCSSPropertyIDListBorrowed,
>                     nsCSSPropertyIDSetBorrowedMut)
> +SERVO_BINDING_FUNC(Servo_GetAnimationValues, void,
> +                   RawServoDeclarationBlockBorrowed declarations,
> +                   RawGeckoElementBorrowed,

parameter name? (element, I believe)

::: layout/style/ServoBindingList.h:235
(Diff revision 1)
>                     nsCSSPropertyIDSetBorrowedMut)
> +SERVO_BINDING_FUNC(Servo_GetAnimationValues, void,
> +                   RawServoDeclarationBlockBorrowed declarations,
> +                   RawGeckoElementBorrowed,
> +                   ServoComputedValuesBorrowed style,
> +                   RawServoStyleSetBorrowed set,

style_set? Although this is called raw_data further down, that seems to be overly generic.

::: layout/style/ServoBindingList.h:236
(Diff revision 1)
> +SERVO_BINDING_FUNC(Servo_GetAnimationValues, void,
> +                   RawServoDeclarationBlockBorrowed declarations,
> +                   RawGeckoElementBorrowed,
> +                   ServoComputedValuesBorrowed style,
> +                   RawServoStyleSetBorrowed set,
> +                   RawGeckoAnimationValueListBorrowedMut result)

This is called animation_values further down

::: servo/ports/geckolib/glue.rs:2602
(Diff revision 1)
> +    let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
> +    let style = ComputedValues::as_arc(&style);
> +    let metrics = get_metrics_provider_for_product();
> +
> +    let element = GeckoElement(element);
> +    let parent_element = element.inheritance_parent();
> +    let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data());
> +    let parent_style = parent_data.as_ref().map(|d| d.styles().primary.values());
> +
> +    let mut context = create_context(&data, &metrics, style, &parent_style);

This seems to overlap a lot with Servo_GetComputedKeyframeValues. Does it make sense to factor out a common function for this?

Perhaps the following few lines too.

::: servo/ports/geckolib/glue.rs:2620
(Diff revision 1)
> +    let guard = global_style_data.shared_lock.read();
> +
> +    let declarations = Locked::<PropertyDeclarationBlock>::as_arc(&declarations);
> +    let guard = declarations.read_with(&guard);
> +    for (index, anim) in guard.to_animation_value_iter(&mut context, &default_values).enumerate() {
> +        unsafe { animation_values.set_len((index + 1) as u32) };

I guess we don't know the length of the iterator in advance so we can't set the capacity ahead of time.
Attachment #8874682 - Flags: review?(bbirtles) → review+
Comment on attachment 8874685 [details]
Bug 1358966 - Use assign_utf8 to set servo's string into nsAString.

https://reviewboard.mozilla.org/r/146050/#review150050

r=me, thanks!
Attachment #8874685 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8874681 [details]
Bug 1358966 - Factor out a process that creates AnimationValue iterator from PropertyDeclarationBlock.

https://reviewboard.mozilla.org/r/146042/#review150262
Attachment #8874681 - Flags: review?(manishearth) → review+
Comment on attachment 8874694 [details]
Bug 1358966 - Serialize multiple AnimationValue(s) for sub properties of a shorthand into single shorthand string.

https://reviewboard.mozilla.org/r/146068/#review150280
Attachment #8874694 - Flags: review?(manishearth) → review+
Comment on attachment 8874682 [details]
Bug 1358966 - Add a new FFI to convert PropertyDeclarationBlock into nsTArray<RefPtr<RawServoAnimationValue>>.

https://reviewboard.mozilla.org/r/146044/#review149992

> This seems to overlap a lot with Servo_GetComputedKeyframeValues. Does it make sense to factor out a common function for this?
> 
> Perhaps the following few lines too.

Yeah, this repetition is irritating.  I will try the factoring out later, actually I did try before but couldn't because of lifetime issue.
Attachment #8874688 - Attachment is obsolete: true
Attachment #8874688 - Flags: review?(bbirtles)
Attachment #8874690 - Attachment is obsolete: true
Attachment #8874690 - Flags: review?(bbirtles)
Attachment #8874691 - Attachment is obsolete: true
Attachment #8874691 - Flags: review?(bbirtles)
Comment on attachment 8874683 [details]
Bug 1358966 - Split mCSSValue into a RawServoAnimationValue and a StyleAnimationValue.

https://reviewboard.mozilla.org/r/146046/#review150448

::: dom/smil/nsSMILCSSValueType.cpp:35
(Diff revision 2)
>    ValueWrapper(nsCSSPropertyID aPropID, const AnimationValue& aValue)
> -    : mPropID(aPropID), mCSSValue(aValue) {}
> +    : mPropID(aPropID)
> +  {
> +    if (aValue.mServo) {
> +      mServoValue = aValue.mServo;
> +      return;
> +    }
> +    mGeckoValue = aValue.mGecko;
> +  }

I haven't checked the other patches in this series, but I wonder if we actually need this constructor any more?

::: dom/smil/nsSMILCSSValueType.cpp:109
(Diff revision 2)
>  // If neither argument is null, this method generally does nothing, though it
>  // may apply a workaround for the special case where a 0 length-value is mixed
>  // with a eUnit_Float value.  (See comment below.)

If neither argument is null, this method does nothing.

::: dom/smil/nsSMILCSSValueType.cpp:113
(Diff revision 2)
>  //
>  // If neither argument is null, this method generally does nothing, though it
>  // may apply a workaround for the special case where a 0 length-value is mixed
>  // with a eUnit_Float value.  (See comment below.)
>  //
>  // |aZeroValueStorage| should be a null AnimationValue. This is used for the

"a reference to a RefPtr<RawServoAnimationValue>"

::: dom/smil/nsSMILCSSValueType.cpp:113
(Diff revision 2)
>  // |aZeroValueStorage| should be a null AnimationValue. This is used for the
>  // Servo backend where we may need to allocate a new ServoAnimationValue to
>  // represent the appropriate zero value.

We can probably also drop "for the Servo backend where" and replace it with "since".

::: dom/smil/nsSMILCSSValueType.cpp:117
(Diff revision 2)
>  //
>  // |aZeroValueStorage| should be a null AnimationValue. This is used for the
>  // Servo backend where we may need to allocate a new ServoAnimationValue to
>  // represent the appropriate zero value.
>  //
>  // Returns true on success, or false.

While we're updating this,

"..., or false otherwise."

::: dom/smil/nsSMILCSSValueType.cpp:141
(Diff revision 2)
> +  MOZ_ASSERT(aValue1 && aValue2,
> +             "expecting at least one non-null value");

Shouldn't this be aValue1 || aValue2 ?

::: dom/smil/nsSMILCSSValueType.cpp:148
(Diff revision 2)
> +
>    if (!aValue1) {
> -    aValue1 = GetZeroValueForUnit(aValue2->mGecko.GetUnit());
> +    aValue1 = GetZeroValueForUnit(aValue2->GetUnit());
>      return !!aValue1; // Fail if we have no zero value for this unit.
>    }
> -  if (!aValue2) {
> +  if (aValue2) {

if (!aValue2) ?

::: dom/smil/nsSMILCSSValueType.cpp:260
(Diff revision 2)
>    if (leftWrapper) {
>      if (rightWrapper) {
>        // Both non-null
>        NS_WARNING_ASSERTION(leftWrapper != rightWrapper,
>                             "Two nsSMILValues with matching ValueWrapper ptr");
> -      return (leftWrapper->mPropID == rightWrapper->mPropID &&
> +      return leftWrapper == rightWrapper;

*leftWrapper == *rightWrapper
(In reply to Brian Birtles (:birtles) from comment #52)

> ::: dom/smil/nsSMILCSSValueType.cpp:141
> (Diff revision 2)
> > +  MOZ_ASSERT(aValue1 && aValue2,
> > +             "expecting at least one non-null value");
> 
> Shouldn't this be aValue1 || aValue2 ?

Good catch! Maybe this and the below are the culprit of the crash on the try.
Thank you!
Comment on attachment 8874687 [details]
Bug 1358966 - Make RawServoAnimationValue in ValueWrapper an array.

https://reviewboard.mozilla.org/r/146054/#review150450

::: dom/smil/nsSMILCSSValueType.cpp:356
(Diff revision 2)
> -                 ? valueToAddWrapper->mServoValue
> -                 : destWrapper->mServoValue;
> +                 ? valueToAddWrapper->mServoValues[0]
> +                 : destWrapper->mServoValues[0];

This is another culplit. We should check IsEmpty() insterad.

::: dom/smil/nsSMILCSSValueType.cpp:444
(Diff revision 2)
>  
>    const ValueWrapper* fromWrapper = ExtractValueWrapper(aFrom);
>    const ValueWrapper* toWrapper = ExtractValueWrapper(aTo);
>    MOZ_ASSERT(toWrapper, "expecting non-null endpoint");
>  
> -  if (toWrapper->mServoValue) {
> +  if (toWrapper->mServoValues[0]) {

An here.
Comment on attachment 8874683 [details]
Bug 1358966 - Split mCSSValue into a RawServoAnimationValue and a StyleAnimationValue.

https://reviewboard.mozilla.org/r/146046/#review150454

I've still yet to review the interpolate parts but so far looks good.

I feel like wrapping this switching up in an AnimationValue-like class would have been a bit cleaner, but hopefully all this code is short-lived anyway (famous last words).

::: dom/smil/nsSMILCSSValueType.cpp:288
(Diff revision 2)
> +  if (!FinalizeServoAnimationValues(valueToAdd, destValue, zeroValueStorage)) {
> +    return false;
> +  }
> +
> +  // Handle barely-initialized "zero" destination.
> +  if (!aDestWrapper) {
> +    aDest.mU.mPtr = aDestWrapper = new ValueWrapper(property, *destValue);
> +  }
> +
> +  if (aDestWrapper && &aDestWrapper->mServoValue != destValue) {
> +    aDestWrapper->mServoValue = *destValue;
> +  }

The last two blocks here have their order switched from the Gecko case. Also, we don't include the comment explaining what the last block is doing.

Furthermore we probably don't need the pointer comparison since rather than saving us from copying a StyleAnimationValue object, in this case it just saves us an extra addref/release which we probably don't need to worry about.

So could we write this as:

  if (!FinalizeServoAnimationValues(valueToAdd, destValue, zeroValueStorage)) {
    return false;
  }

  // FinalizeServoAnimationValues may have updated destValue so we should make
  // sure the aDest and aDestWrapper outparams are up-to-date.
  if (aDestWrapper) {
    aDestWrapper->mServoValue = *destValue;
  } else {
    // aDest may be a barely-initialized "zero" destination.
    aDest.mU.mPtr = aDestWrapper = new ValueWrapper(property, *destValue);
  }

::: dom/smil/nsSMILCSSValueType.cpp:405
(Diff revision 2)
>           : NS_ERROR_FAILURE;
>  }
>  
> +static nsresult
> +ComputeDistanceForServo(const ValueWrapper* aFromWrapper,
> +                        const ValueWrapper* aToWrapper,

Make this a reference since we expect it to never be null?
Comment on attachment 8874683 [details]
Bug 1358966 - Split mCSSValue into a RawServoAnimationValue and a StyleAnimationValue.

https://reviewboard.mozilla.org/r/146046/#review150458

This looks fine but I'm surprised that extending AnimationValue didn't work out?

For what it's worth, I imagined we could move AnimationValue to AnimationValue.{h,cpp} and then add a parallel class called ExtendedAnimationValue (or something like that) where the only difference is that in place of mServoValue it would have an array, mServoValues. Then it would have similar methods like SerializeSpecifiedValue / ComputeDistance etc. plus methods for interpolating etc.

It seems like that would create less duplicated code here but maybe that's not the case?

::: dom/smil/nsSMILCSSValueType.cpp:460
(Diff revision 2)
> +  const StyleAnimationValue* startCSSValue = aStartWrapper ?
> +    &aStartWrapper->mGeckoValue : nullptr;

Coding style here.

::: dom/smil/nsSMILCSSValueType.cpp:499
(Diff revision 2)
> +                                      *endValue,
> +                                      aUnitDistance).Consume();
> +  if (!result) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  aResult.mU.mPtr = new ValueWrapper(aEndWrapper->mPropID, Move(result));

Is the Move() here necessary? Do we add an a ValueWrapper ctor later in this patch series that takes an rvalue ref?

::: dom/smil/nsSMILCSSValueType.cpp:779
(Diff revision 2)
> -  wrapper->mCSSValue.SerializeSpecifiedValue(wrapper->mPropID, aString);
> +  if (wrapper->mServoValue) {
> +    Servo_AnimationValue_Serialize(wrapper->mServoValue,
> +                                   wrapper->mPropID,
> +                                   &aString);
> +    return;
> +  }
> +
> +  DebugOnly<bool> uncomputeResult =
> +    StyleAnimationValue::UncomputeValue(wrapper->mPropID,
> +                                        wrapper->mGeckoValue,
> +                                        aString);

(We're duplicating code here from AnimationValue::SerializeSpecifiedValue which is reasonable, since we will later extend this code to cover arrays of values. However, it seems like if we are going to have this similar code it would be better done in a variant of AnimationValue that lives along side AnimationValue so we can maintain the two code paths easily. e.g. AnimationValue::SerializeSpecifiedValue and ExtendedAnimationValue::SerializeSpecifiedValue. What do you think?)
Attachment #8874683 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #56)
> Comment on attachment 8874683 [details]
> Bug 1358966 - Split mCSSValue into a RawServoAnimationValue and a
> StyleAnimationValue.
> 
> https://reviewboard.mozilla.org/r/146046/#review150458
> 
> This looks fine but I'm surprised that extending AnimationValue didn't work
> out?
> 
> For what it's worth, I imagined we could move AnimationValue to
> AnimationValue.{h,cpp} and then add a parallel class called
> ExtendedAnimationValue (or something like that) where the only difference is
> that in place of mServoValue it would have an array, mServoValues. Then it
> would have similar methods like SerializeSpecifiedValue / ComputeDistance
> etc. plus methods for interpolating etc.
> 
> It seems like that would create less duplicated code here but maybe that's
> not the case?

Yeah, we could add the new extending class for shorthand but will that still be useful when we migrate SMIL onto web animation architecture?
I am not really sure how it looks like for now but given that currently we don't use such class for web animations, I doubt it. Even if it will be necessary for SMIL onto web animation architecture, I think we should do such encapsulation at the time of the migration. It would be done with less efforts, I guess. 

> ::: dom/smil/nsSMILCSSValueType.cpp:499
> (Diff revision 2)
> > +                                      *endValue,
> > +                                      aUnitDistance).Consume();
> > +  if (!result) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  aResult.mU.mPtr = new ValueWrapper(aEndWrapper->mPropID, Move(result));
> 
> Is the Move() here necessary? Do we add an a ValueWrapper ctor later in this
> patch series that takes an rvalue ref?

That's my mistake when refactoring the old code.

> ::: dom/smil/nsSMILCSSValueType.cpp:779
> (Diff revision 2)
> > -  wrapper->mCSSValue.SerializeSpecifiedValue(wrapper->mPropID, aString);
> > +  if (wrapper->mServoValue) {
> > +    Servo_AnimationValue_Serialize(wrapper->mServoValue,
> > +                                   wrapper->mPropID,
> > +                                   &aString);
> > +    return;
> > +  }
> > +
> > +  DebugOnly<bool> uncomputeResult =
> > +    StyleAnimationValue::UncomputeValue(wrapper->mPropID,
> > +                                        wrapper->mGeckoValue,
> > +                                        aString);
> 
> (We're duplicating code here from AnimationValue::SerializeSpecifiedValue
> which is reasonable, since we will later extend this code to cover arrays of
> values. However, it seems like if we are going to have this similar code it
> would be better done in a variant of AnimationValue that lives along side
> AnimationValue so we can maintain the two code paths easily. e.g.
> AnimationValue::SerializeSpecifiedValue and
> ExtendedAnimationValue::SerializeSpecifiedValue. What do you think?)

I am wondering how long we will maintain current SMIL code. I thought we will start the migration after branching 57, or at the latest it's right after 57 release. We will stick on the current SMIL code for years?  If it's for years, I am OK with adding the new class but if it's for a half of year and also the migrated SMIL code is not clear to me yet, I can tell it's a good thing or not.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #57)
> Yeah, we could add the new extending class for shorthand but will that still
> be useful when we migrate SMIL onto web animation architecture?
> I am not really sure how it looks like for now but given that currently we
> don't use such class for web animations, I doubt it. Even if it will be
> necessary for SMIL onto web animation architecture, I think we should do
> such encapsulation at the time of the migration. It would be done with less
> efforts, I guess.

I was thinking that isolating the code for negotiating between the different backends would have avoided a couple of the issues above, but it's not worth redoing for that.

> I am wondering how long we will maintain current SMIL code. I thought we
> will start the migration after branching 57, or at the latest it's right
> after 57 release.

That would be nice but these plans tend to get delayed, especially with all the feature work we have accumulating.
Comment on attachment 8874687 [details]
Bug 1358966 - Make RawServoAnimationValue in ValueWrapper an array.

https://reviewboard.mozilla.org/r/146054/#review150478

::: dom/smil/nsSMILCSSValueType.cpp:35
(Diff revision 2)
>    ValueWrapper(nsCSSPropertyID aPropID, const AnimationValue& aValue)
>      : mPropID(aPropID)
>    {
>      if (aValue.mServo) {
> -      mServoValue = aValue.mServo;
> +      mServoValues.AppendElement(aValue.mServo);
>        return;
>      }
>      mGeckoValue = aValue.mGecko;
>    }

(Again, I wonder if we need this ctor? Is it used?)

::: dom/smil/nsSMILCSSValueType.cpp:56
(Diff revision 2)
> -    if (mServoValue && aOther.mServoValue) {
> -      return Servo_AnimationValue_DeepEqual(mServoValue, aOther.mServoValue);
> +    size_t len = mServoValues.Length();
> +    if (len != aOther.mServoValues.Length()) {
> +      return false;
> +    }
> +    if (!mServoValues.IsEmpty() && !aOther.mServoValues.IsEmpty()) {
> +      for (size_t i = 0; i < len; i++) {
> +        if (!Servo_AnimationValue_DeepEqual(mServoValues[i],
> +                                            aOther.mServoValues[i])) {
> +          return false;
> -    }
> +        }
> -    return !mServoValue && !aOther.mServoValue &&
> -           mGeckoValue == aOther.mGeckoValue;
> -  }
> +      }
> +      return true;
> +    }

Might be a little more clear as:

    if (!mServoValues.IsEmpty()) {
      size_t len = mServoValues.Length();
      if (len != aOther.mServoValues.Length()) {
        return false;
      }
      for (size_t i = 0; i < len; i++) {
        if (!Servo_AnimationValue_DeepEqual(mServoValues[i],
                                            aOther.mServoValues[i])) {
          return false;
        }
      }
      return true;
    }

::: dom/smil/nsSMILCSSValueType.cpp:69
(Diff revision 2)
> -    return !mServoValue && !aOther.mServoValue &&
> -           mGeckoValue == aOther.mGeckoValue;
> -  }
> +      }
> +      return true;
> +    }
> +    return mGeckoValue == aOther.mGeckoValue;

A blank line before this to separate the Servo code path from the Gecko code path would help skimmability.
Attachment #8874687 - Flags: review?(bbirtles) → review+
Comment on attachment 8874684 [details]
Bug 1358966 - Use Servo_GetAnimationValues.

https://reviewboard.mozilla.org/r/146048/#review150482

::: dom/smil/nsSMILCSSValueType.cpp:621
(Diff revision 2)
>                                    aPresContext->EffectiveTextZoom());
>    }
>    return true;
>  }
>  
> -static already_AddRefed<RawServoAnimationValue>
> +static AutoTArray<RefPtr<RawServoAnimationValue>, 1>

Perhaps we should add a typedef for this type to the underlying patch.

::: layout/style/ServoStyleSet.h:363
(Diff revision 2)
> +  AutoTArray<RefPtr<RawServoAnimationValue>, 1>
> +  GetAnimationValues(RawServoDeclarationBlock* aDeclarations,
> +                     dom::Element* aElement,
> +                     ServoComputedValuesBorrowed aComputedValues);

Rather than expose the AutoTArray<RefPtr<RawServoAnimationValue>, 1> type here, can we just take a references to an nsTArray as a parameter?
Attachment #8874684 - Flags: review?(bbirtles) → review+
Comment on attachment 8874686 [details]
Bug 1358966 - Call Servo_AnimationValues_Interpolate for each sub properties of shorthand.

https://reviewboard.mozilla.org/r/146052/#review150484

::: dom/smil/nsSMILCSSValueType.cpp:496
(Diff revision 2)
> +  AutoTArray<RefPtr<RawServoAnimationValue>, 1> results;
> +  for (size_t i = 0, len = aEndWrapper->mServoValues.Length(); i < len; i++) {
> -  const RefPtr<RawServoAnimationValue>* startValue =
> +    const RefPtr<RawServoAnimationValue>* startValue =
> -    aStartWrapper ? &aStartWrapper->mServoValues[0] : nullptr;
> -  const RefPtr<RawServoAnimationValue>* endValue = &aEndWrapper->mServoValues[0];
> +      aStartWrapper ? &aStartWrapper->mServoValues[i] : nullptr;
> +    const RefPtr<RawServoAnimationValue>* endValue =
> +      &aEndWrapper->mServoValues[i];

Don't we need to handle the case where aStartWrapper->mServoValues is shorter than aEndWrapper->mServoValues?

(If the two lists are always going to be the same length then we should assert that)

::: dom/smil/nsSMILCSSValueType.cpp:496
(Diff revision 2)
>  InterpolateForServo(const ValueWrapper* aStartWrapper,
>                      const ValueWrapper* aEndWrapper,
>                      double aUnitDistance,
>                      nsSMILValue& aResult)
>  {
> +  AutoTArray<RefPtr<RawServoAnimationValue>, 1> results;

Can we initialize the capacity to the appropriate length here?

e.g.

  size_t len = aEndWrapper->mServoValues.Length();
  results.SetCapacity(len);

  // And then use |len| in the following loop.

That will depend a bit on how/if we handle mismatched length lists though (next comment).
Attachment #8874686 - Flags: review?(bbirtles) → review+
Comment on attachment 8874683 [details]
Bug 1358966 - Split mCSSValue into a RawServoAnimationValue and a StyleAnimationValue.

https://reviewboard.mozilla.org/r/146046/#review150488

::: dom/smil/nsSMILCSSValueType.cpp:456
(Diff revision 2)
>           : NS_ERROR_FAILURE;
>  }
>  
> +static nsresult
> +InterpolateForGecko(const ValueWrapper* aStartWrapper,
> +                    const ValueWrapper* aEndWrapper,

Likewise can we make this a reference?

::: dom/smil/nsSMILCSSValueType.cpp:480
(Diff revision 2)
> +  return NS_ERROR_FAILURE;
> +}
> +
> +static nsresult
> +InterpolateForServo(const ValueWrapper* aStartWrapper,
> +                    const ValueWrapper* aEndWrapper,

And can we make this a reference too?

(Reading the subsequent patches its hard to remember all the invariants we expect to hold around these parameters. Using a reference would document and enforce at least one of them.)
Comment on attachment 8874689 [details]
Bug 1358966 - Compute distance for each sub properties of shorthand.

https://reviewboard.mozilla.org/r/146058/#review150486

::: dom/smil/nsSMILCSSValueType.cpp:415
(Diff revision 2)
>  static nsresult
>  ComputeDistanceForServo(const ValueWrapper* aFromWrapper,
>                          const ValueWrapper* aToWrapper,
>                          double& aDistance)
>  {
> +  double squareDistance = 0;
> +
> +  for (size_t i = 0, len = aToWrapper->mServoValues.Length(); i < len; i++) {
> -  const RefPtr<RawServoAnimationValue>* fromValue =
> +    const RefPtr<RawServoAnimationValue>* fromValue =
> -    aFromWrapper ? &aFromWrapper->mServoValues[0] : nullptr;
> -  const RefPtr<RawServoAnimationValue>* toValue = &aToWrapper->mServoValues[0];
> +      aFromWrapper ? &aFromWrapper->mServoValues[i] : nullptr;
> +    const RefPtr<RawServoAnimationValue>* toValue =
> +      &aToWrapper->mServoValues[i];

As with the previous patch, we need to be careful how we handle mismatched length lists by at least adding appropriate assertions to document what we expect.

::: dom/smil/nsSMILCSSValueType.cpp:433
(Diff revision 2)
> +    if (len == 1) {
> +      aDistance = distance;
> +      return NS_OK;
> +    }

(I'm not sure this is needed -- it would be simpler without and I don't think we need to worry about precision too much for distance calculation. But perhaps there are tests that fail without this?)
Attachment #8874689 - Flags: review?(bbirtles) → review+
Comment on attachment 8874692 [details]
Bug 1358966 - Call additive or accumulative functions for each sub properties of shorthand.

https://reviewboard.mozilla.org/r/146064/#review150490

::: dom/smil/nsSMILCSSValueType.cpp:295
(Diff revision 2)
> +  size_t len = aValueToAddWrapper
> +               ? aValueToAddWrapper->mServoValues.Length()
> +               : aDestWrapper->mServoValues.Length();
> +  for (size_t i = 0; i < len; i++) {
> -  const RefPtr<RawServoAnimationValue>* valueToAdd =
> +    const RefPtr<RawServoAnimationValue>* valueToAdd =
> -    aValueToAddWrapper ? &aValueToAddWrapper->mServoValues[0] : nullptr;
> +      aValueToAddWrapper ? &aValueToAddWrapper->mServoValues[i] : nullptr;
> -  const RefPtr<RawServoAnimationValue>* destValue =
> +    const RefPtr<RawServoAnimationValue>* destValue =
> -    aDestWrapper ? &aDestWrapper->mServoValues[0] : nullptr;
> +      aDestWrapper ? &aDestWrapper->mServoValues[i] : nullptr;

Likewise here we need to either assert or check that the lists, when present, are of equal length

::: dom/smil/nsSMILCSSValueType.cpp:308
(Diff revision 2)
> -  // Handle barely-initialized "zero" destination.
> +    // Handle barely-initialized "zero" destination.
> -  if (!aDestWrapper) {
> +    if (!aDestWrapper) {
> -    aDest.mU.mPtr = aDestWrapper = new ValueWrapper(property, *destValue);
> +      aDest.mU.mPtr = aDestWrapper = new ValueWrapper(property, *destValue);
> -  }
> +    }
>  
> -  if (aDestWrapper && &aDestWrapper->mServoValues[0] != destValue) {
> -    aDestWrapper->mServoValues[0] = *destValue;
> +    if (aDestWrapper && &aDestWrapper->mServoValues[i] != destValue) {
> +      aDestWrapper->mServoValues[i] = *destValue;
> -  }
> +    }

If we are creating a new ValueWrapper, don't we need to append to it in subsequent iterations rather than just assuming element[i] exists?

Or, better still, pre-allocate all the necessary slots and fill them in with null ptrs:

e.g.

  // FinalizeServoAnimationValues may have updated destValue so we should make
  // sure the aDest and aDestWrapper outparams are up-to-date.
  if (aDestWrapper) {
    MOZ_ASSERT(aDestWrapper->mServoValues.Length() > i);
    aDestWrapper->mServoValues[i] = *destValue;
  } else {
    // aDest may be a barely-initialized "zero" destination.
    aDest.mU.mPtr = aDestWrapper = new ValueWrapper(property, *destValue);
    aDestWrapper->mServoValues.SetLength(len);
  }
Attachment #8874692 - Flags: review?(bbirtles) → review+
Comment on attachment 8874693 [details]
Bug 1358966 - Drop a redundant include for DeclarationBlockInlines.h.

https://reviewboard.mozilla.org/r/146066/#review150442

::: commit-message-92424:1
(Diff revision 2)
> +Bug 1358966 - Drop a redandunt include for DeclarationBlockInlines.h. r?birtles

redundant
Attachment #8874693 - Flags: review?(bbirtles) → review+
Comment on attachment 8874695 [details]
Bug 1358966 - nscsspropertyid_is_animatable handles shorthand property as well.

https://reviewboard.mozilla.org/r/146070/#review150492

Can you write a comment that describes why if any subproperty is not animatable the whole thing is considered not animatable. I would have expected the opposite (i.e. if any of the longhands are animatable, then we consider the shorthand animatable--that's what we do elsewhere). I know we discussed this, but I can't remember why we decided to do this.
Attachment #8874695 - Flags: review?(bbirtles)
Comment on attachment 8874696 [details]
Bug 1358966 - Enable shorthand properties for SMIL.

https://reviewboard.mozilla.org/r/146072/#review150510
Attachment #8874696 - Flags: review?(bbirtles) → review+
Comment on attachment 8874697 [details]
Bug 1358966 - A reftest for overflow shorthand animation in SMIL.

https://reviewboard.mozilla.org/r/146074/#review150444

::: commit-message-34b28:1
(Diff revision 2)
> +Bug 1358966 - A reftest for overflow shorhand animation in SMIL. r?birtles

shorthand

::: layout/reftests/svg/smil/anim-overflow-shorthand.svg:12
(Diff revision 2)
> +    <animate attributeName="overflow"
> +             calcMode="linear"
> +             begin="0s" dur="2s"
> +             from="hidden" to="hidden"
> +             fill="freeze"/>

(Could we just use <set> here, or does that not exercise the code path we want to use?)
Attachment #8874697 - Flags: review?(bbirtles) → review+
Comment on attachment 8874694 [details]
Bug 1358966 - Serialize multiple AnimationValue(s) for sub properties of a shorthand into single shorthand string.

https://reviewboard.mozilla.org/r/146068/#review150530

::: dom/smil/nsSMILCSSValueType.cpp:810
(Diff revision 2)
> -                                        aString);
> +                                          aString);
> +    return;
> +  }
> +
> +  if (nsCSSProps::IsShorthand(wrapper->mPropID)) {
> +    // In case of shorthand on servo, we iterate over all mCSSValues array since

s/over all mCSSValues array/over the mServoValues array/

::: dom/smil/nsSMILCSSValueType.cpp:811
(Diff revision 2)
> +    // we have multiple AnimationValue in the array for shorthand sub
> +    // properties.

s/AnimationValue/AnimationValues/

s/for shorthand sub properties/for each longhand component/

::: layout/style/ServoBindingList.h:266
(Diff revision 2)
>                     RawServoAnimationValueBorrowed to)
>  SERVO_BINDING_FUNC(Servo_AnimationValue_Serialize, void,
>                     RawServoAnimationValueBorrowed value,
>                     nsCSSPropertyID property,
>                     nsAString* buffer)
> +// Serialize AnimationValues for a shorthand property.

Is the comment needed here? We don't have it elsewhere in this file.

::: servo/ports/geckolib/glue.rs:575
(Diff revision 2)
> +        Ok(shorthand) => shorthand,
> +        _ => return,
> +    };
> +
> +    // Convert RawServoAnimationValue(s) into a vector of PropertyDeclaration
> +    // so that we can use reference of the PropertyDeclaration without worring

worrying
Attachment #8874694 - Flags: review?(bbirtles) → review+
Comment on attachment 8874687 [details]
Bug 1358966 - Make RawServoAnimationValue in ValueWrapper an array.

https://reviewboard.mozilla.org/r/146054/#review150478

> (Again, I wonder if we need this ctor? Is it used?)

This is used from nsSMILCSSProperty::GetBaseValue, if we drop the ctor we need an overloaded function of nsSMILCSSValueType::ValueFromAnimationValue.

I will re-visit this issue (and extending AnimationValue issue) once we fixed all other bugs caused by unclear reasons.
A try with updated patches, I am really hoping I didn't break anything while addressing review. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07546ad30ef2050134823a028692056971d06bd0
Comment on attachment 8874695 [details]
Bug 1358966 - nscsspropertyid_is_animatable handles shorthand property as well.

https://reviewboard.mozilla.org/r/146070/#review151038
Attachment #8874695 - Flags: review?(bbirtles) → review+
Attachment #8874681 - Attachment is obsolete: true
Attachment #8874685 - Attachment is obsolete: true
Attachment #8874695 - Attachment is obsolete: true
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f4f2486fc3a
Drop parent style argument from Gecko_UpdateAnimations. r=birtles
https://hg.mozilla.org/integration/autoland/rev/78fc301cf3b7
Drop RawGeckoAnimationValueList. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c54f6b4989cc
Add a new FFI to convert PropertyDeclarationBlock into nsTArray<RefPtr<RawServoAnimationValue>>. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4d87f2bf4b10
Split mCSSValue into a RawServoAnimationValue and a StyleAnimationValue. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f234d46a9b40
Make RawServoAnimationValue in ValueWrapper an array. r=birtles
https://hg.mozilla.org/integration/autoland/rev/00953e22f9f6
Use Servo_GetAnimationValues. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4e730eaffad1
Call Servo_AnimationValues_Interpolate for each sub properties of shorthand. r=birtles
https://hg.mozilla.org/integration/autoland/rev/854d41e31f4b
Compute distance for each sub properties of shorthand. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a9f39ff3b27b
Call additive or accumulative functions for each sub properties of shorthand. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9dfcdc2bfc8a
Drop a redundant include for DeclarationBlockInlines.h. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3eef803b781f
Serialize multiple AnimationValue(s) for sub properties of a shorthand into single shorthand string. r=birtles,manishearth
https://hg.mozilla.org/integration/autoland/rev/fee301b94b59
Enable shorthand properties for SMIL. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f47c91dc7c8e
A reftest for overflow shorthand animation in SMIL. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: