Closed Bug 1257874 Opened 8 years ago Closed 8 years ago

Stop using getFrames and use the new chrome-only getProperties to get animated properties and their keyframes in the timeline

Categories

(DevTools :: Inspector: Animations, defect, P1)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: pbro, Assigned: nchevobbe)

References

Details

(Whiteboard: [btpp-fix-now])

Attachments

(1 file)

Bug 1254419 just landed and provides a nice getProperties API for us to use in the devtools backend in order to retrieve the list of animated properties and their various keyframe values.

We should switch to using this instead of getFrames, because getFrames's return value is changing with bug 1245748.
Assignee: nobody → chevobbe.nicolas
Add a getProperties function to the animation actor to map KeyframeEffectReadOnly.getProperties
Call this new function in animation-detail and adapt the code as the structure
of the returned object structure is different from what getFrames returns.
Adapt a couple tests to the new object structure.

Review commit: https://reviewboard.mozilla.org/r/41183/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41183/
Attachment #8732443 - Flags: review?(pbrosset)
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41183/diff/1-2/
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

https://reviewboard.mozilla.org/r/41183/#review38081

Before spending more time reviewing this in more details, I want to share a first comment:
we unfortunately still need to support older devices (see https://wiki.mozilla.org/DevTools/Backwards_Compatibility).
This means that we need the UI to detect if `getProperties` is available, and if not, fall back to using `getFrames`.
See `getServerTraits` in animation-controller.js to see how we detect actor capabilities. I believe we need a new trait in there that tells us if the backend supports `getProperties`.
If the UI is connected to a server that does not support it, then that means the backend is pre-FF48 and it's fine to use `getFrames` instead since it still returns the expected data.
But that means you need to keep the frames to tracks converstion code too. That's unfortunate, because it means more code, but this is unfortunately needed.

One suggestion though: in animation.js, inside `AnimationPlayerFront`, you could add new custom function that switches between `getFrames` and `getProperties` and converts to tracks:

```
  getTracks: Task.async(function*(serverTraits) {
    if (serverTraits.hasGetProperties) {
      let properties = yield this.getProperties();
      // ... Code to convert to tracks just like in getTracksFromProperties
    } else {
      let frames = yield this.getFrames();
      // ... Code to convert to tracks just like in getTracksFromFrames
    }
  }),
```

This would make the code in `AnimationDetails` simpler I think. But you'll need to find a way to get the traits all the way to there.
Attachment #8732443 - Flags: review?(pbrosset)
Thanks Patrick, I'll see how to get the traits. I did thought about version, but only for the client -> version ( that's why I left `getFrames` on the actor ), but not in the other way round.

Is there a way to test those things (older client with newer server, older server with newer client ) ?
Flags: needinfo?(pbrosset)
We only really care about a new UI connecting to an old server (so with your patch, the new UI would call `getProperties` but the old server would not have this method).
You can test this with the "connect" screen:
- open an older firefox, say aurora, with profile 1,
- open gcli and type `listen 8000`
- open your current build of firefox with your patch, with profile 2,
- go to the tools menu and choose "connect..."
- in the page that appears, enter the port 8000 and then connect.

=> this gives you a toolbox running in the new firefox version, but connected to the debugger server that runs in aurora.
Flags: needinfo?(pbrosset)
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41183/diff/2-3/
Attachment #8732443 - Flags: review?(pbrosset)
Attachment #8732443 - Flags: review?(pbrosset)
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

https://reviewboard.mozilla.org/r/41183/#review38419

Looks mostly good. I just have a few comments that I'd like addressed before R+'ing this change.

::: devtools/client/animationinspector/components/animation-details.js:67
(Diff revision 3)
> -   * each with a list of frames.
> +   * @param {AnimationPlayerActor} animation
> +   * @param {Object} serverTraits the list of server-side capabilities
> +   * @return {Object} A list of tracks, one per animated property, each
> +   * with a list of keyframes
>     */
> -  getTracksFromFrames: function(frames) {
> +  getTracks: Task.async(function*(animation, serverTraits) {

Why pass traits as an argument here, they can be access with `this.serverTraits` already, right?

::: devtools/client/animationinspector/components/animation-details.js:70
(Diff revision 3)
> +   * with a list of keyframes
>     */
> -  getTracksFromFrames: function(frames) {
> +  getTracks: Task.async(function*(animation, serverTraits) {
>      let tracks = {};
>  
> +    if (serverTraits.hasGetProperties) {

Please add a comment before this if that explains in details what getFrames and getProperties are, and how getFrames has changed in FF48 which is why we're using getProperties now, etc...

::: devtools/client/animationinspector/components/animation-details.js:80
(Diff revision 3)
> +          tracks[name].push({
> +            value: valueObject.value,
> +            offset: valueObject.offset
> +          });

Why not just `tracks[name].push(valueObject);` ?
It looks like `valueObject` is exactly what you need.
If you don't want to do this because it has other properties that you want to skip, then I suggest re-writing the for loop like so:

```
for (let {value, offset} of propertyObject.values) {
  tracks[name].push({value, offset});
}
```

::: devtools/client/animationinspector/components/animation-timeline.js:307
(Diff revision 3)
> -      let details = new AnimationDetails();
> +      let win = this.inspector.sidebar.getWindowForTab("animationinspector");
> +      let details = new AnimationDetails(win.AnimationsController.traits);

I don't like this too much, using the inspector in order to access the animation-controller from the outside and get its traits. It looks like a round-about way to do what you need to do.

I would prefer passing `AnimationsController.traits` when calling the `AnimationsTimeline` constructor in animation-panel.js
So that you have access to the traits inside the `AnimationsTimeline` object, and can then pass them here when constructing the `AnimationDetails` component.

::: devtools/client/animationinspector/test/browser_animation_keyframe_click_to_set_time.js:25
(Diff revision 3)
>  
>    info("Expand the animation");
>    yield clickOnAnimation(panel, 0);
>  
>    info("Click on the first keyframe of the first animated property");
> -  yield clickKeyframe(panel, 0, "backgroundColor", 0);
> +  yield clickKeyframe(panel, 0, "background-color", 0);

I looked at the code to understand this change and realized that `getProperties` returned css property names like `background-color` while `getFrames` returned them like `backgroundColor`.
That's the reason for this change, right?

I don't think that's going to be a problem, but could you make sure you test connecting to an older backend that doesn't have `getProperties` to confirm that it still work fine there?
https://reviewboard.mozilla.org/r/41183/#review38419

> Why pass traits as an argument here, they can be access with `this.serverTraits` already, right?

indeed, I was unsure about passing both animation and serverTraits or acceeding them through this.

> Why not just `tracks[name].push(valueObject);` ?
> It looks like `valueObject` is exactly what you need.
> If you don't want to do this because it has other properties that you want to skip, then I suggest re-writing the for loop like so:
> 
> ```
> for (let {value, offset} of propertyObject.values) {
>   tracks[name].push({value, offset});
> }
> ```

yes, valueObject contains too much stuff we do not care here. 
I hadn't thought I could loop through it like this, will do

> I don't like this too much, using the inspector in order to access the animation-controller from the outside and get its traits. It looks like a round-about way to do what you need to do.
> 
> I would prefer passing `AnimationsController.traits` when calling the `AnimationsTimeline` constructor in animation-panel.js
> So that you have access to the traits inside the `AnimationsTimeline` object, and can then pass them here when constructing the `AnimationDetails` component.

That's what I originally did. I gave up on this because I figured this way to access AnimationController and thought it would be better not to clutter AnimationsTimeline constructor with serverTraits.

> I looked at the code to understand this change and realized that `getProperties` returned css property names like `background-color` while `getFrames` returned them like `backgroundColor`.
> That's the reason for this change, right?
> 
> I don't think that's going to be a problem, but could you make sure you test connecting to an older backend that doesn't have `getProperties` to confirm that it still work fine there?

Yes, it is.
I tested my patch on an olver server ( ffx 45 ) with connect and it did pick up getFrames and showed the right properties in the animation detail.
https://reviewboard.mozilla.org/r/41183/#review38419

> Yes, it is.
> I tested my patch on an olver server ( ffx 45 ) with connect and it did pick up getFrames and showed the right properties in the animation detail.

Sorry, I was not very precise here. 
When we display the animated property in the animation detail, we pass the property name to AnimationDetail's getCssPropertyName §  https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/components/animation-details.js#161 ) , in order to transform "backgroundColor", returned by getFrames, to its css counterpart, "background-color".

So now, getProperties already returns "background-color" so we have the same behaviour than before.
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41183/diff/3-4/
Attachment #8732443 - Flags: review?(pbrosset)
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

https://reviewboard.mozilla.org/r/41183/#review38445

::: devtools/client/animationinspector/components/animation-details.js:69
(Diff revisions 3 - 4)
>     */
> -  getTracks: Task.async(function*(animation, serverTraits) {
> +  getTracks: Task.async(function*() {
>      let tracks = {};
>  
> -    if (serverTraits.hasGetProperties) {
> -      let properties = yield animation.getProperties();
> +    /*
> +     * getFrames is a AnimationPlayorActor that returns data about the

Some rephrasing needed on this line:
"getFrames is an AnimationPlayerActor method that ..."

::: devtools/client/animationinspector/components/animation-details.js:71
(Diff revisions 3 - 4)
>      let tracks = {};
>  
> -    if (serverTraits.hasGetProperties) {
> -      let properties = yield animation.getProperties();
> +    /*
> +     * getFrames is a AnimationPlayorActor that returns data about the
> +     * keyframes of the animation.
> +     * In FF48, the data it returns change, and will hold only longhand

"The data it returns change*s*"
Attachment #8732443 - Flags: review?(pbrosset) → review+
https://reviewboard.mozilla.org/r/41183/#review38445

> Some rephrasing needed on this line:
> "getFrames is an AnimationPlayerActor method that ..."

duh

> "The data it returns change*s*"

As seen on IRC, data is plural ( even if controversial )
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41183/diff/4-5/
Keywords: checkin-needed
(In reply to Nicolas Chevobbe from comment #14)
> Waiting for
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8b1b1b76748

try run is over and there is no dt* error, so everything seems fine.
Keywords: checkin-needed
Comment on attachment 8732443 [details]
MozReview Request: Bug 1257874 - Use getProperties instead of getFrames in animation-detail. r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41183/diff/5-6/
https://hg.mozilla.org/mozilla-central/rev/371244335c10
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blocks: 1260680
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.