Closed
Bug 1164327
Opened 9 years ago
Closed 9 years ago
Resizing performance tool doesn't not maintain selection accurately
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jsantell, Assigned: bgrins)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Blocks: perf-tool-overview
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 3•9 years ago
|
||
(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
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment 4•9 years ago
|
||
Feel free to take a glance at bug 1122662 if you want to. I've haven't been very successful with it.
Assignee | ||
Comment 5•9 years ago
|
||
I think this does the trick. Still needs test
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
This seems to do the trick. What do you think?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23bfd1e9a95c
Attachment #8609477 -
Flags: review?(vporof)
Assignee | ||
Updated•9 years ago
|
Attachment #8609056 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8609080 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
Comment on attachment 8609477 [details] [diff] [review]
selection-resize.patch
Review of attachment 8609477 [details] [diff] [review]:
-----------------------------------------------------------------
Nice and simple
Attachment #8609477 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
Had to do a bit of math in the test to account for different a different window size in Windows.
Attachment #8609477 -
Attachment is obsolete: true
Attachment #8609915 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Comment 15•9 years ago
|
||
Keywords: checkin-needed
Comment 16•9 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•