[timeline] marker sidebar (select markers in the timeline)

VERIFIED FIXED in Firefox 36

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 36
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox36 verified, firefox37 verified)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
Selecting a marker in the waterfall should display marker's details in a sidebar.
(Assignee)

Comment 1

3 years ago
Created attachment 8496733 [details] [diff] [review]
v0.1

Victor, this is a very early patch, but before moving forward, I'd like you to take a quick look first.

Initially, I was hiding the sidebar by default, but then clicking on a marker would trigger a waterfall reconstruction. A reconstruction is not cheap, and then we need to restore both the selection and the scroll position, which is not easy. So here, I show the sidebar by default. Only a resize triggers a reconstruction.

Let me know what you think (don't go too much into the details, it's an early draft).
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8496733 - Flags: feedback?(vporof)
Comment on attachment 8496733 [details] [diff] [review]
v0.1

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

Yeah, all the logic for keeping a selection is (not necessarily messy, but) a lot of code. Let's not go this route, always show the sidebar.

::: browser/devtools/timeline/timeline.js
@@ +234,5 @@
>        let end = start + this.overview.width * OVERVIEW_INITIAL_SELECTION_RATIO;
>        this.overview.setSelection({ start, end });
>      } else {
>        let duration = markers.endTime - markers.startTime;
> +      this.waterfall.setData(markers, 0, duration, false);

Nit: a stray `false` argument means that a switch to an options object is appropriate.

::: browser/devtools/timeline/widgets/markerDetails.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +/**
> + * This file contains the rendering code for any markers. Every markers have

This comment is confusing.

@@ +20,5 @@
> +  EventEmitter.decorate(this);
> +  this._document = parent.ownerDocument;
> +  this._parent = parent;
> +  this._splitter = this._document.querySelector("#timeline-waterfall-container > splitter");
> +  this._splitter.addEventListener("mouseup", () => { this.emit("resize") });

I think you can actually use the "resize" event on splitters. But I'm not sure.
Attachment #8496733 - Flags: feedback?(vporof)
"Let's not go this route, always show the sidebar."

I mean, let's not go this route, but always show the sidebar instead.
(Assignee)

Comment 4

3 years ago
Victor, is it worth it to land that? Or do you think it's better to wait until we get the perf++ tab?
Flags: needinfo?(vporof)
Let's land this. It should be easy to transplant.
Flags: needinfo?(vporof)
(Assignee)

Comment 6

3 years ago
Created attachment 8522017 [details] [diff] [review]
v1
Attachment #8496733 - Attachment is obsolete: true
Attachment #8522017 - Flags: review?(vporof)
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=78855b424be3

Comment 8

3 years ago
Comment on attachment 8522017 [details] [diff] [review]
v1

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

::: browser/devtools/timeline/timeline.xul
@@ +63,5 @@
>        </hbox>
>  
> +      <hbox id="timeline-waterfall-container" flex="1">
> +        <vbox id="timeline-waterfall" flex="1"/>
> +        <splitter class="devtools-vertical-splitter"/>

Isn't the right class .devtools-side-splitter ?
(Assignee)

Comment 9

3 years ago
Created attachment 8523785 [details] [diff] [review]
v1.1 - rebased
Attachment #8522017 - Attachment is obsolete: true
Attachment #8522017 - Flags: review?(vporof)
Attachment #8523785 - Flags: review?(vporof)
(Assignee)

Comment 10

3 years ago
(In reply to Tim Nguyen [:ntim] from comment #8)
> Isn't the right class .devtools-side-splitter ?

Erf, yes. Thanks :)
(Assignee)

Comment 11

3 years ago
Created attachment 8523791 [details] [diff] [review]
v1.2 - rebased (with correct splitter)
Attachment #8523785 - Attachment is obsolete: true
Attachment #8523785 - Flags: review?(vporof)
Attachment #8523791 - Flags: review?(vporof)
Comment on attachment 8523791 [details] [diff] [review]
v1.2 - rebased (with correct splitter)

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

r+ with nits.

::: browser/devtools/timeline/test/browser_timeline_waterfall-sidebar.js
@@ +12,5 @@
> +  yield TimelineController.toggleRecording();
> +  ok(true, "Recording has started.");
> +
> +  let updated = 0;
> +  panel.panelWin.on(EVENTS.OVERVIEW_UPDATED, () => updated++);

You're not using `updated` in this test. Remove this.

@@ +26,5 @@
> +  yield TimelineController.toggleRecording();
> +  ok(true, "Recording has ended.");
> +
> +  // Select everything
> +  TimelineView.markersOverview.setSelection({start:0, end:panel.panelWin.innerWidth});

Nit: You can use setSelection({ start: 0, end: TimelineView.markersOverview.width }) instead of messing with innerWidth.

::: browser/devtools/timeline/widgets/marker-details.js
@@ +5,5 @@
> +"use strict";
> +
> +/**
> + * This file contains the rendering code for the marker sidebar, displayed
> + * when a marker is selected.

It's actually always displayed, isn't it?

@@ +53,5 @@
> +    this.empty();
> +
> +    let title = this.buildMarkerTypeLabel(marker.name);
> +
> +    let toMs = ms => Math.round(ms * 100) / 100 + "ms";

This needs to be localized, even the number itself, so it's "12,34" or "12.34" where necessary. Use ViewHelpers.L10N.numberWithDecimals to do that.

::: browser/devtools/timeline/widgets/waterfall.js
@@ +109,5 @@
> +  /**
> +   * Keybindings.
> +   */
> +  setupKeys: function() {
> +    this._document.addEventListener("keydown", e => {

Why add the event listener on the document instead of just the waterfall container?

@@ +325,5 @@
> +   * @param number idx
> +   *        Index of the row to select. -1 clears the selection.
> +   */
> +  selectRow: function(idx) {
> +    this._selectedRow = idx;

Nit: rename this to selectedRowIdx, so it's move obvious that it's a number.

@@ +328,5 @@
> +  selectRow: function(idx) {
> +    this._selectedRow = idx;
> +
> +    // Unselect
> +    let prev = this._listContents.querySelector(".waterfall-marker-container.selected");

Nit: could just use this._listContents.children[this._selectedRowIdx], right? (before setting the new this._selectedRowIdx).

@@ +382,5 @@
> +    }
> +    yDelta = parentRect.bottom - rowRect.bottom;
> +    if (yDelta < 0) {
> +      parent.scrollTop -= yDelta;
> +    }

Can't you use parent.ensureElementIsVisible (assuming parent can query the nsIScrollBoxObject interface). Otherwise, just row.scrollIntoView()?
Attachment #8523791 - Flags: review?(vporof) → review+
Created attachment 8524772 [details]
Screen Shot 2014-11-18 at 2.20.27 PM.png

Also, can you make sure that the borders on selected marker bars are always white, or a more contrasty color? See screenshot.
Another thing: when the toolbox is docked to the side, can the marker sidebar be displayed on the bottom, to save some horizontal space?

Comment 15

3 years ago
Created attachment 8525492 [details] [diff] [review]
add DOM Event details to marker sidebar

This adds a bit more metadata to the sidebar.  Now that the marker
details patch is in, I think this (or something like it) should be
rolled into the fix for this bug.
Blocks: 1050770
(Assignee)

Comment 16

3 years ago
Created attachment 8525972 [details] [diff] [review]
interdiff
Attachment #8525492 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8525973 [details] [diff] [review]
v2

Victor, I addressed your comments and changed a couple of other things (to show the metadata too).

Another review would be welcome. Attach the interdiff.
Attachment #8523791 - Attachment is obsolete: true
Attachment #8525973 - Flags: review?(vporof)
(Assignee)

Comment 18

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=de6b9c29cbac
Comment on attachment 8525972 [details] [diff] [review]
interdiff

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

::: browser/devtools/timeline/timeline.xul
@@ +72,3 @@
>          <vbox id="timeline-waterfall" flex="1"/>
>          <splitter class="devtools-side-splitter"/>
> +        <vbox id="timeline-waterfall-details" class="theme-sidebar" width="150" height="150"/>

At some point we'll want these dimensions to be user-configurable. File a followup bug to use prefs for the width and height (assuming that's for when the toolbox is docked to the side?).

::: browser/devtools/timeline/widgets/waterfall.js
@@ +334,5 @@
> +      let bar = prev.querySelector(".waterfall-marker-bar");
> +      // Restore border color.
> +      if (bar) {
> +        bar.style.borderColor = bar.getAttribute("borderColor");
> +      }

Can't this simply be done in css? If there's a selected class, border-color: something !important.
Comment on attachment 8525973 [details] [diff] [review]
v2

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

See interdiff review above.
Attachment #8525973 - Flags: review?(vporof) → review+
Created attachment 8526273 [details]
Screen Shot 2014-11-20 at 3.28.18 PM.png

You'll also probably want to change the border color in the left sidebar.
(Assignee)

Comment 22

3 years ago
> ::: browser/devtools/timeline/widgets/waterfall.js
> @@ +334,5 @@
> > +      let bar = prev.querySelector(".waterfall-marker-bar");
> > +      // Restore border color.
> > +      if (bar) {
> > +        bar.style.borderColor = bar.getAttribute("borderColor");
> > +      }
> 
> Can't this simply be done in css? If there's a selected class, border-color:
> something !important.

I don't see how. Styles (foo.style && <foo style="">) have higher priority than any CSS rules.

Comment 23

3 years ago
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #22)
> > ::: browser/devtools/timeline/widgets/waterfall.js
> > @@ +334,5 @@
> > > +      let bar = prev.querySelector(".waterfall-marker-bar");
> > > +      // Restore border color.
> > > +      if (bar) {
> > > +        bar.style.borderColor = bar.getAttribute("borderColor");
> > > +      }
> > 
> > Can't this simply be done in css? If there's a selected class, border-color:
> > something !important.
> 
> I don't see how. Styles (foo.style && <foo style="">) have higher priority
> than any CSS rules.

Unless you've set !important; inside foo.style or <foo style="">, you can still override it from the stylesheet using !important;.
(Assignee)

Comment 24

3 years ago
You're both right about !important. Fixed it.

https://tbpl.mozilla.org/?tree=Fx-Team&rev=c3d184a01153
(Assignee)

Updated

3 years ago
Blocks: 1103829
https://hg.mozilla.org/mozilla-central/rev/c3d184a01153
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Flags: qe-verify+
Blocks: 1107849
Verified fixed on Nightly 37.0a1 (2014-12-04) and Aurora 36.0a2 (2014-12-04), using Windows 7 64-bit, Ubuntu 14.04 LTS 64-bit and Mac OS X 10.9.5.

Filed Bug 1107849 for a resizing issue affecting the sidebar.
Status: RESOLVED → VERIFIED
status-firefox36: --- → verified
status-firefox37: --- → verified
Sorry for the spam. Moving bugs to Firefox :: Developer Tools: Performance Tools (Profiler/Timeline).

dkl
Component: Developer Tools: Timeline → Developer Tools: Performance Tools (Profiler/Timeline)
You need to log in before you can comment on or make changes to this bug.