Closed
Bug 1074106
Opened 10 years ago
Closed 9 years ago
[timeline] marker sidebar (select markers in the timeline)
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox36 verified, firefox37 verified)
VERIFIED
FIXED
Firefox 36
People
(Reporter: paul, Assigned: paul)
References
Details
Attachments
(4 files, 5 obsolete files)
5.46 KB,
image/png
|
Details | |
18.04 KB,
patch
|
Details | Diff | Splinter Review | |
37.51 KB,
patch
|
vporof
:
review+
|
Details | Diff | Splinter Review |
9.34 KB,
image/png
|
Details |
Selecting a marker in the waterfall should display marker's details in a sidebar.
Assignee | ||
Comment 1•10 years ago
|
||
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).
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
"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•10 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)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8496733 -
Attachment is obsolete: true
Attachment #8522017 -
Flags: review?(vporof)
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=78855b424be3
Comment 8•10 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•10 years ago
|
||
Attachment #8522017 -
Attachment is obsolete: true
Attachment #8522017 -
Flags: review?(vporof)
Attachment #8523785 -
Flags: review?(vporof)
Assignee | ||
Comment 10•10 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•10 years ago
|
||
Attachment #8523785 -
Attachment is obsolete: true
Attachment #8523785 -
Flags: review?(vporof)
Attachment #8523791 -
Flags: review?(vporof)
Comment 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
Also, can you make sure that the borders on selected marker bars are always white, or a more contrasty color? See screenshot.
Comment 14•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8525492 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
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•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=de6b9c29cbac
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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+
Comment 21•10 years ago
|
||
You'll also probably want to change the border color in the left sidebar.
Assignee | ||
Comment 22•10 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•10 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•9 years ago
|
||
You're both right about !important. Fixed it. https://tbpl.mozilla.org/?tree=Fx-Team&rev=c3d184a01153
Comment 25•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3d184a01153
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•9 years ago
|
Flags: qe-verify+
Comment 26•9 years ago
|
||
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.
Comment 27•9 years ago
|
||
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)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•