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)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox46 --- wontfix
firefox47 --- affected
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(2 files)

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)
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.
Attachment #8746380 - Flags: review?(gijskruitbosch+bugs)
(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!
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.
Attachment #8751542 - Flags: review?(gijskruitbosch+bugs) → review+
(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: nobody → markh
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
https://hg.mozilla.org/mozilla-central/rev/7efca54b4920
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1277669
You need to log in before you can comment on or make changes to this bug.