1087877 - [timeline] user should be able to filter out any type of marker

Status: RESOLVED FIXED

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

Comment 1

[Paul Rouget [:paul]]

Attachment: 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.

Comment 2

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

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.

Comment 3

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

(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.

Comment 4

[Paul Rouget [:paul]]

Attachment: v0.2

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 6

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

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 7

[Paul Rouget [:paul]]

Attachment: v1

Comment 8

[Paul Rouget [:paul]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c21d4f27710

Comment 9

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

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.

Comment 10

[Paul Rouget [:paul]]

(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.

Comment 11

[Paul Rouget [:paul]]

Attachment: v1.1

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

Comment 12

[Paul Rouget [:paul]]

Attachment: v1.2

https://tbpl.mozilla.org/?tree=Try&rev=04e8781a843b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Paul Rouget [:paul]]

Attachment: 1.3

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dcf56ba0cac2

Comment 17

[David Lawrence [:dkl]]

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

dkl