Closed Bug 1400115 Opened 2 years ago Closed 2 years ago

Create a base pane for new animation inspector

Categories

(DevTools :: Inspector: Animations, enhancement, P3)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Also, we will introduce a new pref to enable new animation inspector.
Summary: Create a base html for new animation inspector → Create a base pane for new animation inspector
Severity: normal → enhancement
Priority: -- → P3
Comment on attachment 8909231 [details]
Bug 1400115 - Part 2: Introduce a pref to implement the new Animation panel behind.

https://reviewboard.mozilla.org/r/180826/#review185926

::: commit-message-9ac6c:1
(Diff revision 1)
> +Bug 1400115 - Part 2: Introduce a new pref. r?pbro

Please make this a bit more self-explanatory, like "Introduce a pref to implement the new Animation panel behind"
Attachment #8909231 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset <:pbro> from comment #5)
> Comment on attachment 8909231 [details]
> Bug 1400115 - Part 2: Introduce a new pref.
> 
> https://reviewboard.mozilla.org/r/180826/#review185926
> 
> ::: commit-message-9ac6c:1
> (Diff revision 1)
> > +Bug 1400115 - Part 2: Introduce a new pref. r?pbro
> 
> Please make this a bit more self-explanatory, like "Introduce a pref to
> implement the new Animation panel behind"

OK, thanks!
Comment on attachment 8909230 [details]
Bug 1400115 - Part 1: Create a base pane.

https://reviewboard.mozilla.org/r/180824/#review186274

Let's try and avoid the iframe with this rewrite. You can see how this was done with the layout panel.

::: devtools/client/inspector/animation/components/App.js:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {createClass, DOM} = require("devtools/client/shared/vendor/react");

s/{createClass, DOM}/{ createClass, DOM }.

We also typically do DOM: dom

::: devtools/client/inspector/animation/index.html:1
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

From the looks of it, it looks like we are gonna keep using an iframe for the Animation Inspector. I think we should actually try to remove the iframe for the animation inspector.
Attachment #8909230 - Flags: review?(gl)
Comment on attachment 8909232 [details]
Bug 1400115 - Part 3: Add test.

https://reviewboard.mozilla.org/r/180828/#review186278

Clearing the review for the test since I asked for adding the animation inspector without using an iframe. I am assuming this will change the test.
Attachment #8909232 - Flags: review?(gl)
Comment on attachment 8909230 [details]
Bug 1400115 - Part 1: Create a base pane.

https://reviewboard.mozilla.org/r/180824/#review186274

Okay, I'll do that.
Thanks!
Comment on attachment 8909231 [details]
Bug 1400115 - Part 2: Introduce a pref to implement the new Animation panel behind.

Please let me clear this r+ once because we change the structure.
Attachment #8909231 - Flags: review+
Comment on attachment 8909232 [details]
Bug 1400115 - Part 3: Add test.

https://reviewboard.mozilla.org/r/180828/#review186460

::: devtools/client/inspector/animation/test/head.js:73
(Diff revision 1)
> +  const win = inspector.sidebar.getWindowForTab(TAB_NAME);
> +
> +  // In e10s, if we wait for underlying toolbox actors to
> +  // load (by setting DevToolsUtils.testing to true), we miss the
> +  // "animationinspector-ready" event on the sidebar, so check to see if the
> +  // iframe is already loaded.
> +  return win.document.readyState === "complete"
> +         ? promise.resolve()
> +         : inspector.sidebar.once("animationinspector-ready");

I think Gabriel has suggested to drop the iframe, so this part will most probably have to change.
Also, now that we'll be using React and Redux, I guess there are better ways to wait for the panel to be ready, right? Like checking that the data store contains the right things.
Attachment #8909232 - Flags: review?(pbrosset)
(In reply to Gabriel [:gl] (ΦωΦ) from comment #7)
> From the looks of it, it looks like we are gonna keep using an iframe for
> the Animation Inspector. I think we should actually try to remove the iframe
> for the animation inspector.
I agree with Gabriel on this one, if we can remove sub iframes inside each DevTools panel, then that's good.
Having said that, just a heads up that we are using vh units in the animation tool. We use them for drawing the vertical lines in the timeline. So we'll need to find another solution for those now, if we don't have an iframe.
Comment on attachment 8909230 [details]
Bug 1400115 - Part 1: Create a base pane.

https://reviewboard.mozilla.org/r/180824/#review187372

::: devtools/client/inspector/animation/animation.js:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { createElement } = require("devtools/client/shared/vendor/react");
> +const App = require("./components/App");

Add a new line before this.

I like to separte out the requires - ones with absolute path versus relative paths

::: devtools/client/inspector/animation/components/App.js:7
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {createClass, DOM: dom} = require("devtools/client/shared/vendor/react");

s/{createClass, DOM: dom}/{ createClass, DOM: dom } to be consistent

::: devtools/client/inspector/animation/components/App.js:12
(Diff revision 2)
> +const {createClass, DOM: dom} = require("devtools/client/shared/vendor/react");
> +
> +const App = createClass({
> +  displayName: "AnimationInspector",
> +
> +  render: () => {

render() { }

::: devtools/client/inspector/animation/components/App.js:13
(Diff revision 2)
> +
> +const App = createClass({
> +  displayName: "AnimationInspector",
> +
> +  render: () => {
> +    return dom.div({id: "animation-inspector"});

Probably better to call this animation-container
Attachment #8909230 - Flags: review?(gl) → review+
Comment on attachment 8909230 [details]
Bug 1400115 - Part 1: Create a base pane.

https://reviewboard.mozilla.org/r/180824/#review187378

::: devtools/client/inspector/animation/animation.js:17
(Diff revision 2)
> +}
> +
> +AnimationInspector.prototype = {
> +
> +  init() {
> +    this.app = createElement(App);

s/this.app/this.provider
Comment on attachment 8909231 [details]
Bug 1400115 - Part 2: Introduce a pref to implement the new Animation panel behind.

https://reviewboard.mozilla.org/r/180826/#review187376

::: devtools/client/inspector/inspector.js:656
(Diff revision 2)
>  
>      if (this.target.form.animationsActor) {
> +      const animationId = "animationinspector";
> +      const animationTitle =
> +        INSPECTOR_L10N.getStr("inspector.sidebar.animationInspectorTitle");
> +      if (Services.prefs.getBoolPref("devtools.new-animationinspector.enabled")) {

Add a new line before this to separate the if statement block

::: devtools/client/inspector/inspector.js:674
(Diff revision 2)
> +              return this.animationinspector.app;
> +            }
> +          },
> +          defaultTab == animationId);
> +      } else {
> +        // Append animation inspector iframe.

I think we might want to avoid removing the actual iframe from inspector.xhtml until we have fully converted everything to react/redux. We can simply this code by simply changing the animationId to something like "newanimationinspector" and use the same animationTitle when we do this.sidebar.addTab()
Comment on attachment 8909232 [details]
Bug 1400115 - Part 3: Add test.

https://reviewboard.mozilla.org/r/180828/#review187380

::: devtools/client/inspector/animation/test/head.js:18
(Diff revision 2)
> +// Enable new animation inspector.
> +Services.prefs.setBoolPref("devtools.new-animationinspector.enabled", true);
> +
> +// Auto clean-up when a test ends
> +registerCleanupFunction(async function () {
> +  await closeAnimationInspector();

I don't think we need to do this since we are removing the current tab anyways. I am gonna defer to Patrick on this.

::: devtools/client/inspector/animation/test/head.js:30
(Diff revision 2)
> +// (safer here because if the test fails, then the pref is never reverted)
> +registerCleanupFunction(() => {
> +  Services.prefs.clearUserPref("devtools.new-animationinspector.enabled");
> +});
> +
> +const TAB_NAME = "animationinspector";

s/animationinspector/newanimationinspector from proposed changes to part 2

::: devtools/client/inspector/animation/test/head.js:35
(Diff revision 2)
> +const TAB_NAME = "animationinspector";
> +
> +/**
> + * Open the toolbox, with the inspector tool visible and the animationinspector
> + * sidebar selected.
> + * @return a promise that resolves when the inspector is ready.

s/@return a promise/@return {Promise}

::: devtools/client/inspector/animation/test/head.js:39
(Diff revision 2)
> + * sidebar selected.
> + * @return a promise that resolves when the inspector is ready.
> + */
> +const openAnimationInspector = async function () {
> +  const { inspector, toolbox } = await openInspectorSidebarTab(TAB_NAME);
> +  const AnimationInspector = inspector.animationinspector;

This is usual for us to camel case like this. I would simply do const { animationinspector: animationInspector } and return { toolbox, inspector, animationInspector, panel }

::: devtools/client/inspector/animation/test/head.js:40
(Diff revision 2)
> + * @return a promise that resolves when the inspector is ready.
> + */
> +const openAnimationInspector = async function () {
> +  const { inspector, toolbox } = await openInspectorSidebarTab(TAB_NAME);
> +  const AnimationInspector = inspector.animationinspector;
> +  const panel = inspector.panelWin.document.querySelector("#animation-inspector");

s/animation-inspector/animation-container from changes to part 1

::: devtools/client/inspector/animation/test/head.js:46
(Diff revision 2)
> +  return { toolbox, inspector, AnimationInspector, panel };
> +};
> +
> +/**
> + * Close the toolbox.
> + * @return a promise that resolves when the toolbox has closed.

Add a new line before this to separate the JSDoc comment and the @param/return
Attachment #8909232 - Flags: review?(gl)
Blocks: 1404801
Comment on attachment 8909231 [details]
Bug 1400115 - Part 2: Introduce a pref to implement the new Animation panel behind.

https://reviewboard.mozilla.org/r/180826/#review190672
Attachment #8909231 - Flags: review?(gl) → review+
Comment on attachment 8909232 [details]
Bug 1400115 - Part 3: Add test.

Gonna leave the test to pbro
Attachment #8909232 - Flags: review?(gl)
Hi Daisuke, 

I was curious if you were also planning to use Redux.
(In reply to Gabriel [:gl] (ΦωΦ) from comment #27)
> Hi Daisuke, 
> 
> I was curious if you were also planning to use Redux.

Hi Gabriel,
Yes, I'll use Redux from bug 1404801.
Assignee: nobody → dakatsuka
I talked with Gabriel, we decide to start to use es6 classes from here.
I'll rewrite the reviewed patches as well.
Hi Patrick,
I talked with Gabriel again, we didn't know that we should introduce es6 classes for React.

e.g.
App.js in https://reviewboard.mozilla.org/r/180824/diff/5#index_header

But we need to ignore one eslint rule.
https://reviewboard.mozilla.org/r/190074/diff/1#index_header

It looks we never use class for React in devtools on m-c yet.
But how do you think?
Flags: needinfo?(pbrosset)
Comment on attachment 8909232 [details]
Bug 1400115 - Part 3: Add test.

https://reviewboard.mozilla.org/r/180828/#review195680

::: devtools/client/inspector/animation/test/head.js:16
(Diff revision 5)
> +
> +// Enable new animation inspector.
> +Services.prefs.setBoolPref("devtools.new-animationinspector.enabled", true);
> +
> +// Auto clean-up when a test ends
> +registerCleanupFunction(async function () {

Do we need 2 registerCleanupFunction? I see this one is async, but it doesn't use any await.

::: devtools/client/inspector/animation/test/head.js:28
(Diff revision 5)
> +// (safer here because if the test fails, then the pref is never reverted)
> +registerCleanupFunction(() => {
> +  Services.prefs.clearUserPref("devtools.new-animationinspector.enabled");
> +});
> +
> +const TAB_NAME = "newanimationinspector";

Constants should be after loadSubScript

::: devtools/client/inspector/animation/test/head.js:39
(Diff revision 5)
> + * @return {Promise} that resolves when the inspector is ready.
> + */
> +const openAnimationInspector = async function () {
> +  const { inspector, toolbox } = await openInspectorSidebarTab(TAB_NAME);
> +  const { animationinspector: animationInspector } = inspector;
> +  const panel = inspector.panelWin.document.querySelector("#animation-container");

Use getElementById()
Attachment #8909232 - Flags: review?(gl) → review+
Comment on attachment 8919160 [details]
Bug 1400115 - Part 0: remove prefer-es6-class rule from lint to use pure es6 class.

https://reviewboard.mozilla.org/r/190074/#review195684

r+ assuming this is ok. Otherwise, probably don't land this.
Attachment #8919160 - Flags: review?(gl) → review+
Comment on attachment 8909232 [details]
Bug 1400115 - Part 3: Add test.

https://reviewboard.mozilla.org/r/180828/#review195780

Gabriel's comments make sense to me. Nothing to add here.
Attachment #8909232 - Flags: review?(pbrosset) → review+
(In reply to Daisuke Akatsuka (:daisuke) from comment #38)
> Hi Patrick,
> I talked with Gabriel again, we didn't know that we should introduce es6
> classes for React.
> 
> e.g.
> App.js in https://reviewboard.mozilla.org/r/180824/diff/5#index_header
> 
> But we need to ignore one eslint rule.
> https://reviewboard.mozilla.org/r/190074/diff/1#index_header
> 
> It looks we never use class for React in devtools on m-c yet.
> But how do you think?
We started using ES6 classes in other places in DevTools. I know people are discussing about using them for React components too, but I don't know where this discussion current is. I would advise talking to Jason Laster on the team about this.
Let's ping him here.
Flags: needinfo?(pbrosset) → needinfo?(jlaster)
I talked with Gabriel, we start to use PureComponent for React.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5a7dad17806d95583ebe85af2b137d7ca33a499
Flags: needinfo?(jlaster)
May I land patches for this bug and bug 1404801 ?
Flags: needinfo?(gl)
I talked with Gabriel, we will do following things.
* Remove patch 0
* Remove displayName from each React components we made.
* May not need to call gBrowser.removeCurrentTab() when clean up the test (in part 3).
Attachment #8919160 - Attachment is obsolete: true
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d85267150991
Part 1: Create a base pane. r=gl
https://hg.mozilla.org/integration/autoland/rev/6b82964b4060
Part 2: Introduce a pref to implement the new Animation panel behind. r=gl,pbro
https://hg.mozilla.org/integration/autoland/rev/ec9072a356c0
Part 3: Add test. r=gl,pbro
Flags: needinfo?(gl)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.