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.

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 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.

(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!

Review commit: https://reviewboard.mozilla.org/r/52083/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/52083/

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 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.

(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.

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/