Closed Bug 1254419 Opened 8 years ago Closed 8 years ago

Add a chrome-only GetProperties() method to KeyframeEffectReadOnly

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(9 files)

58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
hiro
: 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
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
This is:

a) So we can keep testing our frame-to-property mapping after we update getFrames() to preserve the author-specified frames (bug 1245748)
b) So DevTools can continue to use this information (in place of getFrames() once it changes) to expose *how* frames are being interpreted and run
c) So, in future, we can expose more per-value information such as per-value composite modes etc.

The idea is to build on the API exposed in bug 1196114, possible renaming getPropertyState() to simply getProperties().
Later in this patch series when we convert tests from web-platform tests to
mochitest-chrome tests, some of the test cases that use zero-length segments
(overlapping keyframes at certain offsets) would trigger failed assertions
in KeyframeEffectReadOnly::ComposeStyle. This is because this method was
originally written with CSS animations in mind where segments cannot be
zero-length. Furthermore, when these same tests cases are run as
web-platform-tests, the failed assertions are not visible.

This patch adjusts the handling of segments to allow zero-length segments and
adds a test to check that the handling matches that defined in Web Animations
which is summarized in the following informative note,

  "this procedure permits overlapping keyframes. The behavior is that at the
  point of overlap the output value jumps to the value of the last defined
  keyframe at that offset. For overlapping frames at 0 or 1, the output value
  for iteration progress values less than 0 or greater than or equal to 1 is the
  value of the first keyframe or the last keyframe in keyframes
  respectively."[1]

[1] https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect

Review commit: https://reviewboard.mozilla.org/r/40097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40097/
This better matches the order in the WebIDL and, once we rename
GetPropertyState to GetProperties it will make sense for GetFrames and
GetProperties to be side-by-side.

Review commit: https://reviewboard.mozilla.org/r/40099/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40099/
We are now extending this API to include more than just metadata about each
animated property but also the property values themselves.

Note that we can't use the name AnimationProperty for the dictionary since
we already use that name internally and [BinaryName] doesn't seem to apply to
dictionaries. An alternative might be to rename the internal object to
AnimationPropertyStruct like we already do for Keyframe.

Review commit: https://reviewboard.mozilla.org/r/40101/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40101/
I think the reason we originally didn't do this is that the
"isRunningOnCompositor" status might be misleading for animations that are
being overridden. That is, there are some animations we don't send to the
compositor because they are being overridden by another animation (e.g. a
CSS animation touching the 'transform' animation will cause a CSS transition
on the same property not to run, despite the fact that transitions apply
higher in the cascade). This is not merely a performance optimization but means
we don't have to do the cascade on the compositor.

In the future, once we introduce additive animation, we won't be able to handle
this so simply since it an animation will be able to be partially overridden.
Instead, consumers of this API will need to look at the 'composite' member of
the various animation values to see if an animation is being fully or partially
overridden.

As a result, this API really should return all running animations, even if they
are currently being overridden.

Review commit: https://reviewboard.mozilla.org/r/40105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40105/
https://reviewboard.mozilla.org/r/40097/#review36809

::: dom/animation/KeyframeEffect.cpp:598
(Diff revision 1)
> +    if (segment->mToKey == segment->mFromKey) {
> +      if (computedTiming.mProgress.Value() < 0) {
> +        *val = segment->mFromValue;
> +      } else {
> +        *val = segment->mToValue;
> +      }
> +      return;
> +    }

I guess you might want to conitnue here to iterate other CSS properties.
Comment on attachment 8730706 [details]
MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40097/diff/1-2/
Attachment #8730706 - Flags: review?(cam)
Comment on attachment 8730707 [details]
MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40099/diff/1-2/
Attachment #8730707 - Flags: review?(hiikezoe)
Comment on attachment 8730708 [details]
MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40101/diff/1-2/
Attachment #8730708 - Flags: review?(cam)
Comment on attachment 8730709 [details]
MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40103/diff/1-2/
Attachment #8730709 - Flags: review?(cam)
Comment on attachment 8730710 [details]
MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40105/diff/1-2/
Attachment #8730710 - Flags: review?(hiikezoe)
Comment on attachment 8730711 [details]
MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40107/diff/1-2/
Attachment #8730711 - Flags: review?(cam)
Comment on attachment 8730712 [details]
MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40109/diff/1-2/
Attachment #8730712 - Flags: review?(cam)
Attachment #8730707 - Flags: review?(hiikezoe) → review+
Comment on attachment 8730707 [details]
MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro

https://reviewboard.mozilla.org/r/40099/#review36817
Comment on attachment 8730706 [details]
MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam

https://reviewboard.mozilla.org/r/40097/#review36829
Attachment #8730706 - Flags: review?(cam) → review+
Comment on attachment 8730708 [details]
MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz

https://reviewboard.mozilla.org/r/40101/#review36835

