1196114 - Animation.IsRunningOnCompositor should indicate its state for each properties.

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

From bug 1151694#c14.

> Created attachment 8649670 [details] [diff] [review] [diff] [review]
> Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly.
> 
> KeyframeEffectReadonly stores the flag for each property respectively.
> This patch adds a test case that animation has two properties.
> One can be run on the compositor, the other can not be.
> 
> One thing is not clear to me is that mIsRunningOnCompositor should be false
> when the animation is running on both of threads?

We should update the WebIDL to expose which properties are running on the compositor. I'm not quite sure how that should look. Thinking out loud:

// This is a really bad name, I'm not sure what to call it
enum RunningStatus {
  "compositor",
  "main-thread"
  // Eventually we could extend this to indicate *why* it's not running
  // on the compositor (e.g. layer too large, layer too small, property
  // affects layout etc.)
};

dictionary AnimationPropertyStatus {
  DOMString property;
  RunningStatus runningStatus;
  // Later we might like to add a member like 'bool isOverridden' to indicate
  // if the property is being overridden by another animation (e.g. CSS animations
  // override CSS transitions and in future script-generated animations will
  // override).
};

partial interface KeyframeEffectReadOnly {
  // Not sure if this should be a FrozenArray or not
  [ChromeOnly] sequence<AnimationProperty> getPropertyStatus();
};

