894806 - Remove Social.provider

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

The social support is moving away from a single global provider - the exact changes will depend on UX (bug 889427).  We should move away from Social.provider to help weed-out non-obvious assumptions about a single, global provider (eg, the SocialErrorListener makes this assumption and has been missed in previous patches which are trying to relax the "1 provider" requirement.)

Shane and I felt that a reasonable first step would be to move towards "SocialUI.provider" - so we still have a "global provider" at the window level - and initially each window will also enforce the same provider being current.  Then, further multi-provider patches can end up being limited to browser-social.js and whereever the UX takes us - eg, we might end up with only the sidebar having the concept of a "current" provider, but allowing every window to have a different one.  Or not.  Either way, it seems clear the concept of Social.provider is going away.

Social.providers and Social.enabled would still exist - just Social.provider goes away.

Comment 1

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

Attachment: Removes use of Social.provider in tests

Largely mechanical changes to the tests, to it's easier in later patches to spot "real" changes.

Comment 2

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

Attachment: WIP with most tests passing

Although mosts tests pass, there are a number of hacks that need some thought - but proves the general concept.  Not really ready for formal feedback, but informal feedback welcome :)

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #2)
> Created attachment 776972 [details] [diff] [review]
> WIP with most tests passing
> 
> Although mosts tests pass, there are a number of hacks that need some
> thought - but proves the general concept.  Not really ready for formal
> feedback, but informal feedback welcome :)

I took a quick look through both patches, it all looks fine, will have to look into a couple of the comments added in some places to refresh my memory (e.g. the reload test)

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: Removes use of Social.provider in tests

updated patch on top of bug 891216, passes tests

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP with most tests passing

updated patch on top of bug 891216, more tests pass, some still failing.

I spent a bunch of time looking at enabled state.  There is some difficulty here that I feel will be removed if we do this refactoring on top of bug 891225.  At that point, we wont need SocialUI.provider any longer as only the sidebar/old toolbarbutton classes will need to have the concept of a selected provider and enabled state will be equivalent of social.providers.length > 0.

per IRC chat with mark, we can hold off on this refactoring until then.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: Removes use of Social.provider in tests

updated on top of bug 891219

Comment 7

[Shane Caraveo (:mixedpuppy)]

bug 935640 removes SocialToolbar, which is a big part of the use of Social.provider.  SocialSidebar grows it's own SocialSidebar.provider, which defers to Social.provider, however that isolates use to the one class that actually needs it.  So, this bug will now depend on bug 935640, which should make this patch much simpler.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: remove Social.provider

This patch removes Social.provider which has a couple of repercussions that are also dealt with in this patch.

- enabled state is now solely based on providers enabled in the addons manager.  This is because there is no single "selected provider" in the system now.  Like addons in general, there is no mechanism for disabling all providers without disabling them in the addon manager.  Since our ability to temporarily disable/enable was dependent on this separation, it is removed.

- the sidebar class is more self contained, tracking its own provider on a per-window basis.  This matches the UX of the other sidebars, allowing for different providers to be loaded in each window.  The state is maintained, so closing with multiple windows/different providers and restarting properly maintains that state.  There no longer is any global current provider pref.  New windows use the state from the window that opened it (same as the other sidebars)

- sidebar is only shown automatically during the activation of a new provider.  most tests do not use the install, so they must manually show the sidebar before running tests if they rely on the sidebar during tests (many do).

Comment 9

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

Comment on attachment 8347538 [details] [diff] [review]
remove Social.provider

Steven, would you be able to give the session store code in this path a quick once-over.  In particular, in browser-social.js there is new code restoreWindowState: function() {} and saveWindowState: function() {}, and I'm not familiar with recent async and cross-process-friendly work being done on session store to know if this is the best way to approach this.

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8347538 [details] [diff] [review]
remove Social.provider

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

::: browser/base/content/browser-social.js
@@ +71,5 @@
> +    if (PrivateBrowsingUtils.isWindowPrivate(window))
> +      return;
> +
> +    // handle SessionStore for the sidebar state
> +    SocialSidebar.restoreWindowState();

I have a feeling this might cause an exception on the first window.

This code is executed before "browser-delayed-startup-finished". Session Store waits on this notification before initializing the first window [1]. So, when |SocialSidebar.restoreWindowState()| calls |SessionStore.getWindowValue(window, "socialSidebar");| the window won't be initialized in Session Store. I'd expect the exception at [2] to be thrown.

