Closed Bug 1302949 Opened 3 years ago Closed 3 years ago

stylo: Compute StyleAnimationValue objects using Servo

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(9 files, 2 obsolete files)

Bug 1302949 - Drop unused aTargetElement parameter from ComputeValuesFromStyleRule;
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1302949 - Store Servo declaration block in keyframe values;
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1302949 - Add a method to serialize a declaration block as a single value;
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1302949 - Serialize specified keyframe values;
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1302949 - Skip invalid animation values;
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1302949 - Parse animation values with Servo backend;
58 bytes, text/x-review-board-request
manishearth
: review+
heycam
: review+
Details
Bug 1302949 - Compute StyleAnimationValue objects from servo declaration blocks;
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1302949 - Skip calling CalculateCumulativeChangeHint
58 bytes, text/x-review-board-request
heycam
: review+
Details
Servo PR
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
Summary: Stylo: Compute StyleAnimationValue objects using Servo → stylo: Compute StyleAnimationValue objects using Servo
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.
Regarding the failure, I'm not sure if we work on windows yet -- but I'd need to see a backtrace.
(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.
(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 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 on attachment 8791851 [details]
Bug 1302949 - Serialize specified keyframe values;

https://reviewboard.mozilla.org/r/79134/#review78736
Attachment #8791851 - Flags: 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 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 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 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+
(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 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+
Attachment #8791853 - Flags: review?(manishearth)
Attachment #8791854 - Flags: review?(manishearth)
Attachment #8791855 - Flags: review?(manishearth)
Attachment #8791856 - Flags: review?(manishearth)
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 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+
Attachment #8791855 - Flags: review?(manishearth)
Attachment #8791856 - Flags: review?(manishearth)
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
(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 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!
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)
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 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 on attachment 8791851 [details]
Bug 1302949 - Serialize specified keyframe values;

https://reviewboard.mozilla.org/r/79134/#review80596
Attachment #8791851 - Flags: review?(cam) → 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 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 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 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)
(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.)
(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.
Attached file Servo PR —
Attachment #8791848 - Attachment is obsolete: true
Attachment #8791854 - Attachment is obsolete: true
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 on attachment 8791856 [details]
Bug 1302949 - Skip calling CalculateCumulativeChangeHint

https://reviewboard.mozilla.org/r/79144/#review82064
Attachment #8791856 - Flags: review?(cam) → review+
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
I was unable to push with autoland due to a "hg error in cmd: hg rewritecommitdescriptions" (for which I filed bug 1307664).
Mark 51 won't fix as this is a new feature for 51.
status-firefox51: affected → wontfix
Blocks: 1338087
You need to log in before you can comment on or make changes to this bug.