r=me though I'm not a big fan of including words like "Class", "Struct" and "Enum" in identifiers.  (Maybe "Details" or something?)
Attachment #8730708 - Flags: review?(cam) → review+
Comment on attachment 8730709 [details]
MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz

https://reviewboard.mozilla.org/r/40103/#review36839
Attachment #8730709 - Flags: review?(cam) → review+
Comment on attachment 8730710 [details]
MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro

https://reviewboard.mozilla.org/r/40105/#review36845

I'd like to see a test case for this.
Attachment #8730710 - Flags: review?(hiikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> https://reviewboard.mozilla.org/r/40097/#review36809
> 
> ::: dom/animation/KeyframeEffect.cpp:598
> (Diff revision 1)
> > +    if (segment->mToKey == segment->mFromKey) {
> > +      if (computedTiming.mProgress.Value() < 0) {
> > +        *val = segment->mFromValue;
> > +      } else {
> > +        *val = segment->mToValue;
> > +      }
> > +      return;
> > +    }
> 
> I guess you might want to conitnue here to iterate other CSS properties.

Yes, good catch!
Don't forget to get a DOM peer's review on the patches with .webidl changes (even though they're ChromeOnly).
Comment on attachment 8730711 [details]
MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam

https://reviewboard.mozilla.org/r/40107/#review36843

::: dom/animation/KeyframeEffect.cpp:1811
(Diff revision 2)
> +                    const StyleAnimationValue& aValue,
> +                    AnimationPropertyValueDictionary& aResult)
> +{
> +  aResult.mOffset.Construct(aOffset);
> +
> +  nsAutoString stringValue;

If you use nsString, will the call to Construct be able to share the string rather than being forced to copy it from the stack?

::: dom/animation/KeyframeEffect.cpp:1863
(Diff revision 2)
> +      propertyDictionary.mValues.Value().AppendElement(fromValue,
> +                                                       mozilla::fallible);

Not sure whether it's better to silently fail on OOM here or to do something explicit.
Attachment #8730711 - Flags: review?(cam) → review+
Comment on attachment 8730712 [details]
MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam

https://reviewboard.mozilla.org/r/40109/#review36857
Attachment #8730712 - Flags: review?(cam) → review+
Comment on attachment 8730706 [details]
MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40097/diff/2-3/
Attachment #8730706 - Attachment description: MozReview Request: Bug 1254419 - Fix zero-length segment handling → MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam
Comment on attachment 8730707 [details]
MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40099/diff/2-3/
Attachment #8730707 - Attachment description: MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames → MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro
Comment on attachment 8730708 [details]
MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40101/diff/2-3/
Attachment #8730708 - Attachment description: MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties() → MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam
Attachment #8730708 - Flags: review?(bzbarsky)
Comment on attachment 8730709 [details]
MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40103/diff/2-3/
Attachment #8730709 - Attachment description: MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDictionary → MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDictionary; r=heycam
Attachment #8730709 - Flags: review?(bzbarsky)
Comment on attachment 8730710 [details]
MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40105/diff/2-3/
Attachment #8730710 - Attachment description: MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden → MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro
Comment on attachment 8730711 [details]
MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40107/diff/2-3/
Attachment #8730711 - Attachment description: MozReview Request: Bug 1254419 - Fill in values sequence in getProperties() → MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam
Comment on attachment 8730712 [details]
MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40109/diff/2-3/
Attachment #8730712 - Attachment description: MozReview Request: Bug 1254419 - Add tests for getProperties() → MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam
Attachment #8730708 - Flags: review?(bzbarsky) → review+
Comment on attachment 8730708 [details]
MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz

https://reviewboard.mozilla.org/r/40101/#review37011

r=me
Comment on attachment 8730709 [details]
MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz

https://reviewboard.mozilla.org/r/40103/#review37013

r=me assuming it's expected that the member will be missing in some cases.  If it will always be present, just make it a required member.
Attachment #8730709 - Flags: review?(bzbarsky) → review+
Comment on attachment 8730706 [details]
MozReview Request: Bug 1254419 - Fix zero-length segment handling; r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40097/diff/3-4/
Comment on attachment 8730707 [details]
MozReview Request: Bug 1254419 - Move GetPropertyState alongside GetFrames; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40099/diff/3-4/
Comment on attachment 8730708 [details]
MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40101/diff/3-4/
Attachment #8730708 - Attachment description: MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam → MozReview Request: Bug 1254419 - Rename getPropertyState() to getProperties(); r=heycam, r=bz
Comment on attachment 8730709 [details]
MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40103/diff/3-4/
Attachment #8730709 - Attachment description: MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDictionary; r=heycam → MozReview Request: Bug 1254419 - Add values member to AnimationPropertyDetails; r=heycam, r=bz
Comment on attachment 8730710 [details]
MozReview Request: Bug 1254419 - Return animation property information from getProperties() even if the property is overridden; r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40105/diff/3-4/
Comment on attachment 8730711 [details]
MozReview Request: Bug 1254419 - Fill in values sequence in getProperties(); r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40107/diff/3-4/
Comment on attachment 8730712 [details]
MozReview Request: Bug 1254419 - Add tests for getProperties(); r=heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40109/diff/3-4/
At the same time we also make the 'warning' member of AnimationPropertyDetails
no longer nullable and simply use the absence of the member to indicate "no
warning" (which is what we were already doing -- we were never actually setting
it to null).