Is this not what you're seeing?


[1] http://hg.mozilla.org/mozilla-central/file/77e658ad2c26/browser/components/sessionstore/src/SessionStore.jsm#l907
[2] http://hg.mozilla.org/mozilla-central/file/77e658ad2c26/browser/components/sessionstore/src/SessionStore.jsm#l1711

Comment 11

[Shane Caraveo (:mixedpuppy)]

> I have a feeling this might cause an exception on the first window.
> 
> This code is executed before "browser-delayed-startup-finished". 

The patch specifically moves the call to SocialUI.init to the code wrapped inside SessionStore.promiseInitialized, which is also where tabs are restored iiuc.

Comment 12

[Steven MacLeod [:smacleod]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> The patch specifically moves the call to SocialUI.init to the code wrapped
> inside SessionStore.promiseInitialized, which is also where tabs are
> restored iiuc.

Ah, I missed that (didn't look over the entire patch). The Session Store related stuff LGTM then.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: remove Social.provider

updated patch with a couple minor fixes

Comment 14

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

Comment on attachment 8355308 [details] [diff] [review]
remove Social.provider

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

Looks fine to me and a worthwhile cleanup (although removing Social.enabled could have made it even better ;), notwithstanding the following comments.  Also, I think we agreed we should get formal UX sign-off on removing the top-level "enable/disable social" UI, but I can't see any reference to that in the bug comments.

::: browser/base/content/browser-social.js
@@ +80,3 @@
>      }
> +    // allow above notifications to finish up before restoring any
> +    setTimeout(this.initWindowState, 50);

the comment appears truncated and isn't clear (ie, what "above notifications" and "restoring any" what?)  Also, what makes us sure that 50ms is enough time?  (ie, what would the ramification be if it wasn't?)  I assume that's for the sessionstore to complete - but shouldn't we be using an observer for that?

@@ +135,3 @@
>            SocialSidebar.unloadSidebar();
> +
> +          // currently only the sidebar and flyout have a selected provider.

this comment and the one above should be merged, and no comment between the 2 unload calls.

