1155521 - Figure out what to do with users to have the Pocket add-on / SocialAPI button installed

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Justin Dolske [:Dolske]]

Pocket will be integrated with Firefox by default, but some users already have the Pocket add-on or SocialAPI button installed. It would be silly for those users to have double UI for Pocket function, so we should figure out what to do.

Waiting to hear back from Pocket, but seems likely that we would basically disable/uninstall the addon/socialAPI for users who already have it. And so something so that Firefox users don't needlessly see offers for the addon/socialAPI when it's already built in.

Depending on what we do this bug might be split into bugs specific to the aadd-on and SocialAPI button.

Comment 1

[Dave Townsend [:mossop]]

The simplest route for the add-on would be for AMO to push force compatibility setting making it incompatible with the newer version of Firefox. This would make it impossible for the user to re-enable so maybe that isn't what we want?

Comment 2

[Shane Caraveo (:mixedpuppy)]

For socialapi, call Social.uninstallProvider("getpocket.com", callback).

Comment 3

[Justin Dolske [:Dolske]]

Oh, here's a fun complication: We're shipping the integration only enabled for en-US, but any user could have the addon/SocialAPI button installed.

So we either need to force our stuff to be enabled for those pre-existing users, or leave the addon/social API enabled for those users until we bring the integration to their locale.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Dave Townsend [:mossop] from comment #1)
> The simplest route for the add-on would be for AMO to push force
> compatibility setting making it incompatible with the newer version of
> Firefox. This would make it impossible for the user to re-enable so maybe
> that isn't what we want?

Yeah... Worth considering that some users *will* want to keep (or re-enable) the AMO add-on, if our newly built-in version isn't at feature-parity with it. (Not sure if we'll be at feature parity or not, but if we're not, we shouldn't forcibly downgrade those users to a less-feature-rich experience, at least not in a way that they can't work around.)

Comment 5

[Justin Dolske [:Dolske]]

Basic plan here is: if the built-in Pocket feature is available for the user's locale, we'll use it and disable the SocialAPI Pocket button if it was already installed. But if the built-in Pocket feature is available for the user's locale and the user currently has the AMO addon installed, we'll use the addon instead of the built-in code.

There are a few more tiny details, Drew will be handling this.

Comment 6

[Drew Willcoxon :adw]

Attachment: WIP patch

There's more I need to do here, but is this the right idea?

Comment 7

[Drew Willcoxon :adw]

Elaboration on the basic plan from comment 5: https://etherpad.mozilla.org/Czb0JhjJDP

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8598376 [details] [diff] [review]
WIP patch

Review of attachment 8598376 [details] [diff] [review]:
-----------------------------------------------------------------

feedback- since this isn't using the CustomizableWidgets array anymore. We'll need to figure out how to still use that.

::: browser/app/profile/firefox.js
@@ +1902,5 @@
>  #ifdef NIGHTLY_BUILD
>  pref("dom.serviceWorkers.enabled", true);
>  #endif
>  
> +pref("browser.pocket.enabled", true);

This should stay false until we're ready to enable it (not yet)

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ -1211,5 @@
>        }
>      };
>  
> -    CustomizableWidgets.push(pocketButton);
> -    CustomizableUI.addListener(pocketButton);

We shouldn't remove our built-in widgets from this code path (not using the CustomizableWidgets array). The widget should be in this array so it flows through the _defineBuiltInWidgets code path in CustomizableUI. This affects default placements and if a widget can claim that it should have a default position in the navbar or menu panel.

We will land the change to make the navbar be the default place for the Pocket button soon. With that patch landed, I'm not sure this will allow Reset Default to place Pocket back in the navbar.

