Victor, I need advice. Once the list of visible markers has changed, I want to update the overview. Not drawing the markers is easy (I just update the local blueprint), but I'm not sure about how to resize the overview. I can destroy and rebuild the overview, that sounds easy. But maybe you'd think of a cleaner way to do that (that would mean digging into Graphs.jsm). Attached the WIP patch.

Comment on attachment 8523810 [details] [diff] [review] v0.1 Review of attachment 8523810 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/timeline/timeline.js @@ +438,5 @@ > this.waterfall.recalculateBounds(); > this.updateWaterfall(); > + }, > + > + _onVisibleMarkersChanged: function(e) { Nit: document even the private functions. @@ +445,5 @@ > + for (let item of items) { > + if (item.hasAttribute("checked")) { > + visibleMarkers.push(item.getAttribute("marker-type")); > + } > + } Instead of all these crazy for loops, why not just let visibleMarkers = Array.map($$("#timelineFilterPopup > [marker-type][checked]"), e => e.getAttribute("marker-type")); @@ +447,5 @@ > + visibleMarkers.push(item.getAttribute("marker-type")); > + } > + } > + let str = JSON.stringify(visibleMarkers); > + Services.prefs.setCharPref("devtools.timeline.visibleMarkers", str); We're trying to use the Prefs helper everywhere. E.g. http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/profiler.js#217 To set a pref, Prefs.visibleMarkers = str; @@ +457,5 @@ > + let popup = $("#timelineFilterPopup"); > + let button = $("#filter-button"); > + > + popup.addEventListener("popupshowing", () => button.setAttribute("open", "true")); > + popup.addEventListener("popuphiding", () => button.removeAttribute("open", "true")); Don't use a second argument for removeAttribute. @@ +459,5 @@ > + > + popup.addEventListener("popupshowing", () => button.setAttribute("open", "true")); > + popup.addEventListener("popuphiding", () => button.removeAttribute("open", "true")); > + > + let visibleMarkers = Services.prefs.getCharPref("devtools.timeline.visibleMarkers"); Prefs @@ +460,5 @@ > + popup.addEventListener("popupshowing", () => button.setAttribute("open", "true")); > + popup.addEventListener("popuphiding", () => button.removeAttribute("open", "true")); > + > + let visibleMarkers = Services.prefs.getCharPref("devtools.timeline.visibleMarkers"); > + visibleMarkers = JSON.parse(visibleMarkers); You should handle the case when the parsing fails, because users can manually edit these prefs. @@ +470,5 @@ > + menuitem.setAttribute("closemenu", "none"); > + menuitem.setAttribute("type", "checkbox"); > + menuitem.setAttribute("marker-type", type); > + > + this._onVisibleMarkersChanged = this._onVisibleMarkersChanged.bind(this); Don't bind this function on every iteration of the loop. @@ +473,5 @@ > + > + this._onVisibleMarkersChanged = this._onVisibleMarkersChanged.bind(this); > + menuitem.addEventListener("command", this._onVisibleMarkersChanged); > + > + if (visibleMarkers.indexOf(type) > -1) { Uber nit: use != instead of >, it makes it more obvious. ::: browser/devtools/timeline/timeline.xul @@ +31,5 @@ > tooltiptext="&timelineUI.recordButton.tooltip;"/> > + <toolbarbutton id="filter-button" > + popup="timelineFilterPopup" > + class="devtools-toolbarbutton" > + tooltiptext="xxx"/> Don't forget to localize this. ::: browser/devtools/timeline/widgets/markers-overview.js @@ +62,5 @@ > }); > } > > MarkersOverview.prototype = Heritage.extend(AbstractCanvasGraph.prototype, { > + fixedHeight: OVERVIEW_HEADER_HEIGHT + OVERVIEW_ROW_HEIGHT * 5, This 5 should be a const somewhere. @@ +82,5 @@ > this._lastGroup = Math.max(this._lastGroup, blueprint[type].group); > } > }, > > + setVisibleMarkers: function(visibleMarkers) { Nit: document this. @@ +90,5 @@ > + filteredBluePrint[type] = TIMELINE_BLUEPRINT[type]; > + } > + } > + this.setBlueprint(filteredBluePrint); > + this.buildGraphImage(); Yeah, you'll always have to do this. The canvas itself needs to be resized, and that will nuke the context, so there's no way to avoid this. @@ +121,5 @@ > // draw all markers sharing the same style at once. > > for (let marker of markers) { > + if (this._paintBatches.has(marker.name)) { > + this._paintBatches.get(marker.name).batch.push(marker); This does a double lookup in the paintBatches map, which can be slow. Please rewrite this as for (let marker of markers) { let storeForMarkerType = this._paintBatches.get(marker.name); if (storeForMarkerType) { storeForMarkerType.batch.push(marker); } } ::: browser/devtools/timeline/widgets/waterfall.js @@ +134,5 @@ > if (e.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN) { > e.preventDefault(); > this.selectNearestRow(this._selectedRow + WATERFALL_ROWCOUNT_ONPAGEUPDOWN); > } > + }, false); Eh? You probably want to change this in another patch? @@ +355,5 @@ > selectNearestRow: function(idx) { > if (idx < 0) { > idx = 0; > } > + if (!this._listContents.querySelector(".waterfall-marker-container:not([is-spacer])")) { This is pretty ugly. I'll need to review your other patch as well.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #1) > Created attachment 8523810 [details] [diff] [review] > v0.1 > > Victor, I need advice. Once the list of visible markers has changed, I want > to update the overview. Not drawing the markers is easy (I just update the > local blueprint), but I'm not sure about how to resize the overview. I can > destroy and rebuild the overview, that sounds easy. But maybe you'd think of > a cleaner way to do that (that would mean digging into Graphs.jsm). > > Attached the WIP patch. tl; dr: Yeah, you'll always have to do this. The canvas itself needs to be resized, and that will nuke the context, so there's no way to avoid this.

