Closed Bug 1228080 Opened 5 years ago Closed 5 years ago

Split the animation-inspector's components into multiple files

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: pbro, Assigned: nchevobbe)

References

Details

Attachments

(1 file, 3 obsolete files)

/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.
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
I'm gonna take a look at this. Can someone assign it to me please ?
Assignee: nobody → chevobbe.nicolas
Attached patch Bug_1228080.patch (obsolete) — Splinter Review
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.
Attachment #8704256 - Flags: review?(pbrosset)
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.
Attachment #8704256 - Flags: review?(pbrosset)
Attached patch Bug_1228080.patch (obsolete) — Splinter Review
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.
Attachment #8704256 - Attachment is obsolete: true
Attachment #8704341 - Flags: review?(pbrosset)
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
Attachment #8704341 - Flags: review?(pbrosset)
Here's the fixed patch
Attachment #8704341 - Attachment is obsolete: true
Attachment #8704360 - Flags: review?(pbrosset)
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.
Attachment #8704360 - Flags: review?(pbrosset) → review+
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).
(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
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.
(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.
Keywords: checkin-needed
This patch doesn't apply. Please rebase.
Keywords: checkin-needed
(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.
Flags: needinfo?(chevobbe.nicolas)
 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 ?
Flags: needinfo?(chevobbe.nicolas) → needinfo?(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.
Flags: needinfo?(ryanvm)
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 ?
https://hg.mozilla.org/mozilla-central/rev/0c13e28343dd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.