1164327 - Resizing performance tool doesn't not maintain selection accurately

Status: RESOLVED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

If I have a selection in the overview graph, say 500px selection, somewhere in the middle of a 1000px graph of a 1000ms recording, 250px from the left, so I have the 250ms-750ms selected, right?

Stretch out the window so it's 2000px now. My selection should have the some proportions, and still have 250ms-750ms selected. Instead, it's still a 500px selection, 250px from the left.

Comment 1

[Brian Grinstead [:bgrins]]

Another weird thing is that once the window is stretched out, the timeline size never shrinks even if the window size is restored.  So if you make window really big and then small again, it seems like the timeline is being clipped

Comment 2

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

Yeah, bug 1122662. I'm having a hard time understanding what the heck is going on. I suspect it's because of the canvases inside the iframes not being able to shrink down, but some quick css massaging didn't help.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #2)
> Yeah, bug 1122662. I'm having a hard time understanding what the heck is
> going on. I suspect it's because of the canvases inside the iframes not
> being able to shrink down, but some quick css massaging didn't help.

I can take a look at this bug, let me know if you want me to look into that one also

Comment 4

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

Feel free to take a glance at bug 1122662 if you want to. I've haven't been very successful with it.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: selection-resize.patch

I think this does the trick.  Still needs test

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: canvas-redraw-WIP.patch

some ideas on how to improve perceived perf on canvas redraws

1) Use deferredtask so it gets redrawn while resizing (up to once per N ms)
2) Don't reset the canvas width until the redraw actually happens to get rid of white flicker
3) Set the canvas's CSS width from code instead of relying on 100% from CSS - this gets rid of the font zooming thing that happens during resizing

Comment 7

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

Comment on attachment 8609080 [details] [diff] [review]
canvas-redraw-WIP.patch

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

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +29,5 @@
>  const L10N = new ViewHelpers.L10N();
>  
>  // Generic constants.
>  
> +const GRAPH_RESIZE_EVENTS_DRAIN = 10; // ms

Is this change necessary? For heavy heavy graphs this might result in choppy resizes.

@@ +780,5 @@
> +    let bounds = this.bounds;
> +    this._iframe.setAttribute("width", bounds.width);
> +    this._iframe.setAttribute("height", bounds.height);
> +    this._canvas.style.width = bounds.width + "px";
> +    this._canvas.style.height =  bounds.height + "px";

I'd like us to avoid doing this when not necessary or when sizes didn't change. Currently this can be called whenever hovering, for example, or moving the mouse. Sounds a bit extreme to me.

Why not keep this in `refresh`? It will cause the widget to redraw anyway, by setting `_shouldRedraw` to true.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #7)
> @@ +780,5 @@
> > +    let bounds = this.bounds;
> > +    this._iframe.setAttribute("width", bounds.width);
> > +    this._iframe.setAttribute("height", bounds.height);
> > +    this._canvas.style.width = bounds.width + "px";
> > +    this._canvas.style.height =  bounds.height + "px";
> 
> I'd like us to avoid doing this when not necessary or when sizes didn't
> change. Currently this can be called whenever hovering, for example, or
> moving the mouse. Sounds a bit extreme to me.
> 
> Why not keep this in `refresh`? It will cause the widget to redraw anyway,
> by setting `_shouldRedraw` to true.

Yeah, I'm not even sure if I will request review on that patch, just stashing some work there.  I moved it there to avoid the 'flicker' that's caused if we allow refreshing during resize (the delay between clearing the canvas by setting width and when the redraw happens.  Likewise, for the CSS width on the frame and canvas if we have a delay between setting it and redrawing then the canvas will sort of zoom itself and cause some weirdness with the text sizing.

I'll have to do some measurements and see if it's even worth proceeding with that.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: selection-resize.patch

This seems to do the trick.  What do you think?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23bfd1e9a95c

Comment 10

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

Comment on attachment 8609477 [details] [diff] [review]
selection-resize.patch

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

Nice and simple

Comment 11

[Brian Grinstead [:bgrins]]

Ah, need some rounding on the test for Windows: TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_graphs-10c.js | The current selection start value is correct (2). - Got 49.1869918699187, expected 50

Comment 12

[Brian Grinstead [:bgrins]]

Actually, that's further off than I thought.  Maybe it's because of the window chrome size being different and the graph width is not quite changing to 50%.  Checking on a local build

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: selection-resize.patch

Had to do a bit of math in the test to account for different a different window size in Windows.

Comment 14

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9121e785778e

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/08eb0e8fcbbc