Split the animation-inspector's components into multiple files

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: pbro, Assigned: nchevobbe)

Tracking

unspecified
Firefox 46
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

4 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

4 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
Posted 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)
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
Posted 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)
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
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 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
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

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