Closed Bug 1374881 Opened 3 years ago Closed 2 years ago

stylo: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: xidorn, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The comment in code indicates this should have been fixed by bug 1311257 but it hasn't, so file a separate bug.

dom/base/test/test_domwindowutils.html is affected by this at least.
Oh, I didn't know nsDOMWindowUtils:GetUnanimatedComputedStyle... Thank you Xidorn!
Implementing this for Stylo should knock out about ~50 DevTools failures.
No longer blocks: stylo-devtools-tests
This will be trivial once bug 1379505, just a matter of add a new RuleInclusion variant, I guess, and handling that in StyleResolver, to get a new style without those rules without complicating the traversal code.
Assignee: nobody → bwerth
Marking P1 for the high DevTools failure count blocked on this.
Priority: -- → P1
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> This will be trivial once bug 1379505, just a matter of add a new
> RuleInclusion variant, I guess, and handling that in StyleResolver, to get a
> new style without those rules without complicating the traversal code.

Upon reflection, and even though this may work, this would skip animation rules on any element, not just the target element. I think that'd be ok, but would be needlessly inefficient, and we can probably reuse GetBaseComputedValuesForElement.
Attachment #8887730 - Flags: review?(emilio+bugs)
I think this patch implements the API correctly. Locally, it gets further on dom/base/test/test_domwindowutils.html, but I don't see any unexpected passes on https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea1c4b0de2da70c8fc23620c60c1737a8988805a&selectedJob=115734992. Which tests do we need to turn back on?
(In reply to Brad Werth [:bradwerth] from comment #8)
> I think this patch implements the API correctly. Locally, it gets further on
> dom/base/test/test_domwindowutils.html, but I don't see any unexpected
> passes on
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ea1c4b0de2da70c8fc23620c60c1737a8988805a&selectedJob=1
> 15734992. Which tests do we need to turn back on?

Most usages are in DevTools tests mentioned in bug 1379985.  They aren't run in automation for Stylo yet.

To try them locally:

$ ./mach test devtools/server/tests/browser/browser_animation_getProperties.js

and 

$ ./mach test devtools/client/animationinspector/test

should work.  It's possible the tests will hit other new errors, but ideally you at least won't see errors about this specific API anymore.
Comment on attachment 8887730 [details]
Bug 1374881 Part 1: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle for servo.

https://reviewboard.mozilla.org/r/158636/#review164730

So the approach looks reasonable, but there seems to be a bit more logic on the Gecko codepath we probably also need... I'm not familiar with the semantics of ths API, but seems like Cameron and Brian reviewed the initial implementation in bug 1210796. I'd probably prefer them to take a look.

If the code you have is enough, then the code should work for both style backends and we could just remove all the extra logic.

::: dom/base/nsDOMWindowUtils.cpp:2899
(Diff revision 2)
>    if (!shell) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (shell->StyleSet()->IsServo()) {
> +    //ServoComputedValues* values = styleContext->AsServo()->ComputedValues();

nit: this should go away.
Attachment #8887730 - Flags: review?(emilio+bugs)
Attachment #8887730 - Flags: review?(cam)
Attachment #8887730 - Flags: review?(bbirtles)
(In reply to Brad Werth [:bradwerth] from comment #8)
> I think this patch implements the API correctly. Locally, it gets further on
> dom/base/test/test_domwindowutils.html, but I don't see any unexpected
> passes on
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ea1c4b0de2da70c8fc23620c60c1737a8988805a&selectedJob=1
> 15734992. Which tests do we need to turn back on?

Ah, and dom/base/test/test_domwindowutils.html itself is a mochitest outside the style system, which we also don't run currently for Stylo in automation.
Comment on attachment 8887730 [details]
Bug 1374881 Part 1: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle for servo.

https://reviewboard.mozilla.org/r/158636/#review164932

::: dom/base/nsDOMWindowUtils.cpp:2898
(Diff revision 3)
> +  if (shell->StyleSet()->IsServo()) {
> +    RefPtr<nsComputedDOMStyle> computedStyle =
> +      NS_NewComputedDOMStyle(
> +        element, aPseudoElement, shell,
> +        nsComputedDOMStyle::StyleType::eAll,
> +        nsComputedDOMStyle::AnimationFlag::eWithoutAnimation);
> +    computedStyle->GetPropertyValue(propertyID, aResult);
> +    return NS_OK;
> +  }
> +
>    nsIAtom* pseudo = nsCSSPseudoElements::GetPseudoAtom(aPseudoElement);
>    RefPtr<nsStyleContext> styleContext =
>      nsComputedDOMStyle::GetUnanimatedStyleContextNoFlush(element,
>                                                           pseudo, shell);

I think it possible it would be nicer if we could use the |styleContext| like we do in the Gecko code path. That would save us flushing style (unless we encounter cases like later in this function where we don't get the right value for discrete animations).

I think we can get the ServoComputedValues from the style context (although I also think I saw Xidorn suggesting we could possibly drop this method from nsStyleContext). Then I think we can use Servo_ComputedValues_ExtractAnimationValue to get the ServoAnimationValue and then Servo_AnimationValue_Serialize to serialize it?

Would that work?
(In reply to Brian Birtles (:birtles) from comment #13)

> I think we can get the ServoComputedValues from the style context (although
> I also think I saw Xidorn suggesting we could possibly drop this method from
> nsStyleContext). Then I think we can use
> Servo_ComputedValues_ExtractAnimationValue to get the ServoAnimationValue
> and then Servo_AnimationValue_Serialize to serialize it?

The updated patch follows this approach and appears to work in my local testing.
Comment on attachment 8887730 [details]
Bug 1374881 Part 1: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle for servo.

https://reviewboard.mozilla.org/r/158636/#review165534

r=me with the following change

::: dom/base/nsDOMWindowUtils.cpp:2904
(Diff revision 4)
> -  if (shell->StyleSet()->IsServo()) {
> -    return NS_ERROR_NOT_IMPLEMENTED;
> +    RefPtr<RawServoAnimationValue> value =
> +      Servo_ComputedValues_ExtractAnimationValue(styleContext->AsServo(),
> +                                                 propertyID).Consume();
> +    Servo_AnimationValue_Serialize(value, propertyID, &aResult);

I think we need to null-check |value| here since Servo_AnimationValue_Serialize expects it to be non-null.

I suppose we should just return NS_ERROR_FAILURE in that case like we do for StyleAnimationValue::ExtractComputedValue.
Attachment #8887730 - Flags: review?(bbirtles) → review+
Comment on attachment 8887730 [details]
Bug 1374881 Part 1: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle for servo.

https://reviewboard.mozilla.org/r/158636/#review165562
Attachment #8887730 - Flags: review?(cam) → review+
Since this should resolve a large number of DevTools errors and Brad is on PTO this week, I'll attempt to finish this up.
Assignee: bwerth → jryans
Status: NEW → ASSIGNED
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/33ecd88b5adb
Implement nsDOMWindowUtils::GetUnanimatedComputedStyle for servo. r=birtles,heycam
(Let's leave it assigned to Brad, I didn't really do much here...)
Assignee: jryans → bwerth
https://hg.mozilla.org/mozilla-central/rev/33ecd88b5adb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.