Closed
Bug 1374881
Opened 7 years ago
Closed 7 years ago
stylo: Implement nsDOMWindowUtils::GetUnanimatedComputedStyle
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
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.
Comment 1•7 years ago
|
||
Oh, I didn't know nsDOMWindowUtils:GetUnanimatedComputedStyle... Thank you Xidorn!
Blocks: stylo-inidomutils, stylo-devtools-tests
No longer blocks: stylo-inidomutils
Blocks: 1379985
Implementing this for Stylo should knock out about ~50 DevTools failures.
No longer blocks: stylo-devtools-tests
Comment 3•7 years ago
|
||
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•7 years ago
|
Assignee: nobody → bwerth
Marking P1 for the high DevTools failure count blocked on this.
Priority: -- → P1
Comment 5•7 years ago
|
||
(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•7 years ago
|
Attachment #8887730 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 8•7 years 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•7 years 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)
Updated•7 years ago
|
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•7 years 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?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
(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 16•7 years ago
|
||
mozreview-review |
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 17•7 years ago
|
||
mozreview-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
Comment 19•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/33ecd88b5adb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•