821262 - Refactor social enabled/active/provider states

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

As discussed on IRC:

* We don't need an explicit pref for the whitelist - it can be deduced from the builtin manifests.

* Social.enabled should simply reflect if Social.provider is set.  If Social.enabled is false, Social.provider will be null.  If Social.enabled is true, Social.provider must be set and it will be "active" (ie, will have a worker etc).

* provider.enabled no longer exists - only Social.provider will be enabled, no others will be.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: First cut at the social components

This is a first cut.  It doesn't fix the UI much - I'll add that as a part 2.  Adding this now to grab some early feedback.

Apart from the things mentioned in comment 0:

* Social.enabled is now just a "shortcut" for Social.provider != null.  Social is enabled automagically whenever you set Social.provider = provider.  Social.enabled could actually die completely - it's now used primarily in the tests - but it's not clear this is worthwhile - thoughts?

* Social.active no longer exists at all.

* provider.active/provider.enabled no longer exists.  Social.provider must be enabled, all other providers must not be.  SocialService.getProvider/getProviderList now only returns providers that have previously been activated, and there is a new function for grabbing the manifest of a "builtin" provider for activation purposes.

* Whitelist is dead - new methods "canActivateOrigin()" and an implementation that looks at the prefs.

* Does need more activation tests

* No longer need social.skipLoadingProviders pref - no builtin providers are loaded by the tests as they aren't active.

* Passes tests but a little work needs to be done.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 691792 [details] [diff] [review]
First cut at the social components

my first impression is that it looks good and I like where it is going, but when I apply the patch, nothing works.  fb was already activated, but no ui shows up.  resetting all prefs, later trying new profile, I cannot activate fb.  Toggling social.enabled pref on doesn't make activation work either.  I don't see any errors in the console.

I'm assuming the tests work because they explicitly set Social.provider.

digging a little: 

Social.activateFromOrigin no longer returns a provider, breaking stuff in _activationEventHandler.

On startup, it seems like a provider is never set.  Social.init calls _updateProviderCache, but since this.enabled == false, it never sets the current provider on startup.

Comment 3

[Shane Caraveo (:mixedpuppy)]

SocialService.enableProvider (and disableProvider) seem like extra code, why not just have provider.enable()?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> SocialService.enableProvider (and disableProvider) seem like extra code, why
> not just have provider.enable()?

The main reason for this is that I don't want "just anyone" to enable a provider.  The Social object maintains the invariant that only 1 provider can be enabled at a time.  If anyone could call provider.enable() that could be broken unless we make things much more complex (eg, have Social add a listener and automatically disables the previously enabled one and adjust Social.provider when this happens)

I could agree this is just "fluff" though, so I'll think a little more about this.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 691792 [details] [diff] [review]
First cut at the social components

Overall, this does make things much simpler. The inversion of control with Social.jsm managing more state feels a bit unorthodox, but makes more sense given our current model.

diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   _updateActiveUI: function SocialUI_updateActiveUI() {

>     if (!Social.provider)
>       return;

>+    let label = gNavigatorBundle.getFormattedString(Social.provider ?

This now returns early if !Social.provider, but then continues to null check it 3 times.

>-    if (Social.provider.profile === undefined) {
>+    if (provider.profile === undefined) {

I would probably go the other way and switch all uses of "provider" in this function with "Social.provider" - seems clearer that way, and no significant perf difference.

>   populateProviderMenus: function SocialToolbar_renderProviderMenus() {

>-      this._populateProviderMenu(providerMenuSep, activeProviders);
>+      this._populateProviderMenu(providerMenuSep, Social.providers);

There's no need to pass this as an argument now, _populateProviderMenu can just refer to Social.providers directly.

>diff --git a/browser/modules/Social.jsm b/browser/modules/Social.jsm

>+// Add a pref observer for the enabled state
>+function prefObserver(subject, topic, data) {

>+  let changed;

This seems to be set, but never read?

>   _setProvider: function (provider, notify) {

>     if (this._provider) {
>-      if (this.enabled)
>-        this._provider.enabled = true;
>+      SocialService.enableProvider(this._provider);
>       this._currentProviderPref = this._provider.origin;
>-    } else {
>-      Services.prefs.clearUserPref("social.provider.current");

Don't we need to keep this, so the pref isn't wrong when Social.provider is set to null?

>   get uiVisible() {
>-    return this.provider && this.provider.enabled;
>+    return !!this.provider;

At this point the helper probably isn't useful.

>   toggle: function Social_toggle() {

>+    if (newEnabled) {
>+      this.setDefaultProvider();
>+    } else {
>+      this.provider = null;

this.enabled = newEnabled;?

>   // Activation functionality
>   activateFromOrigin: function (origin) {
>+    let finalizeActivate = function(provider) {

>+        if (provider == this.provider)
>+          return null;

This can just be "return;" to avoid a "function sometimes doesn't return a value" strict warning.

>-    return provider;

Doesn't this need to continue returning the provider? browser-social uses it to show the "you just activated" notification.

>+    let manifest = SocialService.getBuiltinProviderManifest(origin);
>+    SocialService.addProvider(manifest, function(provider) {

Rather than exposing "manifests", why not just add a SocialService.getBuiltInProvider(origin, callback) that creates and returns the provider? Also avoids adding a synchronous API to SocialService, which would be problematic if we decided to later switch to specifying built-in providers using e.g. JSON on disk instead of prefs.

>   deactivateFromOrigin: function (origin, oldOrigin) {
>     let provider = this._getProviderFromOrigin(origin);
>+    SocialService.removeProvider(origin, function() {
>+      if (provider && provider == this.provider) {
>+        this.provider.active = false;

Hrm, this seems odd. Shouldn't this set active=false unconditionally? Though actually, the SocialService already sets active=false on removed engines, so this shouldn't be needed at all?

>+        this.provider = this._getProviderFromOrigin(oldOrigin);
>+        if (!this.provider)
>+          this.provider = this.providers.filter(function (p) p.active)[0];

This looks wrong, since you got rid of the "active" getter on providers.

>diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm

> let SocialServiceInternal = {

>+  get manifests() {

>+    let appinfo = Cc["@mozilla.org/xre/app-info;1"]
>+                    .getService(Ci.nsIXULRuntime);

appinfo is unused here.

> SocialProvider.prototype = {

>-  get enabled() {
>-  set enabled(val) {

I think I would like to keep this, just as a convenience method. Seems nicer than having to use SocialService.enableProvider/disableProvider. I don't think it's necessary for us to try to enforce that no one else can enable a provider.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Updated based on comments and other issues fixed

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> >   _updateActiveUI: function SocialUI_updateActiveUI() {
> 
> >     if (!Social.provider)
> >       return;
> 
> >+    let label = gNavigatorBundle.getFormattedString(Social.provider ?
> 
> This now returns early if !Social.provider, but then continues to null check
> it 3 times.

Yeah - this is already broken (ie, even before this patch it null checks, but still tries to do something when provider is null.)

I've left this broken-ness in this patch, but the UI patch fixes it.

FWIW, this patch is really just trying to keep the status quo for the UI, warts and all - the next patch "fixes" it.


> I would probably go the other way and switch all uses of "provider" in this
> function with "Social.provider" - seems clearer that way, and no significant
> perf difference.
...
> >-      this._populateProviderMenu(providerMenuSep, activeProviders);
> >+      this._populateProviderMenu(providerMenuSep, Social.providers);
> 
> There's no need to pass this as an argument now, _populateProviderMenu can
> just refer to Social.providers directly.
...
> >+  let changed;
> 
> This seems to be set, but never read?

All done.

> >   _setProvider: function (provider, notify) {
> 
> >     if (this._provider) {
> >-      if (this.enabled)
> >-        this._provider.enabled = true;
> >+      SocialService.enableProvider(this._provider);
> >       this._currentProviderPref = this._provider.origin;
> >-    } else {
> >-      Services.prefs.clearUserPref("social.provider.current");
> 
> Don't we need to keep this, so the pref isn't wrong when Social.provider is
> set to null?

We want to keep the preference set even when social is disabled so when it gets reenabled we enable the correct one.

> At this point the helper probably isn't useful.
...
> this.enabled = newEnabled;?
...
> This can just be "return;" to avoid a "function sometimes doesn't return a
> value" strict warning.

All done.

> >-    return provider;
> 
> Doesn't this need to continue returning the provider? browser-social uses it
> to show the "you just activated" notification.

Yep - but it's now done via callback than returned.

> Rather than exposing "manifests", why not just add a
> SocialService.getBuiltInProvider(origin, callback) that creates and returns
> the provider? Also avoids adding a synchronous API to SocialService, which
> would be problematic if we decided to later switch to specifying built-in
> providers using e.g. JSON on disk instead of prefs.

I ended up with SocialService.addBuiltinProvider, so "manifests" doesn't escape from SocialServiceInternal.

> >   deactivateFromOrigin: function (origin, oldOrigin) {
> >     let provider = this._getProviderFromOrigin(origin);
> >+    SocialService.removeProvider(origin, function() {
> >+      if (provider && provider == this.provider) {
> >+        this.provider.active = false;
> 
> Hrm, this seems odd. Shouldn't this set active=false unconditionally? Though
> actually, the SocialService already sets active=false on removed engines, so
> this shouldn't be needed at all?

Fixed.

> >+        this.provider = this._getProviderFromOrigin(oldOrigin);
> >+        if (!this.provider)
> >+          this.provider = this.providers.filter(function (p) p.active)[0];
> 
> This looks wrong, since you got rid of the "active" getter on providers.

It was :)  Fixed.

> appinfo is unused here.
> > SocialProvider.prototype = {
> 
> >-  get enabled() {
> >-  set enabled(val) {

All done.

A couple of other notable things:

In Social.jsm:

+  get defaultProviderOrigin() {
+  setDefaultProvider: function() {

defaultProviderOrigin is a "safe" wrapper around _currentProviderPref() - it tries to use _currentProviderPref, but if that happens to refer to an origin that doesn't exist (eg, after the provider was removed), it falls back to using the first provider.  This consolidates where the logic for "what provider will be enable" and ends up being used by the UI patch I'll attach next.

Note the tests all work with this patch, but as mentioned, the UI probably has some warts.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Update the UI based on previous patch and other multi-provider considerations

Another fairly large and intrusive one.  Some notes:

browser-social now references SocialService.jsm simply to call SocialService.getProvider() - maybe we should just expose this directly in Social?

There is now SocialUI.provider and SocialUI.providers getters, and browser-social uses them in preference to Social.*.  The reason is we can consolidate the "chromeless" checks here to pretend social is disabled in the window, and probably more importantly, the per-window private-browsing patch will be able to hook into this to also "disable" social on a per-window basis.

The strings for "toggle" have changed.  Instead of "Turn off {provider}" the string now says "Turn off Social Features" - possibly not the best, but bug 789672 is asking Boriss to look at this.  Similarly, "Remove from Nightly" now reads "Remove {provider} from {brand}" as all that does is remove a provider, leaving social enabled if other providers exist.

Even when social is disabled, the UI still refers to the provider (eg, the provider icon is still shown in the toolbar, and "Remove {provider} from {brand}" is still exposed.  Thus, a new SocialUI.getCurrentProviderManifest() function exists - even when social is disabled, it returns the metadata for the "default provider" so the name and icon can be used.  Ultimately we probably should consider changing our UI here - social doesn't *look* disabled while a provider icon remains in the toolbar - so this might be able to be removed later (and is also the reason I didn't expose it via Social yet)

A couple of other broadcaster update tweaks etc - eg, Firefox now starts up with an identical UI when social is disabled (currently if you exit with social disabled the UI isn't identical to how it was when you exited).

Activation/deactivation works, tests pass :)

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 692187 [details] [diff] [review]
Updated based on comments and other issues fixed

Please ignore the browser.properties changes in this patch - they are also in the "ui fixes" patch and I've fixed things in my patch queue.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated against trunk

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: updated against trunk

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 693173 [details] [diff] [review]
updated against trunk

The addEventListener for this._activationEventHandler needs a bind(this), otherwise there are some failures in activationEventHandler (e.g. this.provider).

Once bind is added, the undo still sets some things wrong.  The icon for the toolbar button is the first provider in the list, rather than the loaded providers icon.  I haven't tracked that down entirely, but getCurrentProviderManifest looks suspect, and is what is used to set icons in some paths.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Attachment: Remove social.active, Social.provider always matched enabled state

Updated against trunk

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Attachment: Fixes to social ui

This patch fixes the 2 problems found by Shane in the previous one and also includes activation tests.  Also:

* There is a new notification sent when the number of providers changes.  Our current UI doesn't really need this today (only the "current" provider is ever removed, meaning a provider-set notification is sent), but this change is forward looking to when a non-current provider may be removed (and is needed by one of the activation tests, which tests the removal of a non-current provider)

* There is a CheckSocialUI function in head.js - it is in head.js as the function initially came from the "per-window PB" patch, so given it will be used by the per-window tests when they do land, I figured it might as well go into head.js now.  Many of the other tests could also call this, but that's not part of this patch.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 693735 [details] [diff] [review]
Remove social.active, Social.provider always matched enabled state

This is the same patch Shane already gave f+ on, so removing that request.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 694106 [details] [diff] [review]
Fixes to social ui


>+  // Gets the "manifest" for the current provider. If Social is disabled, it
>+  // gets the one that would be current and returns it.
>+  // XXX - this should be considered temporary while the UI when social is
>+  // disabled still references the provider that *would* be current if it
>+  // was enabled.

That comment is a bit weird, not entirely sure what you're trying to say.

>+  getCurrentProviderManifest: function(cbdone) {
>+    // We don't actually have a manifest object - pretend we do.  Maybe this
>+    // should be part of Social, but having it here doesn't seem too bad for
>+    // now...
>+    function providerToManifest(provider) {
>+      let result = {}
>+      for (let attr of ["name", "iconURL", "workerURL", "sidebarURL", "origin"])
>+        result[attr] = provider[attr];
>+      return result;
>     }

I would like to see that as part of the SocialProvider class, it would make it easier to keep in sync later (e.g. share may add a url).

I'm not really clear on why we need a "manifest" object, what is wrong with just getting a provider instance the way you do below:

>+    // XXX - seeing this is temporary, we don't have this import at the
>+    // top-level - it's unlikely we would remember to remove it!
>+    let SocialService = Cu.import("resource://gre/modules/SocialService.jsm", {}).SocialService;
>+    SocialService.getProvider(origin, function(provider) {
>+      cbdone(provider ? providerToManifest(provider) : null);
>+    });


Outside of that, I still get the incorrect icon when I "undo" an activation.  But I had to hand patch since this is touch bitrotted.  Happy to retest that with an updated patch in case I missed something.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> Comment on attachment 694106 [details] [diff] [review]
> Fixes to social ui
> 
> 
> >+  // Gets the "manifest" for the current provider. If Social is disabled, it
> >+  // gets the one that would be current and returns it.
> >+  // XXX - this should be considered temporary while the UI when social is
> >+  // disabled still references the provider that *would* be current if it
> >+  // was enabled.
> 
> That comment is a bit weird, not entirely sure what you're trying to say.

Which part?

The first 2 lines:  "manifest" is in scare quotes as we don't currently have the concept of a manifest.  The term "current provider" only makes sense if social is enabled - no provider is current if social is disabled.  So this function returns whatever provider *would* be current if social was enabled.

The next 3 lines: Our UI currently references the "current provider" even when social is disabled.  IMO this is a problem that should be fixed - social should not need to reference a provider when it is disabled - mainly as social does not *look* disabled while a provider icon is visible.

Does that make sense?

> >+  getCurrentProviderManifest: function(cbdone) {
> >+    // We don't actually have a manifest object - pretend we do.  Maybe this
> >+    // should be part of Social, but having it here doesn't seem too bad for
> >+    // now...
> >+    function providerToManifest(provider) {
> >+      let result = {}
> >+      for (let attr of ["name", "iconURL", "workerURL", "sidebarURL", "origin"])
> >+        result[attr] = provider[attr];
> >+      return result;
> >     }
> 
> I would like to see that as part of the SocialProvider class, it would make
> it easier to keep in sync later (e.g. share may add a url).

See above - I don't want to move that to the SocialProvider class as it is a short-term hack that should be removed.  We will not need this function, nor the concept of a manifest once we fix the UI in browser-social.js to not require a provider when social is disabled.  I'm reluctant to move code needed just for a short-term hack to the SocialProvider class.

> I'm not really clear on why we need a "manifest" object, what is wrong with
> just getting a provider instance the way you do below:

Mainly as Social is disabled at this point, so returning a provider seems misguided - in theory providers should fail to be instantiated while social is disabled.  We don't want a provider object, we only want metadata about it.

Does all the above make sense?

> Outside of that, I still get the incorrect icon when I "undo" an activation.
> But I had to hand patch since this is touch bitrotted.  Happy to retest that
> with an updated patch in case I missed something.

Did you happen to see if the new test passes?  If test does pass but you still see the wrong icon, it implies the tests aren't working as expected either.

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #16)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> > Comment on attachment 694106 [details] [diff] [review]
> > Fixes to social ui
> > 
> > 
> > >+  // Gets the "manifest" for the current provider. If Social is disabled, it
> > >+  // gets the one that would be current and returns it.
> > >+  // XXX - this should be considered temporary while the UI when social is
> > >+  // disabled still references the provider that *would* be current if it
> > >+  // was enabled.
> > 
> > That comment is a bit weird, not entirely sure what you're trying to say.
> 
> Which part?

The XXX part just doesn't read well, specifically my brain doesn't wrap around "while the UI when social".

> The first 2 lines:  "manifest" is in scare quotes as we don't currently have
> the concept of a manifest.  The term "current provider" only makes sense if
> social is enabled - no provider is current if social is disabled.  So this
> function returns whatever provider *would* be current if social was enabled.
> 
> The next 3 lines: Our UI currently references the "current provider" even
> when social is disabled.

As I understand, that is only for the provider icon when disabled.  The addon used a different icon in disabled state, I wonder if we should do that.


> 
> > >+  getCurrentProviderManifest: function(cbdone) {
> > 
> > I would like to see that as part of the SocialProvider class, it would make
> > it easier to keep in sync later (e.g. share may add a url).
> 
> See above - I don't want to move that to the SocialProvider class as it is a
> short-term hack that should be removed.  We will not need this function, nor
> the concept of a manifest once we fix the UI in browser-social.js to not
> require a provider when social is disabled.  

What is the plan on fixing that, should it be a part of this patch (or bug)?

> > I'm not really clear on why we need a "manifest" object, what is wrong with
> > just getting a provider instance the way you do below:
> 
> Mainly as Social is disabled at this point, so returning a provider seems
> misguided - in theory providers should fail to be instantiated while social
> is disabled.  We don't want a provider object, we only want metadata about
> it.
> 
> Does all the above make sense?

Makes sense, though I don't see a problem with getting a provider instance when disabled.  We could keep the manifests we load in SocialService so we don't have to rebuild them, though it seems we wont need that later.

> > Outside of that, I still get the incorrect icon when I "undo" an activation.
> > But I had to hand patch since this is touch bitrotted.  Happy to retest that
> > with an updated patch in case I missed something.
> 
> Did you happen to see if the new test passes?  If test does pass but you
> still see the wrong icon, it implies the tests aren't working as expected
> either.

The "toolbar button has provider icon" tests fail.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)

> The XXX part just doesn't read well, specifically my brain doesn't wrap
> around "while the UI when social".

OK - I'll fix that.

> As I understand, that is only for the provider icon when disabled.  The
> addon used a different icon in disabled state, I wonder if we should do that.

I think we should, yeah.

> What is the plan on fixing that, should it be a part of this patch (or bug)?

I think it should be part of a different UX-assigned bug,  As you mention above, I think we need an icon specifically for this purpose.  Or something :)  Either way, I think some UX love here is desperately needed.  This patch makes the UI more sane - it just doesn't make it good :)

> Makes sense, though I don't see a problem with getting a provider instance
> when disabled.  We could keep the manifests we load in SocialService so we
> don't have to rebuild them, though it seems we wont need that later.

Yeah, this is exactly what I was thinking - if we probably will not need it later, adding it to SocialService just makes it likely the code will not be removed (ie, that we will be left with dead code which adds complexity that is never actually used).

I'm happy to reconsider this (and I'm sure Gavin will have his own opinions), but I'm trying to err on the side of keeping short-term hacks localized to where the hacks exist.

> The "toolbar button has provider icon" tests fail.

Ahh, great, thanks - that probably does mean the patch was applied incorrectly (or more likely, a recent change to use listStyleImage instead of the "image" attribute simply broke this part).  I'll update it next week.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Attachment: Fixes to social ui

This update applies against the current trunk and has been updated to reflect the recent change which uses style.listStyleImage instead of the 'image' attribute for the toolbar items.  I've fixed (hopefull) the comment for getCurrentProviderManifest but haven't moved it to SocialProvider as per my most recent comments in this bug.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3 changes

The patch applies on top of the other two patches.

These are changes that I think we should do to these patches after spending more time reviewing them.  The changes in Social.jsm simplify some of the logic there.  The changes browser-social get rid of the extra manifest stuff and just rely on the provider classes that already exist in the Social object.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 693735 [details] [diff] [review]
Remove social.active, Social.provider always matched enabled state

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>   updateButtonHiddenState: function SocialToolbar_updateButtonHiddenState() {

>-    let socialEnabled = Social.uiVisible;
>+    let socialEnabled = Social.provider;

should use "!!" here just to be safe

>diff --git a/browser/base/content/test/social/browser_social_shareButton.js b/browser/base/content/test/social/browser_social_shareButton.js

> function testInitial(finishcb) {
>   ok(Social.provider, "Social provider is active");
>-  ok(Social.provider.enabled, "Social provider is enabled");
>   let port = Social.provider.getWorkerPort();
>   ok(port, "Social provider has a port to its FrameWorker");

Why remove this?

>diff --git a/browser/modules/Social.jsm b/browser/modules/Social.jsm

>   // Sets the current provider and enables and activates it. Also disables the
>   // previously set provider, and optionally notifies observers of the change.

This comment needs an update.

>   _setProvider: function (provider, notify) {

>+    Services.obs.notifyObservers(null, "social:pref-changed", enabled ? "enabled" : "disabled");

Shouldn't this only notify if the state is changing? We don't want to notify both provider-set and pref-changed every time someone switches providers.

>+  get defaultProviderOrigin() {

>+    origin = this.providers[0].origin;
>+    if (origin && this._getProviderFromOrigin(origin))
>+      return origin;

This check seems useless.

>   _updateProviderCache: function (providers, notifyProviderChange) {

>+    if (!curProviderOrigin) {

>+          curProviderOrigin = this._currentProviderPref = providers[0];

This needs to be providers[0].origin, right?

>   toggle: function Social_toggle() {
>-    this.enabled = this._disabledForSafeMode ? false : !this.enabled;
>+    let newEnabled = this._disabledForSafeMode ? false : !this.enabled;
>+    this.enabled = newEnabled;

This change seems unnecessary.

>   deactivateFromOrigin: function (origin, oldOrigin) {

>+    // removeProvider ends up setting this.provider to null (and this
>+    // disabling social) via the provider-removed observer if the provider was
>+    // current, so remember now if that is the case.
>+    let wasCurrent = provider && provider == this.provider;

I think it's currently impossible for this to be called for a non-this.provider provider at the moment - may be worth noting.

>diff --git a/toolkit/components/social/MozSocialAPI.jsm b/toolkit/components/social/MozSocialAPI.jsm

>     SocialService.getProvider(origin, function(provider) {
>-      if (provider && provider.workerURL && provider.enabled) {
>+      if (provider && provider.workerURL) {

I think this check should remain - we don't want to inject the API for active but disabled providers (at least not yet - bug 820601).

>diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm

>     if (enable == SocialServiceInternal.enabled &&
>         !Services.appinfo.inSafeMode)
>       return;
> 
>-    Services.prefs.setBoolPref("social.enabled", enable);
>     this._setEnabled(enable);
>   },
>   _setEnabled: function _setEnabled(enable) {

Now that _setEnabled has only one caller, there's no need to have it be a separate function. Also, the safe mode code is broken by this, because _setEnabled does the same check without the safemode condition. I guess we should probably just remove it, though that would regress parts of bug 786095 (Jared/Felipe may want to weigh in on this)

>+  addBuiltinProvider: function addBuiltinProvider(origin, onDone) {

>+    return null;

Should just be "return;" to avoid a strict warning.

>   removeProvider: function removeProvider(origin, onDone) {

>-    let provider = SocialServiceInternal.providers[origin];
>-    provider.enabled = false;
>-
>-    ActiveProviders.delete(provider.origin);
>+    SocialServiceInternal.providers[origin]._terminate();
>+    ActiveProviders.delete(origin);

This could stay as-is now.

>   _activate: function _activate() {

>+    this._activated = true;

>   _terminate: function _terminate() {
>+    if (!this._activated)
>+      return;

>+    this._activated = false;
>   },

>   getWorkerPort: function getWorkerPort(window) {
>-    if (!this.workerURL || !this.enabled)
>+    if (!this._activated || !this.workerURL)

These changes seem unnecessary now that we're keeping SocialProvider.enabled.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Since Mark is on PTO, I'll take over doing any rework on the patches, with Gavin/Jared doing review.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 698491 [details] [diff] [review]
part 3 changes

I'm a little torn by these changes.  On one hand it does simplify the code in the short term.  However, in the longer term I believe getCurrentProviderManifest can be removed entirely - at which point I don't think these changes would still be desirable, but would be far more difficult to undo.  IOW, I don't think Social.getDefaultProvider() makes sense as an API in the longer term (it overlaps with Social.provider without an obvious distinction)

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 696452 [details] [diff] [review]
Fixes to social ui

>diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js

>-    Social.init(this._providerReady.bind(this));
>+    Social.init(function() {
>+      // init all the UI components.
>+      SocialShareButton.init();
>+      SocialToolbar.init();
>+      SocialSidebar.init();
>+      this._updateEnabledState();

Hmm, this is what ends up updating the "active" broadcaster - is it really a good idea to call it last?

Also, _updateEnabledState calls a bunch of update() methods that we weren't previously calling on window load. For example, SocialSidebar.update is now called twice on window load, AFAICT - once via SocialSidebar.init, and then via the SocialSidebar.update() call in _updateEnabledState. Similar deal with SocialShareButton.updateButtonHiddenState - called via _updateEnabledState but also via SocialShareButton.init -> SocialShareButton.updateProvider. I don't doubt that there are existing inefficiencies like that too, but we should try to at least not make this worse :/

>+  get _chromeless() {

>+    // This property is "fixed" for a window, so avoid doing the check above
>+    // multiple times...
>+    this._chromeless = chromeless;

You need to delete this._chromeless to override the getter, I think.

>+  get provider() {

>+    // Returns the current provider *for this window*.  This will also get
>+    // a check for a per-window private-browsing model later...
>+    if (this._chromeless)
>+      return null;

This is a bit scary, since decouples the "enabled" state for a window from other observable/global social state. Putting the provider reference on the window also suggests that different windows can have different providers, and there's a bunch of reasons why that wouldn't work at the moment. I would kind of like to avoid making this change and instead introduce a per-window "SocialUI.enabled" state or somesuch, but I guess that's potentially a larger change.

>+  // XXX - this should be considered temporary: Currently the UI still shows a
>+  // provider name and image even when social is disabled - that should
>+  // change to a generic icon and text in that case (and this entire function
>+  // can then be removed).

I agree that this is gross and icky and that we should followup quickly to fix it as you describe.

>+      shareButton.hidden = !SocialUI.provider || SocialUI.provider.recommendInfo == null ||
>                            !Social.haveLoggedInUser() ||
>                            !this.canSharePage(gBrowser.currentURI);

This and the SocialChatbar.isAvailable getter highlight that it's a bit odd to now be tracking "provider" per-window, but not haveLoggedInUser. Users of the latter need to be sure they're also checking the former. That was kind of true before (in that they also checked the state of the feature in general using Social.uiVisible or Social.enabled), but it was clearer that way that that's what they were doing. Maybe we should keep a SocialUI.enabled getter (it can just return !!this.provider, but the semantics are clearer).

>   init: function SocialToolbar_init() {

>     let accesskey = gNavigatorBundle.getString("social.remove.accesskey");
>     let removeCommand = document.getElementById("Social:Remove");

Bit odd to be only setting the access key. Can you add a comment about where the label gets set?

>   updateButtonHiddenState: function SocialToolbar_updateButtonHiddenState() {

>-    let socialEnabled = Social.provider;
>+    let socialEnabled = !!SocialUI.provider;

Ah, already fixed from my previous review comment :)

>   updateButton: function SocialToolbar_updateButton() {

>+    let icons = SocialUI.provider.ambientNotificationIcons;

>+    if (!SocialUI.provider ||

(pre-existing) non-sequitur.

>   updateProvider: function () {

>+    if (!SocialUI.provider)
>       return;
>     this.updateProfileInfo();

I think it would be a lot clearer for this null check to move to updateProfileInfo.

>   // Whether the sidebar can be shown for this window.
>   get canShow() {
>-    return Social.provider && Social.provider.sidebarURL && !this.chromeless;

>+    return SocialUI.provider && SocialUI.provider.sidebarURL;

Hrm, removing the disabledchrome check I understand (to fix bug 821768), but don't we want to continue not showing the sidebar in popup windows?

>diff --git a/browser/base/content/test/social/browser_social_activation.js b/browser/base/content/test/social/browser_social_activation.js

>+function postTestCleanup(callback) {

I generally these "clean up the world indiscriminately" approaches since they can cover up bugs, but they are often expedient.

>diff --git a/browser/base/content/test/social/head.js b/browser/base/content/test/social/head.js

>+function checkSocialUI(win) {

>+  dump("LSI: " + win.SocialToolbar.button.style.listStyleImage + "\n");

Need to lose this before landing. Some of this function is a bit "close to the code" (e.g. in that it checks that some getters match the same conditions that they just return, which isn't a super-useful check), but that's not a huge deal.

>diff --git a/browser/base/content/test/social/social_worker.js b/browser/base/content/test/social/social_worker.js

>       case "social.initialize":
>+        if (location.hash.indexOf("no-profile") >= 0)
>+          return;
>         // This is the workerAPI port, respond and set up a notification icon.
>         // For multiprovider tests, we support acting like different providers
>         // based on the domain we load from.
>         apiPort = port;

The early-return should probably come after the setting of apiPort?

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>-social.remove.label=Remove from %S
>+social.remove.label=Remove %1$S from %2$S

>-social.turnOff.label=Turn off %S
>+social.turnOff.label=Turn off Social Features

>-social.turnOn.label=Turn on %S
>+social.turnOn.label=Turn on Social Features

We'll definitely need better strings here... "Social Features" feels really klunky (particularly capitalized that way).

When changing strings, you need to also change their names to notify localizers of the change.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 698491 [details] [diff] [review]
part 3 changes

I think I agree with Mark - if we weren't going to remove this, I think this is probably an improvement, but it's probably better to keep things ugly so we make sure we remove them ;)

The last hunk of this patch is good, though!

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Attachment: Remove social.active, updated to m-c

Updated to resolve merge conflicts, but no other feedback yet addressed.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Attachment: Fixes to social ui, updated to m-c

Updated to fix merge conflicts, but no other feedback comments addressed.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Attachment: part 1: Remove social.active, updated based on feedback

Updating this in the interests of helping push this along for the sake of the dependent per-window PB patch.

All feedback comments not mentioned have been addressed as requested.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #21)
> Comment on attachment 693735 [details] [diff] [review]

> >   // Sets the current provider and enables and activates it. Also disables the
> >   // previously set provider, and optionally notifies observers of the change.
> 
> This comment needs an update.

The only change I made here was to remove the "and activates it" - I hope that is what was meant.

> >   deactivateFromOrigin: function (origin, oldOrigin) {
> 
> >+    // removeProvider ends up setting this.provider to null (and this
> >+    // disabling social) via the provider-removed observer if the provider was
> >+    // current, so remember now if that is the case.
> >+    let wasCurrent = provider && provider == this.provider;
> 
> I think it's currently impossible for this to be called for a
> non-this.provider provider at the moment - may be worth noting.

I didn't make this change as I'm reluctant to add a comment indicating it is "currently" impossible to call this when the provider isn't current - it should work when that is no longer true, but the comment would be likely to remain and be incorrect.

I'd be happy to remove the "wasCurrent" check completely though if that is desired.

> >diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm
> 
> >     if (enable == SocialServiceInternal.enabled &&
> >         !Services.appinfo.inSafeMode)
> >       return;
> > 
> >-    Services.prefs.setBoolPref("social.enabled", enable);
> >     this._setEnabled(enable);
> >   },
> >   _setEnabled: function _setEnabled(enable) {
> 
> Now that _setEnabled has only one caller, there's no need to have it be a
> separate function. Also, the safe mode code is broken by this, because
> _setEnabled does the same check without the safemode condition. I guess we
> should probably just remove it, though that would regress parts of bug
> 786095 (Jared/Felipe may want to weigh in on this)

I did fold the function and removed the redundant check - so now the only check is the initial one which includes the 'inSafeMode' check.  I think this resolves that concern.

> >+  addBuiltinProvider: function addBuiltinProvider(origin, onDone) {
> 
> >+    return null;
> 
> Should just be "return;" to avoid a strict warning.

Given that was the last statement in the function, I just removed the return entirely.

Comment 29

[Mark Hammond [:markh] [:mhammond]]

Attachment: part 2: Fixes to social ui, most review comments addressed.

This is a slightly rushed update to the patch based on Gavin's comments.  I hope Shane can take this over for next week.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)
> Comment on attachment 696452 [details] [diff] [review]
> Fixes to social ui
> 
> >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js
> 
> >-    Social.init(this._providerReady.bind(this));
> >+    Social.init(function() {
> >+      // init all the UI components.
> >+      SocialShareButton.init();
> >+      SocialToolbar.init();
> >+      SocialSidebar.init();
> >+      this._updateEnabledState();
> 
> Hmm, this is what ends up updating the "active" broadcaster - is it really a
> good idea to call it last?

I don't see a problem with calling it last as all the other components check the state when necessary.  Also, as _updateEnabledState also calls various update() methods, the init() functions really do need to be called first.

> Also, _updateEnabledState calls a bunch of update() methods that we weren't
> previously calling on window load. For example, SocialSidebar.update is now
> called twice on window load, AFAICT - once via SocialSidebar.init, and then
> via the SocialSidebar.update() call in _updateEnabledState. Similar deal
> with SocialShareButton.updateButtonHiddenState - called via
> _updateEnabledState but also via SocialShareButton.init ->
> SocialShareButton.updateProvider. I don't doubt that there are existing
> inefficiencies like that too, but we should try to at least not make this
> worse :/

Yeah :(  I've fixed the sidebar, but the ShareButton is more problematic, as it really does need to update the hidden state based on provider info changing *and* the feature itself being disabled.  Maybe Shane can see what he can do here.

> This is a bit scary, since decouples the "enabled" state for a window from
> other observable/global social state. Putting the provider reference on the
> window also suggests that different windows can have different providers,
> and there's a bunch of reasons why that wouldn't work at the moment. I would
> kind of like to avoid making this change and instead introduce a per-window
> "SocialUI.enabled" state or somesuch, but I guess that's potentially a
> larger change.

Yes, that's a good idea.  This patch drops SocialUI.provider and SocialUI.providers, and adds a SocialUI.enabled boolean.  Note that other parts of the code assume that if SocialUI.enabled is true, Social.provider must be non-null, but I think that's ok.

> >   updateButton: function SocialToolbar_updateButton() {
> 
> >+    let icons = SocialUI.provider.ambientNotificationIcons;
> 
> >+    if (!SocialUI.provider ||
> 
> (pre-existing) non-sequitur.

Not sure what you mean here?

> >   updateProvider: function () {
> 
> >+    if (!SocialUI.provider)
> >       return;
> >     this.updateProfileInfo();
> 
> I think it would be a lot clearer for this null check to move to
> updateProfileInfo.

Actually, I just removed it as updateProfileInfo already handled the provider being null.

> >   // Whether the sidebar can be shown for this window.
> >   get canShow() {
> >-    return Social.provider && Social.provider.sidebarURL && !this.chromeless;
> 
> >+    return SocialUI.provider && SocialUI.provider.sidebarURL;
> 
> Hrm, removing the disabledchrome check I understand (to fix bug 821768), but
> don't we want to continue not showing the sidebar in popup windows?

This now uses Social.enabled, and that in-turn checks SocialUI._chromeless, which IIUC, means canShow will return false for popups.

> >diff --git a/browser/base/content/test/social/browser_social_activation.js b/browser/base/content/test/social/browser_social_activation.js
> 
> >+function postTestCleanup(callback) {
> 
> I generally these "clean up the world indiscriminately" approaches since
> they can cover up bugs, but they are often expedient.

Yeah - I left that alone as I feel it really isn't that bad in this context.

> >diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties
> 
> >-social.remove.label=Remove from %S
> >+social.remove.label=Remove %1$S from %2$S
> 
> >-social.turnOff.label=Turn off %S
> >+social.turnOff.label=Turn off Social Features
> 
> >-social.turnOn.label=Turn on %S
> >+social.turnOn.label=Turn on Social Features
> 
> We'll definitely need better strings here... "Social Features" feels really
> klunky (particularly capitalized that way).

Hrmph - not sure what to do about this - for now, I just dropped the capital "F".

All other comments addressed.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #29)
> > >   updateButton: function SocialToolbar_updateButton() {
> > 
> > >+    let icons = SocialUI.provider.ambientNotificationIcons;
> > 
> > >+    if (!SocialUI.provider ||
> > 
> > (pre-existing) non-sequitur.
> 
> Not sure what you mean here?

Unconditional use of "Social.provider" followed by a null check of it - null check will never return false because we would have thrown earlier on in this function.

> This now uses Social.enabled, and that in-turn checks SocialUI._chromeless,
> which IIUC, means canShow will return false for popups.

Indeed, forgot about that.

> Hrmph - not sure what to do about this - for now, I just dropped the capital
> "F".

Boriss can probably advise - we need a generic name for the feature in general.

Comment 31

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (away Jan 16-23) from comment #30)
> (In reply to Mark Hammond (:markh) from comment #29)

> > Hrmph - not sure what to do about this - for now, I just dropped the capital
> > "F".
> 
> Boriss can probably advise - we need a generic name for the feature in
> general.

I've created bug 831442 to address the strings and whether we should use a default icon when disabled.

Comment 32

[Shane Caraveo (:mixedpuppy)]

Attachment: rollup off all patches

This patch rolls the others together.  It also fixes SocialUI._updateActiveUI (now commented for correct behavior).  I've introduced a couple cleanups from the part 3 patch while maintaining some of the isolation of code that will later get removed when bug 831442 is addressed.

Comment 33

[(no longer active)]

Is there an ETA on the reviews here?  Also another question, is this going to be safe enough for Aurora?  This blocks bug 808215, which is the last big thing that needs to be backported to Aurora in order for us to ship per-window private browsing in Firefox 20.

Thanks!

Comment 34

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3 changes v2

The rollup patch is part 1 + part 2 + the patches in part 3 v2.

The changes in part 3 v2 remove redundant work.  

In Social.jsm;

The setDefaultProvider/defaltProviderOrigin combo looped twice to set a provider rather than just once.

In browser-social.jsm;

Serializing an instance just to set a name and icon is extra code, just use the provider instance we already have.

Comment 35

[Shane Caraveo (:mixedpuppy)]

Attachment: part 3 changes v2

adds another fix I had forgot about, specifically in _updateActiveUI.  The button was removed if we disabled social, it should remain so the user can reenable social.

Comment 36

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

Comment on attachment 703019 [details] [diff] [review]
rollup off all patches

I tried reviewing this roll-up patch, and looking at the other patches, but there is just too much going on here for me to wrap my head around.

If this is a collection of issues that need refactoring, I would rather that they be broken apart in to separate bugs. Smaller patches are easier to review and easier to understand what the changes entail.

Comment 37

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1

This rebases the patch on top of the landed private window support.  

I am including the simplification of the setDefaultProvider as it avoids the double lookup in the original.

Comment 38

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2

part 2, rebased on top of the private window patch that landed.

This also makes use of the simplified _defaultProvider from part 1, and gets rid of the serialization from one object to another (see getCurrentProvider).

Comment 39

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #36)
> Comment on attachment 703019 [details] [diff] [review]
> rollup off all patches
> 
> I tried reviewing this roll-up patch, and looking at the other patches, but
> there is just too much going on here for me to wrap my head around.
> 
> If this is a collection of issues that need refactoring, I would rather that
> they be broken apart in to separate bugs. Smaller patches are easier to
> review and easier to understand what the changes entail.

Unfortunately these just cannot be broken down beyond the 2 patches.  This is an important change and both Mark and myself have gone over these changes in detail, and I have been running/testing/building on top of these patches for some time now.

Comment 40

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 706060 [details] [diff] [review]
part 1

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

Looks good, apart from keeping Social.active (and I can't see any code left that relies on it)

::: browser/modules/Social.jsm
@@ +110,5 @@
>        return;
>      }
>  
>      if (!this._addedObservers) {
>  #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING

This #ifndef block can now wrap the entire block.

@@ +168,5 @@
>        } else if (aData == "exit") {
>          // if the user has explicitly re-enabled social in PB mode, then upon
>          // leaving we want to tear the world down then reenable to prevent
>          // information leaks during this transition.
> +        // The next 2 lines rely on the fact that setting this.provider to

this comment doesn't make much sense anymore - I'd just delete the last 2 lines of the comment block.

@@ +194,4 @@
>    },
>  
>    get active() {
> +    return this.providers.length > 0;

the previous patch killed Social.active entirely (it's even in the bug title).  I think we should kill it - it adds no real value and keeps the confusion between "active" and "enabled".  I don't think there is anything wrong with the couple of places that need this concept to explicitly check providers.length - explicit is good and would make that code easier to understand too.

Comment 41

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 706061 [details] [diff] [review]
part 2

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

Looks good to me, but f- for the change which both Gavin and I didn't like in previous iterations.  With that fixed it would be f+.

There are some references in head.js to this bug - all such references should now be able to be removed (specifically, Social:ToggleNotifications - I think you removed one other todo from head.js in part1)

::: browser/base/content/browser-social.js
@@ +40,5 @@
> +      // init all the UI components.
> +      SocialShareButton.init();
> +      SocialToolbar.init();
> +      SocialSidebar.init();
> +      this._updateEnabledState();

I think Gavin's previous comment on this is still valid (or hasn't been refuted):
"""
Also, _updateEnabledState calls a bunch of update() methods that we weren't previously calling on window load. For example, SocialSidebar.update is now called twice on window load, AFAICT - once via SocialSidebar.init, and then via the SocialSidebar.update() call in _updateEnabledState. Similar deal with SocialShareButton.updateButtonHiddenState - called via _updateEnabledState but also via SocialShareButton.init -> SocialShareButton.updateProvider. I don't doubt that there are existing inefficiencies like that too, but we should try to at least not make this worse :/
"""

This is a little tricky, but we should see what we can do (and IIRC, the removal of SocialSidebar.update() from SocialSidebar.init() was the best I could come up with).  Another alternative is another bug for yet another refactor along the lines of:
* Have each "social widget" define just a few public functions - "init", "providerchanged", "enabledchanged" etc, and have SocialUI just call them.  The responsibility for not doing extra work then becomes more localised in each of the widgets rather than in SocialUI itself.

@@ +332,2 @@
>      }
> +    cbdone(Social._defaultProvider);

isn't this the exact change that both Gavin and I previously said should not be done?  f- for that alone, given we've both explicitly made that comment previously.

@@ +791,5 @@
>    updateProvider: function () {
> +    if (!SocialUI.enabled) {
> +      // even if there is no provider, we want to try and set the icon to the
> +      // provider that would be current
> +      SocialUI.getCurrentProvider(function(manifest) {

this doesn't make any sense with your change above - it's no longer a "manifest" and the provider could just be passed as-is.  However, it is the change above which should change, not this :)

::: browser/base/content/test/social/Makefile.in
@@ +11,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  _BROWSER_FILES = \
>                   head.js \
> +                 browser_social_activation.js \

even though it was my patch which introduced this, I was thinking yesterday that the activation tests can almost certainly end up in part1 rather than part2.  However, I don't think that is a big deal...

Comment 42

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 706061 [details] [diff] [review]
part 2

chatting with Shane on IRC, I see my initial API concerns no longer apply, so I'm withdrawing my objection.

Comment 43

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1

feedback addressed

Comment 44

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #41)
> Comment on attachment 706061 [details] [diff] [review]
> part 2

> ::: browser/base/content/browser-social.js
> @@ +40,5 @@
> > +      // init all the UI components.
> > +      SocialShareButton.init();
> > +      SocialToolbar.init();
> > +      SocialSidebar.init();
> > +      this._updateEnabledState();
> 
> I think Gavin's previous comment on this is still valid (or hasn't been
> refuted):
> """
> Also, _updateEnabledState calls a bunch of update() methods that we weren't


I have a way to address this I think, but I'm going to do it in a separate patch or perhaps a follow up bug.

Comment 45

[Shane Caraveo (:mixedpuppy)]

Attachment: part 2

addresses feedback, except that in comment #44

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 706171 [details] [diff] [review]
part 1

>diff --git a/browser/modules/Social.jsm b/browser/modules/Social.jsm

>+#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING

This is no longer useful anymore given bug 818800, right?

>+#ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING
>   observe: function(aSubject, aTopic, aData) {

This can be removed too.

>   deactivateFromOrigin: function (origin, oldOrigin) {
>     let provider = this._getProviderFromOrigin(origin);

>+    SocialService.removeProvider(origin, function() {

This function used to handle an "origin" param that doesn't exist - now it will throw (because removeProvider will throw). May be worth keeping the null check of _getProviderFromOrigin's result.

>diff --git a/toolkit/components/social/SocialService.jsm b/toolkit/components/social/SocialService.jsm

>-    if (!enable)
>-      SocialServiceInternal.providerArray.forEach(function (p) p.enabled = false);

Can we keep this? It's a safe guard against the UI code not properly tracking its provider, and also a global switch that may be useful.

Comment 47

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1 rebased

rebased and a couple small changes

- add feedback from review
- fix _updateActiveUI so support Social.provider==null
- fix deactivateFromOrigin to always reset to oldProvider prior to removing deactivated provider

sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=5966ac5ad49b

Probably wont send to inbound tonight, and with additional changes asking a quick r+ again.

Comment 48

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 710438 [details] [diff] [review]
part 1 rebased

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

driveby

::: browser/base/content/browser-social.js
@@ +166,5 @@
>      toggleCommand.setAttribute("hidden", enabled ? "false" : "true");
>  
>      if (enabled) {
> +      // enabled == true means we at least have a defaultProvider
> +      let provider = Social.provider ? Social.provider : Social.defaultProvider;

Social.provider || Social.defaultProvider; ?

::: browser/modules/Social.jsm
@@ +97,5 @@
> +    let provider = this._getProviderFromOrigin(this._currentProviderPref);
> +    return provider || this.providers[0];
> +  },
> +
> +  setDefaultProvider: function() {

Do we really need this?  At a quick glance, it looks like a setter for defaultProvider (or looks as though it acts like one).  Given it is only a 1 line impl, I think it would be better to replace the few callers with this.provider = this.defaultProvider as it is more explicit about what is going on.

@@ +137,5 @@
>        // used.
>        try {
>          let active = Services.prefs.getBoolPref("social.active");
>          if (active) {
> +          Services.prefs.clearUserPref("social.active");

This pref should no longer be cleared (some other bug I can't be bothered to look up removed this line recently)

@@ +147,5 @@
> +    // If social is currently disabled there's nothing else to do.
> +    if (!SocialService.enabled)
> +      return;
> +    // Otherwise set the provider.
> +    let currentProvider = this._getProviderFromOrigin(curProviderOrigin);

It seems possible (although unlikely) we could end up with no provider set if this._currentProviderPref happens to reference a removed provider.  It seems like this could be made more robust by dropping curProviderOrigin completely - eg:

  // do the migration.
  if (!this._currentProviderPref) {
    ...
    this._currentProviderPref = providers[0].origin;
  }
  ...
  // Otherwise set the provider
  this._setProvider(this.defaultProvider, notifyProviderChange);

Just a thought - I'm not too bothered by this.

Comment 49

[Mark Hammond [:markh] [:mhammond]]

I wrote:
> It seems possible (although unlikely) we could end up with no provider set
> if this._currentProviderPref happens to reference a removed provider.  It
> seems like this could be made more robust by dropping curProviderOrigin
> completely - eg:
> 
>   // do the migration.
>   if (!this._currentProviderPref) {
>     ...
>     this._currentProviderPref = providers[0].origin;

Actually, the line above would also be unnecessary - we already know _currentProviderPref is empty, so this.defaultProvider will already select providers[0] in that case...

Comment 50

[Mark Hammond [:markh] [:mhammond]]

doh!  Ignore everything about '_curProviderPref' - my suggestion doesn't handle Social.active being false.

Comment 51

[Shane Caraveo (:mixedpuppy)]

Attachment: part 1 rebased + fixes

After digging around code related to Marks previous comments, I realized that migration from pre-multiprovider to multiprovider+this refactoring had broken.  I tested that out, and came up with an additional change.  You'll see "migrateSettings" in SocialService.jsm, along with the removal of the old migration code in Social.jsm.  It is now necessary for this migration to live in SocialService since we need to use the manifest list to activate the provider.

Comment 52

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 710952 [details] [diff] [review]
part 1 rebased + fixes

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

IIUC, the migration issue was that during the first run after multi-provider lands, there are no providers in ActiveProviders, and hence SocialService.providers (and thus Social.providers) ends up empty.  This was not a problem pre-patch as (a) Social.providers included non-active providers and (b) the migration code explicitly set the new provider's .active attribute to true.  So I agree that new change is necessary.  All my other comments were addressed too - thanks!

Comment 53

[Mark Hammond [:markh] [:mhammond]]

Attachment: As landed.

Gavin gave this the OK in IRC.  Also forgot about the xpcshell tests yet-again, so some more tweaks there, which rs=me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b57874cb3be5

Comment 54

[Ed Morley [:emorley]]

Backed out for OS X test timeouts, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19529159&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=19529768&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/99751bd8c759

Comment 55

[Shane Caraveo (:mixedpuppy)]

Attachment: test fix

This attempts to fix the test failure, which is not reproducible on my local builds.

https://tbpl.mozilla.org/?tree=Try&rev=e12680fc495c

Comment 56

[Shane Caraveo (:mixedpuppy)]

a try with debug builds, since the test failures were in debug

https://tbpl.mozilla.org/?tree=Try&rev=2012e58992c3

Comment 57

[Shane Caraveo (:mixedpuppy)]

ok, trying this again.  The small change in the test worked in my push to try, so we'll see if it can stick in inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/599add40520f

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/599add40520f

Comment 59

[u279076]

Marking this verified fixed. I've not noticed any regressions through the last couple of months of multi-provider testing.