Closed
Bug 1268349
Opened 4 years ago
Closed 4 years ago
BrowserUITelemetry always reports sync-button has moved from the default location.
Categories
(Firefox :: General, defect)
Firefox
General
Not set
Tracking
()
RESOLVED
FIXED
Firefox 49
People
(Reporter: markh, Assigned: markh)
References
Details
Attachments
(2 files)
3.58 KB,
patch
|
Details | Diff | Splinter Review | |
58 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
BrowserUITelemetry maintains yet another list of "default placements" for the menu panel. When we added the sync-button, I neglected to notice this, so every UITelemetry payload reports "sync-button" as being in nondefaultAdded, which is wrong. This trivial patch fixes that. It includes a test to try and stop it happening in the future (which demonstrates some weirdness around the social-share-button, but that's a yak for another day) I'm thinking we should request uplift on this patch, so as UITelemtry dashboards start working again they aren't reporting erroneous data for this button.
Attachment #8746380 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 1•4 years ago
|
||
Comment on attachment 8746380 [details] [diff] [review] 0002-Bug-XXXXXXX-tell-BrowserUITelemetry-that-the-sync-bu.patch Review of attachment 8746380 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/test/browser_BrowserUITelemetry_defaults.js @@ +1,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +// The purpose if this test is to ensure that by default, BrowserUITelemetry > +// isn't reporting any UI customizations. This is primarily to changes to oops - qref failure - I fixed the typo here so it reads "*so* changes to..."
Comment 2•4 years ago
|
||
Comment on attachment 8746380 [details] [diff] [review] 0002-Bug-XXXXXXX-tell-BrowserUITelemetry-that-the-sync-bu.patch Review of attachment 8746380 [details] [diff] [review]: ----------------------------------------------------------------- We should also remove it from PALETTE_ITEMS, no? Also, thank you so much for writing that test! ::: browser/modules/test/browser_BrowserUITelemetry_defaults.js @@ +1,2 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ Nit: no license in tests ( http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ ) @@ +6,5 @@ > +// customizableUI (eg, new buttons, button location changes) also have a > +// corresponding BrowserUITelemetry change. > + > +function test() { > + Nit: no empty line here please. @@ +13,5 @@ > + Cu.import("resource:///modules/BrowserUITelemetry.jsm", s); > + > + let { CustomizableUI, BrowserUITelemetry } = s; > + > + CustomizableUI.reset(); Instead, could you please assert: Assert.ok(CustomizableUI.inDefaultState, "No other test should have left CUI in a dirty state."); Hopefully that doesn't orange out in the current state of the tree... if it does, we should clean it up. If that's a lot of work, we can move that to a separate bug and use a todo() here and call CUI.reset() after... but really, we shouldn't have to clean up after other tests. Hopefully there's not a lot going on in browser/modules (I thought that the profile got reset for every dir we run? But I don't see social tests in this dir and the comment later on suggests that they might have run earlier... Can you clarify?) so we should be OK.
Attachment #8746380 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 3•4 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > Nit: no license in tests ( > http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public- > domain/ ) Funny you should say that - also thought "no license", but just the other day day I re-read that post, and based on bug 788511 comment 10 I re-interpreted my understanding to be "CC0 is the default - we aren't doing a mass re-license, but adding a CC0 license would make it clearer and avoid requiring anyone to spelunk through history to ensure the test post-dates the change". > (I thought that the profile got reset for every > dir we run? That would be (good!) news to me for mochi, but in general I agree we should assert. Thanks!
Assignee | ||
Comment 4•4 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/52083/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52083/
Attachment #8751542 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 5•4 years ago
|
||
Comment on attachment 8751542 [details] MozReview Request: Bug 1268349 - tell BrowserUITelemetry that the sync-button is in the menu panel by default. r=Gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52083/diff/1-2/
Comment 6•4 years ago
|
||
Comment on attachment 8751542 [details] MozReview Request: Bug 1268349 - tell BrowserUITelemetry that the sync-button is in the menu panel by default. r=Gijs https://reviewboard.mozilla.org/r/52083/#review49059 ::: browser/modules/test/browser_BrowserUITelemetry_defaults.js:20 (Diff revision 2) > + // This one is a bit weird - the "social-share-button" is dynamically added > + // to the toolbar as the feature is first used - but it's listed as being in > + // the toolbar by default so it doesn't end up in nondefaultAdded once it > + // is created. The end result is that it ends up in defaultRemoved before > + // the feature has been activated. > + Assert.deepEqual(result.defaultRemoved, ["social-share-button"]); Followup for this? I think the fix might actually be to just not have it in UITelemetry's "default items in the toolbar" list... Doesn't need to block this bug though.
Attachment #8751542 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #6) > Followup for this? Bug 1273358 - I'll land this patch with a reference to that bug. > I think the fix might actually be to just not have it in > UITelemetry's "default items in the toolbar" list... Doesn't need to block > this bug though. I think that will just change things so it's incorrect *after* social has been activated rather than before.
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → markh
Assignee | ||
Comment 8•4 years ago
|
||
Comment on attachment 8751542 [details] MozReview Request: Bug 1268349 - tell BrowserUITelemetry that the sync-button is in the menu panel by default. r=Gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52083/diff/2-3/
Attachment #8751542 -
Attachment description: MozReview Request: Bug 1268349 - tell BrowserUITelemetry that the sync-button is in the menu panel by default. r?Gijs → MozReview Request: Bug 1268349 - tell BrowserUITelemetry that the sync-button is in the menu panel by default. r=Gijs
Comment 10•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7efca54b4920
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in
before you can comment on or make changes to this bug.
Description
•