Review commit: https://reviewboard.mozilla.org/r/40675/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40675/
Attachment #8731517 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8730709 [details]
> MozReview Request: Bug 1254419 - Add values member to
> AnimationPropertyDictionary; r=heycam
> 
> https://reviewboard.mozilla.org/r/40103/#review37013
> 
> r=me assuming it's expected that the member will be missing in some cases. 
> If it will always be present, just make it a required member.

I didn't realise we used required for dictionary objects we're returning but I can see it simplifies the code and documents the expected output better. I've updated all the members of these two dictionaries accordingly in a follow-up patch.
Comment on attachment 8731517 [details]
MozReview Request: Bug 1254419 - Make always-set members of AnimationProperty(Value)Details required

https://reviewboard.mozilla.org/r/40675/#review37193

::: dom/animation/KeyframeEffect.cpp:1843
(Diff revision 1)
>      }
>  
> -    propertyDetails.mValues.Construct();
> +    if (!propertyDetails.mValues.SetCapacity(
> -    if (!propertyDetails.mValues.Value().SetCapacity(
>            property.mSegments.Length(), mozilla::fallible)) {
>        MOZ_CRASH("Out of memory allocating values array");

Why bother passing mozilla::fallible if you plan to MOZ_CRASH anyway?  Either don't pass it or handle the failure without crashing, I'd think...  (yes, I know, preexisting, probably fodder for a separate bug).

r=me
Attachment #8731517 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #43)
> Comment on attachment 8731517 [details]
> MozReview Request: Bug 1254419 - Make always-set members of
> AnimationProperty(Value)Details required
> 
> https://reviewboard.mozilla.org/r/40675/#review37193
> 
> ::: dom/animation/KeyframeEffect.cpp:1843
> (Diff revision 1)
> >      }
> >  
> > -    propertyDetails.mValues.Construct();
> > +    if (!propertyDetails.mValues.SetCapacity(
> > -    if (!propertyDetails.mValues.Value().SetCapacity(
> >            property.mSegments.Length(), mozilla::fallible)) {
> >        MOZ_CRASH("Out of memory allocating values array");
> 
> Why bother passing mozilla::fallible if you plan to MOZ_CRASH anyway? 
> Either don't pass it or handle the failure without crashing, I'd think... 
> (yes, I know, preexisting, probably fodder for a separate bug).

I'm probably missing something but mValues here is a Sequence<> which inherits from FallibleTArray so I think we have to pass mozilla::fallible here. It doesn't seem worth making this method throw just to handle this more gracefully?
Oh, it's a Sequence, OK.

If the list of things we're adding to it is not under page control, this is fine, but could use a comment.  If it's under page control, we should probably handle this without crashing.  Doesn't have to throw, could just pretend like the list is empty or something if that would be simpler....
Comment on attachment 8731548 [details]
MozReview Request: Bug 1254419 - Throw if we fail to allocate memory for a values array in getProperties()

https://reviewboard.mozilla.org/r/40699/#review37231

::: dom/animation/KeyframeEffect.cpp:1844
(Diff revision 1)
>        propertyDetails.mWarning.Construct(localizedString);
>      }
>  
> -    if (!propertyDetails.mValues.SetCapacity(
> -          property.mSegments.Length(), mozilla::fallible)) {
> -      MOZ_CRASH("Out of memory allocating values array");
> +    if (!propertyDetails.mValues.SetCapacity(property.mSegments.Length(),
> +                                             mozilla::fallible)) {
> +      aRv.Throw(NS_ERROR_FAILURE);

NS_ERROR_OUT_OF_MEMORY, please.

::: dom/animation/KeyframeEffect.cpp:1845
(Diff revision 1)
>      }
>  
> -    if (!propertyDetails.mValues.SetCapacity(
> -          property.mSegments.Length(), mozilla::fallible)) {
> -      MOZ_CRASH("Out of memory allocating values array");
> +    if (!propertyDetails.mValues.SetCapacity(property.mSegments.Length(),
> +                                             mozilla::fallible)) {
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      aProperties.Clear();

No need to do that.  It'll get ignored by the bindings anyway, and clear itself in its destructor.

r=me
Attachment #8731548 - Flags: review?(bzbarsky) → review+
Blocks: 1257874
You need to log in before you can comment on or make changes to this bug.