I do 2 things in this patch I'm not happy with: - destroying the overview; - the .sort().reduce().reduce() magic in _getFilteredBluePrint. What do you recommend?

Comment on attachment 8527451 [details] [diff] [review] v0.2 Review of attachment 8527451 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/timeline/timeline.js @@ +495,5 @@ > + let filteredBluePrint = bluePrintAsArray.reduce((p,c) => { > + p[c.type] = c.value; > + return p; > + }, {}); > + return filteredBluePrint; This is fantastically convoluted, and unfortunately for good reason. However, I'd write it like so: let filteredBlueprint = Cu.cloneInto(TIMELINE_BLUEPRINT, {}); let maybeRemovedGroups = new Set(); let removedGroups = new Set(); // 1. Remove hidden markers from the blueprint. for (let hiddenMarkerName of hiddenMarkers) { maybeRemovedGroups.add(filteredBlueprint[hiddenMarkerName].group); delete filteredBluePrint[hiddenMarkerName]; } // 2. Get a list of all the groups that will be removed. for (let removedGroup of maybeRemovedGroups) { let markerNames = Object.keys(filteredBluePrint); let allGroupsRemoved = markerNames.every(e => filteredBlueprint[e].group != removedGroup); if (allGroupsRemoved) { removedGroups.add(removedGroup); } } // 3. Offset groups. for (let removedGroup of removedGroups) { for (let [, markerDetails] of Iterator(filteredBlueprint)) { if (markerDetails.group > removedGroup) { markerDetails.group--; } } } It's a bit shorter and easier to digest IMHO. @@ +504,5 @@ > + * and overview. > + */ > + _onHiddenMarkersChanged: Task.async(function(e) { > + let hiddenMarkers = Array.map($$("#timelineFilterPopup > [marker-type]:not([checked])"), > + e => e.getAttribute("marker-type")); Nit: let menuItems = $$("#timelineFilterPopup > [marker-type]:not([checked])"); let hiddenMarkers = Array.map(menuItems, e => e.getAttribute("marker-type")); @@ +532,5 @@ > + let markers = TimelineController.getMarkers(); > + this.markersOverview.setData({ interval, markers }); > + this.markersOverview.selectionEnabled = true; > + this.markersOverview.setSelection(selection); > + } This is indeed quite gross. All you need to do is this.markersOverview.setBlueprint(blueprint); this.markersOverview.refresh({ force: true }); and add a options param in AbstractCanvasGraph.prototype.refresh to force refresh even if the dimensions haven't changed. @@ +547,5 @@ > + popup.addEventListener("popuphiding", () => button.removeAttribute("open")); > + > + this._onHiddenMarkersChanged = this._onHiddenMarkersChanged.bind(this); > + > + for (let type in TIMELINE_BLUEPRINT) { Nit: for (let [markerName, markerDetails] of Iterator(TIMELINE_BLUEPRINT)) { ... } for a nicer loop and better variable names. ::: browser/devtools/timeline/widgets/markers-overview.js @@ +66,5 @@ > selectionLineColor: OVERVIEW_SELECTION_LINE_COLOR, > selectionBackgroundColor: OVERVIEW_SELECTION_BACKGROUND_COLOR, > selectionStripesColor: OVERVIEW_SELECTION_STRIPES_COLOR, > > + get fixedHeight() { Nit: add a comment describing how this is dependent on a blueprint being set beforehand and the relationship to groups. ::: browser/devtools/timeline/widgets/waterfall.js @@ +103,5 @@ > this._buildMarkers(this._listContents, markers, startTime, endTime, dataScale); > this.selectRow(this._selectedRowIdx); > }, > > + setBlueprint: function(blueprint) { Nit: document all methods, like for markers-overview.js

Comment on attachment 8534816 [details] [diff] [review] v1 Review of attachment 8534816 [details] [diff] [review]: ----------------------------------------------------------------- Great success ::: browser/devtools/timeline/test/browser_timeline_filters.js @@ +40,5 @@ > + ok($(".waterfall-marker-bar[type=Paint]"), "Found at least one 'Paint' marker"); > + > + let heightBefore = overview.fixedHeight; > + EventUtils.synthesizeMouseAtCenter(menuItem1, {type: "mouseup"}, panel.panelWin); > + yield nextTick(); Are you using this to wait for an animation frame on the canvas? This might be flimsy, but we'll see.

(In reply to Victor Porof [:vporof][:vp] from comment #9) > Comment on attachment 8534816 [details] [diff] [review] > v1 > > Review of attachment 8534816 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great success > > ::: browser/devtools/timeline/test/browser_timeline_filters.js > @@ +40,5 @@ > > + ok($(".waterfall-marker-bar[type=Paint]"), "Found at least one 'Paint' marker"); > > + > > + let heightBefore = overview.fixedHeight; > > + EventUtils.synthesizeMouseAtCenter(menuItem1, {type: "mouseup"}, panel.panelWin); > > + yield nextTick(); > > Are you using this to wait for an animation frame on the canvas? This might > be flimsy, but we'll see. No reason. But there's a failure. And I think it's because of how I "hacked" the menuitems to add the bullet.

I had to change a bit how menuitems are built. Wrapping them in hboxes only worked on Windows, and I think it was the reason of tbpl failures on Mac. https://tbpl.mozilla.org/?tree=Try&rev=748c235ff6c9

Backed out for e10s mochitest-dt orange. https://hg.mozilla.org/integration/fx-team/rev/3daf1f629233 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1440257&repo=fx-team

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