1166139 - [regression] the markers filter popup is amazing

Status: VERIFIED FIXED

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

Description

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

Attachment: Screen Shot 2015-05-18 at 10.54.01 PM.png

Comment 1

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

Thought i fixed this hm..

Comment 2

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

Oh this is the popup. Well ok then!

Comment 3

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

Attachment: 1166139-marker-filter-name.patch

Comment 4

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

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.

Comment 5

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

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)

Comment 6

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

Attachment: 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

Comment 7

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

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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e650970b34d3

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

ni? jsantell for STR to verify fix

Comment 10

[Jordan Santell [:jsantell] [@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 )

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/886e53a3c835

Comment 13

[Brian Grinstead [:bgrins]]

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 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8607696 [details] [diff] [review]
1166139-marker-filter-name.patch

Change approved to skip one train as part of the spring campaign.

Comment 15

[Andrei Vaida [:avaida]]

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.