::: browser/components/pocket/Pocket.jsm
@@ +147,5 @@
> +  }),
> +
> +  _shouldBeEnabled: Task.async(function* () {
> +    if (!Services.prefs.getBoolPref("browser.pocket.enabled")) {
> +      // Let the user set browser.pocket.enabled to false as an override.

We can use the browser.pocket.removedByUser pref here instead. That pref is being used if the user removes Pocket from the toolbar and menu panel, which effectively disables Pocket but doesn't interfere with if the feature should exist in the customization palette.

Setting browser.pocket.enabled to false will stop it from showing up in the customization palette. Disabling the feature by removing it from the UI should be easier for users to understand than going to about:config.

@@ +161,5 @@
> +      });
> +    });
> +  }),
> +
> +  get _enabledForLocale() {

This should be a function not a getter since IMO it does a non-trivial amount of work and doesn't cache the result. That is, unless you'd like to refactor this to cache the result and all subsequent times it will return the cached result.

Comment 9

[Drew Willcoxon :adw]

Attachment: patch v2

I think this is all we need to do.

Comment 10

[Marco Mucci [:MarcoM]]

Hi Drew, can you provide a point value.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8598909 [details] [diff] [review]
patch v2

Review of attachment 8598909 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1087,5 @@
>    if (isEnabledForLocale) {
>      let pocketButton = {
>        id: "pocket-button",
> +      defaultArea: CustomizableUI.AREA_NAVBAR,
> +      _introducedInVersion: 5,

_introducedInVersion is tightly coupled with kVersion in CustomizableUI.jsm, and kVersion will need to be incremented. From the looks of it, I *think* your change here will be OK but it means that the button won't actually be introduced to the UI until kVersion is incremented and browser.pocket.enabled is set to true.

On a side note, I wonder what will happen for non-en-US locales after kVersion is incremented. For them, the gSavedState.currentVersion will be increased but the button won't be installed until we enable it for their locale. Marking needinfo for Gijs for this as he probably thought about it when he worked on the Panic button.

@@ +1230,5 @@
>      CustomizableUI.addListener(pocketButton);
> +
> +    // Uninstall the Pocket social provider if it exists.
> +    let origin = "getpocket.com";
> +    SocialService.getProvider(origin, provider => {

I'm not sure what the answer is here, but this will uninstall the socialapi provider on each start up. Is this something that we should only do once? If so, then it could be moved to the CustomizableUI._introduceNewBuiltinWidgets function.

::: browser/modules/BrowserUITelemetry.jsm
@@ +48,5 @@
>      "nav-bar": [
>        "urlbar-container",
>        "search-container",
>        "bookmarks-menu-button",
> +      "pocket-button",

I think this should wait until browser.pocket.enabled is set to true and kVersion is incremented. As it is right now with this patch, telemetry won't match reality as the button isn't yet there for people by default.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

See question midway through comment #11.

Comment 13

[:Gijs (he/him)]

This is difficult. Don't really have forget-button-inspired ideas, because that didn't get added by default.

The simplest solution here is to turn the pref on by default on Nightly, update the default arrays, turn off the locale list, and use browser.properties on Nightly instead of the content-packaged strings (tbh I'm not sure why those landed on Nightly...). Then we can set kVersion, _introducedInVersion, telemetry, and all the rest of it, and things will Just Work (except if you move between channels, in which case, tough, you have bigger problems).

Uplift is harder. I expect that for the 38.0.5 release, and potentially for 39, we should make the kVersion, _introducedInVersion and telemetry values dependent on the button being enabled and so on. They're all runtime-setup, so that's all doable in principle, assuming nobody loads the modules in question before the prefs system has loaded the user's prefs.js. We might be able to check for this if we want to make sure. This means those values can fluctuate for a release if the user messes with the values of the prefs and/or their browser's locale. That is somewhat unfortunate, but not, I think, ultimately a problem we should worry about.

Jared, does this sound acceptable / like a plan?

Comment 14

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch (away until May 5th) from comment #13)

> The simplest solution here is to turn the pref on by default on Nightly,
> update the default arrays, turn off the locale list

Well, this is unique in that the Pocket site/service itself is only localized in a subset of Firefox locales (~15 vs our ~90), and so I'd expect that we will always need the .enabledLocales list. Otherwise we have primary UI that drops you into a service that might not be available in your language. (Unlike the Forget button, where this was just a temporary measure until L10N caught up with localization as normal.)

In any case, the immediate focus is on 38.0.5 / 39, where localization will be limited anyway.

Comment 15

[Drew Willcoxon :adw]

I'm not really sure what to do here given the conversation between Jared and Gijs, so I'll wait until that's resolved despite the r+.  I'll probably post a new patch anyway based on the discussion below.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> I'm not sure what the answer is here, but this will uninstall the socialapi
> provider on each start up. Is this something that we should only do once? If
> so, then it could be moved to the CustomizableUI._introduceNewBuiltinWidgets
> function.

Correct.  I don't know how much it matters in practice.  Gavin said that we might ask Pocket to take down their social provider page, so it should be hard if not impossible for people to install the provider after we turn on our feature.  (Modulo our feature's locale-specific behavior.)

It would be annoying though if you really like the social provider UX, so you reinstall it (assuming the Pocket page still exists), and then Firefox keeps uninstalling it every time it starts up.  So doing this part only once, like in _introduceNewBuiltinWidgets (or _migrateUI in nsBrowserGlue?), makes sense.  In that case I think we'd need to expose a getter or promise on CustomizableWidgets that tells whether the widget has been created, because I found that it's hard for code outside of it that also runs on startup to tell on its own.  It may be easier to keep the code where it is, but have it check the wasSocialProviderInstalled pref:  If it's present, then don't try to uninstall the provider again.

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Drew Willcoxon :adw from comment #15)
> I'm not really sure what to do here given the conversation between Jared and
> Gijs, so I'll wait until that's resolved despite the r+.  I'll probably post
> a new patch anyway based on the discussion below.
> 
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> > I'm not sure what the answer is here, but this will uninstall the socialapi
> > provider on each start up. Is this something that we should only do once? If
> > so, then it could be moved to the CustomizableUI._introduceNewBuiltinWidgets
> > function.
> 
> Correct.  I don't know how much it matters in practice.  Gavin said that we
> might ask Pocket to take down their social provider page, so it should be
> hard if not impossible for people to install the provider after we turn on
> our feature.  (Modulo our feature's locale-specific behavior.)

There's over 100K activations of Pocket through our directory pages, not sure how many from Pockets activation.  Removing those pages would:

- break any users who are not in locale's supported by the new implementation
- break any users who have not upgraded (assuming support)

We could blocklist the pocket socialapi provider, I think that may even be possible on a per locale basis, but we still run into the issue of turning of a provider prior to user upgrading Firefox. 

> It would be annoying though if you really like the social provider UX, so
> you reinstall it (assuming the Pocket page still exists), and then Firefox
> keeps uninstalling it every time it starts up.  So doing this part only
> once, like in _introduceNewBuiltinWidgets (or _migrateUI in nsBrowserGlue?),
> makes sense.  In that case I think we'd need to expose a getter or promise
> on CustomizableWidgets that tells whether the widget has been created,
> because I found that it's hard for code outside of it that also runs on
> startup to tell on its own.  It may be easier to keep the code where it is,
> but have it check the wasSocialProviderInstalled pref:  If it's present,
> then don't try to uninstall the provider again.

Looking at the latest patch here, it looks like the uninstall only happens if it is installed.  You could also just check for the existence of the pref (social.manifest.getpocket-com) if that feels lighter weight to you.

I would however suggest a change there.  Rather than setting a pref to say it was installed, make a copy of social.manifest.getpocket-com pref into something like social.backup.getpocket-com.  That will preserve the manifest data and allow restoration later if it is ever necessary for some reason.  If/When the new implementation solidifies that pref could be removed.

Comment 17

[Shane Caraveo (:mixedpuppy)]

To add to my last comment...

Pocket (and our directory) can remove the activation pages, and Pocket can leave the implementation pages to support existing users that do not end up upgrading to a version that has the new implementation.  That removes the ability for users to re-activate the socialapi version, so we do away with the problems associated with that.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch (away until May 5th) from comment #13)
> This is difficult. Don't really have forget-button-inspired ideas, because
> that didn't get added by default.
> 
> The simplest solution here is to turn the pref on by default on Nightly,
> update the default arrays, turn off the locale list, and use
> browser.properties on Nightly instead of the content-packaged strings (tbh
> I'm not sure why those landed on Nightly...). Then we can set kVersion,
> _introducedInVersion, telemetry, and all the rest of it, and things will
> Just Work (except if you move between channels, in which case, tough, you
> have bigger problems).
> 
> Uplift is harder. I expect that for the 38.0.5 release, and potentially for
> 39, we should make the kVersion, _introducedInVersion and telemetry values
> dependent on the button being enabled and so on. They're all runtime-setup,
> so that's all doable in principle, assuming nobody loads the modules in
> question before the prefs system has loaded the user's prefs.js. We might be
> able to check for this if we want to make sure. This means those values can
> fluctuate for a release if the user messes with the values of the prefs
> and/or their browser's locale. That is somewhat unfortunate, but not, I
> think, ultimately a problem we should worry about.
> 
> Jared, does this sound acceptable / like a plan?

We could also write a getter for _introducedInVersion so that it only returns the incremented version if the locale is allowed?

Comment 19

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> (In reply to :Gijs Kruitbosch (away until May 5th) from comment #13)
> > This is difficult. Don't really have forget-button-inspired ideas, because
> > that didn't get added by default.
> > 
> > The simplest solution here is to turn the pref on by default on Nightly,
> > update the default arrays, turn off the locale list, and use
> > browser.properties on Nightly instead of the content-packaged strings (tbh
> > I'm not sure why those landed on Nightly...). Then we can set kVersion,
> > _introducedInVersion, telemetry, and all the rest of it, and things will
> > Just Work (except if you move between channels, in which case, tough, you
> > have bigger problems).
> > 
> > Uplift is harder. I expect that for the 38.0.5 release, and potentially for
> > 39, we should make the kVersion, _introducedInVersion and telemetry values
> > dependent on the button being enabled and so on. They're all runtime-setup,
> > so that's all doable in principle, assuming nobody loads the modules in
> > question before the prefs system has loaded the user's prefs.js. We might be
> > able to check for this if we want to make sure. This means those values can
> > fluctuate for a release if the user messes with the values of the prefs
> > and/or their browser's locale. That is somewhat unfortunate, but not, I
> > think, ultimately a problem we should worry about.
> > 
> > Jared, does this sound acceptable / like a plan?
> 
> We could also write a getter for _introducedInVersion so that it only
> returns the incremented version if the locale is allowed?

Heh. Evil, makes my brain hurt, but might work.

Except... what about kVersion? Keeping two possible versions depending on the user's locale is evil. If it's static, you get situations like:

1) es-ES user installs 38.0.5. No pocket button, but kVersion increments to N. _introducedInVersion is undefined / N-1.
2) es-ES user updates to 39/40/wherever we localize it to es-ES. introducedInVersion is now N, but kVersion for the saved state has already been incremented to N.



It seems like it might be easier to somehow specialcase this button in the _introduceNewBuiltinWidgets method.

Comment 20

[:Gijs (he/him)]

Attachment: "Figure out what to do with users to have the Pocket add-on / SocialAPI button installed"

The more I think about it, the more I think the version scheme stuff is liable to many issues, some of which we've already seen with the _migrateUI cruft. How about something like this instead? (untested...)
Using a pref mimics what we advise XUL widget add-ons to do, and doesn't have any 'but what if the version increments but we end up not doing what we said we'd do' issues. Thoughts?

Comment 21

[:Gijs (he/him)]

Comment on attachment 8601587 [details] [diff] [review]
"Figure out what to do with users to have the Pocket add-on / SocialAPI button installed"

I'll be around again in ~3-4 hours if the approach I'm suggesting is not clear.

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 8601587 [details] [diff] [review]
"Figure out what to do with users to have the Pocket add-on / SocialAPI button installed"

Review of attachment 8601587 [details] [diff] [review]:
-----------------------------------------------------------------

This is very similar to the approach I was starting to take today, except I'm storing state in gSavedState as a list of widget IDs instead of prefs.  So f+ from me.  Only reason it's not r+ is that the social provider part of this patch has changed, which I'll post.  Could we have separate patches for your _introduceNewBuiltinWidgets changes and mine?

Comment 23

[Drew Willcoxon :adw]

Comment on attachment 8601587 [details] [diff] [review]
"Figure out what to do with users to have the Pocket add-on / SocialAPI button installed"

Review of attachment 8601587 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1087,5 @@
>        id: "pocket-button",
>        type: "view",
>        viewId: "PanelUI-pocketView",
> +      defaultArea: CustomizableUI.AREA_NAVBAR,
> +      introducedInVersion: "pref",

And this should be prefixed with an underscore, right?  Anyhow, I'll post my part of the patch with this fixed.

Comment 24

[Drew Willcoxon :adw]

Attachment: patch v3

Here's my part of the patch, the non-_introduceNewBuiltinWidgets part.  Changes from my previous patch:

* Sets _introducedInVersion: "pref" to match Gijs's _introduceNewBuiltinWidgets patch.

* Changes how the social provider is uninstalled based on Shane's feedback.  (Thanks Shane!)  It checks social.manifest.getpocket-com to determine if the provider is installed instead of using SocialService.jsm.  And it moves that pref's value to social.backup.getpocket-com, but only if social.backup.getpocket-com does not already exist.

Comment 25

[Drew Willcoxon :adw]

Some customizable mode tests are failing because CustomizableUI.inDefaultState is not true when it should be.  I'm looking at how to fix that.  Maybe pocket-button should not be added to navbarPlacements unless it's enabled, if that's possible, or inDefaultState will need to be smarter.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8601599 [details] [diff] [review]
patch v3

Review of attachment 8601599 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1229,5 @@
> +
> +    // Uninstall the Pocket social provider if it exists and we haven't already
> +    // uninstalled it in this manner.
> +    let originalPref = "social.manifest.getpocket-com";
> +    if (Services.prefs.prefHasUserValue(originalPref)) {

We can take this, but I disagree that we should be using this, since if the logic changes to determine if a social provider is installed this probably won't get updated whereas the SocialService code will.

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8601587 [details] [diff] [review]
"Figure out what to do with users to have the Pocket add-on / SocialAPI button installed"

Review of attachment 8601587 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizableUI.jsm
@@ +298,5 @@
> +      if (widget.defaultArea) {
> +        let shouldAdd = false;
> +        let shouldSetPref = false;
> +        if (widget._introducedInVersion === "pref") {
> +          let prefId = "browser.toolbarbuttons.introduced." + widget.id;

Please move the prefId definition up a level so that the same variable can be shared between getBoolPref and setBoolPref.

Comment 28

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> Comment on attachment 8601599 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8601599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/CustomizableWidgets.jsm
> @@ +1229,5 @@
> > +
> > +    // Uninstall the Pocket social provider if it exists and we haven't already
> > +    // uninstalled it in this manner.
> > +    let originalPref = "social.manifest.getpocket-com";
> > +    if (Services.prefs.prefHasUserValue(originalPref)) {
> 
> We can take this, but I disagree that we should be using this, since if the
> logic changes to determine if a social provider is installed this probably
> won't get updated whereas the SocialService code will.

That's a good point.  But the bigger issue is the removal of the provider.  Unsetting the pref for the provider does not uninstall it properly (unless it is guaranteed to happen prior to SocialService doing anything), you need to use the SocialService uninstall api.  Given that, sorry for leading you down a rabbit hole, but it is probably better to do it the way it was before.  I think the backup of the pref is still a good idea if for any reason this needs to be reverted in the future.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8601599 [details] [diff] [review]
patch v3

drive by...unsetting the pref isn't enough to uninstall properly for the current session (would only work after restart).

Comment 30

[Drew Willcoxon :adw]

Attachment: patch v4

* Uses `introducedInVersion: "pref",` -- no underscore.  I don't know why the underscore worked with the previous patch but not this one.
* Goes back to using SocialService.
* Conditionally adds pocket-button to navbarPlacements to avoid breaking tests.
* Removes the `gSavedState.currentVersion >= kVersion` check in _introduceNewBuiltinWidgets.  This was necessary to get our Pocket button to show up in the nav bar instead of the customization palette when you have the social provider already installed.

In the last point, our Pocket button shows up as the last item in the toolbar even though it's being inserted into navbarPlacements after the bookmarks button.  I don't know why that is but I'm guessing it has to do with saved state.

Comment 31

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #25)
> Some customizable mode tests are failing because
> CustomizableUI.inDefaultState is not true when it should be.  I'm looking at
> how to fix that.  Maybe pocket-button should not be added to
> navbarPlacements unless it's enabled, if that's possible.

Yes, this (ensuring navbarPlacements only has it if the button is enabled) is the correct solution to that issue, I believe.




(In reply to Drew Willcoxon :adw from comment #22)
> Could we have separate patches for
> your _introduceNewBuiltinWidgets changes and mine?

I won't be around much longer tonight; it probably makes sense for you to merge whatever parts of my changes (incl. Jared's comments) make sense. I just posted a patch because I knew I wouldn't be around and thought this was much the clearest way of describing what I thought would work.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8601732 [details] [diff] [review]
patch v4

Review of attachment 8601732 [details] [diff] [review]:
-----------------------------------------------------------------

You may be able to look at http://hg.mozilla.org/mozilla-central/rev/bd024cf6af34 to see how Metro added a Metro-only button and got the defaultPlacement tests working.

::: browser/components/customizableui/CustomizableUI.jsm
@@ +296,5 @@
>      }
>    },
>  
>    _introduceNewBuiltinWidgets: function() {
> +    if (!gSavedState) {

Can you add a comment here saying why we should still enter if currentVersion is greater than kVersion? I want to get ahead of somebody in the future who thinks we can make a simple optimization here.

Comment 33

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #30)
> In the last point, our Pocket button shows up as the last item in the
> toolbar even though it's being inserted into navbarPlacements after the
> bookmarks button.  I don't know why that is but I'm guessing it has to do
> with saved state.

That happens if the navbar is at all customized before enabling Pocket.  I'll see if I can find why that is.

Comment 34

[Drew Willcoxon :adw]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> Can you add a comment here saying why we should still enter if
> currentVersion is greater than kVersion? I want to get ahead of somebody in
> the future who thinks we can make a simple optimization here.

Sure.  I'm actually not 100% sure why that's necessary, but it is.  I had a hunch some state was being reused in that case, preventing the button from showing up, and that seems to be the case.  I'll figure out why it's necessary and add a comment about it, but if you have any insight, it'd be great to hear.

Comment 35

[:Gijs (he/him)]

(In reply to Drew Willcoxon :adw from comment #34)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> > Can you add a comment here saying why we should still enter if
> > currentVersion is greater than kVersion? I want to get ahead of somebody in
> > the future who thinks we can make a simple optimization here.
> 
> Sure.  I'm actually not 100% sure why that's necessary, but it is.  I had a
> hunch some state was being reused in that case, preventing the button from
> showing up, and that seems to be the case.  I'll figure out why it's
> necessary and add a comment about it, but if you have any insight, it'd be
> great to hear.

It's basically because the version number used to be the definitive indicator of whether we had any newly introduced widgets. Now that we're adding a per-widget pref facility, that is no longer true, and so there's no way to determine this beforehand.

We could try to optimize by saving a set (array, because JSON) of "newly seen builtin widgets" into the saved state, but that doesn't seem worth it.

FWIW, I considered removing the kVersion-based stuff entirely, but that seems too invasive for uplift. I would argue we should do it in a followup though; it's confusing and unnecessary to have both, though we'll need a migration step to move the one widget that used it (the hello button stuff) if we think it's important to keep adding that to really old profiles that migrate to latest in one go.

Comment 36

[Pulsebot]

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

Comment 37

[Drew Willcoxon :adw]

Gijs's patch:
https://hg.mozilla.org/integration/fx-team/rev/b2e843b2300d

My patch:
https://hg.mozilla.org/integration/fx-team/rev/e5463416f49a

I had to unbitrot my CustomizableWidgets.jsm changes.  I also had to fix browser_1042100_default_placements_update.js to ignore widgets placed with the new pref-based facility.

browser.pocket.enabled=false:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b6275ed0550

browser.pocket.enabled=true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c1552e512cb

enabled=true had failures in the aforementioned test, which I fixed locally.  I didn't push to try again before landing.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b2e843b2300d
https://hg.mozilla.org/mozilla-central/rev/e5463416f49a

Comment 39

[Justin Dolske [:Dolske]]

Comment on attachment 8601732 [details] [diff] [review]
patch v4

[Triage Comment]

Required for 38.0.5 Pocket launch.

Comment 40

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9308b38ecf3e
https://hg.mozilla.org/releases/mozilla-aurora/rev/af051a3d9f1f
https://hg.mozilla.org/releases/mozilla-release/rev/257c096c7673
https://hg.mozilla.org/releases/mozilla-release/rev/6be4fccbdfa3

Comment 41

[Ada [:adalucinet]]

This issue was tested during our regression testing on 38.0.5b1 (build2: 20150511143336), including additional exploratory testing, across the following platforms: Windows 7 x64, Windows 8.1 x86, Mac OS X 10.9.5 and Ubuntu 14.04 x64. The only new issue found is bug 1163917.

Comment 42

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify flag as verification on Firefox 38.0.5 Beta should suffice.