Split the animation-inspector's components into multiple files

RESOLVED FIXED in Firefox 46

Status

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: pbro, Assigned: nchevobbe)

Tracking

unspecified
Firefox 46

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
/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.
(Reporter)

Updated

3 years ago
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
(Assignee)

Comment 1

3 years ago
I'm gonna take a look at this. Can someone assign it to me please ?
(Reporter)

Updated

3 years ago
Assignee: nobody → chevobbe.nicolas
(Assignee)

Comment 2

3 years ago
Created attachment 8704256 [details] [diff] [review]
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.
Attachment #8704256 - Flags: review?(pbrosset)
(Reporter)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
Created attachment 8704341 [details] [diff] [review]
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.
Attachment #8704256 - Attachment is obsolete: true
Attachment #8704341 - Flags: review?(pbrosset)
(Assignee)

Comment 5

3 years ago
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)
(Assignee)

Comment 6

3 years ago
Created attachment 8704360 [details] [diff] [review]
Patch without time-scale.js and with correct formatting

Here's the fixed patch
Attachment #8704341 - Attachment is obsolete: true
Attachment #8704360 - Flags: review?(pbrosset)
(Reporter)

Comment 7

3 years ago
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+
(Assignee)

Comment 8

3 years ago
Created attachment 8704364 [details] [diff] [review]
Patch without time-scale.js, with correct formatting and fixed commit message
Attachment #8704360 - Attachment is obsolete: true
Attachment #8704364 - Flags: review+
(Assignee)

Comment 9

3 years ago
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).
(Reporter)

Comment 10

3 years ago
(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
(Assignee)

Comment 11

3 years ago
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.
(Reporter)

Comment 12

3 years ago
(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
(Reporter)

Comment 14

3 years ago
(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)
(Assignee)

Comment 15

3 years ago
 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)
(Assignee)

Comment 17

3 years ago
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 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c13e28343dd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46

Comment 20

3 years ago
[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

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.