Closed Bug 1166139 Opened 9 years ago Closed 9 years ago

[regression] the markers filter popup is amazing

Categories

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

defect
Not set
normal

Tracking

(firefox40 verified, firefox41 fixed)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- fixed

People

(Reporter: vporof, Assigned: jsantell)

References

Details

Attachments

(2 files, 1 obsolete file)

      No description provided.
Thought i fixed this hm..
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Oh this is the popup. Well ok then!
Attached patch 1166139-marker-filter-name.patch (obsolete) — Splinter Review
Attachment #8607326 - Flags: review?(vporof)
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)
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/mozilla-central/rev/e650970b34d3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
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?
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
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.