1034669 - Add a way of fading out certain areas of the canvas graphs

Status: RESOLVED FIXED

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

Description

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

The existing selection mechanism is fun and all, but a way of fading out certain parts of the graphs is also needed. See attachment 8447297 [details] for an example.

Comment 1

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

Attachment: v1

Implements the required behavior. Also, fixes an off-by-one error in the bar graph rendering algorithm which I spotted while writing tests for this – essentially, the last bar would never get rendered.

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8451172 [details] [diff] [review]
v1

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

Looking pretty good.
I wasn't able to try it though, it seems you've deleted your framerate addon and I didn't keep it locally.
I could always apply the giant profiler mega patch though ...
Anyway, here are some comments. Globally I'm fine with the changes, these are mainly minor comments.
I only have one no so minor comment about the relation between snapEdges and blocksBoundingRects, they seem to be very similar and built around the same time. It seems like it would be possible to use only blocksBoundingRects and get rid of snapEdges.

::: browser/devtools/shared/test/browser_graphs-02.js
@@ +77,5 @@
> +    "The graph should now have the highlights set.");
> +
> +  graph.setHighlights([]);
> +  ok(graph.hasHighlights(),
> +    "The graph should shouldn't have anything highlighted.");

"should shouldn't" ?
By the way, do you really want an empty array to be considered as highlights? I think filtering for missing and empty arrays in setHighlights might be better.

@@ +81,5 @@
> +    "The graph should shouldn't have anything highlighted.");
> +
> +  graph.setHighlights(null);
> +  ok(!graph.hasHighlights(),
> +    "The graph should should have everything highlighted.");

should or should ? :)

::: browser/devtools/shared/test/browser_graphs-11b.js
@@ +51,5 @@
> +  }, {
> +    delta: 600, values: [3, 2, 0]
> +  }]);
> +
> +  is(graph.snapEdges.toSource(), "[{start:0, end:66.66666666666666}, {start:68.66666666666666, end:133.33333333333331}, {start:135.33333333333331, end:200}, {start:202, end:266.66666666666663}, {start:268.66666666666663, end:333.3333333333333}, {start:335.3333333333333, end:400}]",

For this test's sake I think we should be rounding these values. It would make it easier to read and maintain I think. Also, are we sure that these floating point numbers are going to be exactly the same across all test platforms?

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +30,5 @@
>  const GRAPH_REGION_LINE_WIDTH = 1; // px
>  const GRAPH_REGION_LINE_COLOR = "rgba(237,38,85,0.8)";
>  
> +const GRAPH_HIGHLIGHTS_MASK_BACKGROUND = "rgba(255,255,255,0.75)";
> +const GRAPH_HIGHLIGHTS_MASK_STRIPES = "rgba(255,255,255,0.5)";

Just a question: what's the plan for making these colors part of our theme and change when we switch from light to dark?
I think we want at some stage to move from our preprocessed CSS to using CSS variables, but I see more and more places where we also need theme color variables in JS. So I think we should start thinking putting all of our variables in properties file instead.

@@ +376,5 @@
> +    let height = this._height;
> +
> +    // Draw the background mask.
> +
> +    let pattern = AbstractCanvasGraph.getStripePattern({

I can't really see it all that well in the attached GIF, but don't you think a plain background would be preferable here? If the goal is just to hide some parts of the graph, I tend to think a monochrome semi-transparent background would be enough, especially that selections already have stripes.

@@ -650,1 @@
>      if (this._cachedHighlightMask) {

I think this patch might need rebasing since the patch it's built on has changed.

@@ +1536,5 @@
>      // Iterate over the blocks, then the bars, to draw all rectangles of
>      // the same color in a single pass. See the @constructor for more
>      // information about the data source, and how a "bar" contains "blocks".
>  
> +    this._blocksBoundingRects = [];

I see why we need this, but why don't we also use it to replace snapEdges? It seems that some of the same information is stored in both these arrays.
Especially that they're both populated at the same time.

@@ +1674,5 @@
>      itemNode.className = "bar-graph-widget-legend-item";
>  
>      let colorNode = this._document.createElementNS(HTML_NS, "span");
>      colorNode.setAttribute("view", "color");
> +    colorNode.setAttribute("index", this._legendNode.childNodes.length);

nit: I think data attributes are a little bit nicer to use thanks to the dataset API.
This would allow you to do e.target.dataset.index in _onLegendMouseOver.

@@ +1679,5 @@
>      colorNode.style.backgroundColor = color;
> +    colorNode.addEventListener("mouseover", this._onLegendMouseOver);
> +    colorNode.addEventListener("mouseout", this._onLegendMouseOut);
> +    colorNode.addEventListener("mousedown", this._onLegendMouseDown);
> +    colorNode.addEventListener("mouseup", this._onLegendMouseUp);

I feel silly for asking, but don't we need to remove these listeners on destroy? I never really got around to knowing whether the browser removed them automatically when the element got removed.

Comment 3

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> Comment on attachment 8451172 [details] [diff] [review]
> v1
> 
> Review of attachment 8451172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking pretty good.
> I wasn't able to try it though, it seems you've deleted your framerate addon
> and I didn't keep it locally.

Thanks for the review! Yes, it was getting pretty hard keeping the add-on up to date with the latest actor changes, and since the new profiler will be a superset of that, I chose to only maintain *it* instead.

> I could always apply the giant profiler mega patch though ...

There are try builds in the patch that you can try out!

> I only have one no so minor comment about the relation between snapEdges and
> blocksBoundingRects, they seem to be very similar and built around the same
> time. It seems like it would be possible to use only blocksBoundingRects and
> get rid of snapEdges.
> 

It actually is a bit ugly. The reason for it is that setHighlights is an AbstractCanvasGraph method, not BarGraph method. So `snapEdges` is used to "massage" the data received from the outside, while `blocksBoundingRects` is used to highlight stuff internally (when you hover items in the legend). I do agree there's a tiny bit of overlap. Since there's no use-case for `setHighlights` on LineGraphs at the moment, I'll just make it a pure BarGraph implementation and lose the `snapEdges`.

> ::: browser/devtools/shared/test/browser_graphs-02.js
> @@ +77,5 @@
> > +    "The graph should now have the highlights set.");
> > +
> > +  graph.setHighlights([]);
> > +  ok(graph.hasHighlights(),
> > +    "The graph should shouldn't have anything highlighted.");
> 
> "should shouldn't" ?

Heh.

> By the way, do you really want an empty array to be considered as
> highlights? I think filtering for missing and empty arrays in setHighlights
> might be better.

Yup. An empty array means there's nothing highlighted, so everything is faded out. We need to make a distinction between  "everything visible", "everything faded out" and "some stuff faded out".

> 
> @@ +81,5 @@
> > +    "The graph should shouldn't have anything highlighted.");
> > +
> > +  graph.setHighlights(null);
> > +  ok(!graph.hasHighlights(),
> > +    "The graph should should have everything highlighted.");
> 
> should or should ? :)
> 

Damn it!

> ::: browser/devtools/shared/test/browser_graphs-11b.js
> @@ +51,5 @@
> > +  }, {
> > +    delta: 600, values: [3, 2, 0]
> > +  }]);
> > +
> > +  is(graph.snapEdges.toSource(), "[{start:0, end:66.66666666666666}, {start:68.66666666666666, end:133.33333333333331}, {start:135.33333333333331, end:200}, {start:202, end:266.66666666666663}, {start:268.66666666666663, end:333.3333333333333}, {start:335.3333333333333, end:400}]",
> 
> For this test's sake I think we should be rounding these values. It would
> make it easier to read and maintain I think. Also, are we sure that these
> floating point numbers are going to be exactly the same across all test
> platforms?
> 

Try was all green, and the test was much easier to write this way. However, I could refactor this into integer checks...

> ::: browser/devtools/shared/widgets/Graphs.jsm
> @@ +30,5 @@
> >  const GRAPH_REGION_LINE_WIDTH = 1; // px
> >  const GRAPH_REGION_LINE_COLOR = "rgba(237,38,85,0.8)";
> >  
> > +const GRAPH_HIGHLIGHTS_MASK_BACKGROUND = "rgba(255,255,255,0.75)";
> > +const GRAPH_HIGHLIGHTS_MASK_STRIPES = "rgba(255,255,255,0.5)";
> 
> Just a question: what's the plan for making these colors part of our theme
> and change when we switch from light to dark?

I have no plan :) but it's certainly something that needs to be taken care of.