@@ +692,5 @@
>  
>  SocialSidebar = {
>    // Whether the sidebar can be shown for this window.
>    get canShow() {
> +    if (!SocialUI.enabled && !document.mozFullScreen)

this condition doesn't look right - do you mean |if (!SocialUI.enabled || document.mozFullScreen)|?

@@ +697,2 @@
>        return false;
> +    return [p for (p of Social.providers) if (p.sidebarURL)].length > 0;

Maybe |return Social.providers.some(p => p.sidebarURL);|?

@@ +862,5 @@
> +        provider = Social._getProviderFromOrigin(origin);
> +      if (!provider && providers.length > 0)
> +        provider = providers[0];
> +      if (provider)
> +        this.provider = provider;

should this be setting this._provider?  It seems strange that the provider getter would have the side-effects of causing all the updating that the setter does.

@@ +881,3 @@
>    },
>  
>    setProvider: function(origin) {

seeing we are refactoring, can we remove this function?  It seems wrong and confusing to have a .provider setter and a setProvider() method that takes an origin.

@@ +883,5 @@
>    setProvider: function(origin) {
> +    this.provider = Social._getProviderFromOrigin(origin);
> +  },
> +
> +  providerDisabled: function(origin) {

this should probably be renamed to something like disableProvider() - as it stands it sounds like it returns a bool.  OTOH though, it seems strange it is here at all - I guess I need to look at the raw patch and find how it is actually used :)

::: browser/base/content/test/social/browser_share.js
@@ +108,5 @@
>      ok(port, "provider has a port");
>      let testTab;
>      let testIndex = 0;
>      let testData = corpus[testIndex++];
>      

please kill this trailing whitespace while we are touching this

::: browser/base/content/test/social/browser_social_activation.js
@@ +156,5 @@
>        ok(panel.hidden, "activation panel is not showing");
>        executeSoon(aCallback);
>      } else {
>        waitForProviderLoad(function() {
> +        is(SocialSidebar.provider.origin, manifest.origin, "new provider is active");

can we add a check here that the sidebar was previously not showing, but now is showing due to the activation event?

@@ +294,5 @@
>          // uninstall the provider
>          clickAddonRemoveButton(blanktab, function(addon) {
>            checkSocialUI();
>            activateOneProvider(gProviders[0], true, function() {
> +            info("second activation completed");

ditto here - check that the sidebar is now open for the second provider

::: browser/base/content/test/social/browser_social_flyout.js
@@ +101,5 @@
>      const ALLOW_SCRIPTS_TO_CLOSE_PREF = "dom.allow_scripts_to_close_windows";
>      // note clearUserPref doesn't do what we expect, as the test harness itself
>      // changes the pref value - so clearUserPref resets it to false rather than
>      // the true setup by the test harness.
>      let oldAllowScriptsToClose = Services.prefs.getBoolPref(ALLOW_SCRIPTS_TO_CLOSE_PREF);    

more trailing whitespace we can cleanup

::: browser/base/content/test/social/browser_social_sidebar.js
@@ +28,1 @@
>  

add another check here that SocialSidebar.opened is true after the show?

@@ +72,5 @@
> +      // disable social.
> +      SocialService.removeProvider(SocialSidebar.provider.origin, function() {
> +        checkShown(false);
> +        is(Social.providers.length, 0, "no providers left");
> +        defaultFinishChecks();  

trailing whitespace

::: browser/base/content/test/social/browser_social_window.js
@@ +6,1 @@
>  

can we add a test to this (or maybe the sidebar) test that a new window has the same sidebar state as an existing window?

@@ +37,5 @@
>      return;
>    }
>    waitForCondition(function() w.closed,
>                     function() {
> +                    info("window closed, "+createdWindows.length+" windows left");

please add spaces around operators

::: browser/base/content/test/social/head.js
@@ +117,5 @@
>        }
>        removeProvider(m.origin, callback);
>      });
>    }
>    function finishSocialTest(cleanup) {

hrm - this change concerns me as I think this was added to prevent some obscure orange.  Not sure what to suggest though...

::: browser/modules/test/unit/social/head.js
@@ +89,5 @@
> +
> +  let internalManager = Cc["@mozilla.org/addons/integration;1"].
> +                     getService(Ci.nsIObserver).
> +                     QueryInterface(Ci.nsITimerCallback);
> +  

trailing whitespace

@@ +136,5 @@
> +      active[m.origin] = 1;
> +    activeVal.data = JSON.stringify(active);
> +    Services.prefs.setComplexValue("social.activeProviders",
> +                                   Ci.nsISupportsString, activeVal);
> +  

trailing whitespace here and a few lines below

::: toolkit/components/social/test/xpcshell/test_SocialServiceMigration21.js
@@ -22,5 @@
>    DEFAULT_PREFS.setCharPref(manifest.origin, JSON.stringify(manifest));
>    Services.prefs.setBoolPref("social.active", true);
>  
> -  // Enable the service for this test
> -  Services.prefs.setBoolPref("social.enabled", true);

hrm - migration is an interesting question - what if we migrate and find social.enabled = false but providers installed?

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: remove Social.provider

updated patch and try

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

Comment 17

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

Comment on attachment 8361087 [details] [diff] [review]
remove Social.provider

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

Another concern: it seems that users with the sidebar previously open will now find it closed - is that the case?

But phew - that's a big patch :)  LGTM in general, but I think I'd like another peek once the comments are addressed (and unless I mis-understand something, so ensure the right thing happens with the removal of the sidebar.open pref)

::: browser/base/content/browser-social.js
@@ +75,5 @@
> +      this.initWindowState();
> +    });
> +  },
> +
> +  initWindowState: function() {

See comment in Social.jsm - I'm not sure this is the best way to do this.

@@ +78,5 @@
> +
> +  initWindowState: function() {
> +    // anything after this should never happen in private browsing mode, e.g.
> +    // saving/restoring the state of the sidebar
> +    if (PrivateBrowsingUtils.isWindowPrivate(window))

should maybe use this.enabled here?

@@ +693,5 @@
>  
>    // Whether the user has toggled the sidebar on (for windows where it can appear)
>    get opened() {
> +    let broadcaster = document.getElementById("socialSidebarBroadcaster");
> +    return !broadcaster.hidden && !document.mozFullScreen;

is this mozFullScreen check really needed?  It seems that as canShow checks this it shouldn't be necessary (and if it was, it implies something else didn't check canShow when it should have)

@@ +697,5 @@
> +    return !broadcaster.hidden && !document.mozFullScreen;
> +  },
> +
> +  restoreWindowState: function() {
> +    let data = SessionStore.getWindowValue(window, "socialSidebar");

should there be a canShow() here?  Seems unlikely but possible the new window will be full-screen?

@@ +708,5 @@
> +      document.getElementById("social-sidebar-browser").setAttribute("origin", data.origin);
> +      if (!data.hidden)
> +        this.show(data.origin);
> +      else
> +        this.hide();

this .hide here seems unnecessary - is this ever going to be called when it is already shown?  If so, why isn't .hide() also called when data is null?

@@ +772,5 @@
>            this.unloadSidebar,
>            Services.prefs.getIntPref("social.sidebar.unload_timeout_ms")
>          );
>        }
> +    } else if (this.provider) {

it doesn't seem possible this.provider can be null here - hideSidebar can only be false if this.canShow returns true which implies there must be a provider.

@@ +842,4 @@
>      }
>    },
>  
>    // provider will move to a sidebar specific member in bug 894806

this comment should be removed now, right?

@@ +843,5 @@
>    },
>  
>    // provider will move to a sidebar specific member in bug 894806
> +  _provider: null,
> +  setDefaultProvider: function() {

This should probably be named something like .ensureProvider() - the name made me think it was always going to set the provider to the "default" provider, but if there is already a provider we early return.

@@ +868,5 @@
> +  set provider(provider) {
> +    if (!provider || provider.sidebarURL) {
> +      this._provider = provider;
> +      if (!provider) {
> +        document.getElementById("social-sidebar-browser").removeAttribute("origin");

this seems strange - why aren't we also adding the origin attribute if there is a provider?

@@ +1302,5 @@
>      let src = aNotificationFrame.getAttribute("src");
>      aNotificationFrame.removeAttribute("src");
>      aNotificationFrame.webNavigation.loadURI("about:socialerror?mode=tryAgainOnly&url=" +
> +                                             encodeURIComponent(src) + "&origin=" +
> +                                             encodeURIComponent(aNotificationFrame.getAttribute("origin")),

this line is probably a little too long

::: browser/base/content/test/social/browser_social_activation.js
@@ +156,5 @@
>        ok(panel.hidden, "activation panel is not showing");
>        executeSoon(aCallback);
>      } else {
>        waitForProviderLoad(function() {
> +        is(SocialSidebar.provider.origin, manifest.origin, "new provider is active");

should we explicitly check here that the sidebar is open?  (I'm not sure if it is possible SocialSidebar.provider will be set if it is closed)

::: browser/base/content/test/social/browser_social_window.js
@@ +6,2 @@
>  
>  let SocialService = Cu.import("resource://gre/modules/SocialService.jsm", {}).SocialService;

it seems that either this test (or browser_social_sidebar) could do with a test that different windows can have different sidebars)

::: browser/modules/Social.jsm
@@ +84,3 @@
>  
>      if (this.initialized) {
> +      // The call to _updateProviderCache below would normally send this

why has this moved here?  ISTM that this will cause creation of a new window to update all existing windows, which isn't what we want.  Can we resolve the promise with a boolean to indicate if the caller of this needs to take this special action?

::: browser/modules/test/unit/social/test_socialDisabledStartup.js
@@ +12,5 @@
>  
>  function testStartupDisabled() {
>    // wait on startup before continuing
> +  do_check_false(Social.enabled, "Social is disabled");
> +  do_check_eq(Social.providers.length, 0, "two social providers available");

"zero social providers available"?

::: modules/libpref/src/init/all.js
@@ -55,5 @@
>  pref("browser.cache.disk.max_entry_size",    51200);  // 50 MB
>  pref("browser.cache.memory.enable",         true);
>  // -1 = determine dynamically, 0 = none, n = memory capacity in kilobytes
>  //pref("browser.cache.memory.capacity",     -1);
> -// Max-size (in KB) for entries in memory cache. Set to -1 for no limit.  

I don't think you should touch all the unrelated trailing whitespace in this bug

::: toolkit/components/social/SocialService.jsm
@@ +33,5 @@
>   */
>  
>  // Internal helper methods and state
>  let SocialServiceInternal = {
> +  get enabled() this.providerArray.length > 0,

A setter that throws might be a good idea, just incase...?

@@ +220,5 @@
>    }
> +  if (Services.prefs.prefHasUserValue("social.enabled")) {
> +    enabled = Services.prefs.getBoolPref("social.enabled");
> +  }
> +  dump("***** enabled is "+enabled+"\n");

stray debug dump

@@ +270,5 @@
>        }
> +      // as of fx 29, we no longer rely on social.enabled. migration from prior
> +      // versions should disable all service addons if social.enabled=false
> +      if (enabled === false) {
> +        dump("**** remove active provider "+origin+"\n");

another dump

@@ +424,5 @@
>      ActiveProviders.delete(provider.origin);
>  
>      delete SocialServiceInternal.providers[origin];
> +    // disable the api if we have no enabled providers
> +    if (SocialServiceInternal.providerArray.length == 0)

if (!SocialServiceInternal.enabled) - but OTOH, why not unconditionally set MozSocialAPI.enabled = SocialServiceInternal.enabled?

::: toolkit/components/social/test/browser/browser_workerAPI.js
@@ -21,2 @@
>    SocialService.addProvider(manifest, function (p) {
>      p.enabled = true;

don't we now enable new providers by default?  ie, maybe this should change to a check it is true rather than a set?

::: toolkit/components/social/test/browser/head.js
@@ -3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  let SocialService = Components.utils.import("resource://gre/modules/SocialService.jsm", {}).SocialService;
>  
> -function ensureSocialEnabled() {

should this be replaced with a check that there are no providers left installed after each test (TBH, there might already be an existing one - I didn't open the file to check :)

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: remove Social.provider

feedback implemented

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

Comment 19

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

Comment on attachment 8383827 [details] [diff] [review]
remove Social.provider

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

Looks good, thanks.  r=me with Social:Toggle removed, plus 2 tests which I mentioned last time:

* Check multiple windows can have different sidebar open states and with different providers - given this is a key feature of this patch it should have tests.
* (I guess I only implied this one :) - Test the code you added which does the migration of the sidebar.open pref.

::: browser/base/content/browser.xul
@@ -1072,5 @@
>                          autocheck="false"
>                          command="Social:ToggleNotifications"
>                          label="&social.toggleNotifications.label;"
>                          accesskey="&social.toggleNotifications.accesskey;"/>
> -              <menuitem class="social-toggle-menuitem" command="Social:Toggle"/>

It looks like the Social:Toggle command can be removed too?

::: browser/base/content/test/social/browser_social_window.js
@@ +110,5 @@
> +              let sbrowser1 = w1.document.getElementById("social-sidebar-browser");
> +              is(manifest.origin, sbrowser1.getAttribute("origin"), "w1 sidebar origin matches");
> +              let sbrowser2 = w2.document.getElementById("social-sidebar-browser");
> +              is(manifest2.origin, sbrowser2.getAttribute("origin"), "w2 sidebar origin matches");
> +              

nit: trailing whitespace

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond [:markh] from comment #19)
> Comment on attachment 8383827 [details] [diff] [review]
> remove Social.provider

> It looks like the Social:Toggle command can be removed too?

already was.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: remove Social.provider

additional tests per review comments

Comment 22

[Shane Caraveo (:mixedpuppy)]

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

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: leak-fixes.patch

I couldn't figure out what exactly is causing bug 980517 but I could come up with a patch that fixes sessionstore leaks locally for me. Maybe it fixes all of the leaks?

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8387887 [details] [diff] [review]
leak-fixes.patch


I did a quick try and it seems to have fixed the issues I was seeing on the try server, running a more complete bc test run:

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

Assuming that passes, I'll merge this into the larger patch rather than landing it separately.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> Assuming that passes, I'll merge this into the larger patch rather than
> landing it separately.

Great!

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: remove Social.provider and refactor

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

combined patch incorporating fixes from ttaubert, carrying forward r+

Comment 27

[Shane Caraveo (:mixedpuppy)]

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

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c75d5601b154

Comment 30

[Luís Miguel [:quicksaver] (not reading bugmail)]

Question, will this be uplifted to beta? I ask because of http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/SocialService.jsm#276:
>        // as of fx 29, we no longer rely on social.enabled. migration from prior
>        // versions should disable all service addons if social.enabled=false
but this patch was only merged in fx30 (aurora).

This is actually very important to me, because my add-on was relying on a lot that was removed with this patch, especially the removed preferences, so I need to know in which fx version will this be true (and it would actually be better if this came in fx29 with Australis already, for me at least although I know that's being a little egocentric, lots less work for downwards compatibility :) )

Comment 31

[Shane Caraveo (:mixedpuppy)]

The comment is incorrect, it didn't make it to fx29.  It wont be uplifted as it is too large a change to push to beta.