Flamegraph overlaps headers when moving it up/down

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

37 Branch
Firefox 40
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8588349 [details]
Screen Shot 2015-04-04 at 12.32.42 PM.png

When dragging the flamegraph up and down, it renders ontop of the overview time markers
Regression from bug 1121180.
Created attachment 8590025 [details] [diff] [review]
1151246-flamegraphrender.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10f2c8ebc69
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8590025 - Flags: review?(vporof)
Yes this uses two loops, but usually around what, 10 ticks? I think splitting these up is fine.
Comment on attachment 8590025 [details] [diff] [review]
1151246-flamegraphrender.patch

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

There's duplicated and unused code in both functions now. (e.g. computing a label, drawing the header background twice etc.).

We should only have a single function for drawing the tick lines. And another single function for drawing the header background and numbers (and maybe the lines again, only for the header, if we really want to; I'd say this isn't very important).

Let's share some code between these two methods and avoid dead variables.
Attachment #8590025 - Flags: review?(vporof)
Created attachment 8591134 [details] [diff] [review]
1151246-flamegraphrender.patch
Attachment #8590025 - Attachment is obsolete: true
Attachment #8591134 - Flags: review?(vporof)
Comment on attachment 8591134 [details] [diff] [review]
1151246-flamegraphrender.patch

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

::: browser/devtools/shared/widgets/FlameGraph.js
@@ +420,5 @@
> +   * @param number dataOffset, dataScale, from, to, renderText
> +   *        Offsets and scales the data source by the specified amount.
> +   *        from and to determine the Y position of how far the stroke
> +   *        should be drawn.
> +   *        This is used for scrolling the visualization.

"This is used for scrolling the visualization." -- a little bit misleading. Not for, but when.
Attachment #8591134 - Flags: review?(vporof) → review+
Created attachment 8591358 [details] [diff] [review]
1151246-flamegraphrender.patch

always correctin' my bad grammar
Attachment #8591134 - Attachment is obsolete: true
Attachment #8591358 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f1dc03434438
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f1dc03434438
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.