Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
a month ago
19 hours ago

People

(Reporter: xidorn, Assigned: bradwerth)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a month ago
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!
Blocks: 1357705, 1357715
No longer blocks: 1357705
Blocks: 1379985
Implementing this for Stylo should knock out about ~50 DevTools failures.
No longer blocks: 1357715
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)

Updated

9 days ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a day ago
Attachment #8887730 - Flags: review?(emilio+bugs)
(Assignee)

Comment 8

a day ago
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 10

a day ago
mozreview-review
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 hidden (mozreview-request)

Comment 13

19 hours ago
mozreview-review
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?
You need to log in before you can comment on or make changes to this bug.