For the time being, I suspect IsRunningOnCompositor should return true if *any* of the properties are running on the compositor.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> From bug 1151694#c14.
> 
> > Created attachment 8649670 [details] [diff] [review]
> > Part 1: Move mIsRunningOnCompositor flag to KeyframeEffectReadOnly.
> > 
> > KeyframeEffectReadonly stores the flag for each property respectively.
> > This patch adds a test case that animation has two properties.
> > One can be run on the compositor, the other can not be.
> > 
> > One thing is not clear to me is that mIsRunningOnCompositor should be false
> > when the animation is running on both of threads?
> 
> We should update the WebIDL to expose which properties are running on the
> compositor. I'm not quite sure how that should look. Thinking out loud:
> 
> // This is a really bad name, I'm not sure what to call it
> enum RunningStatus {
>   "compositor",
>   "main-thread"

Once bug 1166500 is fixed, I'd add 'hidden' or 'out-of-view' here.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed webidl

How about this?

Changes from comment#0.

* propertyStatus is a readonly attribute instead of function.
* propertyStatus is a frozen array since caller should not change the object.
* RunningStatus has 'Animation' prefix since it is exposed in mozilla::dom namespace.
* AnimationRunningStatus is a dictionary since it will have 'reason' field which describes the reason why the animation property is not running on the compositor.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8700469 [details] [diff] [review]
Proposed webidl

Olli, could you please take a look this webidl?
We'd like to expose the information which animation property is running on the compositor respectively for devtools.

Comment 4

[Brian Birtles (:birtles)]

Comment on attachment 8700469 [details] [diff] [review]
Proposed webidl

>+enum AnimationRunningThread {
>+  "compositor",
>+  "main-thread"
>+};

We shouldn't call this "thread" since on B2G the compositor is a different process.

Also, I think you probably need a "not running" value or make the member Nullable.

AnimationRunningLocation?

>+dictionary AnimationRunningStatus {
>+  AnimationRunningThread thread;

s/thread/where/ ?

>+  // DOMString reason = "";

I guess this should be an enum so we can localize the strings.

>+dictionary AnimationPropertyStatus {
>+  DOMString property;
>+  AnimationRunningStatus runningStatus;
>+  // bool isOverridden

This would probably be useful, but should be an enum since in future there might be different states (e.g. partially overridden once we do addition and are interpolating between a two higher-priority values one of which is additive).

>+partial interface KeyframeEffectReadOnly {
>+  [ChromeOnly, Cached, Frozen, Affects=Nothing]
>+  readonly attribute sequence<AnimationPropertyStatus> propertyStatus;
>+};

You can't have a sequence attribute. You need a FrozenArray for that. What's wrong with a method that returns a sequence, anyway? Using cached and having to clear it away every time this changes seems unnecessary?

I don't really know how this should look. I guess it's not worth doing the whole maplike setup for this. An array you have to iterate over is probably fine.

'propertyStatus' seems a bit weird. It's more like 'runningStatus' but where the result happens to be per-property.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> Comment on attachment 8700469 [details] [diff] [review]
> Proposed webidl
> 
> >+enum AnimationRunningThread {
> >+  "compositor",
> >+  "main-thread"
> >+};
> 
> We shouldn't call this "thread" since on B2G the compositor is a different
> process.
> 
> Also, I think you probably need a "not running" value or make the member
> Nullable.

Thanks! Exactly! Nullable looks quite reasonable to me.

> AnimationRunningLocation?
> >+dictionary AnimationRunningStatus {
> >+  AnimationRunningThread thread;
> 
> s/thread/where/ ?

Nice names!

> >+  // DOMString reason = "";
> 
> I guess this should be an enum so we can localize the strings.

Thanks. I did not think of localization at all.

> >+dictionary AnimationPropertyStatus {
> >+  DOMString property;
> >+  AnimationRunningStatus runningStatus;
> >+  // bool isOverridden
> 
> This would probably be useful, but should be an enum since in future there
> might be different states (e.g. partially overridden once we do addition and
> are interpolating between a two higher-priority values one of which is
> additive).

I don't actually know the flag can be used, so I will remove the flag here.

> >+partial interface KeyframeEffectReadOnly {
> >+  [ChromeOnly, Cached, Frozen, Affects=Nothing]
> >+  readonly attribute sequence<AnimationPropertyStatus> propertyStatus;
> >+};
> 
> You can't have a sequence attribute. You need a FrozenArray for that. What's
> wrong with a method that returns a sequence, anyway? Using cached and having
> to clear it away every time this changes seems unnecessary?
> I don't really know how this should look. I guess it's not worth doing the
> whole maplike setup for this. An array you have to iterate over is probably
> fine.

Then, how about entires() like this?

 var animation = div.getAnimations()[0];
 for (var [property, status] of animation.effect.entries()) {
   status.where; // "compositor" or "main-thread"
   property; // CSS property name
 }

Or properties()?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8700469 [details] [diff] [review]
Proposed webidl

Clearing feedback request to Olli for now.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8700469 [details] [diff] [review]
Proposed webidl

I wonder why Affects=Nothing and why not just Constant?
You anyway cache the frozen array.

But a method returning the sequence might be nicer.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thanks Olli for taking your precious time.

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8700469 [details] [diff] [review]
> Proposed webidl
> 
> I wonder why Affects=Nothing and why not just Constant?
> You anyway cache the frozen array.

That's because the running status will change depending on window size or web animations APIs.

> But a method returning the sequence might be nicer.

I will use method here. Thanks!

Comment 9

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > You can't have a sequence attribute. You need a FrozenArray for that. What's
> > wrong with a method that returns a sequence, anyway? Using cached and having
> > to clear it away every time this changes seems unnecessary?
> > I don't really know how this should look. I guess it's not worth doing the
> > whole maplike setup for this. An array you have to iterate over is probably
> > fine.
> 
> Then, how about entires() like this?
> 
>  var animation = div.getAnimations()[0];
>  for (var [property, status] of animation.effect.entries()) {
>    status.where; // "compositor" or "main-thread"
>    property; // CSS property name
>  }
> 
> Or properties()?

runningStatus() => sequence<AnimationPropertyStatus>

dictionary AnimationPropertyStatus {
  DOMString property;
  bool runningOnCompositor;
  Nullable<AnimationPerformanceWarning> warning;
};

enum AnimationPerformanceWarning {
  "property-cannot-be-animated-on-compositor",
  "content-too-large-to-layerize",
  "content-too-small-to-layerize",
  "cannot-animate-preserve3d-on-compositor",
  "cannot-animate-when-svg-transform-is-in-use",
  "geometry-property-in-use"
  ...
};

I'm not sure about this enum, actually. I'm not sure if it's odd to have to change the WebIDL every time one of these reasons changes. Also, if we're doing remote debugging with DevTools then we might be targeting a version of Firefox with a different set of enum values. Maybe it would make sense to just use a DOMString in English and have DevTools recognize the strings and localize them? Then if there was a mismatch the user would just see the English string?

Also, for some messages we might want to pass parameters such as the maximum layer size and the content size?

Would something like the following work?

dictionary AnimationPerformanceWarning {
  AnimationPerformanceWarningType type; // enum value for localization
  DOMString message; // a default string in English to use when the enum value isn't recognized
  sequence<DOMString> parameters; // extra values to substitute when localizing the string
};

AnimationPerformanceWarningType {
  "property-cannot-animate-on-compositor",
  "content-too-large",
  "content-too-small",
  ...
};

e.g. If the content was too large for layerization you'd get

 anim.effect.runningStatus();
 => [ { property: 'transform',
        runningOnCompositor: false,
        warning: { type: "content-too-large",
                   message: "Frame size (800 x 1200) is bigger than the viewport (800 x 1000)",
                   parameters: [ "800", "1200", "800", "1000" ] }
      } ]

Maybe that's going over the top?

Patrick, do you have any other APIs where you're doing this sort of thing? Does the above seem right?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> enum AnimationPerformanceWarning {
>   "property-cannot-be-animated-on-compositor",
>   "content-too-large-to-layerize",
>   "content-too-small-to-layerize",
>   "cannot-animate-preserve3d-on-compositor",
>   "cannot-animate-when-svg-transform-is-in-use",
>   "geometry-property-in-use"
>   ...
> };
> 
> I'm not sure about this enum, actually. I'm not sure if it's odd to have to
> change the WebIDL every time one of these reasons changes. Also, if we're
> doing remote debugging with DevTools then we might be targeting a version of
> Firefox with a different set of enum values. Maybe it would make sense to
> just use a DOMString in English and have DevTools recognize the strings and
> localize them? Then if there was a mismatch the user would just see the
> English string?

We can't pass localized strings directly? I.e., localizing messages with nsIStringBundle in C++, then passing them to JS?

Comment 11

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> (In reply to Brian Birtles (:birtles) from comment #9)
> > enum AnimationPerformanceWarning {
> >   "property-cannot-be-animated-on-compositor",
> >   "content-too-large-to-layerize",
> >   "content-too-small-to-layerize",
> >   "cannot-animate-preserve3d-on-compositor",
> >   "cannot-animate-when-svg-transform-is-in-use",
> >   "geometry-property-in-use"
> >   ...
> > };
> > 
> > I'm not sure about this enum, actually. I'm not sure if it's odd to have to
> > change the WebIDL every time one of these reasons changes. Also, if we're
> > doing remote debugging with DevTools then we might be targeting a version of
> > Firefox with a different set of enum values. Maybe it would make sense to
> > just use a DOMString in English and have DevTools recognize the strings and
> > localize them? Then if there was a mismatch the user would just see the
> > English string?
> 
> We can't pass localized strings directly? I.e., localizing messages with
> nsIStringBundle in C++, then passing them to JS?

Maybe that's what we should do for the 'message' field. But I think if we're debugging remotely, we want to use the language of the client?

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles) from comment #9)
> e.g. If the content was too large for layerization you'd get
> 
>  anim.effect.runningStatus();
>  => [ { property: 'transform',
>         runningOnCompositor: false,
>         warning: { type: "content-too-large",
>                    message: "Frame size (800 x 1200) is bigger than the
> viewport (800 x 1000)",
>                    parameters: [ "800", "1200", "800", "1000" ] }
>       } ]
> 
> Maybe that's going over the top?
> 
> Patrick, do you have any other APIs where you're doing this sort of thing?
> Does the above seem right?
Hm, that definitely looks like an API we could use in devtools, maybe a little bit over the top though, but I don't have enough experience with this to know for sure. We don't have many instances of similar things in our code.

I can think of 1 example: CSP warnings in the console. When you have a CSP security warning on a page, it gets displayed in the console, and that string is logged directly from CPP code: https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/security/csp.properties#8

I think it'd be enough for the runningStatus() method to return warnings as strings only as suggested in comment 10. Not sure the remote debugging use case is important enough to do another level of localization in devtools.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Thank you, Patrick.  After discussion with Brian, we have decided to return the warning message as string like this:

  anim.effect.runningStatus();
  => [ { property: 'transform',
         runningOnCompositor: false,
         warning: { 
           message: "Frame size (800 x 1200) is bigger than the
 viewport (800 x 1000)"
         }
       } ]

Comment 14

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d349a1a7321

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1196114 - Part 1: Add SetPerformanceWarning. r?birtles

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

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

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

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

Test cases for async transform animations of frames with SVG transforms
can not be written here because FrameLayerBuilder::GetDedicatedLayer fails
in such frame's case.

Message fixes:
* Insert a space in warning message related to geometric properties.
* Remove a period after 'bug 1186204'.

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

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

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

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8718658 [details]
MozReview Request: Bug 1196114 - Part 1: Add SetPerformanceWarning. r?birtles

https://reviewboard.mozilla.org/r/34673/#review31613

::: dom/animation/EffectCompositor.h:190
(Diff revision 1)
> +  static void SetPerformanceWarning(const nsIFrame* aFrame,

// Associates a warning string with the animated
// property on the specified frame indicating why,
// for example, the property could not be animated
// on the compositor.

::: dom/animation/EffectCompositor.h:192
(Diff revision 1)
> +                                    nsAString& aMessage);

Why is aMessage not a const ref?

::: dom/animation/KeyframeEffect.h:144
(Diff revision 1)
> +  nsString mWarningMessage;

This seems a little bit expensive? I guess we are effectively adding the following members to every animated property?

  size_type  mFixedCapacity;
  char_type* mFixedBuf;

Do you think this is reasonable? Or should be use a Maybe<> type?

::: dom/animation/KeyframeEffect.cpp:2097
(Diff revision 1)
> +  }

Should we add an assertion here if the property is not found? I don't think we expect to call this in that case do we?

Or is it possible we might?

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

https://reviewboard.mozilla.org/r/34675/#review31615

It looks pretty good to me but I'd like to have smaug/bs look at the API. In particular, I suspect we don't need the extra warning dictionary and I suspect we should rename runningStatus().

::: dom/animation/KeyframeEffect.h:312
(Diff revision 1)
> +  void RunningStatus(nsTArray<AnimationPropertyStatus>& aStatus);

Can we make this method const?

::: dom/animation/KeyframeEffect.cpp:1810
(Diff revision 1)
> +  for (AnimationProperty& property : mProperties) {

Can we make this a const ref?

::: dom/animation/KeyframeEffect.cpp:1817
(Diff revision 1)
> +      NS_ConvertUTF8toUTF16(nsCSSProps::GetStringValue(property.mProperty)));

I think we can use NS_ConvertASCIItoUTF16 since property names are always ASCII at this stage? Until we support custom properties?

::: dom/animation/test/chrome/test_running_status.html:1
(Diff revision 1)
> +<!doctype html>

We should rename this file if we rename the runningStatus() method.

::: dom/animation/test/chrome/test_running_status.html:44
(Diff revision 1)
> +    assert_equals(sortedActual[i].property,
> +                  sortedExpected[i].property,
> +                  'CSS property name should be ' + sortedExpected[i].property);
> +    assert_equals(sortedActual[i].runningOnCompositor,
> +                  sortedExpected[i].runningOnCompositor,
> +                  'runningOnCompositor property should be ' +
> +                  sortedExpected[i].runningOnCompositor);

I think assert_equals already prints the expected values so we could just make the message 'CSS property name should match' and likewise for runningOnCompositor?

::: dom/webidl/KeyframeEffect.webidl:52
(Diff revision 1)
> +  AnimationPerformanceWarning warning;

Why is this a separate dictionary instead of just being a (possibly nullable) DOMString?

I'm sure there's a reason but I can't remember it.

::: dom/webidl/KeyframeEffect.webidl:60
(Diff revision 1)
> +  [ChromeOnly] sequence<AnimationPropertyStatus> runningStatus();

How about getPropertyStatus()? getPropertyInfo()?

I'd like to get smaug or bz's feedback about this whole API.

Comment 21

[Brian Birtles (:birtles)]

Comment on attachment 8718660 [details]
MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

https://reviewboard.mozilla.org/r/34677/#review31617

> Test cases for async transform animations of frames with SVG transforms
> can not be written here because FrameLayerBuilder::GetDedicatedLayer fails
> in such frame's case.

Can you explain this further?

::: dom/animation/KeyframeEffect.h:329
(Diff revision 1)
> +  // Whenever returning true, |aOutPerformanceWarning| stores the reason why

Nit: "When returning true, ..."

::: dom/animation/KeyframeEffect.h:332
(Diff revision 1)
> +                                       nsAString& aOutPerformanceWarning) const;

Just aPerformanceWarning? Do we normally name out-params aOut...?

::: dom/animation/KeyframeEffect.h:389
(Diff revision 1)
> -  // animations for |aFrame|. Any limitations that are encountered are
> +  // animations for |aFrame|. The reason of the limitation is stored in

Nit: "The reason for the limitation..."

::: dom/animation/KeyframeEffect.h:389
(Diff revision 1)
> -  // animations for |aFrame|. Any limitations that are encountered are
> +  // animations for |aFrame|. The reason of the limitation is stored in

When returning true, the reason for the limitation...

::: dom/animation/KeyframeEffect.h:391
(Diff revision 1)
> -  // If |aContent| is nullptr, no logging is performed
> +  bool CanAnimateTransformOnCompositor(const nsIFrame* aFrame,

Why do we make this no longer static? I don't think it accesses any member variables?

::: dom/animation/test/chrome/test_running_status.html:55
(Diff revision 1)
> +                    sortedExpected[i].warning.message + '"');

Again, I think assert_equals will print the actual and expected messages for us.

::: dom/animation/test/chrome/test_running_status.html:220
(Diff revision 1)
> +    // viewport depends on test environemt.

Nit: environment

Looks good but I'd like to take another look later since I think we might have to rework this a bit based on changes to part 2.

Comment 22

[Brian Birtles (:birtles)]

Comment on attachment 8718661 [details]
MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

https://reviewboard.mozilla.org/r/34679/#review31619

This looks good to me but I'm not familiar enough with this part of our code (localizing strings) so I think someone else should review this part to make sure we're using this API correctly.

::: dom/locales/en-US/chrome/layout/layout_errors.properties:14
(Diff revision 1)
> +AnimationWarningBackfaceVisibilityHidden=Gecko bug: Async animation of 'backface-visibility: hidden' transforms is not supported.  See bug 1186204

Can we drop the "Gecko bug" part and just make this:

  Async animation of 'backface-visibility: hidden' transforms is not supported. See bug 1186204.

(I actually suspect we don't even need the bug number. It might be enough to just put that in the source code.)

I'm sure there is someone else who is more qualified to comment on this than me, however.

::: dom/locales/en-US/chrome/layout/layout_errors.properties:15
(Diff revision 1)
> +AnimationWarningContentTooLarge=Performance warning: Async animation disabled because frame size (%1$S, %2$S) is bigger than the viewport (%3$S, %4$S) or the visual rectangle (%5$S, %6$S) is larger than the max allowable value (%7$S)

Can we drop the "Performance warning: " prefix? I think DevTools can add this if they want (since all these errors are, in fact, performance warnings of some kind).

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

Olli, we'd like to get your thoughts on this WebIDL change.  Please see Brian's comment in comment#20.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

AnimationPerformanceWarning doesn't seem to bring in anything new to the API.
Perhaps AnimationPerformanceWarning.warning could be just a nullable DOMString.

First I didn't like the suggested getPropertyStatus, but it feels better than runningStatus.
And get* prefix is a hint that a new object is being returned.

Though, now I started to wonder whether it should be 'State' and not 'Status', and the dictionary then AnimationPropertyState. The title of this bug is after all about state, some comments were also talking about states, not statuses.
We have "readyState" and such in the web platform, but less "status". But I'm not a native English speaker.
getPropertyState() ?

So, I don't have too good ideas for this.
This all doesn't matter very much, given that the API is exposed to [ChromeOnly].

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Thanks a lot, Olli!  I am going to change AnimationPerformanceWarning as just a nullable DOMString because I can't remember the reason why I did use another dictionary there.
getPropertyState() is pretty reasonable to me.  Brian, any thoughts from a native English speaker points of view?

Comment 26

[Brian Birtles (:birtles)]

I think either getPropertyState() is fine. The other changes sound fine.

One other idea I was wondering about was whether we should return a MozMap instead?

Then, if you wanted to find out if the 'transform' property was running on the compositor, I think you could write:

  let transformState = anim.effect.getPropertyState().transform;
  if (transformState && transformState.isRunninOnCompositor) {
    ...
  }

Is that better? I haven't used MozMaps before so I don't know if there are any drawbacks but it seems like an easier API than searching through the returned sequence?

Then again, maybe DevTools will always iterate through the whole array anyway so maybe it doesn't matter.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Wow.  MozMaps is pretty attractive to me, if we can access properties by both way:
getPropertyState().backgroundColor and getPropertyState()['background-color'].  Just adding two different keys into MozMap?  I will check it.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/34679/#review31619

I am going to ask Axel to review this.  Axel, could you please take time to reviewing this?  We are going to pass some localized messages to devtools though WebIDL.

> Can we drop the "Gecko bug" part and just make this:
> 
>   Async animation of 'backface-visibility: hidden' transforms is not supported. See bug 1186204.
> 
> (I actually suspect we don't even need the bug number. It might be enough to just put that in the source code.)
> 
> I'm sure there is someone else who is more qualified to comment on this than me, however.

Dropped "Gecko bug" prefix and the bug numbers. 

I am going find someone to check the messages from now on.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718658 [details]
MozReview Request: Bug 1196114 - Part 1: Add SetPerformanceWarning. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34673/diff/1-2/

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34675/diff/1-2/

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718660 [details]
MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34677/diff/1-2/

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718661 [details]
MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34679/diff/1-2/

Comment 33

[Francesco Lodolo [:flod]]

https://reviewboard.mozilla.org/r/34679/#review32251

Left a few comments (I realize I have no clue how to steal a review flag on MozReview).

Besides missing localization notes, there's nothing wrong with the strings from a localization point of view. 

Just to avoid any misunderstanding, l10n feedback is not feedback on the copy
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Do_I_need_l10n_feedback

::: dom/locales/en-US/chrome/layout/layout_errors.properties:14
(Diff revision 2)
> +AnimationWarningBackfaceVisibilityHidden=Async animation of 'backface-visibility: hidden' transforms is not supported

I would add a localization note before this group of strings to explain that text included between quotes should not be localized, since they're code fragment.

I also wonder about consistency: these strings don't end with a full stop, the existing ones before. Are they used in the same context?

::: dom/locales/en-US/chrome/layout/layout_errors.properties:15
(Diff revision 2)
> +AnimationWarningContentTooLarge=Async animation disabled because frame size (%1$S, %2$S) is bigger than the viewport (%3$S, %4$S) or the visual rectangle (%5$S, %6$S) is larger than the max allowable value (%7$S)

Please add a note explaining what these variables will be replaced with.

"Allowed" instead of "allowable"?

Comment 34

[Francesco Lodolo [:flod]]

Comment on attachment 8718661 [details]
MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

Oh wait, I can do it from the old Bugzilla :-)

As explained above, strings look OK, but please add a couple of localization notes.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Thank you, Franceso for your quickly feedback!  I am very surprised you suddenly appeared!

 (In reply to Francesco Lodolo [:flod] from comment #33)
> https://reviewboard.mozilla.org/r/34679/#review32251
> 
> Left a few comments (I realize I have no clue how to steal a review flag on
> MozReview).
> 
> Besides missing localization notes, there's nothing wrong with the strings
> from a localization point of view. 
> 
> Just to avoid any misunderstanding, l10n feedback is not feedback on the copy
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#Do_I_need_l10n_feedback
> 
> ::: dom/locales/en-US/chrome/layout/layout_errors.properties:14
> (Diff revision 2)
> > +AnimationWarningBackfaceVisibilityHidden=Async animation of 'backface-visibility: hidden' transforms is not supported
> 
> I would add a localization note before this group of strings to explain that
> text included between quotes should not be localized, since they're code
> fragment.

Thanks. Will do.

> I also wonder about consistency: these strings don't end with a full stop,
> the existing ones before. Are they used in the same context?

It's intentional because the previous messages were output in user's console, I mean stdout, but now the messages are shown on somewhere in devtools window.

> ::: dom/locales/en-US/chrome/layout/layout_errors.properties:15
> (Diff revision 2)
> > +AnimationWarningContentTooLarge=Async animation disabled because frame size (%1$S, %2$S) is bigger than the viewport (%3$S, %4$S) or the visual rectangle (%5$S, %6$S) is larger than the max allowable value (%7$S)
> 
> Please add a note explaining what these variables will be replaced with.

Will do.

> "Allowed" instead of "allowable"?

I think it's "allowable" but I will ask someone who is familiar with this limitation.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Hello Milan,  I have a question about a message[1] you added in bug 1060431.

[1] https://hg.mozilla.org/mozilla-central/rev/c061af84898d#l1.39

The question is the word 'allowable' is preferable to 'allowed' or not, or either will be fine?
I think the 'allowable' implies that the restriction is caused by user devices, not Gecko.  Is this correct?
Thank you,

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

https://reviewboard.mozilla.org/r/34675/#review32273

r+ for the .webidl

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> Hello Milan,  I have a question about a message[1] you added in bug 1060431.
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/c061af84898d#l1.39
> 
> The question is the word 'allowable' is preferable to 'allowed' or not, or
> either will be fine?
> I think the 'allowable' implies that the restriction is caused by user
> devices, not Gecko.  Is this correct?
> Thank you,

When we get to the point where the only remaining Gecko problems are due to grammar, we'll be in great shape :)  I don't read "allowable" as "device restriction", rather than the 4k limit we set, but if it reads that way, I don't mind the change.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Thank you, Milan.  I will change the word to "allowed" then.

Comment 40

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> Comment on attachment 8718659 [details]
> MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus
> interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/34675/diff/1-2/

I did forget to mention about MozMap.  I thought it works as well as Map, but it doesn't yet (no iterator with key-value pairs).  I've decided to use sequence for now.

Comment 41

[Brian Birtles (:birtles)]

Comment on attachment 8718658 [details]
MozReview Request: Bug 1196114 - Part 1: Add SetPerformanceWarning. r?birtles

https://reviewboard.mozilla.org/r/34673/#review32417

::: dom/animation/EffectCompositor.h:190
(Diff revision 2)
> +  static void SetPerformanceWarning(const nsIFrame* aFrame,

This needs a comment explaining what it does.

::: dom/animation/KeyframeEffect.h:147
(Diff revision 2)
> +  Maybe<nsString> mWarningMessage;

Call this mPerformanceWarning?

::: dom/animation/KeyframeEffect.h:329
(Diff revision 2)
> +  // on the compositor.

Nit: The wrapping of this comment is a bit odd, seems to be too narrow.

::: dom/animation/KeyframeEffect.cpp:599
(Diff revision 2)
> +      if (aIsRunning) {

This needs a comment, e.g.

      // We currently only set a performance warning message when animations
      // cannot be run on the compositor, so if this animation is running on
      // the compositor we don't need a message.

::: dom/animation/KeyframeEffect.cpp:2134
(Diff revision 2)
> +}

Nit: Space after this }

Comment 42

[Brian Birtles (:birtles)]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

https://reviewboard.mozilla.org/r/34675/#review32423

::: dom/animation/KeyframeEffect.cpp:1847
(Diff revision 2)
> +    if (!property.mWinsInCascade) {

I think we should file a separate bug for exposing this information. For example, it would be useful in DevTools if a property that was overridden was greyed out. However, I'm not sure mWinsInCascade is completely accurate for that so this would have to be a separate bug.

Comment 43

[Brian Birtles (:birtles)]

Comment on attachment 8718660 [details]
MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

https://reviewboard.mozilla.org/r/34677/#review32419

This all looks good but I just want to be sure that the cost of making ShouldBlockCompositorAnimations etc. return a string isn't significant.

::: dom/animation/EffectCompositor.cpp:110
(Diff revision 2)
> -    if (effect->ShouldBlockCompositorAnimations(aFrame)) {
> +    nsAutoString performanceWarning;
> +    if (effect->ShouldBlockCompositorAnimations(aFrame,
> +                                                performanceWarning)) {

I'm a little bit concerned that we are now allocating a string (and, in the next patch, localizing it) even if we never use it. Do we call this method on every tick? Is there some way we can avoid this?

I'd like to have some idea of the cost of this.

::: dom/animation/test/chrome/test_animation_property_state.html:252
(Diff revision 2)
> +}, 'transform of nsIFrame with SVG transform');

Can we add some tests that performance warnings are cleared when the condition is resolved? I think we expect authors to see these warnings, then tweak their content until the issue is fixed.

So, a test like:

a) Test a transform animation on an SVG element runs on the compositor with no warning.
b) Then add a transform attribute to the element and check that it no longer runs on the compositor and the appropriate warning is set.
c) Then remove the transform attribute and check that the warning is cleared (and it runs on the compositor).

Ideally we should do that type of test for every type of warning, or at least do one test for the code path that sets the warning on the effect and one test for the code path that sets the warning on all effects on the frame.

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8718661 [details]
MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

https://reviewboard.mozilla.org/r/34679/#review32421

::: dom/animation/KeyframeEffect.cpp:2066
(Diff revision 2)
> -  // via HasOpacity().
> +  // via HasOpacity(). See bug bug 779598.

Nit: bug bug

Comment 45

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #43)
> Comment on attachment 8718660 [details]
> MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning
> messages. r?birtles
> 
> https://reviewboard.mozilla.org/r/34677/#review32419
> 
> This all looks good but I just want to be sure that the cost of making
> ShouldBlockCompositorAnimations etc. return a string isn't significant.
> 
> ::: dom/animation/EffectCompositor.cpp:110
> (Diff revision 2)
> > -    if (effect->ShouldBlockCompositorAnimations(aFrame)) {
> > +    nsAutoString performanceWarning;
> > +    if (effect->ShouldBlockCompositorAnimations(aFrame,
> > +                                                performanceWarning)) {
> 
> I'm a little bit concerned that we are now allocating a string (and, in the
> next patch, localizing it) even if we never use it. Do we call this method
> on every tick? Is there some way we can avoid this?
> 
> I'd like to have some idea of the cost of this.

The most cumbersome thing to achieve it is the message generated in nsDisplayTransform::ShouldPrerenderTransformedContent.  It needs additional parameters which represent various size information.  I did initially try to confine all messages in KeyframEffectReadOnly class.  But unfortunately a change [1] has made nsContentUtils::FormatLocalizedString with parameter's length private.  So I gave up confining the messages in KeyframEffectReadOnly.  Yes, we can at least confine other messages and I will figure out how to solve it a little more.



[1] https://hg.mozilla.org/mozilla-central/rev/bd1bb076db6e#l1.12

Comment 46

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> (In reply to Brian Birtles (:birtles) from comment #43)
> > Comment on attachment 8718660 [details]
> > MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning
> > messages. r?birtles
> > 
> > https://reviewboard.mozilla.org/r/34677/#review32419
> > 
> > This all looks good but I just want to be sure that the cost of making
> > ShouldBlockCompositorAnimations etc. return a string isn't significant.
> > 
> > ::: dom/animation/EffectCompositor.cpp:110
> > (Diff revision 2)
> > > -    if (effect->ShouldBlockCompositorAnimations(aFrame)) {
> > > +    nsAutoString performanceWarning;
> > > +    if (effect->ShouldBlockCompositorAnimations(aFrame,
> > > +                                                performanceWarning)) {
> > 
> > I'm a little bit concerned that we are now allocating a string (and, in the
> > next patch, localizing it) even if we never use it. Do we call this method
> > on every tick? Is there some way we can avoid this?
> > 
> > I'd like to have some idea of the cost of this.
> 
> The most cumbersome thing to achieve it is the message generated in
> nsDisplayTransform::ShouldPrerenderTransformedContent.  It needs additional
> parameters which represent various size information.  I did initially try to
> confine all messages in KeyframEffectReadOnly class.  But unfortunately a
> change [1] has made nsContentUtils::FormatLocalizedString with parameter's
> length private.  So I gave up confining the messages in
> KeyframEffectReadOnly.  Yes, we can at least confine other messages and I
> will figure out how to solve it a little more.

Thanks. I'm not saying you have to change this, I just want to understand how expensive this change is.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

I think it is actually expensive that is better to avoid as far as possible.  It's worth trying, I think.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #43)
> 
> Can we add some tests that performance warnings are cleared when the
> condition is resolved? I think we expect authors to see these warnings, then
> tweak their content until the issue is fixed.
> 
> So, a test like:
> 
> a) Test a transform animation on an SVG element runs on the compositor with
> no warning.
> b) Then add a transform attribute to the element and check that it no longer
> runs on the compositor and the appropriate warning is set.
> c) Then remove the transform attribute and check that the warning is cleared
> (and it runs on the compositor).
> 
> Ideally we should do that type of test for every type of warning, or at
> least do one test for the code path that sets the warning on the effect and
> one test for the code path that sets the warning on all effects on the frame.

Thanks for really great suggestion! I am just writing those tests now.  And it reveals a new bug, bug 1253164!

Comment 49

[Hiroyuki Ikezoe (:hiro)]

I am going to upload an additional patch to store warning information as a struct named AnimationPerformanceWarning for avoid allocations of message string.  With this additional patch, each warning message is generated only when getPropertyState() is called.  I did try to split this patch into smaller pieces but stopped doing it because the smaller pieces became ambiguous for the purpose.

Comment 50

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/34673/#review31613

> This seems a little bit expensive? I guess we are effectively adding the following members to every animated property?
> 
>   size_type  mFixedCapacity;
>   char_type* mFixedBuf;
> 
> Do you think this is reasonable? Or should be use a Maybe<> type?

I've decided to use Maybe<> here.

> Should we add an assertion here if the property is not found? I don't think we expect to call this in that case do we?
> 
> Or is it possible we might?

We can't add the assertion there because EffectCompositor::SetPerformanceWarning is called for nsIFrame and unfortunately not all the KeyframeEffects associated with the nsIFrame have the CSS property.

Comment 51

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/34675/#review32423

> I think we should file a separate bug for exposing this information. For example, it would be useful in DevTools if a property that was overridden was greyed out. However, I'm not sure mWinsInCascade is completely accurate for that so this would have to be a separate bug.

Filed bug 1252730.

Comment 52

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/34677/#review31617

When I did try it first, I may not add any element inside the SVG.  This patch now contains the SVG case.  And now I realized I should have fixed the commit message.  I will revise it later.

Comment 53

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/34679/#review31619

Please check that the usage of localized string and APIs is correct

Comment 54

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718658 [details]
MozReview Request: Bug 1196114 - Part 1: Add SetPerformanceWarning. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34673/diff/2-3/

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34675/diff/2-3/

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718660 [details]
MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34677/diff/2-3/

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718661 [details]
MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34679/diff/2-3/

Comment 58

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1196114 - Part 5: Store performce warning information as enum type. r?birtles

The warning messages are generated only when getPropertyState() is called.

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

Comment 59

[Brian Birtles (:birtles)]

Comment on attachment 8718660 [details]
MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

https://reviewboard.mozilla.org/r/34677/#review34559

::: dom/animation/test/chrome/test_animation_property_state.html:62
(Diff revisions 2 - 3)
> +// Force to check that the animation is runnion on compositor and

s/runnion/running/

Also, I'm not sure what "Force to check" means. Perhaps just "Check"?

::: dom/animation/test/chrome/test_animation_property_state.html:64
(Diff revisions 2 - 3)
> +function assert_property_state_on_compositor(actual, expected) {

I don't quite understand why we need this. It seems very similar to assert_animation_property_state_equals and it seems like we don't use this function in some places where we could?

Could we just modify assert_animation_property_state_equals a little and re-use it? (e.g. if the property is not expected to be running on the compositor also check that the 'warning' property does not exist?)

::: dom/animation/test/chrome/test_animation_property_state.html:74
(Diff revisions 2 - 3)
> +    assert_equals(sortedActual[i].runningOnCompositor,
> +                  true,
> +                  'runningOnCompositor property should be true');

Nit: I think we can use assert_true here.

::: dom/animation/test/chrome/test_animation_property_state.html:226
(Diff revisions 2 - 3)
> +        div.style = '';
> +        return waitForFrame();

Nit: I think these two lines are indented too far.

Comment 60

[Brian Birtles (:birtles)]

Comment on attachment 8726121 [details]
MozReview Request: Bug 1196114 - Part 5: Store performce warning information as enum type. r?birtles

Can we use std::initializer_list for this?

We have a cross-platform wrapper now: https://dxr.mozilla.org/mozilla-central/source/mfbt/InitializerList.h

That way instead of doing:

  const int32_t params[7]  = {
      nsPresContext::AppUnitsToIntCSSPixels(frameSize.width),
      nsPresContext::AppUnitsToIntCSSPixels(frameSize.height),
      nsPresContext::AppUnitsToIntCSSPixels(refSize.width),
      nsPresContext::AppUnitsToIntCSSPixels(refSize.height),
      nsPresContext::AppUnitsToIntCSSPixels(visual.width),
      nsPresContext::AppUnitsToIntCSSPixels(visual.height),
      nsPresContext::AppUnitsToIntCSSPixels(maxInAppUnits)
  };

  EffectCompositor::SetPerformanceWarningWithParams(
      aFrame, eCSSProperty_transform,
      AnimationPerformanceWarning::Type::ContentTooLarge,
      params);

Presumably we could just do:

  EffectCompositor::SetPerformanceWarningWithParams(
      aFrame, eCSSProperty_transform,
      AnimationPerformanceWarning::Type::ContentTooLarge,
      { nsPresContext::AppUnitsToIntCSSPixels(frameSize.width),
        nsPresContext::AppUnitsToIntCSSPixels(frameSize.height),
        nsPresContext::AppUnitsToIntCSSPixels(refSize.width),
        nsPresContext::AppUnitsToIntCSSPixels(refSize.height),
        nsPresContext::AppUnitsToIntCSSPixels(visual.width),
        nsPresContext::AppUnitsToIntCSSPixels(visual.height),
        nsPresContext::AppUnitsToIntCSSPixels(maxInAppUnits) });

Does that work?

If it does, I think it would allow us to drop some of the template functions
and other juggling we have to do?


Also, I haven't gone through this patch properly yet so I'm probably missing
something, but I wonder why we can't just use an nsTArray in
AnimationPerformanceWarning instead of a fixed-size buffer for every warning.
If it's to avoid heap allocations then I think we can do that with
AutoTArray. But, like I said, I haven't looked through this patch in enough
detail to understand why we're doing it this way.

Comment 61

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #59)
> ::: dom/animation/test/chrome/test_animation_property_state.html:64
> (Diff revisions 2 - 3)
> > +function assert_property_state_on_compositor(actual, expected) {
> 
> I don't quite understand why we need this. It seems very similar to
> assert_animation_property_state_equals and it seems like we don't use this
> function in some places where we could?
> 
> Could we just modify assert_animation_property_state_equals a little and
> re-use it? (e.g. if the property is not expected to be running on the
> compositor also check that the 'warning' property does not exist?)

Yes, we could. But it is not easy, I think.  There are test cases that isRunningOnCompositor is false without warning. E.g. background-color animations.

Also it needs to change each expected values before being passed to the assertion  when we check that tested animation causing a warning is running on compositor if the obstacle style is removed.
e.g. something like this:

assert_animation_property_state_equals(
  actual,
  subtext.expected.map(function(expected) {
    var modified = Object.assign({}, expected);
    modified.runningOnCompositor = true;
    delete modified.warning;
    return modified;
  });

I did actually begin to write it,  but it was hard to understand.  Now I realized the map can be factored in the assertion function if we have another flag in the expected array or another argument against the assertion.  I'd still prefer the separated assertion, though.

Comment 62

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #60)
> Comment on attachment 8726121 [details]
> MozReview Request: Bug 1196114 - Part 5: Store performce warning information
> as enum type. r?birtles
> 
> Can we use std::initializer_list for this?
> 
> We have a cross-platform wrapper now:
> https://dxr.mozilla.org/mozilla-central/source/mfbt/InitializerList.h
> 
> That way instead of doing:
> 
>   const int32_t params[7]  = {
>       nsPresContext::AppUnitsToIntCSSPixels(frameSize.width),
>       nsPresContext::AppUnitsToIntCSSPixels(frameSize.height),
>       nsPresContext::AppUnitsToIntCSSPixels(refSize.width),
>       nsPresContext::AppUnitsToIntCSSPixels(refSize.height),
>       nsPresContext::AppUnitsToIntCSSPixels(visual.width),
>       nsPresContext::AppUnitsToIntCSSPixels(visual.height),
>       nsPresContext::AppUnitsToIntCSSPixels(maxInAppUnits)
>   };
> 
>   EffectCompositor::SetPerformanceWarningWithParams(
>       aFrame, eCSSProperty_transform,
>       AnimationPerformanceWarning::Type::ContentTooLarge,
>       params);
> 
> Presumably we could just do:
> 
>   EffectCompositor::SetPerformanceWarningWithParams(
>       aFrame, eCSSProperty_transform,
>       AnimationPerformanceWarning::Type::ContentTooLarge,
>       { nsPresContext::AppUnitsToIntCSSPixels(frameSize.width),
>         nsPresContext::AppUnitsToIntCSSPixels(frameSize.height),
>         nsPresContext::AppUnitsToIntCSSPixels(refSize.width),
>         nsPresContext::AppUnitsToIntCSSPixels(refSize.height),
>         nsPresContext::AppUnitsToIntCSSPixels(visual.width),
>         nsPresContext::AppUnitsToIntCSSPixels(visual.height),
>         nsPresContext::AppUnitsToIntCSSPixels(maxInAppUnits) });
> 
> Does that work?

Thanks! I guess you probably mean that:

   EffectCompositor::SetPerformanceWarningWithParams(
     aFrame, eCSSProperty_transform, 
     AnimationPerformanceWarning(ContentTooLarge,
       { nsPresContext::AppUnitsToIntCSSPixels(frameSize.width),
         ...
       });

But anyway, that will work.  I will do it.

> Also, I haven't gone through this patch properly yet so I'm probably missing
> something, but I wonder why we can't just use an nsTArray in
> AnimationPerformanceWarning instead of a fixed-size buffer for every warning.
> If it's to avoid heap allocations then I think we can do that with
> AutoTArray. But, like I said, I haven't looked through this patch in enough
> detail to understand why we're doing it this way.

Yes, I've been thinking whether we should use Maybe<nsTArray> or not.  I was worrying about the cost of initialization of the array in the constructor.  Now it just uses memcpy there.  I've been also thinking we could use Maybe<int32_t[10]>, but there is no such an idiot code in our tree.  I wanted to use fixed length there anyway, but now I think the fixed length is not really matter,  we could use Maybe<nsTArray> if we use std::initializer_list.

Comment 63

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> > Also, I haven't gone through this patch properly yet so I'm probably missing
> > something, but I wonder why we can't just use an nsTArray in
> > AnimationPerformanceWarning instead of a fixed-size buffer for every warning.
> > If it's to avoid heap allocations then I think we can do that with
> > AutoTArray. But, like I said, I haven't looked through this patch in enough
> > detail to understand why we're doing it this way.
> 
> Yes, I've been thinking whether we should use Maybe<nsTArray> or not.  I was
> worrying about the cost of initialization of the array in the constructor. 
> Now it just uses memcpy there.  I've been also thinking we could use
> Maybe<int32_t[10]>, but there is no such an idiot code in our tree.  I
> wanted to use fixed length there anyway, but now I think the fixed length is
> not really matter,  we could use Maybe<nsTArray> if we use
> std::initializer_list.

If we don't generate these objects so much AutoTArray<int32_t, 7> would be ok and would avoid a separate heap allocation. But a regular nsTArray<int32_t> is probably fine.

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718658 [details]
MozReview Request: Bug 1196114 - Part 1: Add SetPerformanceWarning. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34673/diff/3-4/

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718659 [details]
MozReview Request: Bug 1196114 - Part 2: Add AnimationPropertyStatus interface and KeyframeEffectReadOnly.runningStatus(). r?birtles,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34675/diff/3-4/

Comment 66

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718660 [details]
MozReview Request: Bug 1196114 - Part 3: Set AnimationPerformanceWarning messages. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34677/diff/3-4/

Comment 67

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8718661 [details]
MozReview Request: Bug 1196114 - Part 4: Localize messages for animation performance warnings. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34679/diff/3-4/

Comment 68

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726121 [details]
MozReview Request: Bug 1196114 - Part 5: Store performce warning information as enum type. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37755/diff/1-2/

Comment 69

[Brian Birtles (:birtles)]

Comment on attachment 8726121 [details]
MozReview Request: Bug 1196114 - Part 5: Store performce warning information as enum type. r?birtles

https://reviewboard.mozilla.org/r/37755/#review34631

Looks much better! r=me with comments addressed.

::: dom/animation/AnimationPerformanceWarning.h:15
(Diff revision 2)
> +namespace dom {

Should this be in the dom namespace? I thought that was mainly for DOM interfaces etc.?

::: dom/animation/AnimationPerformanceWarning.h:48
(Diff revision 2)
> +  // NOTE: This constexpr can't be forward declaration, so if you want to use

can't be forward declared

::: dom/animation/AnimationPerformanceWarning.h:54
(Diff revision 2)
> +  // Indicating why this property could not be animated on the compositor.

Nit: Indicates

::: dom/animation/AnimationPerformanceWarning.h:57
(Diff revision 2)
> +  // Place holders for additional parameters to be used for localization.

Optional parameters that may be used for localization.

?

::: dom/animation/AnimationPerformanceWarning.cpp:9
(Diff revision 2)
> +#include "nsContentUtils.h" // For nsContentUtils::GetLocalizedString

Nit: GetLocalizeString/FormatLocalizedString

(We probably don't need this comment, however. It's mostly useful when the class/function you're using has a different name to the header file.)

::: dom/animation/AnimationPerformanceWarning.cpp:26
(Diff revision 2)
> +      // We can pass parameters whose length is greater than 7 to

Nit: We can pass an array of parameters whose length...

Should we assert here too that kMaxParamsForLocalization >= mParams->Length()?

::: dom/animation/EffectCompositor.h:191
(Diff revision 2)
> -  // Associates a warning message with effects on |aFrame| if the effect
> +  // Associates performance warning with effects on |aFrame| if the effect

Associates a performance warning with each effect on |aFrame| that animates |aProperty|.

::: dom/animation/KeyframeEffect.h:23
(Diff revision 2)
> +// For AnimationPerformanceWarning::Type This type can't be forward declaration.

Nit: ... can't be forward declared.
(Not sure this comment is really needed, however.)

::: dom/animation/KeyframeEffect.h:339
(Diff revision 2)
> -  // on the compositor.
> +  // compositor. |aParams| and |aParamsLength| is optional parameters which

Nit: are optional parameters

::: dom/animation/KeyframeEffect.cpp:1898
(Diff revision 2)
> -      state.mWarning.Construct(property.mPerformanceWarning.value());
> +    if (property.mPerformanceWarning.isSome() &&

Nit: No need for .isSome()

Comment 70

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726121 [details]
MozReview Request: Bug 1196114 - Part 5: Store performce warning information as enum type. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37755/diff/2-3/

Comment 71

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/37755/#review34653

I did not know that Maybe<AutoTArray> causes a problem.  Now changed to nsTArray instead.
https://treeherder.mozilla.org/logviewer.html#?job_id=17603929&repo=try  and bug 1159433#c3

Comment 72

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8726121 [details]
MozReview Request: Bug 1196114 - Part 5: Store performce warning information as enum type. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37755/diff/3-4/

Comment 73

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c17ae85657
https://hg.mozilla.org/integration/mozilla-inbound/rev/8479018ce405
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb3df050c01
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0b2eb05697
https://hg.mozilla.org/integration/mozilla-inbound/rev/605c2c5d1ab0

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b2c17ae85657
https://hg.mozilla.org/mozilla-central/rev/8479018ce405
https://hg.mozilla.org/mozilla-central/rev/eeb3df050c01
https://hg.mozilla.org/mozilla-central/rev/ad0b2eb05697
https://hg.mozilla.org/mozilla-central/rev/605c2c5d1ab0

Comment 75

[Ton [:Tonnes]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #74)
> https://hg.mozilla.org/mozilla-central/rev/ad0b2eb05697
> +AnimationWarningTransformSVG=Async 'transform' animations of aFrames with SVG transforms is not supported

animations -> animation (like the other 3)?

Comment 76

[Brian Birtles (:birtles)]

(In reply to Ton from comment #75)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #74)
> > https://hg.mozilla.org/mozilla-central/rev/ad0b2eb05697
> > +AnimationWarningTransformSVG=Async 'transform' animations of aFrames with SVG transforms is not supported
> 
> animations -> animation (like the other 3)?

Yeah, it looks like those strings could do with some tweaking. I think Hiro used the existing warning messages.

Since we're going to be exposing this in DevTools which already has a tooltip that says something like "This animation is running on the compositor", we should probably refer to compositor animations rather than async animations.