> I think we want at some stage to move from our preprocessed CSS to using CSS
> variables, but I see more and more places where we also need theme color
> variables in JS. So I think we should start thinking putting all of our
> variables in properties file instead.
> 

Sounds like a good idea. In the meantime though, we could just listen for "theme-change" events (that should be emitted from theme-switching.js) and just change/redraw the graphs as necessary.

> @@ +376,5 @@
> > +    let height = this._height;
> > +
> > +    // Draw the background mask.
> > +
> > +    let pattern = AbstractCanvasGraph.getStripePattern({
> 
> I can't really see it all that well in the attached GIF, but don't you think
> a plain background would be preferable here? If the goal is just to hide
> some parts of the graph, I tend to think a monochrome semi-transparent
> background would be enough, especially that selections already have stripes.
> 

I tried with both a striped and a plain background, and stripes looked a bit better. You can download the try builds from bug 879008 and see for yourself. Darrin hasn't complained when he saw it, so let's keep it this way for now.

> @@ -650,1 @@
> >      if (this._cachedHighlightMask) {
> 
> I think this patch might need rebasing since the patch it's built on has
> changed.
> 

Yup.

> @@ +1674,5 @@
> >      itemNode.className = "bar-graph-widget-legend-item";
> >  
> >      let colorNode = this._document.createElementNS(HTML_NS, "span");
> >      colorNode.setAttribute("view", "color");
> > +    colorNode.setAttribute("index", this._legendNode.childNodes.length);
> 
> nit: I think data attributes are a little bit nicer to use thanks to the
> dataset API.
> This would allow you to do e.target.dataset.index in _onLegendMouseOver.
> 

TIL.

> @@ +1679,5 @@
> >      colorNode.style.backgroundColor = color;
> > +    colorNode.addEventListener("mouseover", this._onLegendMouseOver);
> > +    colorNode.addEventListener("mouseout", this._onLegendMouseOut);
> > +    colorNode.addEventListener("mousedown", this._onLegendMouseDown);
> > +    colorNode.addEventListener("mouseup", this._onLegendMouseUp);
> 
> I feel silly for asking, but don't we need to remove these listeners on
> destroy? I never really got around to knowing whether the browser removed
> them automatically when the element got removed.

They do get GC/CCd automatically these days. It's still usually a good idea to remove them manually though, I'll see what I can do.

Comment 4

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

Attachment: v2

Addressed all comments except changing the way highlights and bounding rects are verified in the test. There's simply too much data to verify, and writing all the comparisons by hand is just busywork. Based on my try runs, it does seem that all these values are the same across all platforms (and I'm also being careful to make the graph of a fixed size and overriding the device pixel ratio).

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8454563 [details] [diff] [review]
v2

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

Thanks for addressing the comments. This v2 looks good to me.

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +373,5 @@
>      return !!this._data;
>    },
>  
>    /**
> +   * Gets whether or not this graph has any highlighted areas.

"Gets whether or not this graph has any mask applied."

Comment 6

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

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a555278f76a