Closed Bug 1074106 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86_64
All
defect
Not set

Tracking

(firefox36 verified, firefox37 verified)

VERIFIED FIXED
Firefox 36
Tracking Status
firefox36 --- verified
firefox37 --- verified

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(4 files, 5 obsolete files)

Selecting a marker in the waterfall should display marker's details in a sidebar.
Attached patch v0.1 (obsolete) — Splinter Review
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.
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)
Attached patch v1 (obsolete) — Splinter Review
Attachment #8496733 - Attachment is obsolete: true
Attachment #8522017 - Flags: review?(vporof)
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 ?
Attached patch v1.1 - rebased (obsolete) — Splinter Review
Attachment #8522017 - Attachment is obsolete: true
Attachment #8522017 - Flags: review?(vporof)
Attachment #8523785 - Flags: review?(vporof)
(In reply to Tim Nguyen [:ntim] from comment #8)
> Isn't the right class .devtools-side-splitter ?

Erf, yes. Thanks :)
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+
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?
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
Attached patch interdiffSplinter Review
Attachment #8525492 - Attachment is obsolete: true
Attached patch v2Splinter Review
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)
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+
You'll also probably want to change the border color in the left sidebar.
> ::: 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.
(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;.
You're both right about !important. Fixed it.

https://tbpl.mozilla.org/?tree=Fx-Team&rev=c3d184a01153
Blocks: 1103829
https://hg.mozilla.org/mozilla-central/rev/c3d184a01153
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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
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)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.