1228080 - Split the animation-inspector's components into multiple files

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

/devtools/client/animationinspector/components.js

This file is growing quickly.
It contains all the UI components for the animation-inspector.
We should split it in several files and move them to a new components directory.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

I'm gonna take a look at this. Can someone assign it to me please ?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1228080.patch

I added one file for each exported class in components.js .
I ran the tests ( ./mach test devtools/client/animationinspector/test/* ) and they all passed.
I hope I got it right, feel free to tell me what could be done better.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8704256 [details] [diff] [review]
Bug_1228080.patch

Review of attachment 8704256 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome work, that's going to make the code a lot easier to navigate, thanks for working on this.
I'm generally very happy with the changes and would like to R+ it.

I just have a series of really minor formatting comments (which you can avoid next time if you setup eslint locally (https://wiki.mozilla.org/DevTools/CodingStandards to have it locally in your editor, and './mach eslint --setup' to install the right plugins and './mach eslint' to run it from the command line).

Also, I'd like TimeScale to be moved to utils.js instead of as a component.

::: devtools/client/animationinspector/components/animation-details.js
@@ +154,5 @@
> + */
> +function getCssPropertyName(jsPropertyName) {
> +  return jsPropertyName.replace(/[A-Z]/g, "-$&").toLowerCase();
> +}
> +exports.getCssPropertyName = getCssPropertyName;
\ No newline at end of file

nit: Please add an empty line at the end of this file.

::: devtools/client/animationinspector/components/animation-target-node.js
@@ +316,5 @@
> +    }
> +
> +    this.emit("target-retrieved");
> +  })
> +};
\ No newline at end of file

nit: Please add an empty line at the end of this file.

::: devtools/client/animationinspector/components/animation-time-block.js
@@ +1,5 @@
> +"use strict";
> +
> +const {Cu} = require("chrome");
> +Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");
> +const {Task} = Cu.import("resource://gre/modules/Task.jsm", {});

Task isn't used in this module, please remove this import.

@@ +152,5 @@
> +  // Older servers don't send the type.
> +  return state.type
> +    ? L10N.getFormatStr("timeline." + state.type + ".nameLabel", state.name)
> +    : state.name;
> +}
\ No newline at end of file

nit: Please add an empty line at the end of this file.

::: devtools/client/animationinspector/components/animation-timeline.js
@@ +1,3 @@
> +"use strict";
> +
> +const {Cu} = require("chrome");

Cu isn't used here, please remove this require.

::: devtools/client/animationinspector/components/keyframes.js
@@ +66,5 @@
> +      value: e.target.getAttribute("title"),
> +      x: e.target.offsetLeft + e.target.closest(".frames").offsetLeft
> +    });
> +  }
> +};
\ No newline at end of file

nit: Please add an empty line at the end of this file.

::: devtools/client/animationinspector/components/rate-selector.js
@@ +91,5 @@
> +    }
> +  }
> +};
> +
> +let sortedUnique = arr => [...new Set(arr)].sort((a, b) => a > b);
\ No newline at end of file

nit: Please add an empty line at the end of this file.

::: devtools/client/animationinspector/components/time-scale.js
@@ +17,5 @@
> + * For the helper to know how to convert, it needs to know all the animations.
> + * Whenever a new animation is added to the panel, addAnimation(state) should be
> + * called. reset() can be called to start over.
> + */
> +var TimeScale = {

So TimeScale is actually not a UI component, it's more of a helper object that UI components use to know how wide some widget should be in the timeline.
I think we should take the opportunity of this bug to move it to utils.js instead.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug_1228080.patch

Sorry for the formatting, I thought I had eslint turned on but it seems that Bug 1229858 messed with my node version. 
I moved Timescale to util.js and ran the tests again, which all passed.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8704341 [details] [diff] [review]
Bug_1228080.patch

My bad, the time-scale.js file is not removed from this patch. I'll post a fixed one later

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Patch without time-scale.js and with correct formatting

Here's the fixed patch

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8704360 [details] [diff] [review]
Patch without time-scale.js and with correct formatting

Review of attachment 8704360 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Thanks.
I don't remember, do you have TRY push access? If so, please push to try, otherwise let me know, I'll do it for you.
Also, please amend the commit message to include the "; r=pbro" suffix.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Patch without time-scale.js, with correct formatting and fixed commit message

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

I don't think I have access to TRY. I tried to push using commands from https://wiki.mozilla.org/ReleaseEngineering/TryServer but all I get is remote: Permission denied (publickey).

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #9)
> I don't think I have access to TRY. I tried to push using commands from
> https://wiki.mozilla.org/ReleaseEngineering/TryServer but all I get is
> remote: Permission denied (publickey).
Ok, so you don't have access. If you intend to contribute more in the future, you should consider requesting access by reading through https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/ and https://www.mozilla.org/en-US/about/governance/policies/commit/

In the meantime, I've pushed this patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4510ed019be6

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Hi Patrick, thank you for pushing it. 
The job is completed. There are 6 failed tests, but I'm not sure if it's due to my patch.

I filed Bug 1237302 to gain access to TRY. May you vouch for me ? 
Thank you.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #11)
> Hi Patrick, thank you for pushing it. 
> The job is completed. There are 6 failed tests, but I'm not sure if it's due
> to my patch.
Yep, those are unrelated. Let's ask for check this patch in.
> I filed Bug 1237302 to gain access to TRY. May you vouch for me ? 
Done.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This patch doesn't apply. Please rebase.

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> This patch doesn't apply. Please rebase.
Hm, weird, I've tried applying the patch on latest fx-team and it worked fine.
Nicolas, can you re-apply your patch on latest fx-team when you get a chance and merge if needed.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

 I was working on mozilla-central so I had to clone fx-team ( I was told on IRC that I should work on the latest, so will I ).
I apply the patch to fx-team ( hg import with and without --no-commit ), and had no errors.
I did not launched the tests ( because I have to ./mach build and it takes an hour and a half on my laptop ), but I don't really know what's going on.

Ryan, can you explain me more in detail the error you got ? Maybe I'm not applying the patch as it should be ?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

$ hg import -e bz://1228080
applying bz://1228080
Fetching... done
Parsing... done
patching file devtools/client/animationinspector/test/unit/test_getCssPropertyName.js
Hunk #1 FAILED at 1
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/animationinspector/test/unit/test_getCssPropertyName.js.rej
abort: patch failed to apply

That's on fx-team tip.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Ryan, I pulled and update and tried : 

$hg import -e https://bug1228080.bmoattachments.org/attachment.cgi\?id\=8704364
applying https://bug1228080.bmoattachments.org/attachment.cgi?id=8704364

( I didn't found anything on your cool bz:// syntax, what's that ? )

And that's it, my editor showed up with the commit message, and everything seems fine. 
Patrick, maybe you could try to apply the patch again, as you will know better what's going on ?

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0c13e28343dd

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c13e28343dd

Comment 20

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected