Closed Bug 1406287 Opened 7 years ago Closed 7 years ago

React-ify time header and tick line for animation list

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1406287 - Part 1: Implement basic layout.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1406287 - Part 2: Implement animation time tick and label.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1406287 - Part 3: Correspond for changing size of sidebar.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1406287 - Part 4: Add test for time ticks.
59 bytes, text/x-review-board-request
gl
: review+
Details 
In this bug, implement time header and tick line for animation list.
Severity: normal → enhancement
Priority: -- → P3
These patches assume that we can use es6 classes for React.
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review199380

::: devtools/client/inspector/animation/components/AnimationListContainer.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I feel like we can probably just remove this component have have this entire dom.div() and the AnimationListHeader and AnimationList be placed inside of App.js.

::: devtools/client/themes/animation.css:12
(Diff revision 3)
>  :root {
>    --animation-even-background-color: rgba(0,0,0,0.05);
>    --command-pick-image: url(chrome://devtools/skin/images/command-pick.svg);
> +  /* How wide should the sidebar be (should be wide enough to contain long
> +     property names like 'border-bottom-right-radius' without ellipsis) */
> +  --default-sidebar-width: 200px;

Does this need to be a variable? I didn't see it used anywhere else.

If the point of the variable was to indicate what the value represented, we can also just do that by adding the comment where we define the css.

::: devtools/client/themes/animation.css:14
(Diff revision 3)
>    --command-pick-image: url(chrome://devtools/skin/images/command-pick.svg);
> +  /* How wide should the sidebar be (should be wide enough to contain long
> +     property names like 'border-bottom-right-radius' without ellipsis) */
> +  --default-sidebar-width: 200px;
> +  /* The size of a keyframe marker in the keyframes diagram */
> +  --keyframes-marker-size: 10px;

Same as above.
Attachment #8920460 - Flags: review?(gl)
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review199380

> Does this need to be a variable? I didn't see it used anywhere else.
> 
> If the point of the variable was to indicate what the value represented, we can also just do that by adding the comment where we define the css.

Ah, yes. Although we don't use this variable in here, is going to use in bug 1406285.
However, yes. It's better to define at when we need.
Thanks, I'll drop this.
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review199706

::: devtools/client/inspector/animation/components/App.js:35
(Diff revision 4)
>      return dom.div(
>        {
>          id: "animation-container"
>        },
>        animations.length
> -      ? AnimationList(
> +      ? dom.div(

It is probably ok to reintroduce the AnimationListContainer component.

::: devtools/client/themes/animation.css:21
(Diff revision 4)
>  :root.theme-firebug {
>    --command-pick-image: url(chrome://devtools/skin/images/firebug/command-pick.svg);
>  }
>  
> +/* Settings for animation-list-header */
> +.animation-list-header {

The list header should actually conform with the height of the other sidepanel toolbars which has a total height of 24px. See #ruleview-toolbar-container and #computed-toolbar and should use .devtools-toolbar class.
Attachment #8920460 - Flags: review?(gl)
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review199706

> It is probably ok to reintroduce the AnimationListContainer component.

Okay, will reintroduce.

> The list header should actually conform with the height of the other sidepanel toolbars which has a total height of 24px. See #ruleview-toolbar-container and #computed-toolbar and should use .devtools-toolbar class.

Ya, thanks!
Comment on attachment 8920461 [details]
Bug 1406287 - Part 2: Implement animation time tick and label.

https://reviewboard.mozilla.org/r/191458/#review199844

::: devtools/client/inspector/animation/utils/utils.js:28
(Diff revision 4)
> +  }
> +
> +  let numIters = 0;
> +  let multiplier = 1;
> +  let interval;
> +  while (true) {

Can you make sure there are new lines that separates these while/if/for statement blocks.
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review201270

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:14
(Diff revision 6)
> +
> +class AnimationTimeTickList extends PureComponent {
> +  render() {
> +    return dom.div(
> +      {
> +        className: "animation-time-tick-list"

Let's go with "animation-timeline-tick-list"

::: devtools/client/themes/animation.css:22
(Diff revision 6)
>    --command-pick-image: url(chrome://devtools/skin/images/firebug/command-pick.svg);
>  }
>  
> +/* Settings for animation-list-header */
> +.animation-time-tick-list {
> +  height: 100%;

I would prefer not to have to setup our layout using position: absolute if possible. 

I would recommend trying flexbox or grid if possible to setup the layout.
Attachment #8920460 - Flags: review?(gl) → review+
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review201274

::: devtools/client/themes/animation.css:22
(Diff revision 6)
>    --command-pick-image: url(chrome://devtools/skin/images/firebug/command-pick.svg);
>  }
>  
> +/* Settings for animation-list-header */
> +.animation-time-tick-list {
> +  height: 100%;

Maybe this isn't actually possible now that I continue to review this.
Comment on attachment 8920461 [details]
Bug 1406287 - Part 2: Implement animation time tick and label.

https://reviewboard.mozilla.org/r/191458/#review201272

::: devtools/client/inspector/animation/components/AnimationListContainer.js:12
(Diff revision 6)
>  const { createFactory, DOM: dom, PropTypes, PureComponent } =
>    require("devtools/client/shared/vendor/react");
>  
>  const AnimationList = createFactory(require("./AnimationList"));
>  const AnimationListHeader = createFactory(require("./AnimationListHeader"));
> +const TimeScale = require("../utils/timescale");

Add a new line before TimeScale.

This is simply to group the requires implicitly. For example,

// React Components
const AnimationList = require() 
const AnimationListHeader = require()

// Utils
const TimeScale = require(..)

Don't actually add the comments.

::: devtools/client/inspector/animation/components/AnimationListContainer.js:23
(Diff revision 6)
>      };
>    }
>  
>    render() {
>      const { animations } = this.props;
>  

Remove this blank line.

::: devtools/client/inspector/animation/components/AnimationTimeTickItem.js:1
(Diff revision 6)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

If we change AnimationTime to AnimationTimeLine from part 1, we should rename these files.

::: devtools/client/inspector/animation/components/AnimationTimeTickItem.js:23
(Diff revision 6)
> +  render() {
> +    const { position, text } = this.props;
> +
> +    return dom.div(
> +      {
> +        className: "animation-time-tick-item",

s/time/timeline

::: devtools/client/inspector/animation/components/AnimationTimeTickItem.js:26
(Diff revision 6)
> +    return dom.div(
> +      {
> +        className: "animation-time-tick-item",
> +        style: { left: `${ position }%` }
> +      },
> +      dom.div(),

Do we need this div? It seems like we can simply style animation-time-tick-item to have the border-left styles.

Otherwise, can you add a class to this div and change .animation-time-tick-item > div to reflect this class. I think people will get confused by having an empty div.

::: devtools/client/inspector/animation/components/AnimationTimeTickItem.js:27
(Diff revision 6)
> +      {
> +        className: "animation-time-tick-item",
> +        style: { left: `${ position }%` }
> +      },
> +      dom.div(),
> +      dom.label(null, text)

Prefer {} to null in case we want to add some props in the future.

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:12
(Diff revision 6)
> -const { DOM: dom, PureComponent } =
> +const { createFactory, DOM: dom, PropTypes, PureComponent } =
>    require("devtools/client/shared/vendor/react");
> +const ReactDOM = require("devtools/client/shared/vendor/react-dom");
> +
> +const AnimationTimeTickItem = createFactory(require("./AnimationTimeTickItem"));
> +const { findOptimalTimeInterval } = require("../utils/utils");

Add a new line before.

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:24
(Diff revision 6)
> +    return {
> +      timescale: PropTypes.object.isRequired,
> +    };
> +  }
> +
> +  constructor() {

constructor(props)

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:25
(Diff revision 6)
> +      timescale: PropTypes.object.isRequired,
> +    };
> +  }
> +
> +  constructor() {
> +    super();

super(props)

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:72
(Diff revision 6)
> +
>      return dom.div(
>        {
>          className: "animation-time-tick-list"
> -      }
> +      },
> +      tickList.map(tickItem => {

This can be simplified to be:

tickList.map(tickItem => AnimationTimeTickItem(tickItem))

::: devtools/client/inspector/animation/utils/l10n.js:8
(Diff revision 6)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { LocalizationHelper } = require("devtools/shared/l10n");
> +module.exports =

I would prefer if we follow the similar conventions for our l10n files. See http://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/utils/l10n.js

::: devtools/client/inspector/animation/utils/timescale.js:9
(Diff revision 6)
> +
> +"use strict";
> +
> +const L10N = require("./l10n");
> +
> +const MILLIS_TIME_FORMAT_MAX_DURATION = 4000;

Add a comment for this constant.

s/MILLIS/MILLISECOND or TIME_FORMAT_MAX_DURATION_IN_MS

::: devtools/client/inspector/animation/utils/timescale.js:12
(Diff revision 6)
> +const L10N = require("./l10n");
> +
> +const MILLIS_TIME_FORMAT_MAX_DURATION = 4000;
> +
> +/**
> + * The TimeScale helper object is used to know which size should something be

I think you meant timeline when you said "something" we should clarify what that something is.

::: devtools/client/inspector/animation/utils/timescale.js:69
(Diff revision 6)
> +    this.maxEndTime = Math.max(this.maxEndTime, endTime);
> +  }
> +
> +  /**
> +   * Convert a distance in % to a time, in the current time scale.
> +   * @param {Number} distance

Add a new line to separate the function description from the @param.

::: devtools/client/inspector/animation/utils/timescale.js:79
(Diff revision 6)
> +  }
> +
> +  /**
> +   * Convert a distance in % to a time, in the current time scale.
> +   * The time will be relative to the current minimum start time.
> +   * @param {Number} distance

Same as above.

::: devtools/client/inspector/animation/utils/timescale.js:90
(Diff revision 6)
> +  }
> +
> +  /**
> +   * Depending on the time scale, format the given time as milliseconds or
> +   * seconds.
> +   * @param {Number} time

Same as above.

::: devtools/client/inspector/animation/utils/utils.js:35
(Diff revision 6)
> +    for (let i = 0; i < OPTIMAL_TIME_INTERVAL_MULTIPLES.length; i++) {
> +      interval = OPTIMAL_TIME_INTERVAL_MULTIPLES[i] * multiplier;
> +      if (minTimeInterval <= interval) {
> +        return interval;
> +      }
> +    }

Add a new line before and after an for/if block to separate the logical blocks.

::: devtools/client/inspector/animation/utils/utils.js:39
(Diff revision 6)
> +      }
> +    }
> +    if (++numIters > OPTIMAL_TIME_INTERVAL_MAX_ITERS) {
> +      return interval;
> +    }
> +    multiplier *= 10;

Add a new line before multiplier

::: devtools/client/inspector/animation/utils/utils.js:81
(Diff revision 6)
>           stateA.fill === stateB.fill &&
>           stateA.iterationCount === stateB.iterationCount &&
>           stateA.iterationStart === stateB.iterationStart;
>  }
>  
> +module.exports.findOptimalTimeInterval = findOptimalTimeInterval;

module.exports isn't quite right here. 

s/module.exports/exports for all of these.

::: devtools/client/themes/animation.css:13
(Diff revision 6)
>    --animation-even-background-color: rgba(0,0,0,0.05);
>    --command-pick-image: url(chrome://devtools/skin/images/command-pick.svg);
>  }
>  
>  :root.theme-dark {
>    --animation-even-background-color: rgba(255,255,255,0.05);

Can you space separate the rgba values like you have done so for the color below.

::: devtools/client/themes/animation.css:20
(Diff revision 6)
>  
>  :root.theme-firebug {
>    --command-pick-image: url(chrome://devtools/skin/images/firebug/command-pick.svg);
>  }
>  
>  /* Settings for animation-list-header */

We should rename these comments since saying Settings is repetitive. 

We should group the styles based on components and their component names.

For example,
/* Animation Timeline Tick List */

::: devtools/client/themes/animation.css:33
(Diff revision 6)
> +.animation-time-tick-item {
> +  position: absolute;
> +}
> +
> +.animation-time-tick-item > div {
> +  border-left: 0.5px solid rgba(128, 136, 144, .5);

I assume this isn't a photon color and should probably be updated while we are at it.

::: devtools/client/themes/animation.css:40
(Diff revision 6)
> +  position: absolute;
> +}
> +
> +.animation-time-tick-item > label {
> +  position: relative;
> +  top: -1px;

We should try avoid styling with using position if it can be avoided as well here.

::: devtools/client/themes/animation.css:43
(Diff revision 6)
> +.animation-time-tick-item > label {
> +  position: relative;
> +  top: -1px;
> +}
> +
>  /* Settings for animations element */

We should also reorganize our CSS rules based on the order of appearance in the DOM. 

For example,

AnimationContainer
AnimationListContainer
AnimationListHeader
AnimationList
Attachment #8920461 - Flags: review?(gl)
Comment on attachment 8920462 [details]
Bug 1406287 - Part 3: Correspond for changing size of sidebar.

https://reviewboard.mozilla.org/r/191460/#review201284

::: devtools/client/inspector/animation/actions/index.js:15
(Diff revision 6)
>    // Update the list of animation.
>    "UPDATE_ANIMATIONS",
>    // Update state of the picker enabled.
>    "UPDATE_ELEMENT_PICKER_ENABLED",
> +  // Update sidebar size.
> +  "UPDATE_SIDEBAR_SIZE"

Add ,

::: devtools/client/inspector/animation/actions/moz.build:9
(Diff revision 6)
>  
>  DevToolsModules(
>      'animations.js',
>      'element-picker.js',
> -    'index.js'
> +    'index.js',
> +    'sidebar.js'

Add a trailing comma at the end

::: devtools/client/inspector/animation/animation.js:13
(Diff revision 6)
>  const { createElement, createFactory } = require("devtools/client/shared/vendor/react");
>  const { Provider } = require("devtools/client/shared/vendor/react-redux");
>  
>  const App = createFactory(require("./components/App"));
>  const { isAllTimingEffectEqual } = require("./utils/utils");
>  const { updateAnimations } = require("./actions/animations");

Add a new line before this to create an implicit grouping of all the actions.

::: devtools/client/inspector/animation/animation.js:14
(Diff revision 6)
>  const { Provider } = require("devtools/client/shared/vendor/react-redux");
>  
>  const App = createFactory(require("./components/App"));
>  const { isAllTimingEffectEqual } = require("./utils/utils");
>  const { updateAnimations } = require("./actions/animations");
> +const { updateSidebarSize } = require("./actions/sidebar");

Move this below const { updateElementPickerEnabled } . = require("./actions/element-picker") to keep the requires alphabetically sorted.

::: devtools/client/inspector/animation/animation.js:23
(Diff revision 6)
>    constructor(inspector) {
>      this.inspector = inspector;
>  
>      this.toggleElementPicker = this.toggleElementPicker.bind(this);
>      this.update = this.update.bind(this);
> +    this.onSidebarResized = this.onSidebarResized.bind(this);

Move this after this.onElementPicker to keep the bindings alphabeticalized.

::: devtools/client/inspector/animation/animation.js:50
(Diff revision 6)
>      );
>      this.provider = provider;
>  
>      this.inspector.selection.on("new-node-front", this.update);
>      this.inspector.sidebar.on("newanimationinspector-selected", this.update);
> +    this.inspector.toolbox.on("inspector-sidebar-resized", this.onSidebarResized);

We don't actually want to listen to this all the time and updating when the panel isn't visible. We should set up and remove this listener depending on whether or not the sidebar was selected. See http://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/grid-inspector.js#563

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:21
(Diff revision 6)
>  const TIME_GRADUATION_MIN_SPACING = 40;
>  
>  class AnimationTimeTickList extends PureComponent {
>    static get propTypes() {
>      return {
>        timescale: PropTypes.object.isRequired,

s/timescale/timeScale to be consistent

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:22
(Diff revision 6)
>  
>  class AnimationTimeTickList extends PureComponent {
>    static get propTypes() {
>      return {
>        timescale: PropTypes.object.isRequired,
> +      sidebarWidth: PropTypes.number.isRequired,

Move this before timescale to keep the props alphabetically ordered.

::: devtools/client/inspector/animation/components/AnimationTimeTickList.js:46
(Diff revision 6)
> +
> +  shouldComponentUpdate(nextProps, nextState) {
> +    if (this.state.tickList.length !== nextState.tickList.length) {
> +      return true;
> +    }
> +    for (let i = 0; i < this.state.tickList.length; i++) {

Add new lines before and after if/for blocks.

::: devtools/client/shared/components/splitter/SplitBox.js:44
(Diff revision 6)
>      splitterSize: PropTypes.string,
>      // True if the splitter bar is vertical (default is vertical).
>      vert: PropTypes.bool,
>      // Style object.
>      style: PropTypes.object,
> +    // Call when controlled panel was resized.

SplitBox.js needs to be rebased.
Attachment #8920462 - Flags: review?(gl)
Comment on attachment 8920463 [details]
Bug 1406287 - Part 4: Add test for time ticks.

https://reviewboard.mozilla.org/r/191462/#review201288

::: devtools/client/inspector/animation/test/browser_animation_animation_list_exists.js:6
(Diff revision 6)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
>  requestLongerTimeout(2);

We probably don't need this timeout for now.

::: devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:6
(Diff revision 6)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +requestLongerTimeout(2);

Do we need this?

::: devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:15
(Diff revision 6)
> +// * time tick item elements existence
> +// * count and label of time tick elements changing by the sidebar width
> +
> +const { findOptimalTimeInterval } =
> +  require("devtools/client/inspector/animation/utils/utils");
> +const TimeScale = require("devtools/client/inspector/animation/utils/timescale");

Move this before const { findOptimalTimeinterval

::: devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:23
(Diff revision 6)
> +// AnimationTimeTickList component.
> +const TIME_GRADUATION_MIN_SPACING = 40;
> +
> +add_task(async function () {
> +  await addTab(URL_ROOT + "doc_simple_animation.html");
> +

Remove this blank line

::: devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:25
(Diff revision 6)
> +
> +add_task(async function () {
> +  await addTab(URL_ROOT + "doc_simple_animation.html");
> +
> +  const { animationInspector, inspector, panel } = await openAnimationInspector();
> +

Remove this blank line

::: devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:60
(Diff revision 6)
> +
> +  const width = animationTimeTickListEl.offsetWidth;
> +  const animationDuration = timescale.getDuration();
> +  const minTimeInterval = TIME_GRADUATION_MIN_SPACING * animationDuration / width;
> +  const interval = findOptimalTimeInterval(minTimeInterval);
> +  const nb = Math.ceil(animationDuration / interval);

s/nb/expectedTickItem

::: devtools/client/inspector/animation/test/browser_animation_animation_list_time_tick.js:66
(Diff revision 6)
> +
> +  const timeTickItemEls = listHeaderEl.querySelectorAll(".animation-time-tick-item");
> +  is(timeTickItemEls.length, nb, "The expected number of time ticks were found");
> +
> +  info("Make sure graduations are evenly distributed and show the right times");
> +  timeTickItemEls.forEach((tick, i) => {

Avoid forEach with es6 for of loops
Attachment #8920463 - Flags: review?(gl) → review+
Comment on attachment 8920461 [details]
Bug 1406287 - Part 2: Implement animation time tick and label.

https://reviewboard.mozilla.org/r/191458/#review201272

> I assume this isn't a photon color and should probably be updated while we are at it.

This is same color to tick line of network monitor.
https://searchfox.org/mozilla-central/source/devtools/client/performance/modules/waterfall-ticks.js#11
But, should we replace?
Comment on attachment 8920460 [details]
Bug 1406287 - Part 1: Implement basic layout.

https://reviewboard.mozilla.org/r/191456/#review204012

::: devtools/client/inspector/animation/components/App.js:34
(Diff revision 7)
>      return dom.div(
>        {
>          id: "animation-container"
>        },
>        animations.length
> -      ? AnimationList(
> +      ? AnimationListContainer(

Fix the formatting of this.

animations.length ?

See https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/components/Grid.js#58
Comment on attachment 8920461 [details]
Bug 1406287 - Part 2: Implement animation time tick and label.

https://reviewboard.mozilla.org/r/191458/#review204014

::: devtools/client/inspector/animation/components/AnimationListContainer.js:32
(Diff revision 7)
>        {
>          className: "animation-list-container"
>        },
> -      AnimationListHeader(),
> +      AnimationListHeader(
> +        {
> +          timescale

We should just pass the animations into AnimationListHeader and AnimationTimelineTickList and create the timescale in the component that needs it.

::: devtools/client/inspector/animation/components/AnimationTimelineTickItem.js:14
(Diff revision 7)
> +
> +class AnimationTimeTickItem extends PureComponent {
> +  static get propTypes() {
> +    return {
> +      position: PropTypes.number.isRequired,
> +      text: PropTypes.string.isRequired,

Rename text to timeTickLabel to be more descriptive of what text is.

::: devtools/client/inspector/animation/components/AnimationTimelineTickList.js:39
(Diff revision 7)
> +    this.updateTickList();
> +  }
> +
> +  updateTickList() {
> +    const { timescale } = this.props;
> +

Remove this blank line

::: devtools/client/inspector/animation/components/AnimationTimelineTickList.js:48
(Diff revision 7)
> +    const minTimeInterval = TIME_GRADUATION_MIN_SPACING * animationDuration / width;
> +    const intervalLength = findOptimalTimeInterval(minTimeInterval);
> +    const intervalWidth = intervalLength * width / animationDuration;
> +
> +    const tickList = [];
> +    for (let i = 0; i <= width / intervalWidth; i++) {

Store this (width / intervalWidth) into a variable to avoid recalculating.

::: devtools/client/inspector/animation/components/AnimationTimelineTickList.js:49
(Diff revision 7)
> +    const intervalLength = findOptimalTimeInterval(minTimeInterval);
> +    const intervalWidth = intervalLength * width / animationDuration;
> +
> +    const tickList = [];
> +    for (let i = 0; i <= width / intervalWidth; i++) {
> +      const position = 100 * i * intervalWidth / width;

Store (100 * intervalWidth / width) into a variable to avoid recalculating.

::: devtools/client/inspector/animation/utils/timescale.js:12
(Diff revision 7)
> +const { getFormatStr } = require("./l10n");
> +
> +// Use in formatTime function.
> +// If total duration for all animations is eqaul to or less than
> +// TIME_FORMAT_MAX_DURATION_IN_MS, the text which expresses time is in milliseconds.
> +// Otherwise, second.

Reword to:

If total duration for all animations is eqaul to or less than TIME_FORMAT_MAX_DURATION_IN_MS, the text which expresses time is in milliseconds, and seconds otherwise. Use in formatTime function.

::: devtools/client/inspector/animation/utils/timescale.js:16
(Diff revision 7)
> +// TIME_FORMAT_MAX_DURATION_IN_MS, the text which expresses time is in milliseconds.
> +// Otherwise, second.
> +const TIME_FORMAT_MAX_DURATION_IN_MS = 4000;
> +
> +/**
> + * TimeScale object holds total duration, start time and end time etc from all animations

Reword to:

TimeScale object holds the total duration, start time and end time information for all animations which should be displayed, and is used to calculate the displayed area for each animation.

::: devtools/client/inspector/animation/utils/timescale.js:28
(Diff revision 7)
> + */
> +class TimeScale {
> +  constructor(animations) {
> +    this.minStartTime = Infinity;
> +    this.maxEndTime = 0;
> +    animations.forEach(animation => {

Don't use .forEach and prefer ES6 for .. of

::: devtools/client/inspector/animation/utils/timescale.js:36
(Diff revision 7)
> +  }
> +
> +  /**
> +   * Add a new animation to time scale.
> +   *
> +   * @param {Object} state A PlayerFront.state object.

Format @param like https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/grid-inspector.js#195

::: devtools/client/inspector/animation/utils/timescale.js:39
(Diff revision 7)
> +   * Add a new animation to time scale.
> +   *
> +   * @param {Object} state A PlayerFront.state object.
> +   */
> +  addAnimation(state) {
> +    let {previousStartTime, delay, duration, endDelay,

Format this like https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/components/GridItem.js#120 in alphabetical order of the props

::: devtools/client/inspector/animation/utils/timescale.js:42
(Diff revision 7)
> +   */
> +  addAnimation(state) {
> +    let {previousStartTime, delay, duration, endDelay,
> +         iterationCount, playbackRate} = state;
> +
> +    endDelay = typeof endDelay === "undefined" ? 0 : endDelay;

You can specify the default value of 0 while destructuring.

::: devtools/client/inspector/animation/utils/utils.js:7
(Diff revision 7)
>   * 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";
>  
> +// How many times, maximum, can we loop before we find the optimal time

Reword to:

The maximum number of times we can loop before we find the optimal time interval in the timeline graph.

::: devtools/client/inspector/animation/utils/utils.js:17
(Diff revision 7)
> +
> +/**
> + * Find the optimal interval between time graduations in the animation timeline
> + * graph based on a minimum time interval.
> + *
> + * @param {Number} minTimeInterval Minimum time in ms in one interval

Format @param like https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-highlighter-utils.js#220

::: devtools/client/themes/animation.css:20
(Diff revision 7)
>  
>  :root.theme-firebug {
>    --command-pick-image: url(chrome://devtools/skin/images/firebug/command-pick.svg);
>  }
>  
> -/* Settings for animation-list-header */
> +/* AnimationListHeader */

Space out the words in this file:

Animation List Header,
Animation Timeline Tick List
etc
Attachment #8920461 - Flags: review?(gl) → review+
Comment on attachment 8920462 [details]
Bug 1406287 - Part 3: Correspond for changing size of sidebar.

https://reviewboard.mozilla.org/r/191460/#review204018

::: devtools/client/inspector/animation/actions/index.js:15
(Diff revision 7)
>    // Update the list of animation.
>    "UPDATE_ANIMATIONS",
>    // Update state of the picker enabled.
>    "UPDATE_ELEMENT_PICKER_ENABLED",
> +  // Update sidebar size.
> +  "UPDATE_SIDEBAR_SIZE",

Space out the actions like https://searchfox.org/mozilla-central/source/devtools/client/inspector/grids/actions/index.js

::: devtools/client/inspector/animation/animation.js:27
(Diff revision 7)
>      this.toggleElementPicker = this.toggleElementPicker.bind(this);
>      this.update = this.update.bind(this);
>      this.onElementPickerStarted = this.onElementPickerStarted.bind(this);
>      this.onElementPickerStopped = this.onElementPickerStopped.bind(this);
> +    this.onSidebarResized = this.onSidebarResized.bind(this);
> +    this.onSelected = this.onSelected.bind(this);

s/onSelected/onSidebarSelect
Attachment #8920462 - Flags: review?(gl) → review+
Comment on attachment 8920461 [details]
Bug 1406287 - Part 2: Implement animation time tick and label.

https://reviewboard.mozilla.org/r/191458/#review204030

::: devtools/client/inspector/animation/components/AnimationListContainer.js:32
(Diff revision 7)
>        {
>          className: "animation-list-container"
>        },
> -      AnimationListHeader(),
> +      AnimationListHeader(
> +        {
> +          timescale

Ah, I had written on the premise to refer same instance in AnimationList in bug 1406285.
Okay, I'll re-write in this bug along to your suggest, then update bug 1406285 to share the timescale again.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60f15b142093
Part 1: Implement basic layout. r=gl
https://hg.mozilla.org/integration/autoland/rev/7ef35c3887f2
Part 2: Implement animation time tick and label. r=gl
https://hg.mozilla.org/integration/autoland/rev/254946943852
Part 3: Correspond for changing size of sidebar. r=gl
https://hg.mozilla.org/integration/autoland/rev/def7e03f6262
Part 4: Add test for time ticks. r=gl
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: