1050386 - [timeline] build a temporary timeline panel

Status: VERIFIED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Paul Rouget [:paul]]

We want to have a temporary simple timeline panel. We are thinking about merging the timeline into the profiler at some point.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

+1 for merging at some point -- most confusing thing about chrome's devtools is choosing which performance related panel to use for a given problem.

Comment 2

[Paul Rouget [:paul]]

Attachment: v0.0001

Comment 3

[Paul Rouget [:paul]]

Note: we should use a logarithmic scale in the top view.

Comment 4

[Paul Rouget [:paul]]

Attachment: v0.0002

Comment 5

[Paul Rouget [:paul]]

Attachment: v0.0003

Comment 6

[Patrick Brosset <:pbro>]

Victor, in bug 1050376 attachment 8479713 [details] [diff] [review] I changed the names of the markers returned by nsIDocShell::popProfileTimelineMarkers to: "Paint", "Reflow", "Styles".
I don't think they will change again, and they should now be a lot more self-explanatory.

Comment 7

[Victor Porof [:vporof][:vp]]

Ok, sounds good!

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: WIP screenshot

Comment 9

[Victor Porof [:vporof][:vp]]

Attachment: v1

There.

Caveats:
* You can't select a time region while recording. I don't think we should allow this.
* Tests not ready, but there's not a lot to actually test.

Comment 10

[Paul Rouget [:paul]]

Comment on attachment 8480376 [details] [diff] [review]
v1

Busy with some other stuff. Patrick will review this patch.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Victor Porof [:vporof][:vp] from comment #8)
> Created attachment 8480192 [details]
> WIP screenshot
Looking great!

Comment 12

[Patrick Brosset <:pbro>]

I'm reviewing this patch now. For info, it needs rebasing as the actor patch just changed.

Comment 13

[Paul Rouget [:paul]]

Attachment: rebased

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8480376 [details] [diff] [review]
v1

Review of attachment 8480376 [details] [diff] [review]:
-----------------------------------------------------------------

This is an awesome start! I've applied the platform + actor + ui patches and played a little bit with the tool. I really like it. It's immediately useful.
Of course we don't yet display information about markers (what got repainted, what caused a reflow, ...), but that's the next step.

I don't think that the selection not working while recording is a problem, it would be kind of hard to use anyway.

One thing I did notice that I found weird though is the way the timeline refreshes only when there are new markers to display. So, when there are things to display, the timeline adjusts with the passing time, and that feels natural. But when there isn't anything happening, the timeline stops refreshing, which gives the impression that it stopped recording or that something is wrong.
I don't think this should block landing this patch (this is a v1 after all), but it's worth filing a follow-up for discussing about how this should behave exactly.

I've got some remarks/questions about the code. See below.

::: browser/app/profile/firefox.js
@@ +1408,5 @@
>  // Enable the Web Audio Editor
>  pref("devtools.webaudioeditor.enabled", false);
>  
> +// Enable the timeline.
> +pref("devtools.timeline.enabled", true);

This will make the timeline visible by default. Do we want this?
I would say yes because performance tools are important.

