Closed
Bug 1302949
Opened 4 years ago
Closed 4 years ago
stylo: Compute StyleAnimationValue objects using Servo
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(9 files, 2 obsolete files)
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
manishearth
:
review+
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
41 bytes,
text/x-github-pull-request
|
Details | Review |
This covers the first part of the work to get animations running with Stylo. Ultimately we want to retire StyleAnimationValue but I think we need it short-term for: * Passing to the compositor * Doing interpolation on the compositor * Calculating distances
Assignee | ||
Updated•4 years ago
|
Summary: Stylo: Compute StyleAnimationValue objects using Servo → stylo: Compute StyleAnimationValue objects using Servo
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•4 years ago
|
||
Is it possible we don't add a space when serializing var() ? https://treeherder.mozilla.org/logviewer.html#?job_id=27555336&repo=try#L15745 Do we fail the match here, perhaps? Are custom properties stored as declarations? https://dxr.mozilla.org/servo/source/components/style/properties/properties.mako.rs#497 Also, I'm not sure if I need to worry about the following assertion failure on Windows: > thread 'StyleWorker worker 1/6' panicked at 'assertion failed: !self.mBuffer.is_null()', C:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\servo\ports\geckolib\gecko_bindings\sugar\ns_t_array.rs:47 https://treeherder.mozilla.org/logviewer.html#?job_id=27554600&repo=try#L3338 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1fce941d3285e0b294565bea43f6c3c9b0a2762&selectedJob=27554600 I don't think I'm doing anything with nsTArrays, the other platforms appear to be fine, and I can't reproduce it locally.
Comment 14•4 years ago
|
||
Regarding the failure, I'm not sure if we work on windows yet -- but I'd need to see a backtrace.
Comment 15•4 years ago
|
||
(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #13) > Is it possible we don't add a space when serializing var() ? > https://treeherder.mozilla.org/logviewer. > html#?job_id=27555336&repo=try#L15745 > > Do we fail the match here, perhaps? Are custom properties stored as > declarations? We don't fail the match, but custom properties fall inside the `value_is_unparsed` category. I don't remember why that exists in the first place, but yeah, it seems we're not adding a space there. https://dxr.mozilla.org/servo/source/components/style/properties/properties.mako.rs#1022 > Also, I'm not sure if I need to worry about the following assertion failure > on Windows: > > thread 'StyleWorker worker 1/6' panicked at 'assertion failed: !self.mBuffer.is_null()', C:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\servo\ports\geckolib\gecko_bindings\sugar\ns_t_array.rs:47 > https://treeherder.mozilla.org/logviewer.html#?job_id=27554600&repo=try#L3338 > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c1fce941d3285e0b294565bea43f6c3c9b0a2762&selectedJob=2 > 7554600 > > I don't think I'm doing anything with nsTArrays, the other platforms appear > to be fine, and I can't reproduce it locally. That assertion seems rather scary, and means that we have an nsTArray with a null buffer (which I thought was impossible, given nsTArrays are initialized with the static nsTArrayHeader). If that can indeed be possible, we'd need to adjust the code. Otherwise more investigation is needed to determine how we're arriving to that state.
Comment 16•4 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15) > If that can indeed be possible, we'd need > to adjust the code. Otherwise more investigation is needed to determine how > we're arriving to that state. (In any case, I don't think that investigation should block this bug, btw).
Comment 17•4 years ago
|
||
mozreview-review |
Comment on attachment 8791850 [details] Bug 1302949 - Add a method to serialize a declaration block as a single value; https://reviewboard.mozilla.org/r/79132/#review78734 ::: servo/ports/geckolib/glue.rs:483 (Diff revision 1) > + // FIXME: We are expecting |declarations| to be a declaration block with either a single > + // longhand property-declaration or a series of longhand property-declarations that make > + // up a single shorthand property. As a result, it should be possible to serialize > + // |declarations| as a single declaration. However, we only want to return the *value* from > + // that single declaration. For now, we just manually strip the property name, colon, > + // leading spacing, and trailing space. In future we should find a more robust way to do File a bug on Servo for this, I'll implement it later. ::: servo/ports/geckolib/glue.rs:487 (Diff revision 1) > + // that single declaration. For now, we just manually strip the property name, colon, > + // leading spacing, and trailing space. In future we should find a more robust way to do > + // this. > + let position = string.find(':').unwrap(); > + let value = &string[(position + 2)..]; // Get the value after the first colon and space. > + let length = value.len() - 1; // Strip last semicolon. probably should assert that it *is* a semicolon
Comment 18•4 years ago
|
||
mozreview-review |
Comment on attachment 8791851 [details] Bug 1302949 - Serialize specified keyframe values; https://reviewboard.mozilla.org/r/79134/#review78736
Attachment #8791851 -
Flags: review+
Comment 19•4 years ago
|
||
mozreview-review |
Comment on attachment 8791850 [details] Bug 1302949 - Add a method to serialize a declaration block as a single value; https://reviewboard.mozilla.org/r/79132/#review78738
Attachment #8791850 -
Flags: review+
Comment 20•4 years ago
|
||
mozreview-review |
Comment on attachment 8791849 [details] Bug 1302949 - Store Servo declaration block in keyframe values; https://reviewboard.mozilla.org/r/79130/#review78740
Attachment #8791849 -
Flags: review+
Comment 21•4 years ago
|
||
mozreview-review |
Comment on attachment 8791848 [details] Bug 1302949 - Add a method to Gecko bindings for comparing declaration blocks; https://reviewboard.mozilla.org/r/79128/#review78742
Attachment #8791848 -
Flags: review+
Comment 22•4 years ago
|
||
mozreview-review |
Comment on attachment 8791847 [details] Bug 1302949 - Drop unused aTargetElement parameter from ComputeValuesFromStyleRule; https://reviewboard.mozilla.org/r/79126/#review78744 ::: servo/ports/geckolib/glue.rs:380 (Diff revision 1) > + base_length: u32, > + base: *mut ThreadSafeURIHolder, > + referrer: *mut ThreadSafeURIHolder, > + principal: *mut ThreadSafePrincipalHolder) > + -> ServoDeclarationBlockStrong { > + let name = unsafe { from_utf8_unchecked(slice::from_raw_parts(property_bytes, leave a comment that this is temporary till the string bindings happen
Attachment #8791847 -
Flags: review+
Comment 23•4 years ago
|
||
(Note that my r+ is only valid on the Servo side. You should still get review from someone on the gecko side as well, especially for the second half of the commits)
Comment 24•4 years ago
|
||
mozreview-review |
Comment on attachment 8791852 [details] Bug 1302949 - Skip invalid animation values; https://reviewboard.mozilla.org/r/79136/#review78748 ::: dom/animation/KeyframeUtils.cpp:610 (Diff revision 1) > ComputedKeyframeValues* computedValues = result.AppendElement(); > for (const PropertyValuePair& pair : > PropertyPriorityIterator(frame.mPropertyValues)) { > - if (IsInvalidValuePair(pair)) { > + MOZ_ASSERT(!pair.mServoDeclarationBlock || > + styleBackend == StyleBackendType::Servo, > + "Parsed animation values using Servo backend but target" nit: "Parsed animation values" read as a noun clause to me at first. "Animation values were parsed using Servo backend" might be clearer.
Attachment #8791852 -
Flags: review+
Updated•4 years ago
|
Attachment #8791853 -
Flags: review?(manishearth)
Attachment #8791854 -
Flags: review?(manishearth)
Attachment #8791855 -
Flags: review?(manishearth)
Attachment #8791856 -
Flags: review?(manishearth)
Comment 25•4 years ago
|
||
mozreview-review |
Comment on attachment 8791853 [details] Bug 1302949 - Parse animation values with Servo backend; https://reviewboard.mozilla.org/r/79138/#review78752 ::: dom/animation/KeyframeUtils.cpp:1052 (Diff revision 1) > value.SetTokenStreamValue(tokenStream); > + } else { > + // If we succeeded in parsing with Gecko, but not Servo the animation is > + // not going to work since, for the purposes of animation, we're going to > + // ignore |mValue| when the backend is Servo. > + MOZ_ASSERT(aDocument->GetStyleBackendType() != StyleBackendType::Servo, Should we be asserting here? We usually just log when Servo fails to parse a thing.
Attachment #8791853 -
Flags: review?(manishearth) → review+
Comment 26•4 years ago
|
||
mozreview-review |
Comment on attachment 8791854 [details] Bug 1302949 - Add a method to restyle with an added declaration; https://reviewboard.mozilla.org/r/79140/#review78758
Attachment #8791854 -
Flags: review?(manishearth) → review+
Updated•4 years ago
|
Attachment #8791855 -
Flags: review?(manishearth)
Attachment #8791856 -
Flags: review?(manishearth)
Assignee | ||
Comment 27•4 years ago
|
||
Thanks for all the reviews Manish! I'm trying to work out how we should fix the 'var()' issue from comment 13 before landing any of this. First I need to understand why this check was added in the first place: https://github.com/servo/servo/commit/51e642e875ea25502b92967358e32a30e06ab3dd#commitcomment-19167960
Assignee | ||
Comment 28•4 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #17) > ::: servo/ports/geckolib/glue.rs:483 > (Diff revision 1) > > + // FIXME: We are expecting |declarations| to be a declaration block with either a single > > + // longhand property-declaration or a series of longhand property-declarations that make > > + // up a single shorthand property. As a result, it should be possible to serialize > > + // |declarations| as a single declaration. However, we only want to return the *value* from > > + // that single declaration. For now, we just manually strip the property name, colon, > > + // leading spacing, and trailing space. In future we should find a more robust way to do > > File a bug on Servo for this, I'll implement it later. Filed https://github.com/servo/servo/issues/13423.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 39•4 years ago
|
||
mozreview-review |
Comment on attachment 8791847 [details] Bug 1302949 - Drop unused aTargetElement parameter from ComputeValuesFromStyleRule; https://reviewboard.mozilla.org/r/79124/#review80164 Hi Cameron, sorry to load you up with more reviews. Hopefully these are fairly easy. Manish has looked over most of them already but I just need someone to check the Gecko side. Thanks!
Assignee | ||
Updated•4 years ago
|
Attachment #8791849 -
Flags: review?(cam)
Attachment #8791850 -
Flags: review?(cam)
Attachment #8791851 -
Flags: review?(cam)
Attachment #8791852 -
Flags: review?(cam)
Attachment #8791853 -
Flags: review?(cam)
Attachment #8791855 -
Flags: review?(cam)
Attachment #8791856 -
Flags: review?(cam)
Assignee | ||
Comment 40•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc64216edbe
Comment 41•4 years ago
|
||
mozreview-review |
Comment on attachment 8791849 [details] Bug 1302949 - Store Servo declaration block in keyframe values; https://reviewboard.mozilla.org/r/79130/#review80590
Attachment #8791849 -
Flags: review?(cam) → review+
Comment 42•4 years ago
|
||
mozreview-review |
Comment on attachment 8791850 [details] Bug 1302949 - Add a method to serialize a declaration block as a single value; https://reviewboard.mozilla.org/r/79132/#review80594 r=me on the C++ bits.
Attachment #8791850 -
Flags: review?(cam) → review+
Comment 43•4 years ago
|
||
mozreview-review |
Comment on attachment 8791851 [details] Bug 1302949 - Serialize specified keyframe values; https://reviewboard.mozilla.org/r/79134/#review80596
Attachment #8791851 -
Flags: review?(cam) → review+
Comment 44•4 years ago
|
||
mozreview-review |
Comment on attachment 8791852 [details] Bug 1302949 - Skip invalid animation values; https://reviewboard.mozilla.org/r/79136/#review80604 r=me with that. ::: dom/animation/KeyframeUtils.cpp:595 (Diff revision 2) > dom::Element* aElement, > nsStyleContext* aStyleContext) > { > MOZ_ASSERT(aStyleContext); > MOZ_ASSERT(aElement); > + MOZ_ASSERT(aElement->GetComposedDoc()); Will we never be in here for an element not in the document? Since we're only using it for GetStyleBackendType(), I think it would be safe (and a little cheaper) to use aElement->OwnerDoc(), and then not have to assert it. ::: dom/animation/KeyframeUtils.cpp:1391 (Diff revision 2) > + // For the Servo backend, invalid shorthand values are represented by > + // a null mServoDeclarationBlock member which we skip above in > + // IsInvalidValuePair. Maybe assert in here that pair.mServoDeclarationBlock is non-null?
Attachment #8791852 -
Flags: review?(cam) → review+
Comment 45•4 years ago
|
||
mozreview-review |
Comment on attachment 8791853 [details] Bug 1302949 - Parse animation values with Servo backend; https://reviewboard.mozilla.org/r/79138/#review80882 ::: dom/animation/KeyframeUtils.cpp:1008 (Diff revision 2) > + RefPtr<ServoDeclarationBlock> servoDeclarationBlock = > + Servo_ParseProperty( > + reinterpret_cast<const uint8_t*>(name.get()), name.Length(), > + reinterpret_cast<const uint8_t*>(value.get()), value.Length(), > + reinterpret_cast<const uint8_t*>(baseString.get()), > + baseString.Length(), Nit: I think this indentation is misleading.
Attachment #8791853 -
Flags: review?(cam) → review+
Comment 46•4 years ago
|
||
mozreview-review |
Comment on attachment 8791855 [details] Bug 1302949 - Compute StyleAnimationValue objects from servo declaration blocks; https://reviewboard.mozilla.org/r/79142/#review80884 ::: layout/style/StyleAnimationValue.cpp:3043 (Diff revision 3) > } > > static bool > +ComputeValuesFromStyleContext( > + nsCSSPropertyID aProperty, > + mozilla::CSSEnabledState aEnabledState, Nit: drop the "mozilla::". ::: layout/style/StyleAnimationValue.cpp (Diff revision 3) > +} > + > +static bool > ComputeValuesFromStyleRule(nsCSSPropertyID aProperty, > CSSEnabledState aEnabledState, > - dom::Element* aTargetElement, (The removal of the unused argument would have been better as a separate commit.) ::: layout/style/StyleAnimationValue.cpp:3251 (Diff revision 3) > } > > +/* static */ bool > +StyleAnimationValue::ComputeValues( > + nsCSSPropertyID aProperty, > + mozilla::CSSEnabledState aEnabledState, Nit: here too.
Attachment #8791855 -
Flags: review?(cam) → review+
Comment 47•4 years ago
|
||
mozreview-review |
Comment on attachment 8791856 [details] Bug 1302949 - Skip calling CalculateCumulativeChangeHint https://reviewboard.mozilla.org/r/79144/#review80886 ::: dom/animation/KeyframeEffectReadOnly.cpp:299 (Diff revision 3) > + // FIXME (bug 1303235): Do this for Servo too > + if (aStyleContext->PresContext()->StyleSet()->IsGecko()) { > - CalculateCumulativeChangeHint(aStyleContext); > + CalculateCumulativeChangeHint(aStyleContext); > + } Don't we need to cause KeyframeEffectReadOnly::CanIgnoreIfNotVisible to return false, to disable this optimization? If mCumulateChangeHint remains nsChangeHint(0), then CanIgnoreIfNotVisible will return true (as long as dom.animations.offscreen-throttling is true). Should we check if we have StyleBackendType::Servo in CanIgnoreIfNotVisible, and return false if so?
Attachment #8791856 -
Flags: review?(cam)
Assignee | ||
Comment 48•4 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #44) > Comment on attachment 8791852 [details] > Bug 1302949 - Skip invalid animation values; ... > ::: dom/animation/KeyframeUtils.cpp:595 > (Diff revision 2) > > dom::Element* aElement, > > nsStyleContext* aStyleContext) > > { > > MOZ_ASSERT(aStyleContext); > > MOZ_ASSERT(aElement); > > + MOZ_ASSERT(aElement->GetComposedDoc()); > > Will we never be in here for an element not in the document? Since we're > only using it for GetStyleBackendType(), I think it would be safe (and a > little cheaper) to use aElement->OwnerDoc(), and then not have to assert it. This method only gets called from KeyframeEffectReadOnly::UpdateProperties at which point we definitely have a style context for the element (we assert that), so presumably we have a composed document. If the element doesn't have a composed doc, then what style backend should we be using? Using the style backend for the owner document seems arbitrary? Perhaps it's ok because we know the style backend used for the owner doc will never differ from the composed doc? (It feels odd to make this refer to OwnerDoc() when we know that's not correct--but I guess this code is temporary.)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 59•4 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #47) > Comment on attachment 8791856 [details] > Bug 1302949 - Skip calling CalculateCumulativeChangeHint > > https://reviewboard.mozilla.org/r/79144/#review80886 > > ::: dom/animation/KeyframeEffectReadOnly.cpp:299 > (Diff revision 3) > > + // FIXME (bug 1303235): Do this for Servo too > > + if (aStyleContext->PresContext()->StyleSet()->IsGecko()) { > > - CalculateCumulativeChangeHint(aStyleContext); > > + CalculateCumulativeChangeHint(aStyleContext); > > + } > > Don't we need to cause KeyframeEffectReadOnly::CanIgnoreIfNotVisible to > return false, to disable this optimization? If mCumulateChangeHint remains > nsChangeHint(0), then CanIgnoreIfNotVisible will return true (as long as > dom.animations.offscreen-throttling is true). > > Should we check if we have StyleBackendType::Servo in CanIgnoreIfNotVisible, > and return false if so? Oh yeah, I totally misread NS_IsHintSubset.
Assignee | ||
Comment 60•4 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Attachment #8791848 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8791854 -
Attachment is obsolete: true
Assignee | ||
Comment 69•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=693fb59f6bc2
Comment 70•4 years ago
|
||
mozreview-review |
Comment on attachment 8791847 [details] Bug 1302949 - Drop unused aTargetElement parameter from ComputeValuesFromStyleRule; https://reviewboard.mozilla.org/r/79126/#review82062
Attachment #8791847 -
Flags: review?(cam) → review+
Comment 71•4 years ago
|
||
mozreview-review |
Comment on attachment 8791856 [details] Bug 1302949 - Skip calling CalculateCumulativeChangeHint https://reviewboard.mozilla.org/r/79144/#review82064
Attachment #8791856 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 80•4 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e99f0dc33fcd Store Servo declaration block in keyframe values; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4b6b94f18c Add a method to serialize a declaration block as a single value; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/6e74cfede619 Serialize specified keyframe values; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/16cbe3bc772d Skip invalid animation values; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0be59b6fcc Parse animation values with Servo backend; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/9f188dfb9551 Drop unused aTargetElement parameter from ComputeValuesFromStyleRule; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/a16b04cbfc80 Compute StyleAnimationValue objects from servo declaration blocks; r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/6d687e7fccb4 Skip calling CalculateCumulativeChangeHint; r=heycam
Assignee | ||
Comment 81•4 years ago
|
||
I was unable to push with autoland due to a "hg error in cmd: hg rewritecommitdescriptions" (for which I filed bug 1307664).
Comment 82•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e99f0dc33fcd https://hg.mozilla.org/mozilla-central/rev/ce4b6b94f18c https://hg.mozilla.org/mozilla-central/rev/6e74cfede619 https://hg.mozilla.org/mozilla-central/rev/16cbe3bc772d https://hg.mozilla.org/mozilla-central/rev/5b0be59b6fcc https://hg.mozilla.org/mozilla-central/rev/9f188dfb9551 https://hg.mozilla.org/mozilla-central/rev/a16b04cbfc80 https://hg.mozilla.org/mozilla-central/rev/6d687e7fccb4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 83•4 years ago
|
||
Mark 51 won't fix as this is a new feature for 51.
You need to log in
before you can comment on or make changes to this bug.
Description
•