[regression] the markers filter popup is amazing

VERIFIED FIXED in Firefox 40

Status

()

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

People

(Reporter: vporof, Assigned: jsantell)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 verified, firefox41 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8607305 [details]
Screen Shot 2015-05-18 at 10.54.01 PM.png
(Reporter)

Updated

3 years ago
Blocks: 1145697
Thought i fixed this hm..
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Oh this is the popup. Well ok then!
Created attachment 8607326 [details] [diff] [review]
1166139-marker-filter-name.patch
Attachment #8607326 - Flags: review?(vporof)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8607326 [details] [diff] [review]
1166139-marker-filter-name.patch

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

wow

::: browser/devtools/shared/timeline/global.js
@@ +22,5 @@
>   *   - label: the label used in the waterfall to identify the marker. Can be a
>   *            string or just a function that accepts the marker and returns a string,
>   *            if you want to use a dynamic property for the main label.
> + *            If you use a function for a label, it *must* handle the case where
> + *            no marker is provided for a main label to describe all markers of this type.

I think we can also add a test for this. Mock a bogus marker and somebody creating a function that doesn't handle it properly. Check if the label is the function body.
Attachment #8607326 - Flags: review?(vporof) → review+
If unhandled, it will display `undefined` or throw (if accessing marker.name where there is no marker) -- the printing function will only handle in tools that implement it. Regardless, we can use better error messages here if that does happen (and if someone is adding a marker on their own, they can use a straight forward declaritive approach and most likely not use all these custom formatter functions)
Created attachment 8607696 [details] [diff] [review]
1166139-marker-filter-name.patch

Added tests for when label is undefined (or returns undefined from a function) that throws an error with descriptive message on what to fix, from the perspective of someone adding a marker without familiarity of devtools
Attachment #8607326 - Attachment is obsolete: true
Attachment #8607696 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/e650970b34d3
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e650970b34d3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Blocks: 1167252
ni? jsantell for STR to verify fix
Flags: qe-verify+
Flags: needinfo?(jsantell)
STR: Click the [filter] button (to the right of [clear]) and ensure that all entries have a color swatch as well as a readable name (not some function garbled text like: https://bugzilla.mozilla.org/attachment.cgi?id=8607305 )
Flags: needinfo?(jsantell)
Comment on attachment 8607696 [details] [diff] [review]
1166139-marker-filter-name.patch


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8607696 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/releases/mozilla-aurora/rev/886e53a3c835
status-firefox40: --- → fixed
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8607696 [details] [diff] [review]
1166139-marker-filter-name.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8607696 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Aurora 40.0a2 (2015-06-04), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.

The Filters menu is now displayed properly, with the exception of a Linux-only issue affecting the checkboxes. I've filed Bug 1172412 for that.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.