::: browser/devtools/main.js
@@ +295,5 @@
> +Tools.timeline = {
> +  id: "timeline",
> +  ordinal: 8,
> +  visibilityswitch: "devtools.timeline.enabled",
> +  icon: "chrome://browser/skin/devtools/tool-styleeditor.svg",

We need a new icon for the timeline. Don't we have something we could reuse in the perf panel mockup Darrin made at some stage?
Anyway, this is fine for the time being, but we may need to cc Darrin on the bug for this.

@@ +302,5 @@
> +  label: l10n("timeline.label", timelineStrings),
> +  panelLabel: l10n("timeline.panelLabel", timelineStrings),
> +  tooltip: l10n("timeline.tooltip", timelineStrings),
> +  isTargetSupported: function(target) {
> +    return !target.isAddon;

This makes it work from the browser toolbox and, OMG, it does work and it's awesome!

::: browser/devtools/timeline/timeline.js
@@ +91,5 @@
> +   *        A list of new markers collected since the last time this
> +   *        function was invoked.
> +   */
> +  _onMarkers: function(markers) {
> +    Array.prototype.push.apply(this._markers, markers);

Or this._markers.push(...markers);

@@ +92,5 @@
> +   *        function was invoked.
> +   */
> +  _onMarkers: function(markers) {
> +    Array.prototype.push.apply(this._markers, markers);
> +    TimelineView.overview.setData(this._markers);

Shouldn't the controller not know the internals of the view? I think you should instead call something like TimelineView.handleMakersReceived(this._markers); and then let the view decide which widget(s) to call setData on.

::: browser/devtools/timeline/widgets/global.js
@@ +13,5 @@
> +const STRINGS_URI = "chrome://browser/locale/devtools/timeline.properties";
> +const L10N = new ViewHelpers.L10N(STRINGS_URI);
> +
> +/**
> + * A simple schema for mapping markers to the timeline UI.

Can you add a comment like @see <filename/methodname> for information about the format of this object ?
Even if fill, stroke and label are self-explanatory, group isn't.

@@ +28,5 @@
> +    fill: "hsl(104,57%,71%)",
> +    stroke: "hsl(104,57%,51%)",
> +    label: L10N.getStr("format.reflow")
> +  },
> +  "DisplayList": {

s/DisplayList/Paint with the latest platform patch.

::: browser/devtools/timeline/widgets/overview.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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";
> +

Could you add a comment at the top of this file (and in fact, at the top of every files you added in this new folder) that explains what this file is supposed to be responsible for? This helps future maintainers (especially those that don't know the code) understand without reading the code, and it also somehow prevents it from becoming a dumping ground for unrelated things.

@@ +35,5 @@
> +const OVERVIEW_TIMELINE_STROKES = "#aaa";
> +const OVERIVEW_MARKERS_COLOR_STOPS = [0, 0.1, 0.75, 1];
> +const OVERVIEW_MARKER_DURATION_MIN = 4; // ms
> +const OVERVIEW_GROUP_VERTICAL_PADDING = 6; // px
> +const OVERVIEW_GROUP_ALTERNATING_BACKGROUND = "rgba(0,0,0,0.05)";

The timeline looks a bit weird with the black theme: https://dl.dropboxusercontent.com/u/714210/dark-theme-timeline.png
Can you file a follow-up bug for this?

@@ +67,5 @@
> +    this._paintBatches = new Map();
> +    this._lastGroup = 0;
> +
> +    for (let type in blueprint) {
> +      this._paintBatches.set(type, { style: blueprint[type], queue: [] });

This is probably me nitpicking but I wouldn't name the list of markers per group "queue".
This word gives the impression that it's a data storage that gets filled on one end and consumed on the other. Instead it's just a list of markers that gets emptied after each refresh and filled again after markers are received, but receiving markers and consuming them all always happens at the same rate.

@@ +103,5 @@
> +    let groupHeight = OVERVIEW_BODY_HEIGHT * this._pixelRatio / totalGroups;
> +    let groupPadding = OVERVIEW_GROUP_VERTICAL_PADDING * this._pixelRatio;
> +    let dataScale = this.dataScaleX = Math.min(1, availableWidth / oldestTimestamp);
> +
> +    // Draw the header and overivew background.

s/overivew/overview

@@ +169,5 @@
> +        ctx.rect(left, top, width, height);
> +      }
> +
> +      ctx.fill();
> +      queue.length = 0;

This line requires commenting I think, this would make it easier to understand how the refresh process works.

::: browser/devtools/timeline/widgets/waterfall.js
@@ +131,5 @@
> +    let ticks = this._document.createElement("hbox");
> +    ticks.className = "timeline-header-ticks";
> +    ticks.setAttribute("align", "center");
> +    ticks.setAttribute("flex", "1");
> +    container.appendChild(ticks);

Re-creating these 4 container elements at each refresh looks like wasted effort. They should be created once and for all and just re-filled on refresh.

@@ +175,5 @@
> +        continue;
> +      }
> +      // Only build and display a finite number of markers initially, to
> +      // preserve a snappy UI. After a certain delay, continue building the
> +      // outstanding markers while there's (hopefully) no user interaction.

I really like this idea, especially because I'm guessing most users have the devtools docked to the bottom, and in this position, 30 markers is usually all you can see anyway, so you don't even realize there's a delay for the rest.
I've had to make my window really big and resize the devtools to make them huge to start seeing the delay.
I've got the weird impression that the delay is more than 100ms tough. But perhaps what I'm seeing is the time it takes to draw 30 markers + 100ms.
Did you try making the delay shorter?

@@ +336,5 @@
> +   *
> +   * @param number dataScale
> +   *        @see Waterfall.prototype._buildMarkers
> +   */
> +  _drawWaterfallBackground: function(dataScale) {

I had to read this method's code several times to understand how it worked, which usually means it too complex or deserves a comment explaining the reasons that led to doing it.
I'm pretty confident that using a canvas as a background image and setting pixel colors the way you did was done for a reason and performs fast. Can you add a comment block explaining this reasoning?
Also, did you try other, simpler, techniques to draw the lines?

::: browser/locales/en-US/chrome/browser/devtools/timeline.properties
@@ +7,5 @@
> +# The correct localization of this file might be to keep it in
> +# English, or another language commonly spoken among web developers.
> +# You want to make that choice consistent across the developer tools.
> +# A good criteria is the language in which you'd find the best
> +# documentation on web development on the web.

This comment is obsolete, copy/pasted from the Profiler

::: browser/themes/shared/devtools/timeline.inc.css
@@ +24,5 @@
> +}
> +
> +.timeline-list-contents {
> +  /* Hack: force hardware acceleration */
> +  transform: translateZ(-1px);

Isn't the "will-change" property supposed to avoid this?

@@ +32,5 @@
> +
> +.timeline-header-ticks,
> +.timeline-marker-waterfall {
> +  background-image: -moz-element(#waterfall-background);
> +  background-repeat: repeat-y; /* Background created on a <canvas> in js. */

Please add something like @see browser/devtools/timeline/widgets/waterfall.js

::: toolkit/devtools/server/actors/timeline.js
@@ +27,5 @@
>  const {method, Arg, RetVal} = protocol;
>  const events = require("sdk/event/core");
>  const {setTimeout, clearTimeout} = require("sdk/timers");
>  
> +const DEFAULT_TIMELINE_DATA_PULL_TIMEOUT = 100; // ms

I think anything below 250ms feels kind of immediate to a user.
I thought 300ms was already quite a rapid rate, especially given the amount of data we're transporting (not THAT much right now, but will grow when we add metadata to markers). What about adjusting this to 200ms instead?

@@ +98,5 @@
>      }
>    }),
>  
>    /**
>     * Start/stop recording profile markers.

Missing jsdoc for the interval @param.
If I'm not mistaken, this param isn't used in your patch anyway.
What use case where you thinking about when you added it? Do we need to add this now?

Comment 15

[Victor Porof [:vporof][:vp]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> Comment on attachment 8480376 [details] [diff] [review]
> v1
> 

Thanks for the review Patrick! Replies inline below.

> 
> This is an awesome start! I've applied the platform + actor + ui patches and
> played a little bit with the tool. I really like it. It's immediately useful.
> Of course we don't yet display information about markers (what got
> repainted, what caused a reflow, ...), but that's the next step.
> 
> I don't think that the selection not working while recording is a problem,
> it would be kind of hard to use anyway.
> 

Yeah, I think that makes sense too.

> One thing I did notice that I found weird though is the way the timeline
> refreshes only when there are new markers to display. So, when there are
> things to display, the timeline adjusts with the passing time, and that
> feels natural. But when there isn't anything happening, the timeline stops
> refreshing, which gives the impression that it stopped recording or that
> something is wrong.

You're right, it should update and scale continuously, regardless of whether or not there are any new markers to add.

> I don't think this should block landing this patch (this is a v1 after all),
> but it's worth filing a follow-up for discussing about how this should
> behave exactly.
> 

Should be trivial to fix, so I'll just do it in this bug.

> I've got some remarks/questions about the code. See below.
> 
> ::: browser/app/profile/firefox.js
> @@ +1408,5 @@
> >  // Enable the Web Audio Editor
> >  pref("devtools.webaudioeditor.enabled", false);
> >  
> > +// Enable the timeline.
> > +pref("devtools.timeline.enabled", true);
> 
> This will make the timeline visible by default. Do we want this?
> I would say yes because performance tools are important.
> 

I would say no because this is hardly a prototype. There's really not much useful data shown for the average web developer, and we want to merge the timeline and the profiler sometime in the future. It will be less confusing to the users if this stays disabled for now.

> ::: browser/devtools/main.js
> @@ +295,5 @@
> > +Tools.timeline = {
> > +  id: "timeline",
> > +  ordinal: 8,
> > +  visibilityswitch: "devtools.timeline.enabled",
> > +  icon: "chrome://browser/skin/devtools/tool-styleeditor.svg",
> 
> We need a new icon for the timeline. Don't we have something we could reuse
> in the perf panel mockup Darrin made at some stage?
> Anyway, this is fine for the time being, but we may need to cc Darrin on the
> bug for this.
> 

Do we, though? Again, merging tools in the near future.

> @@ +302,5 @@
> > +  label: l10n("timeline.label", timelineStrings),
> > +  panelLabel: l10n("timeline.panelLabel", timelineStrings),
> > +  tooltip: l10n("timeline.tooltip", timelineStrings),
> > +  isTargetSupported: function(target) {
> > +    return !target.isAddon;
> 
> This makes it work from the browser toolbox and, OMG, it does work and it's
> awesome!
> 

\o/

> ::: browser/devtools/timeline/timeline.js
> @@ +91,5 @@
> > +   *        A list of new markers collected since the last time this
> > +   *        function was invoked.
> > +   */
> > +  _onMarkers: function(markers) {
> > +    Array.prototype.push.apply(this._markers, markers);
> 
> Or this._markers.push(...markers);
> 

Not really, there's a difference:
* array.push(otherArray) pushes the other array (as an object) into the first
* Array.prototype.push.apply(array, otherArray) pushes all the *elements* of the other array into the first

Sure, array.concat(otherArray) would've worked as well, but that instantiates a new array every time and does a shallow copy of the first one, which is wasted effort.

> @@ +92,5 @@
> > +   *        function was invoked.
> > +   */
> > +  _onMarkers: function(markers) {
> > +    Array.prototype.push.apply(this._markers, markers);
> > +    TimelineView.overview.setData(this._markers);
> 
> Shouldn't the controller not know the internals of the view? I think you
> should instead call something like
> TimelineView.handleMakersReceived(this._markers); and then let the view
> decide which widget(s) to call setData on.
> 

I didn't worry too much about it, but you're probably right. I'll change this.

> ::: browser/devtools/timeline/widgets/global.js
> @@ +13,5 @@
> > +const STRINGS_URI = "chrome://browser/locale/devtools/timeline.properties";
> > +const L10N = new ViewHelpers.L10N(STRINGS_URI);
> > +
> > +/**
> > + * A simple schema for mapping markers to the timeline UI.
> 
> Can you add a comment like @see <filename/methodname> for information about
> the format of this object ?
> Even if fill, stroke and label are self-explanatory, group isn't.
> 

Will do.

Groups, btw, are each row in the timeline overview graph. Since multiple markers can be added on the same row (e.g. look at Chrome's tool), I added this property.

> @@ +28,5 @@
> > +    fill: "hsl(104,57%,71%)",
> > +    stroke: "hsl(104,57%,51%)",
> > +    label: L10N.getStr("format.reflow")
> > +  },
> > +  "DisplayList": {
> 
> s/DisplayList/Paint with the latest platform patch.
> 
> ::: browser/devtools/timeline/widgets/overview.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * 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";
> > +
> 
> Could you add a comment at the top of this file (and in fact, at the top of
> every files you added in this new folder) that explains what this file is
> supposed to be responsible for? This helps future maintainers (especially
> those that don't know the code) understand without reading the code, and it
> also somehow prevents it from becoming a dumping ground for unrelated things.
> 

Ok.

> @@ +35,5 @@
> > +const OVERVIEW_TIMELINE_STROKES = "#aaa";
> > +const OVERIVEW_MARKERS_COLOR_STOPS = [0, 0.1, 0.75, 1];
> > +const OVERVIEW_MARKER_DURATION_MIN = 4; // ms
> > +const OVERVIEW_GROUP_VERTICAL_PADDING = 6; // px
> > +const OVERVIEW_GROUP_ALTERNATING_BACKGROUND = "rgba(0,0,0,0.05)";
> 
> The timeline looks a bit weird with the black theme:
> https://dl.dropboxusercontent.com/u/714210/dark-theme-timeline.png
> Can you file a follow-up bug for this?
> 

Will do.

This is about canvas graphs not being properly themed yet. Everything else should be using theme colors already. There's bug 1049820 filed for the profiler's case, I'll file a new one for the timeline.

> @@ +67,5 @@
> > +    this._paintBatches = new Map();
> > +    this._lastGroup = 0;
> > +
> > +    for (let type in blueprint) {
> > +      this._paintBatches.set(type, { style: blueprint[type], queue: [] });
> 
> This is probably me nitpicking but I wouldn't name the list of markers per
> group "queue".
> This word gives the impression that it's a data storage that gets filled on
> one end and consumed on the other. Instead it's just a list of markers that
> gets emptied after each refresh and filled again after markers are received,
> but receiving markers and consuming them all always happens at the same rate.
> 

Well, it is, in fact, a queue. The first marker added is the first one to be drawn, so FIFO. But I'll rename it to something else.

> @@ +103,5 @@
> > +    let groupHeight = OVERVIEW_BODY_HEIGHT * this._pixelRatio / totalGroups;
> > +    let groupPadding = OVERVIEW_GROUP_VERTICAL_PADDING * this._pixelRatio;
> > +    let dataScale = this.dataScaleX = Math.min(1, availableWidth / oldestTimestamp);
> > +
> > +    // Draw the header and overivew background.
> 
> s/overivew/overview
> 
> @@ +169,5 @@
> > +        ctx.rect(left, top, width, height);
> > +      }
> > +
> > +      ctx.fill();
> > +      queue.length = 0;
> 
> This line requires commenting I think, this would make it easier to
> understand how the refresh process works.
> 

Ok.

> ::: browser/devtools/timeline/widgets/waterfall.js
> @@ +131,5 @@
> > +    let ticks = this._document.createElement("hbox");
> > +    ticks.className = "timeline-header-ticks";
> > +    ticks.setAttribute("align", "center");
> > +    ticks.setAttribute("flex", "1");
> > +    container.appendChild(ticks);
> 
> Re-creating these 4 container elements at each refresh looks like wasted
> effort. They should be created once and for all and just re-filled on
> refresh.
> 

It is, but I did a performance.now() check and the time wasted recreating them is basically 0. I don't want to be optimizing this code too much, it's already relatively complicated.

> @@ +175,5 @@
> > +        continue;
> > +      }
> > +      // Only build and display a finite number of markers initially, to
> > +      // preserve a snappy UI. After a certain delay, continue building the
> > +      // outstanding markers while there's (hopefully) no user interaction.
> 
> I really like this idea, especially because I'm guessing most users have the
> devtools docked to the bottom, and in this position, 30 markers is usually
> all you can see anyway, so you don't even realize there's a delay for the
> rest.
> I've had to make my window really big and resize the devtools to make them
> huge to start seeing the delay.
> I've got the weird impression that the delay is more than 100ms tough. But
> perhaps what I'm seeing is the time it takes to draw 30 markers + 100ms.
> Did you try making the delay shorter?
> 

I'll try to make it shorter. Yes, it is the cumulative time to draw 30 markers + 100ms, so that ends up being something like 120, maybe? I would like to leave enough room for ulterior user interaction (like dragging the selection continuously, or resizing it), and 100ms felt plenty. I'll experiment with smaller values though.

> @@ +336,5 @@
> > +   *
> > +   * @param number dataScale
> > +   *        @see Waterfall.prototype._buildMarkers
> > +   */
> > +  _drawWaterfallBackground: function(dataScale) {
> 
> I had to read this method's code several times to understand how it worked,
> which usually means it too complex or deserves a comment explaining the
> reasons that led to doing it.
> I'm pretty confident that using a canvas as a background image and setting
> pixel colors the way you did was done for a reason and performs fast. Can
> you add a comment block explaining this reasoning?
> Also, did you try other, simpler, techniques to draw the lines?
> 

I did. Plain CSS backgrounds are, interestingly, really slow. This seemed to be the fastest approach, and it's the same thing we use for the network monitor's timeline.

> ::: browser/locales/en-US/chrome/browser/devtools/timeline.properties
> @@ +7,5 @@
> > +# The correct localization of this file might be to keep it in
> > +# English, or another language commonly spoken among web developers.
> > +# You want to make that choice consistent across the developer tools.
> > +# A good criteria is the language in which you'd find the best
> > +# documentation on web development on the web.
> 
> This comment is obsolete, copy/pasted from the Profiler
> 

Huh? This is what we use for all our tools.

> ::: browser/themes/shared/devtools/timeline.inc.css
> @@ +24,5 @@
> > +}
> > +
> > +.timeline-list-contents {
> > +  /* Hack: force hardware acceleration */
> > +  transform: translateZ(-1px);
> 
> Isn't the "will-change" property supposed to avoid this?
> 

Yeah, but I don't know how and if it works yet. This one is a safe bet, and we can have a followup to fix all our uses of translateZ.

> @@ +32,5 @@
> > +
> > +.timeline-header-ticks,
> > +.timeline-marker-waterfall {
> > +  background-image: -moz-element(#waterfall-background);
> > +  background-repeat: repeat-y; /* Background created on a <canvas> in js. */
> 
> Please add something like @see browser/devtools/timeline/widgets/waterfall.js
> 

Ok.

> ::: toolkit/devtools/server/actors/timeline.js
> @@ +27,5 @@
> >  const {method, Arg, RetVal} = protocol;
> >  const events = require("sdk/event/core");
> >  const {setTimeout, clearTimeout} = require("sdk/timers");
> >  
> > +const DEFAULT_TIMELINE_DATA_PULL_TIMEOUT = 100; // ms
> 
> I think anything below 250ms feels kind of immediate to a user.
> I thought 300ms was already quite a rapid rate, especially given the amount
> of data we're transporting (not THAT much right now, but will grow when we
> add metadata to markers). What about adjusting this to 200ms instead?
> 

I remember Paul changing this to 100ms. I'll switch to 200ms and see how it feels. Probably fine, especially if I make the graph scroll and resize continuously while recording.

> @@ +98,5 @@
> >      }
> >    }),
> >  
> >    /**
> >     * Start/stop recording profile markers.
> 
> Missing jsdoc for the interval @param.
> If I'm not mistaken, this param isn't used in your patch anyway.
> What use case where you thinking about when you added it? Do we need to add
> this now?

It felt like a good idea. I can remove it.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Victor Porof [:vporof][:vp] from comment #15)
> > ::: browser/app/profile/firefox.js
> > @@ +1408,5 @@
> > >  // Enable the Web Audio Editor
> > >  pref("devtools.webaudioeditor.enabled", false);
> > >  
> > > +// Enable the timeline.
> > > +pref("devtools.timeline.enabled", true);
> > 
> > This will make the timeline visible by default. Do we want this?
> > I would say yes because performance tools are important.
> > 
> 
> I would say no because this is hardly a prototype. There's really not much
> useful data shown for the average web developer, and we want to merge the
> timeline and the profiler sometime in the future. It will be less confusing
> to the users if this stays disabled for now.
I'm good with disabling it by default for now.

> > ::: browser/devtools/timeline/timeline.js
> > @@ +91,5 @@
> > > +   *        A list of new markers collected since the last time this
> > > +   *        function was invoked.
> > > +   */
> > > +  _onMarkers: function(markers) {
> > > +    Array.prototype.push.apply(this._markers, markers);
> > 
> > Or this._markers.push(...markers);
> > 
> 
> Not really, there's a difference:
> * array.push(otherArray) pushes the other array (as an object) into the first
> * Array.prototype.push.apply(array, otherArray) pushes all the *elements* of
> the other array into the first
I meant array.push(...otherArray) not array.push(otherArray) which, indeed, would be wrong. But anyway, that's not important.

> > ::: browser/locales/en-US/chrome/browser/devtools/timeline.properties
> > @@ +7,5 @@
> > > +# The correct localization of this file might be to keep it in
> > > +# English, or another language commonly spoken among web developers.
> > > +# You want to make that choice consistent across the developer tools.
> > > +# A good criteria is the language in which you'd find the best
> > > +# documentation on web development on the web.
> > 
> > This comment is obsolete, copy/pasted from the Profiler
> > 
> 
> Huh? This is what we use for all our tools.
Yeah, sorry I didn't add the comment at the right line (or splinter doesn't extract enough code from the file). What I meant was that there's a comment at the very top of this file that explicitly mentions the "Profiler" instead of the "Timeline".

> > ::: browser/themes/shared/devtools/timeline.inc.css
> > @@ +24,5 @@
> > > +}
> > > +
> > > +.timeline-list-contents {
> > > +  /* Hack: force hardware acceleration */
> > > +  transform: translateZ(-1px);
> > 
> > Isn't the "will-change" property supposed to avoid this?
> > 
> 
> Yeah, but I don't know how and if it works yet. This one is a safe bet, and
> we can have a followup to fix all our uses of translateZ.
Makes sense.

Comment 17

[Victor Porof [:vporof][:vp]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> (In reply to Victor Porof [:vporof][:vp] from comment #15)
> > 
> > Not really, there's a difference:
> > * array.push(otherArray) pushes the other array (as an object) into the first
> > * Array.prototype.push.apply(array, otherArray) pushes all the *elements* of
> > the other array into the first
>
> I meant array.push(...otherArray) not array.push(otherArray) which, indeed,
> would be wrong. But anyway, that's not important.
> 

Whoops I misread you! Sorry!

Comment 18

[Victor Porof [:vporof][:vp]]

Attachment: v2

Rebased, addressed comments.

Comment 19

[Paul Rouget [:paul]]

(In reply to Victor Porof [:vporof][:vp] from comment #18)
> Created attachment 8485923 [details] [diff] [review]
> v2
> 
> Rebased, addressed comments.

Does this patch need a review?

Comment 20

[Victor Porof [:vporof][:vp]]

Not yet.

Comment 21

[Victor Porof [:vporof][:vp]]

Attachment: v3

With tests.

Comment 22

[Victor Porof [:vporof][:vp]]

Attachment: v4

With tests and no typos in global.js :)

Comment 23

[Victor Porof [:vporof][:vp]]

Comment on attachment 8486542 [details] [diff] [review]
v4

Also Paul, can you play with this a bit as well?

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8486542 [details] [diff] [review]
v4

Review of attachment 8486542 [details] [diff] [review]:
-----------------------------------------------------------------

Code changes look good to me.
I've also quickly played with the patch, it feels pretty good.
My opinion would be to ship it now and work on it incrementally after that, but I'll let Paul have a say in it too.

::: browser/devtools/timeline/test/browser.ini
@@ +12,5 @@
> +[browser_timeline_panels.js]
> +[browser_timeline_recording.js]
> +[browser_timeline_waterfall-background.js]
> +[browser_timeline_waterfall-generic.js]
> +[browser_timeline_waterfall-styles.js]

Did you run these new tests with --e10s cmd line option to check that they pass in e10s mode too?
If some of them don't, please add skip conditions here and file a follow-up bug.

::: browser/devtools/timeline/test/head.js
@@ +67,5 @@
> +  gBrowser.removeTab(tab);
> +  return deferred.promise;
> +}
> +
> +function* initFrontend(url) {

initTimelinePanel or something like that would probably be a better name.
I think this function deserves a jsdoc comment because:
- it calls addTab and opens the toolbox, on the timeline panel, so it's really the main helper function tests will be using, so it should be documented as such
- and it is a generator function to yields promises, so it must be run from within a task. Either use Task.async/spawn here and make it not be a generator function and document the fact that it returns a promise OR keep it as such but document the fact that it needs to be run within a task.

@@ +81,5 @@
> +  let panel = toolbox.getCurrentPanel();
> +  return [target, debuggee, panel];
> +}
> +
> +function* teardown(panel) {

Same here. Using generators for our tests has become so useful that we do that all over the place, but it will not be straightforward for someone looking at the code for the first time.

::: browser/locales/en-US/chrome/browser/devtools/timeline.properties
@@ +32,5 @@
> +# LOCALIZATION NOTE (timeline.records):
> +# This string is displayed in the timeline waterfall, as a title for the menu.
> +timeline.records=RECORDS
> +
> +# LOCALIZATION NOTE (label.*):

s/label.*/timeline.label.*

Comment 25

[Paul Rouget [:paul]]

Comment on attachment 8486542 [details] [diff] [review]
v4

This is far beyond what I was expecting for a v1. This is really awesome victor.

2 nits to address later:
- we need an dedicated icon, or maybe re-use the one from the network or performance panel
- when dragging the selection window far to the left, when its left boundary reaches the 0ms limit, it doesn't resize the selection window, but keep panning it. Which moves the waterfall into negative values. It's not bad per se, but unexpected.

Comment 26

[Victor Porof [:vporof][:vp]]

GREEN AS FUCK https://tbpl.mozilla.org/?tree=Try&rev=64d383f18136

Comment 27

[Victor Porof [:vporof][:vp]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #25)
> Comment on attachment 8486542 [details] [diff] [review]
> 2 nits to address later:
> - we need an dedicated icon, or maybe re-use the one from the network or
> performance panel

I changed it to use the netmonitor icon for now. However, it probably won't matter in the end, because we're unifying the tools anyway, right?

> - when dragging the selection window far to the left, when its left boundary
> reaches the 0ms limit, it doesn't resize the selection window, but keep
> panning it. Which moves the waterfall into negative values. It's not bad per
> se, but unexpected.

It's a feature!
I totally planned that.

(agreed though)

Comment 28

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/e3a037fb152f

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out for failures on WinXP ~50% of the time.
https://hg.mozilla.org/integration/fx-team/rev/4e2bb1833c0f

https://tbpl.mozilla.org/php/getParsedLog.php?id=47789354&tree=Fx-Team

Comment 30

[Victor Porof [:vporof][:vp]]

Got 178.79999999999995, expected 178.79999999999998

*shakes fist*

Comment 31

[Victor Porof [:vporof][:vp]]

Fixed failures and relanded:

https://hg.mozilla.org/integration/fx-team/rev/0e3832506e00

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e3832506e00

Comment 33

[Andrei Vaida [:avaida]]

Regression and Exploratory testing found no major issues across the following platforms: Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.8.5. Marking this patch as verified fixed on Aurora 35.0a2 (2014-11-03 / 20141103174737).

Comment 34

[David Lawrence [:dkl]]

Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl