Closed Bug 1346052 Opened 7 years ago Closed 7 years ago

stylo: Implement servo's computed values version of nsDOMWindowUtils::ComputeAnimationDistance

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

References

Details

Attachments

(4 files)

Bug 1346052 - Part 1: Factor out ParseProperty.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1346052 - Part 3: Implement AnimationValue::FromString.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1346052 - Part 4: Implement ComputeAnimationDistance for Servo backend.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
We use this API in test_transitions_per_property.html.
And in Daisuke's soon-to-land DevTools patches.
Blocks: stylo-devtools
Working on ComputeDistance now, and I need to implement this bug for testing my patches in Bug 1332633.
Assignee: nobody → boris.chiou
This bug blocks test_transitions_per_property.html, so promote to P2.
Priority: P3 → P2
Blocks: stylo-css-transitions
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #8859943 - Flags: review?(bbirtles)
Attachment #8859944 - Flags: review?(bbirtles)
Comment on attachment 8859943 [details]
Bug 1346052 - Part 1: Factor out ParseProperty.

https://reviewboard.mozilla.org/r/131528/#review137158

::: dom/animation/KeyframeUtils.h:167
(Diff revision 2)
>     * @return true if |aProperty| is animatable.
>     */
>    static bool IsAnimatableProperty(nsCSSPropertyID aProperty);
> +
> +  /**
> +   * Parse the CSS value into RawServoDeclarationBlock for stylo.

I think 'stylo' is the name of the project, but we don't generally want to use that word in the codebase. Instead, we should just refer to Servo.

So, I think we should just say:

"Parse a string representing a CSS property value into a RawServoDeclarationBlock."

::: dom/animation/KeyframeUtils.h:172
(Diff revision 2)
> +   * @return The parsed specified value. We put the value into
> +   *   RawServoDeclarationBlock even though there is only one specified value.

I think just the following would be enough:

  "The parsed value as a RawServoDeclarationBlock."

If you want to add more explanation, perhaps you could add:

  "We put the value in a declaration block since that is how we represent specified values in Servo."
Attachment #8859943 - Flags: review?(bbirtles) → review+
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137162

Sorry, I'd like to check this again with the changes to take a nsStyleContext to check it makes sense.

::: layout/style/StyleAnimationValue.h:600
(Diff revision 1)
> +  // Compute the distance between *this and other.
> +  // Note: we pass null nsStyleContext to StyleAnimationValue::ComputeDistance,
> +  //       so this method can't compute the distance of transform property.
> +  double ComputeDistance(nsCSSPropertyID aProperty,
> +                         const AnimationValue& aOther) const;

This seems odd to me. Can we just pass the nsStyleContext but allow it to be nullptr?

(Also, is it the transform property in general, or just when we have em-units in calc() expressions?)

One other nit, "between *this and other" should probably be, "between this object and |aOther|".

::: layout/style/StyleAnimationValue.cpp:5291
(Diff revision 1)
> +
> +  if (mServo || aOther.mServo) {
> +    return 0.0;
> +  }
> +
> +  double ret = 0.0;

(Nit: Call this 'distance' instead?)

::: layout/style/StyleAnimationValue.cpp:5297
(Diff revision 1)
> +  if (!StyleAnimationValue::ComputeDistance(aProperty,
> +                                            mGecko,
> +                                            aOther.mGecko,
> +                                            nullptr,
> +                                            ret)) {
> +    NS_WARNING("Cannot compute the distance");

I wonder if we need this warning? I'm not sure, but somehow it seems odd to produce a warning when we actually expect this to happen often.
Attachment #8861913 - Flags: review?(bbirtles)
Comment on attachment 8859944 [details]
Bug 1346052 - Part 3: Implement AnimationValue::FromString.

https://reviewboard.mozilla.org/r/131530/#review137088

Sorry, can we split this patch into two parts?

* One to add the method for getting a computed value
* One to use it to implement nsDOMWindowUtils::ComputeAnimationDistance

::: layout/style/StyleAnimationValue.h:581
(Diff revision 2)
> +  static bool ComputeAnimationValue(nsCSSPropertyID aProperty,
> +                                    dom::Element* aElement,
> +                                    const nsAString& aValue,
> +                                    AnimationValue& aOutput);

Let's just call this |FromString| and make it return an AnimationValue object (which will be null if it fails).

Oh, and let's implement the move ctor/assignment operator for AnimationValue at the same time.

Also, the order of parameters here is odd. |aProperty| and |aValue| should probably go together (either first or last).

::: layout/style/StyleAnimationValue.cpp:5208
(Diff revision 2)
> +/* static */ bool
> +AnimationValue::ComputeAnimationValue(nsCSSPropertyID aProperty,
> +                                      Element* aElement,
> +                                      const nsAString& aValue,
> +                                      AnimationValue& aOutput)
> +{

Let's assert aElement is non-null here.

::: layout/style/StyleAnimationValue.cpp:5219
(Diff revision 2)
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContext(aElement, nullptr, shell);

We should probably mention in this header file that this method flushes style (and hence we don't expect to use it internally).

::: layout/style/StyleAnimationValue.cpp:5223
(Diff revision 2)
> +
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContext(aElement, nullptr, shell);
> +
> +  if (styleContext->StyleSource().IsServoComputedValues()) {
> +    nsPresContext* preContext = nsContentUtils::GetContextForContent(aElement);

presContext

::: layout/style/StyleAnimationValue.cpp:5228
(Diff revision 2)
> +    RefPtr<RawServoDeclarationBlock> declarations =
> +      KeyframeUtils::ParseProperty(aProperty, aValue, doc);

Shouldn't we null-check this?

::: layout/style/StyleAnimationValue.cpp:5231
(Diff revision 2)
> +    RefPtr<nsStyleContext> parentContext = styleContext->GetParentAllowServo();
> +    const ServoComputedValuesWithParent styles = {
> +      styleContext->StyleSource().AsServoComputedValues(),
> +      parentContext ? parentContext->StyleSource().AsServoComputedValues()
> +                    : nullptr
> +    };

Apparently GetParentAllowServo is going away so we should add a comment to that effect.

::: layout/style/StyleAnimationValue.cpp:5244
(Diff revision 2)
> +  if (!StyleAnimationValue::ComputeValue(aProperty, aElement, styleContext,
> +                                         aValue, false, aOutput.mGecko)) {
> +    return false;
> +  }
> +  return true;

Couldn't we just do

  return StyleAnimationValue::ComputeValue(...)

here?

::: servo/ports/geckolib/glue.rs:2005
(Diff revision 2)
> +pub extern "C" fn Servo_AnimationValue_Compute(declarations: RawServoDeclarationBlockBorrowed,
> +                                               style: ServoComputedValuesBorrowed,
> +                                               parent_style: ServoComputedValuesBorrowedOrNull,
> +                                               raw_data: RawServoStyleSetBorrowed)
> +                                               -> RawServoAnimationValueStrong {
> +    use style::values::computed::Context;

(Nit: Add a blank line after this to separate the use declarations from the variable declarations?)
Attachment #8859944 - Flags: review?(bbirtles)
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137162

> This seems odd to me. Can we just pass the nsStyleContext but allow it to be nullptr?
> 
> (Also, is it the transform property in general, or just when we have em-units in calc() expressions?)
> 
> One other nit, "between *this and other" should probably be, "between this object and |aOther|".

In StyleAnimationValue::ComputeDistance(), nsStyleContext is only used for computing distance for mismatched transform lists (for paced spacing), and there is no other usages now. Besides, we don't use nsDOMWindowUtils::ComputeAnimationDistance to compute the distance between two transform lists. Yeah, it's ok to pass nsStyleContext to this method, and I think it is fine for nullptr nsStyleContext because we return 0.0 in ComputeMismatchedTransfromListDistance [1].

[1] http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/layout/style/StyleAnimationValue.cpp#1441-1443

> I wonder if we need this warning? I'm not sure, but somehow it seems odd to produce a warning when we actually expect this to happen often.

OK. I will remove this warning. Thanks.
Comment on attachment 8859944 [details]
Bug 1346052 - Part 3: Implement AnimationValue::FromString.

https://reviewboard.mozilla.org/r/131530/#review137088

> * One to add the method for getting a computed value
> * One to use it to implement nsDOMWindowUtils::ComputeAnimationDistance

Sure. I will split this patch into two parts. Thanks for suggestion.

> Let's just call this |FromString| and make it return an AnimationValue object (which will be null if it fails).
> 
> Oh, and let's implement the move ctor/assignment operator for AnimationValue at the same time.
> 
> Also, the order of parameters here is odd. |aProperty| and |aValue| should probably go together (either first or last).

OK. This sounds better.
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137520

::: layout/style/StyleAnimationValue.h:607
(Diff revision 2)
> +  // Note: we pass null nsStyleContext to StyleAnimationValue::ComputeDistance,
> +  //       so this method can't compute the distance of transform property.

This comment is no longer correct. It only applies if aStyleContext is null, and even if it's null, it simply means the result is 0 if we have mismatched transform lists.

::: layout/style/StyleAnimationValue.cpp:5282
(Diff revision 2)
> +double
> +AnimationValue::ComputeDistance(nsCSSPropertyID aProperty,
> +                                const AnimationValue& aOther,
> +                                nsStyleContext* aStyleContext) const
> +{
> +  MOZ_ASSERT(!mServo || mGecko.IsNull());

This assertion needs a descriptive message to say what invariant it is asserting.

Also, why is it ok for aOther to be uninitialized but not this one? If that's the intended behavior the description of the header should say that.

Otherwise, we probably want similar behavior/assertions to IsInterpolableWith.
Comment on attachment 8859944 [details]
Bug 1346052 - Part 3: Implement AnimationValue::FromString.

https://reviewboard.mozilla.org/r/131530/#review137522

r=me with the following items addressed. Particularly:

* Consistent use of GetComposedDoc() (I think this is what we want, but please check)
* Keeping owning references to objects we continue to refer to after flushing

::: layout/style/StyleAnimationValue.cpp:5315
(Diff revision 3)
> +  nsIDocument* doc = aElement->GetUncomposedDoc();
> +  if (!doc) {
> +    return result;
> +  }
> +
> +  nsIPresShell* shell = doc->GetShell();
> +  if (!shell) {
> +    return result;
> +  }
> +
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContext(aElement, nullptr, shell);

If we're going to flush style and then re-use |doc|, we should take an owning reference to it.

In fact, we should probably put a comment before the call to GetStyleContext mentioning that it flushes style (so we know we need to be careful not to assume that any non-owning references we have are still valid).

::: layout/style/StyleAnimationValue.cpp:5315
(Diff revision 3)
> +{
> +  MOZ_ASSERT(aElement);
> +
> +  AnimationValue result;
> +
> +  nsIDocument* doc = aElement->GetUncomposedDoc();

Any reason we prefer UncomposeDoc here? The call to GetContextForContent below will call GetComposedDoc().

::: layout/style/StyleAnimationValue.cpp:5329
(Diff revision 3)
> +
> +  RefPtr<nsStyleContext> styleContext =
> +    nsComputedDOMStyle::GetStyleContext(aElement, nullptr, shell);
> +
> +  if (styleContext->StyleSource().IsServoComputedValues()) {
> +    nsPresContext* presContext = nsContentUtils::GetContextForContent(aElement);

I think we can use shell->GetPresContext() here (although we probably ought to take an owning reference to |shell| too if we do that)

::: layout/style/StyleAnimationValue.cpp:5359
(Diff revision 3)
> +                               ->ComputeAnimationValue(declarations, styles);
> +    return result;
> +  }
> +
> +  if (!StyleAnimationValue::ComputeValue(aProperty, aElement, styleContext,
> +                                         aValue, false, result.mGecko)) {

Let's put a comment next to the 'false' to mention that it corresponds to |aUseSVGMode|
Attachment #8859944 - Flags: review?(bbirtles) → review+
Comment on attachment 8862453 [details]
Bug 1346052 - Part 4: Implement ComputeAnimationDistance for Servo backend.

https://reviewboard.mozilla.org/r/134262/#review137526

This looks mostly good but I don't understand why we stop passing the style context here. Is it ok to break distance calculation of mismatched transform lists? That will be observable in at least DevTools, right?

::: dom/base/nsDOMWindowUtils.cpp:2693
(Diff revision 1)
> -  if (!StyleAnimationValue::ComputeDistance(property, v1, v2, styleContext,
> +    return NS_ERROR_ILLEGAL_VALUE;
> -                                            *aResult)) {
> -    return NS_ERROR_FAILURE;
>    }
>  
> +  *aResult = v1.ComputeDistance(property, v2);

Why don't we fetch the style context and pass it here? It seems like we used to do that?
Attachment #8862453 - Flags: review?(bbirtles)
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137528

Clearing review request for now. If my review feedback is mistaken, please re-request review.
Attachment #8861913 - Flags: review?(bbirtles)
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137520

> This assertion needs a descriptive message to say what invariant it is asserting.
> 
> Also, why is it ok for aOther to be uninitialized but not this one? If that's the intended behavior the description of the header should say that.
> 
> Otherwise, we probably want similar behavior/assertions to IsInterpolableWith.

You are right. We should have the similar behavior/assertions to IsInterpolableWith. I will update the aseertion conditions.
Comment on attachment 8859944 [details]
Bug 1346052 - Part 3: Implement AnimationValue::FromString.

https://reviewboard.mozilla.org/r/131530/#review137522

> If we're going to flush style and then re-use |doc|, we should take an owning reference to it.
> 
> In fact, we should probably put a comment before the call to GetStyleContext mentioning that it flushes style (so we know we need to be careful not to assume that any non-owning references we have are still valid).

OK. I will add RefPtr<> for |doc|, and add a comment before the call to GetStyleContext.

> Any reason we prefer UncomposeDoc here? The call to GetContextForContent below will call GetComposedDoc().

I just copy the code from [1] nsDOWWindowUtils::ComputeAnimatioValue, and we used GetUncomposedDoc before. Looks like it is a potential bug. I will use GetComposedDoc().

[1] http://searchfox.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#2424
Comment on attachment 8862453 [details]
Bug 1346052 - Part 4: Implement ComputeAnimationDistance for Servo backend.

https://reviewboard.mozilla.org/r/134262/#review137526

> This looks mostly good but I don't understand why we stop passing the style context here.
> Is it ok to break distance calculation of > mismatched transform lists?
> That will be observable in at least DevTools, right?

Oh, yes. I totally forgot DevTools needs it. I will add back the nsStyleContext parameter.
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137882

::: layout/style/StyleAnimationValue.h:606
(Diff revision 3)
>  
>    // Check if |*this| and |aToValue| can be interpolated.
>    bool IsInterpolableWith(nsCSSPropertyID aProperty,
>                            const AnimationValue& aToValue) const;
> +
> +  // Compute the distance between *this and other.

Nit: s/other/aOther/

::: layout/style/StyleAnimationValue.h:611
(Diff revision 3)
> +  // Compute the distance between *this and other.
> +  // If |aStyleContext| is nullptr, we will return 0.0 if we have mismatched
> +  // transform lists.
> +  double ComputeDistance(nsCSSPropertyID aProperty,
> +                         const AnimationValue& aOther,
> +                         nsStyleContext* aStyleContext = nullptr) const;

Do we still need the default argument value of nullptr for |aStyleContext|? I think we don't? If not, let's drop it.
Attachment #8861913 - Flags: review?(bbirtles) → review+
Comment on attachment 8862453 [details]
Bug 1346052 - Part 4: Implement ComputeAnimationDistance for Servo backend.

https://reviewboard.mozilla.org/r/134262/#review137884
Attachment #8862453 - Flags: review?(bbirtles) → review+
Comment on attachment 8861913 [details]
Bug 1346052 - Part 2: Implement AnimationValue::ComputeDistance.

https://reviewboard.mozilla.org/r/133928/#review137882

> Do we still need the default argument value of nullptr for |aStyleContext|? I think we don't? If not, let's drop it.

OK. Let's drop it. Thanks for reviewing these patches. :)
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fec15858da16
Part 1: Factor out ParseProperty. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1b5ece05337f
Part 2: Implement AnimationValue::ComputeDistance. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f8225d865e27
Part 3: Implement AnimationValue::FromString. r=birtles
https://hg.mozilla.org/integration/autoland/rev/83be140280dc
Part 4: Implement ComputeAnimationDistance for Servo backend. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: