this patch updates toolkit/components/social with notifications needed for multiprovider support. This part of socialapi is otherwise multiprovider aware. Once change from the earlier patch is using an added and a removed notification. Another patch will need to adjust the selected provider based on those notifications (e.g. removing the current provider needs to select the next one)

Comment on attachment 679464 [details] [diff] [review] toolkit patch Review of attachment 679464 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/social/SocialService.jsm @@ +134,3 @@ > let provider = SocialServiceInternal.providers[origin]; > + delete SocialServiceInternal.providers[origin]; > + Services.obs.notifyObservers(null, "social:provider-removed", provider.origin); The comment here doesn't seem to match the code - it still "removes" the provider before the observer is fired. It probably makes sense to do what the comment says incase some observer needs to fetch the provider.

Comment on attachment 679871 [details] [diff] [review] browser/modules patch Review of attachment 679871 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1189,5 @@ > // might keep around more than this, but we'll try to get down to this value). > // (This is intentionally on the high side; see bug 746055.) > pref("image.mem.max_decoded_image_kb", 256000); > > // Example social provider This comment should go @@ +1195,3 @@ > // Comma-separated list of nsIURI::prePaths that are allowed to activate > // built-in social functionality. > +pref("social.provider.current", "https://www.facebook.com"); Not sure why this is needed - the user will select a provider to enable the feature (ie, by visiting facebook or some other provider's page), so having a default for the "current" provider should be necessary and may give the perception of preferential treatment for facebook. ::: browser/modules/Social.jsm @@ +25,5 @@ > + > + get provider() { > + return this._provider; > + }, > + trailing whitespace (stooopid komodo :) @@ +29,5 @@ > + > + set provider(aProvider) { > + let oldProvider = this.provider; > + this._provider = aProvider; > + let origin = aProvider ? aProvider.origin : ""; what does it mean to have a null provider in this world? It seems worthwhile that a precondition of social being enabled means a non-null provider. If it turns out this code is called when social is disabled, we probably don't want to override the social.provider.current pref with an empty string, nor firing an observer that the provider has changed to an empty origin. @@ +36,5 @@ > + }, > + > + setProvider: function Social_setProvider(aOrigin, onDone) { > + SocialService.getProvider(aOrigin, function(provider) { > + this.provider = provider; Along the lines of the comment above, this code should probably just Cu.reportError() and return early if the origin can't be found as a provider (and thereby leave the current provider in place) @@ +50,2 @@ > init: function Social_init(callback) { > + // XXX does this need to be switchable after startup? IMO it is fine to never allow social to be enabled in safe mode. @@ +52,2 @@ > this._disabledForSafeMode = Services.appinfo.inSafeMode && this.enabled; > + if (_initialized) { There seems a risk here that the first call might have failed to set a provider, and subsequent calls will now also fail to set a provider due to the early return. I'd be inclined to be even more radical and have Social_init() guarantee it returns with either a provider set *or* Social.enabled set to false.

Comment on attachment 679872 [details] [diff] [review] errorStatus patch Review of attachment 679872 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +77,5 @@ > SocialShareButton.updateProfileInfo(); > SocialChatBar.update(); > break; > case "social:frameworker-error": > + let uri = Services.io.newURI(data, null, null); can we just have the error event send the origin directly? It is more consistent and avoids the prePath hoops. @@ +1024,5 @@ > sbrowser.setAttribute("src", Social.provider.sidebarURL); > sbrowser.addEventListener("load", SocialSidebar._loadListener, true); > + } else > + if (Social.provider.errorState == "frameworker-error") { > + SocialSidebar.setSidebarErrorMessage("frameworker-error"); shouldn't this error state be checked before we check the sidebar src etc?

Comment on attachment 679875 [details] [diff] [review] chrome patch Review of attachment 679875 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +83,5 @@ > try { > this.updateToggleCommand(); > SocialShareButton.updateButtonHiddenState(); > SocialToolbar.updateButtonHiddenState(); > SocialSidebar.updateSidebar(); The code here seems almost identical to social:provider-set, and I suspect both will now be called. I wonder if we can get away with having the pref-changed observer only doing anything when social is disabled and rely on the provider-set notification for enabling? IOW, maybe change the pref-changed to something like: if (!Social.enabled) SocialUI.providerSet(); (and renaming providerSet() accordingly) @@ +133,4 @@ > get toggleCommand() { > return document.getElementById("Social:Toggle"); > }, > + trailing whitespace here and a few other places in this patch ::: browser/base/content/test/browser_social_multiprovider.js @@ +36,5 @@ > + info("has menu? "+el.length+" origin? "+(el.length>0?el[0].getAttribute("origin"):"NONE")); > + return el.length == 1 && el[0].getAttribute("checked") == "true"; > + }, function() { > + // The provider menu is correctly checked - now wait for the profile > + // into to trickle in and check it is reflected in the ui. into->info @@ +99,5 @@ > + }); > + }); > + }); > + } > +} looks good, but it would be great to test some error conditions - eg, attempting to activate a provider that doesn't exist. Probably also need tests (here or elsewhere) for the provider-specific error conditions - which, IIUC, will help exercise the edge case of the "current" provider having enabled=false. ::: browser/base/content/test/social_worker.js @@ +78,5 @@ > break; > case "social.initialize": > // This is the workerAPI port, respond and set up a notification icon. > apiPort = port; > + let profile; might be worth a comment here indicating that this same provider code is used as 2 distinct providers from different origins.

Looking good, although I'm not sure of the benefit of so many interdependent patches...

(In reply to Mark Hammond (:markh) from comment #9) > Looking good, although I'm not sure of the benefit of so many interdependent > patches... The changes in toolkit, browser/module and errorStatus should all work independently, though my patch tree is toolkit->browser/module->err->chrome so the applying the patches might have dependencies. The chrome patch requires all the previous patches.

Comment on attachment 679464 [details] [diff] [review] toolkit patch Review of attachment 679464 [details] [diff] [review]: ----------------------------------------------------------------- Same feedback as Mark.

Comment on attachment 679871 [details] [diff] [review] browser/modules patch Review of attachment 679871 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/Social.jsm @@ +60,5 @@ > + > + let useFirstProvider = function() { > + SocialService.getProviderList(function (providers) { > + if (providers.length) > + this.provider = providers[0]; What should happen if providers.length=0? Should this.provider be set to null? I'm not sure when that would happen though (providers.length = 0). @@ +68,5 @@ > + }.bind(this); > + > + let current; > + try { > + current = Services.prefs.getCharPref("social.provider.current"); When does getCharPref fail here? If the pref is defined in firefox.js, shouldn't it always be a string of length 0 or more? @@ +80,5 @@ > + this.provider = provider; > + if (callback) > + callback(); > + } else { > + useFirstProvider(); This code looks very similar to useFirstProvider. Can the duplication be removed?

I think that this https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#138 is wrong: 138 if (Social.enabled || !Social.provider) 139 return; The handler for ActivateSocialFeature should traverse the events parent document for a html link element, read the manifest and add it to the preferences "social.manifest.*". <link rel="manifest" type="text/json" href="http://axel.nennker.de/socialapi-demo-gh-pages/manifest.json"/> The name of the pref is arbitrary if there is no other pref with the same origin value. I suggest to use "social.manifest." + base64url(manifest.origin) as a name for the pref. So coming back to #138 I think that this should be: 138 if (Social.enabled) 139 return; We should not assume that there is a provider predefined. This handler for ActivateSocialFeature defines one.

(In reply to Axel Nennker from comment #13) > The handler for ActivateSocialFeature should traverse the events parent > document for a html link element, read the manifest and add it to the > preferences "social.manifest.*". We are yet to decide how and when to handle external manifests - this patch is just a WIP which reflects that uncertainty. > So coming back to #138 I think that this should be: > 138 if (Social.enabled) > 139 return; > > We should not assume that there is a provider predefined. This handler for > ActivateSocialFeature defines one. Yes, that sounds correct.

Comment on attachment 679875 [details] [diff] [review] chrome patch Review of attachment 679875 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +982,5 @@ > + let providerMenu = document.getElementById("social-provider-menu"); > + let popup = providerMenu.parentNode; > + // the remove menuseperator is after the provider selection area > + // of the menu > + while (providerMenu.nextSibling.nodeName != "menuseparator") { This should use a class to designate which menuseparator it is. @@ +997,5 @@ > + menuitem.setAttribute("class", "menuitem-iconic"); > + > + let itemText = provider.name; > + menuitem.setAttribute("image", provider.iconURL); > + menuitem.setAttribute("label", itemText); s/itemText/provider.name/ ::: browser/base/content/browser.xul @@ +690,5 @@ > autocheck="false" > command="Social:ToggleNotifications" > label="&social.toggleNotifications.label;" > accesskey="&social.toggleNotifications.accesskey;"/> > + <menuseparator id="social-provider-menu" collapsed="true"/> This should also be added to the browser-menubar.inc implementation, and change the |id| to a |class| due to the duplicate instances.

The easiest way to support "external" manifests is to add the manifest to the CustomEvent. I suggest to change the code on the social providers page to something like this: var manifest = { "origin":"http://axel.nennker.de", "name":"Demo Messenger", "workerURL":"http://axel.nennker.de/socialapi-demo-gh-pages/worker.js", "iconURL":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAU1JREFUeNqckc1qg0AUhY9TsQEXwUWCbrOQbF3Xh+m+6z5EX6ob3ZQKLitBhOpGF020UEziX50JI0ZjaHrgOtfjfHfuHYWmaRAEQUNXLp4PvaG/Xq8FkcKapo3gvxTyPK8R+cvxeBwBfL23XpA/PJ/5s9kMdV2DTJ3Wh1EVZ36/C3INZmsLsxjANKqqArkEZVmGzWaDNE1bo2qjRhRFcBwHRVGwccuyPBVATxT2fR9JkrB8Pp+j/vlCmXyweXe7HYMsy+oYsd8ShelGmq9WKxwOB9x9vgN1icViwcK27f6ZEHmS53l3u3Tjfr8/fWhhriHcFaBgGIZsZkmSoCgKLsk0zZHH7oC2raoqttst4jjGLSL8VgkhWC6Xk61OFqAwl67rNxchQ6NfhI/z/fg6WaD7C7Isd6ZhGF1ePb1BvtKBQB+u6zb4h9qDhF8BBgDVYxk7AyQpfgAAAABJRU5ErkJggg==", "sidebarURL":"http://axel.nennker.de/socialapi-demo-gh-pages/sidebar.htm"}; var customEventInit = {"detail":manifest}; var event = new CustomEvent("ActivateSocialFeature", customEventInit); document.dispatchEvent(event); - no need to retrieve an external manifest over the network - it is easier, I think, if the social provider has to change only one file The event handler here https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-social.js#135 can then extract the manifest from the event, create a new provider and call SocialService.addProvider(newProvider, SocialUI.notificationPanel.openPopup(SocialToolbar.button, "bottomcenter topright"));

Social.jsm might need a new method to add a provider because SocialServer is not defined in SocialUI. file browser-social.js: if (e.detail) { let callback = function(provider) { SocialUI.notificationPanel.openPopup(SocialToolbar.button, "bottomcenter topright"); Social.provider = provider; this._providerReady(); }; Social.addProvider(e.detail, callback.bind(this)); } else { setTimeout(function () { SocialUI.notificationPanel.openPopup(SocialToolbar.button, "bottomcenter topright"); }.bind(this), 0); } file Social.jsm: Social.addProvider should store the new manifest in the preferences addProvider : function(aManifest, onDone) { ... MANIFEST_PREFS.setCharPref(btoa(aManifest), aManifest); // if btoa is visible here SocialService.addProvider(aManifest, onDone); }, or something along these line, I think.

(In reply to Mark Hammond (:markh) from comment #7) > Comment on attachment 679872 [details] [diff] [review] > errorStatus patch > > Review of attachment 679872 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/browser-social.js > @@ +77,5 @@ > > SocialShareButton.updateProfileInfo(); > > SocialChatBar.update(); > > break; > > case "social:frameworker-error": > > + let uri = Services.io.newURI(data, null, null); > > can we just have the error event send the origin directly? It is more > consistent and avoids the prePath hoops. That notification comes from the frameworker (more than one location) which does not have the provider origin. We'd have to do that logic in multiple places to remove it from this one place. > @@ +1024,5 @@ > > sbrowser.setAttribute("src", Social.provider.sidebarURL); > > sbrowser.addEventListener("load", SocialSidebar._loadListener, true); > > + } else > > + if (Social.provider.errorState == "frameworker-error") { > > + SocialSidebar.setSidebarErrorMessage("frameworker-error"); > > shouldn't this error state be checked before we check the sidebar src etc? No. The if statement above that checks to see if the provider has changed. In that case, the errorState should not block an attempt to load.

- we removed port.close in the worker.terminate call and do not unload the iframe before removal - we need to verify that in a real worker that the worker script gets signaled with port.close (and that we're compatible)

sigh, wrong bug for comment #19

fixes comment

addresses earlier feedback. also changes how a provider is enabled, essentially only enabling a provider when it is set as the current provider. The makes the socialapi run only one social worker at any given time.

errorState modification on top of browser/modules patch.

addresses previous feedback. it also roughs out provider.active since that is needed by the menu rendering code. the activation support will be finished up in bug 809709.

note on above feedback requests...I'm working on verifying and updating tests now

Comment on attachment 685953 [details] [diff] [review] browser/modules patch Review of attachment 685953 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/Social.jsm @@ +62,2 @@ > init: function Social_init(callback) { > + if (_initialized) { I'm still not sure how this will handle a first call failing to set a provider? OTOH though, apart from tests I can't see how this could happen given providers.length can only be zero if someone nukes the builtin pref - so possibly not a big deal at this stage. Another option might be to move the "find default provider" code into the enabled setter? ie, setting enabled=false sets this.provider to null, and setting enabled to true finds and enables the default provider, and on failure leaves this.enabled as false. @@ +115,5 @@ > set enabled(val) { > if (!val) { > delete this.errorState; > } > + if (this.provider) see above comment - it seems ideal that if SocialService.enabled == false, this.provider == null. ::: toolkit/components/social/SocialService.jsm @@ +64,5 @@ > prefs.forEach(function (pref) { > try { > var manifest = JSON.parse(MANIFEST_PREFS.getCharPref(pref)); > if (manifest && typeof(manifest) == "object") { > + let provider = new SocialProvider(manifest); where does the inSafeMode check live now? I wonder if that too could end up in the enabled setter for Social? (Apologies if it is in one of the other patches, but I don't trust my memory to remember this ;)

Comment on attachment 685957 [details] [diff] [review] chrome patch Review of attachment 685957 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit confused here - there are multiple patched which change Social.jsm, and only one of them has a reference to a provider.active attribute? ::: browser/base/content/browser-social.js @@ +192,5 @@ > + Social.active = true; > + // Activate and set this provider to the current provider > + Social.provider.active = true; > + Social.provider = provider; > + nit: trailing whitespace here and a few other spots @@ +203,5 @@ > + > + SocialUI.notificationPanel.hidden = false; > + > + setTimeout(function () { > + SocialUI.notificationPanel.openPopup(SocialToolbar.button, "bottomcenter topright"); although not related to this patch, there seems a risk we could end up with multiple activation popups if the provider accidentally fires the event multiple times from the same page > 1 second apart?

getting the correct patch in here

(In reply to Mark Hammond (:markh) from comment #26) > Comment on attachment 685953 [details] [diff] [review] > ::: toolkit/components/social/SocialService.jsm > @@ +64,5 @@ > > prefs.forEach(function (pref) { > > try { > > var manifest = JSON.parse(MANIFEST_PREFS.getCharPref(pref)); > > if (manifest && typeof(manifest) == "object") { > > + let provider = new SocialProvider(manifest); > > where does the inSafeMode check live now? I wonder if that too could end up > in the enabled setter for Social? (Apologies if it is in one of the other > patches, but I don't trust my memory to remember this ;) That set provider.enabled=false if we were in safeMode. Now provider.enabled is always initially false, so I don't think that check is necessary in the creation of the SocialProvider class. Anyhow, I think the code for safemode handling is broken anyway (see _setEnabled), it needs to be reexamined. Unfortunately there were never any safemode tests.

Comment on attachment 685953 [details] [diff] [review] browser/modules patch Review of attachment 685953 [details] [diff] [review]: ----------------------------------------------------------------- Not a full review, I'll wait for another version after our multiprovider meeting and addressing Mark's feedback. ::: browser/modules/Social.jsm @@ +29,5 @@ > + > + set provider(aProvider) { > + if (!aProvider || > + (this.provider && this.provider.origin == aProvider.origin)) { > + return; if aProvider is null, should the setter set this._provider = null and set enabled = false?

Comment on attachment 685980 [details] [diff] [review] chrome patch Review of attachment 685980 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Could probably do with tests of provider.active vs provider.enabled etc, but that's a bit of a nit (and maybe they'll appear in bug 809709?) ::: browser/base/content/browser-social.js @@ +77,5 @@ > + // XXX switch to the new provider > + SocialToolbar.renderProviderMenus(); > + break; > + case "social:provider-removed": > + // XXX if removing the current provider, pick the next one, or This comment looks misplaced? @@ +85,2 @@ > case "social:pref-changed": > + // this will also "unset" if we're disabled The exception handler was removed. I'd hope it was unnecessary but wonder if that is true. If exceptions from the pref change were silent it could cause much head-scratching later. I'm not sure what to suggest here though - just flagging it for thought... @@ +667,5 @@ > > get button() { > return document.getElementById("social-provider-button"); > }, > + it wouldn't be a splinter review without a whitespace nit ;) A couple more in this file too.

Comment on attachment 685980 [details] [diff] [review] chrome patch Review of attachment 685980 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-menubar.inc @@ +536,5 @@ > accesskey="&social.toggleNotifications.accesskey;"/> > <menuseparator class="social-statusarea-separator"/> > <menuitem class="social-toggle-menuitem" command="Social:Toggle"/> > <menuitem class="social-remove-menuitem" command="Social:Remove"/> > + <menuseparator class="social-provider-menu" collapsed="true"/> The other items in browser-menubar.inc use hidden="true" for this type of behavior. Since this isn't a table row, column, column group, and row group it shouldn't make a difference, but it would be better to be consistent with the other menuitems/menuseparators, so please change this to hidden="true" ::: browser/base/content/test/browser_social_multiprovider.js @@ +26,5 @@ > + iconURL: "chrome://branding/content/icon48.png" > + }; > + let origProvider = Social.provider; > + > + // Alot of the actions needed to get the menu rendered happen on different s/Alot/A lot/ @@ +32,5 @@ > + function waitForCorrectProviderUI(next) { > + let menu = document.getElementById("social-statusarea-popup"); > + waitForCondition(function() { > + let el = menu.getElementsByAttribute("origin", Social.provider.origin); > + info("has menu? "+el.length+" origin? "+(el.length>0?el[0].getAttribute("origin"):"NONE")); nit: spaces around operators

I'm combining 3 of the patches into part 1 since they have become more entwined after getting the "only one worker" code in place. The tests in this pass when applying only this patch, but once the ui patch is applied the changes to the tests must be removed. I believe the existing tests cover the changes in this, with the exception of the errorState change, which didn't have tests to begin with. The existing tests certainly caught problems that I had to correct. after this round of feedback I want to move this to review.

This updates the chrome patch and must be applied after part 1. Some of the provider.activate code is started here, but more needs to be done via bug 809709. I've moved the safemode handling into toolkit, it simply works better there.

Comment on attachment 686915 [details] [diff] [review] multiprovider part 1 Review of attachment 686915 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +63,5 @@ > switch (topic) { > case "social:pref-changed": > // Exceptions here sometimes don't get reported properly, report them > // manually :( > + // update the browser side if the social component is toggled I'm not sure what this is doing - isn't it updating the Social object in Social.jsm? If so, I don't see why it is necessary (setting SocialService.enabled sets social.enabled preference before sending the social:pref-changed notification, and Social.enabled is implemented as just getting the social.enabled pref.) Also, the comment seems misleading - I'm not sure what it means by "update the browser side" when this already is in the browser side. @@ +1004,5 @@ > if (sbrowser.getAttribute("origin") != Social.provider.origin) { > sbrowser.setAttribute("origin", Social.provider.origin); > sbrowser.setAttribute("src", Social.provider.sidebarURL); > sbrowser.addEventListener("load", SocialSidebar._loadListener, true); > + } else "else if" on the same line is the style of the rest of this file. But I'm still not sure why moving this 'if' is necessary. When a provider is set current it has the error state cleared, so I'm wondering what is the scenario that forces this change? If setting a provider current did *not* reset the error state (a potential future change), then I'm pretty sure it would need to be as it was (ie, in that scenario, this change would be wrong) ::: browser/modules/Social.jsm @@ +50,5 @@ > + SocialService.getProvider(aOrigin, function(provider) { > + if (provider) { > + this.provider = provider; > + } else { > + Cu.reportError("provider not available for "+aOrigin); nit: operator spaces @@ +65,2 @@ > init: function Social_init(callback) { > + if (_initialized) { Along the lines of my previous comments: This seems a little fragile for a couple of reasons: it's possible the callback will be made with no provider current, and the callback itself isn't going to be used when the provider changes. It seems to me that we could drop the callback arg here, and just have the provider-changed observers do what is necessary regardless of how the provider is set. That way, much of the UI initialization would not be called if Social.init() fails to set a provider, but still *will* be called once a provider is set some other way. A slight downside is that browser-social's observer would need to differentiate between stuff that only needs to be done once the first provider is set vs stuff that needs to be done each time the provider changes, but that sounds reasonable. ::: toolkit/components/social/SocialService.jsm @@ +115,2 @@ > SocialServiceInternal.providers[provider.origin] = provider; > + Services.obs.notifyObservers(null, "social:provider-added", provider.origin); who uses this observer? AFAICT, addProvider is used only by the tests, and given it is really only a single line, I wonder if we should just remove it completely? @@ +125,5 @@ > removeProvider: function removeProvider(origin, onDone) { > if (!(origin in SocialServiceInternal.providers)) > throw new Error("SocialService.removeProvider: no provider with this origin exists!"); > > + // notify removal before shutting down the provider to allow browser to ditto here, but this seems even riskier - what happens when the "current" provider is removed?

Comment on attachment 686916 [details] [diff] [review] multiprovider part 2 (ui) Review of attachment 686916 [details] [diff] [review]: ----------------------------------------------------------------- it's a little confusing to review 2 patches for the same bug which both touch some of the same files! ::: browser/base/content/browser-menubar.inc @@ +543,5 @@ > class="show-only-for-keyboard"/> > <menuseparator class="social-statusarea-separator"/> > <menuitem class="social-toggle-menuitem" command="Social:Toggle"/> > <menuitem class="social-remove-menuitem" command="Social:Remove"/> > + <menuseparator class="social-provider-menu" collapsed="true"/> is this the elt which Jaws said should use 'hidden' instead of 'collapsed'? ::: browser/base/content/browser-social.js @@ +75,5 @@ > + case "social:provider-set": > + // this happens when a provider is ready (assuming there is one) > + this.update(); > + break; > + case "social:provider-added": can this and the -removed one ever be sent except by the tests? @@ +684,5 @@ > return document.getElementById("social-provider-button"); > }, > > + resetButton: function() { > + this.button.setAttribute("image", Social.provider.iconURL); It looks like this can be called at times other than when the provider changes - should this check the current attribute to avoid unnecessary image decoding and painting? @@ +949,5 @@ > + > + renderProviderMenu: function SocialToolbar_renderProviderMenu(providerMenu) > + { > + // Put the list of providers in a submenu: > + SocialService.getProviderList(function SocialToolbar_fillinProviderMenu(providers) { I wonder if getProviderList should take a param to indicate if only active ones are desired? @@ +958,5 @@ > + popup.removeChild(providerMenu.nextSibling); > + } > + // only show a selection if there is more than one > + if (!Social.enabled || enabledProviders.length < 2) { > + providerMenu.collapsed = true; s/collapsed/hidden/ ? ::: browser/base/content/browser.xul @@ +695,5 @@ > accesskey="&social.toggleNotifications.accesskey;"/> > <menuseparator class="social-statusarea-separator"/> > <menuitem class="social-toggle-menuitem" command="Social:Toggle"/> > <menuitem class="social-remove-menuitem" command="Social:Remove"/> > + <menuseparator class="social-provider-menu" collapsed="true"/> or maybe it was this one? ;) ::: browser/base/content/test/browser_social_multiprovider.js @@ +31,5 @@ > + // spins of the event loop, so we just wait until the right thing happens. > + function waitForCorrectProviderUI(next) { > + let menu = document.getElementById("social-statusarea-popup"); > + waitForCondition(function() { > + let el = menu.getElementsByAttribute("origin", Social.provider.origin); it looks like this is the only condition you need to wait for, and the other checks can actually be normal "is/ok" tests? @@ +41,5 @@ > + waitForCondition(function() { > + return Social.provider.profile && Social.provider.profile.displayName; > + }, function() { > + // We have a profile for the selected provider - check it was used. > + let profile = {} this 'profile' object seems unnecessary and 2 variables 'userName' and 'displayName' would be clearer IMO ::: toolkit/components/social/SocialService.jsm @@ +20,5 @@ > */ > > // Internal helper methods and state > let SocialServiceInternal = { > + enabled: Services.appinfo.inSafeMode ? false : Services.prefs.getBoolPref("social.enabled"), is this inSafeMode check enough? Could Social.enabled be called in safe mode and still "do stuff"?

Comment on attachment 686915 [details] [diff] [review] multiprovider part 1 Review of attachment 686915 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1173,5 @@ > // might keep around more than this, but we'll try to get down to this value). > // (This is intentionally on the high side; see bug 746055.) > pref("image.mem.max_decoded_image_kb", 256000); > > +pref("social.manifest.https://www.facebook.com", "{\"origin\":\"https://www.facebook.com\",\"name\":\"Facebook Messenger\",\"workerURL\":\"https://www.facebook.com/desktop/fbdesktop2/socialfox/fbworker.js.php\",\"iconURL\":\"data:image/x-icon;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8%2F9hAAAAX0lEQVQ4jWP4%2F%2F8%2FAyUYTFhHzjgDxP9JxGeQDSBVMxgTbUBCxer%2Fr999%2BQ8DJBuArJksA9A10s8AXIBoA0B%2BR%2FY%2FjD%2BEwoBoA1yT5v3PbdmCE8MAshhID%2FUMoDgzUYIBj0Cgi7ar4coAAAAASUVORK5CYII%3D\",\"sidebarURL\":\"https://www.facebook.com/desktop/fbdesktop2/?socialfox=true\"}"); Don't forget to change the pref name back to social.manifest.facebook ::: browser/base/content/browser-social.js @@ +92,5 @@ > SocialShareButton.updateShareState(); > break; > case "social:frameworker-error": > + let uri = Services.io.newURI(data, null, null); > + SocialService.getProvider(uri.prePath, function(provider) { Since this will only use a single worker, is it necessary to search for the provider based on the origin? Can we get messages from providers that aren't current? ::: browser/modules/Social.jsm @@ +33,5 @@ > + if (aProvider) { > + aProvider.enabled = SocialService.enabled; > + // we're changing providers, clear the new providers errorState to ensure > + // we attempt to load it > + delete aProvider.errorState; As I understand it, setting aProvider.errorState = "" will be faster than deleting the property which will cause the shape to change. ::: toolkit/components/social/SocialService.jsm @@ +187,5 @@ > return; > > this._enabled = enable; > > + delete this.errorState; same thing here about changing this to this.errorState = "".

Comment on attachment 686916 [details] [diff] [review] multiprovider part 2 (ui) Review of attachment 686916 [details] [diff] [review]: ----------------------------------------------------------------- For UI reviews, it is better to either link to a build or upload a screencast demonstrating the workflow/interactions. These types of requests will generally get much faster response times as compared to a patch. I still need to try out the safe mode behavior to see that this didn't regress the desired behavior. I'll do that when it's up for review. ::: browser/base/content/browser-menubar.inc @@ +543,5 @@ > class="show-only-for-keyboard"/> > <menuseparator class="social-statusarea-separator"/> > <menuitem class="social-toggle-menuitem" command="Social:Toggle"/> > <menuitem class="social-remove-menuitem" command="Social:Remove"/> > + <menuseparator class="social-provider-menu" collapsed="true"/> Yeah it is :) ::: browser/base/content/browser-social.js @@ +75,5 @@ > + case "social:provider-set": > + // this happens when a provider is ready (assuming there is one) > + this.update(); > + break; > + case "social:provider-added": Yeah I think Mark's correct here. At least for v1.5, we're not planning on supporting runtime additions or removals to the list of providers (only runtime activations/deactivations & enabling/disabling). Maybe test-only functions like this (if they must exist) should have dump("Test-only function being called. This is a bug if this is not a testing build. Function name=..."). Could use Cu.reportError here but that might require calling some "expect an exception" function. @@ +704,5 @@ > let toggleNotificationsCommand = document.getElementById("Social:ToggleNotifications"); > toggleNotificationsCommand.setAttribute("hidden", !socialEnabled); > > if (!SocialUI.haveLoggedInUser() || !socialEnabled) { > + this.resetButton(); nit: no need for curly brackets if it is a single-line block. @@ +942,5 @@ > + > + renderProviderMenus: function SocialToolbar_renderProviderMenus() { > + let providerMenus = document.getElementsByClassName("social-provider-menu"); > + for (let providerMenu of providerMenus) { > + this.renderProviderMenu(providerMenu); ditto. @@ +947,5 @@ > + } > + }, > + > + renderProviderMenu: function SocialToolbar_renderProviderMenu(providerMenu) > + { nit: opening curly bracket should stay on same line as the start of the function definition @@ +974,5 @@ > + menuitem.setAttribute("oncommand", "Social.setProvider(this.getAttribute('origin'))"); > + } > + popup.appendChild(menuitem); > + } > + providerMenu.collapsed = false; s/collapsed/hidden/ here too ::: browser/base/content/browser.xul @@ +695,5 @@ > accesskey="&social.toggleNotifications.accesskey;"/> > <menuseparator class="social-statusarea-separator"/> > <menuitem class="social-toggle-menuitem" command="Social:Toggle"/> > <menuitem class="social-remove-menuitem" command="Social:Remove"/> > + <menuseparator class="social-provider-menu" collapsed="true"/> All class="social-provider-menu" should use hidden="true" :)

(In reply to Jared Wein [:jaws] from comment #38) > Comment on attachment 686916 [details] [diff] [review] > multiprovider part 2 (ui) > > Review of attachment 686916 [details] [diff] [review]: > ----------------------------------------------------------------- > > For UI reviews, it is better to either link to a build or upload a > screencast demonstrating the workflow/interactions. These types of requests > will generally get much faster response times as compared to a patch. Didn't mean to do a uireview. As I recall, I accidentally selected uireview and put your name in, then deleted your name but apparently didn't clear the flag.

Comment on attachment 686916 [details] [diff] [review] multiprovider part 2 (ui) Oh ok, no harm. Clearing the ui-review request then.

(In reply to Mark Hammond (:markh) from comment #35) > Comment on attachment 686915 [details] [diff] [review] > multiprovider part 1 > > Review of attachment 686915 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/browser-social.js > @@ +63,5 @@ > > switch (topic) { > > case "social:pref-changed": > > // Exceptions here sometimes don't get reported properly, report them > > // manually :( > > + // update the browser side if the social component is toggled > > I'm not sure what this is doing - isn't it updating the Social object in > Social.jsm? If so, I don't see why it is necessary (setting > SocialService.enabled sets social.enabled preference before sending the > social:pref-changed notification, and Social.enabled is implemented as just > getting the social.enabled pref.) There is a setter for Social.enabled. The issue is that Social is where we have knowledge of the current provider. SocialService has no concept of the current provider, and if we ever want to have a provider-per-window we shouldn't add one to it. Social.provider.enabled must be set when the social.enabled pref changes, and Social.enabled is the central place to do that. The Social class does not listen for the pref change, so I am updating where we do listen for it. One could argue that Social should listen for the pref change now, and that may be more logical, I am just avoiding adding more listeners. > Also, the comment seems misleading - I'm not sure what it means by "update > the browser side" when this already is in the browser side. The comment means that it is updating the browser side component Social, since the toolkit side does not do that. I'm sure it can be phrased better. > @@ +1004,5 @@ > > if (sbrowser.getAttribute("origin") != Social.provider.origin) { > > sbrowser.setAttribute("origin", Social.provider.origin); > > sbrowser.setAttribute("src", Social.provider.sidebarURL); > > sbrowser.addEventListener("load", SocialSidebar._loadListener, true); > > + } else > > "else if" on the same line is the style of the rest of this file. > > But I'm still not sure why moving this 'if' is necessary. When a provider > is set current it has the error state cleared, so I'm wondering what is the > scenario that forces this change? If setting a provider current did *not* > reset the error state (a potential future change), then I'm pretty sure it > would need to be as it was (ie, in that scenario, this change would be wrong) > > ::: browser/modules/Social.jsm > @@ +50,5 @@ > > + SocialService.getProvider(aOrigin, function(provider) { > > + if (provider) { > > + this.provider = provider; > > + } else { > > + Cu.reportError("provider not available for "+aOrigin); > > nit: operator spaces > > @@ +65,2 @@ > > init: function Social_init(callback) { > > + if (_initialized) { > > Along the lines of my previous comments: This seems a little fragile for a > couple of reasons: it's possible the callback will be made with no provider > current, and the callback itself isn't going to be used when the provider > changes. if no providers are available, enabled is set to false. The callback really isn't necessary at this point, I can remove it, but we should still attempt to set the provider during the init on the first round. iirc the addon did that work in the provider getter, but since everything is async here, that wont really work. > It seems to me that we could drop the callback arg here, and just have the > provider-changed observers do what is necessary regardless of how the > provider is set. That way, much of the UI initialization would not be > called if Social.init() fails to set a provider, but still *will* be called > once a provider is set some other way. A slight downside is that > browser-social's observer would need to differentiate between stuff that > only needs to be done once the first provider is set vs stuff that needs to > be done each time the provider changes, but that sounds reasonable. This is handled better in the part 2 patch. I had hoped that splitting the patches would make the reviews easier, they are not, I'll recombine into a single patch again. > ::: toolkit/components/social/SocialService.jsm > @@ +115,2 @@ > > SocialServiceInternal.providers[provider.origin] = provider; > > + Services.obs.notifyObservers(null, "social:provider-added", provider.origin); > > who uses this observer? AFAICT, addProvider is used only by the tests, and > given it is really only a single line, I wonder if we should just remove it > completely? Per the above comment, the another part of addProvider behaviour is in the part 2 patch, recombining will make it more obvious. addProvider is necessary for tests right now, and it needs to do more than it did before. the additional "benefit" is that addProvider becomes more ready for the future discovery/install work. > @@ +125,5 @@ > > removeProvider: function removeProvider(origin, onDone) { > > if (!(origin in SocialServiceInternal.providers)) > > throw new Error("SocialService.removeProvider: no provider with this origin exists!"); > > > > + // notify removal before shutting down the provider to allow browser to > > ditto here, but this seems even riskier - what happens when the "current" > provider is removed? social should get deactivated.

(In reply to Mark Hammond (:markh) from comment #36) > Comment on attachment 686916 [details] [diff] [review] > multiprovider part 2 (ui) > > Review of attachment 686916 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: browser/base/content/browser-social.js > @@ +75,5 @@ > > + case "social:provider-set": > > + // this happens when a provider is ready (assuming there is one) > > + this.update(); > > + break; > > + case "social:provider-added": > > can this and the -removed one ever be sent except by the tests? they are still necessary even if only used by the test code right now. > @@ +949,5 @@ > > + > > + renderProviderMenu: function SocialToolbar_renderProviderMenu(providerMenu) > > + { > > + // Put the list of providers in a submenu: > > + SocialService.getProviderList(function SocialToolbar_fillinProviderMenu(providers) { > > I wonder if getProviderList should take a param to indicate if only active > ones are desired? that can be a later change > ::: browser/base/content/test/browser_social_multiprovider.js > @@ +31,5 @@ > > + // spins of the event loop, so we just wait until the right thing happens. > > + function waitForCorrectProviderUI(next) { > > + let menu = document.getElementById("social-statusarea-popup"); > > + waitForCondition(function() { > > + let el = menu.getElementsByAttribute("origin", Social.provider.origin); > > it looks like this is the only condition you need to wait for, and the other > checks can actually be normal "is/ok" tests? not entirely clear on what you're saying > ::: toolkit/components/social/SocialService.jsm > @@ +20,5 @@ > > */ > > > > // Internal helper methods and state > > let SocialServiceInternal = { > > + enabled: Services.appinfo.inSafeMode ? false : Services.prefs.getBoolPref("social.enabled"), > > is this inSafeMode check enough? Could Social.enabled be called in safe > mode and still "do stuff"? The original design was to disable social on safe mode startup, but allow it to be enabled, I don't know why, but am not looking to change that behavior right now.

(In reply to Jared Wein [:jaws] from comment #38) > Comment on attachment 686916 [details] [diff] [review] > multiprovider part 2 (ui) > > Review of attachment 686916 [details] [diff] [review]: > ----------------------------------------------------------------- > > I still need to try out the safe mode behavior to see that this didn't > regress the desired behavior. I'll do that when it's up for review. As I understood the desired behavior, startup in safe mode would have social disabled, but you could enable it. That at least was indicated by the comment. However a later change broke that anyway. This change makes it work that way again. > ::: browser/base/content/browser-social.js > @@ +75,5 @@ > > + case "social:provider-set": > > + // this happens when a provider is ready (assuming there is one) > > + this.update(); > > + break; > > + case "social:provider-added": > > Yeah I think Mark's correct here. At least for v1.5, we're not planning on > supporting runtime additions or removals to the list of providers (only > runtime activations/deactivations & enabling/disabling). > > Maybe test-only functions like this (if they must exist) should have > dump("Test-only function being called. This is a bug if this is not a > testing build. Function name=..."). Could use Cu.reportError here but that > might require calling some "expect an exception" function. We need the functions to work correctly, even if they are currently only used by tests. This worked before, it should continue to work, I don't see a reason to try to hobble it in some fashion.

I don't think SocialService.enabled makes sense in a multi-provider world. Can we just get rid of it? Similarly, I'm not sure Social.enabled in the front end makes sense either. It seems like the states we care about are "whether any social providers are active" (to determine whether we show the UI etc.), which is Social.active, and then we also need to track which of the active providers are enabled (we can probably just enforce "Social.provider" is enabled, all others are not?).

(In reply to Shane Caraveo (:mixedpuppy) from comment #41) > (In reply to Mark Hammond (:markh) from comment #35) > > Comment on attachment 686915 [details] [diff] [review] > > multiprovider part 1 > > > > Review of attachment 686915 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/base/content/browser-social.js > > @@ +63,5 @@ > > > switch (topic) { > > > case "social:pref-changed": > > > // Exceptions here sometimes don't get reported properly, report them > > > // manually :( > > > + // update the browser side if the social component is toggled > > > > I'm not sure what this is doing - isn't it updating the Social object in > > Social.jsm? If so, I don't see why it is necessary (setting > > SocialService.enabled sets social.enabled preference before sending the > > social:pref-changed notification, and Social.enabled is implemented as just > > getting the social.enabled pref.) > > There is a setter for Social.enabled. The issue is that Social is where we > have knowledge of the current provider. SocialService has no concept of the > current provider, and if we ever want to have a provider-per-window we > shouldn't add one to it. Social.provider.enabled must be set when the > social.enabled pref changes, and Social.enabled is the central place to do > that. The Social class does not listen for the pref change, so I am > updating where we do listen for it. One could argue that Social should > listen for the pref change now, and that may be more logical, I am just > avoiding adding more listeners. That still doesn't make sense to me - we have per-window code each setting this global state. If SocialService is indirectly doing this via the notification, it should just set the global. But as Gavin said, maybe just nuke it completely? > > Also, the comment seems misleading - I'm not sure what it means by "update > > the browser side" when this already is in the browser side. > > The comment means that it is updating the browser side component Social, > since the toolkit side does not do that. I'm sure it can be phrased better. Yeah - having each window try and do this doesn't make sense to me. > > if no providers are available, enabled is set to false. The callback really > isn't necessary at this point, I can remove it, but we should still attempt > to set the provider during the init on the first round. Agreed - init should attempt to set the provider, and doing so would just send the provider-changed notifiction - so no need for the callback as any code that needs to run after the provider is set can do so via the observer rather than the callback. > > It seems to me that we could drop the callback arg here, and just have the > > provider-changed observers do what is necessary regardless of how the > > provider is set. That way, much of the UI initialization would not be > > called if Social.init() fails to set a provider, but still *will* be called > > once a provider is set some other way. A slight downside is that > > browser-social's observer would need to differentiate between stuff that > > only needs to be done once the first provider is set vs stuff that needs to > > be done each time the provider changes, but that sounds reasonable. > > This is handled better in the part 2 patch. I had hoped that splitting the > patches would make the reviews easier, they are not, I'll recombine into a > single patch again. That'd be my preference - Jaws? > > > ::: toolkit/components/social/SocialService.jsm > > @@ +115,2 @@ > > > SocialServiceInternal.providers[provider.origin] = provider; > > > + Services.obs.notifyObservers(null, "social:provider-added", provider.origin); > > > > who uses this observer? AFAICT, addProvider is used only by the tests, and > > given it is really only a single line, I wonder if we should just remove it > > completely? > > Per the above comment, the another part of addProvider behaviour is in the > part 2 patch, recombining will make it more obvious. > > addProvider is necessary for tests right now, and it needs to do more than > it did before. the additional "benefit" is that addProvider becomes more > ready for the future discovery/install work. Well, that's basically my point - we don't know what addProvider would look like when we have real discovery - so in practice we have observers and code in the runtime used only by the tests, are potentially misleading to people reading the code and have a high likelihood of being inappropriate when we need them for real anyway. If there are zero uses in the real code we are shipping, I still say move it to tests. > > > @@ +125,5 @@ > > > removeProvider: function removeProvider(origin, onDone) { > > > if (!(origin in SocialServiceInternal.providers)) > > > throw new Error("SocialService.removeProvider: no provider with this origin exists!"); > > > > > > + // notify removal before shutting down the provider to allow browser to > > > > ditto here, but this seems even riskier - what happens when the "current" > > provider is removed? > > social should get deactivated. Well - that wasn't obvious to me, so I think this just reinforces my point above.

(In reply to Shane Caraveo (:mixedpuppy) from comment #43) > We need the functions to work correctly, even if they are currently only > used by tests. This worked before, it should continue to work, I don't see > a reason to try to hobble it in some fashion. I don't understand the reasoning here. They only worked before in the context of the tests - we have no idea if they worked appropriately in the shipped code - indeed, some of the other test hacks we've made around activation and stuff in the tests implies they probably don't.

(In reply to Mark Hammond (:markh) from comment #45) > (In reply to Shane Caraveo (:mixedpuppy) from comment #41) > > (In reply to Mark Hammond (:markh) from comment #35) > > > Comment on attachment 686915 [details] [diff] [review] > > > multiprovider part 1 > > > > > > Review of attachment 686915 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: browser/base/content/browser-social.js > > > @@ +63,5 @@ > > > > switch (topic) { > > > > case "social:pref-changed": > > > > // Exceptions here sometimes don't get reported properly, report them > > > > // manually :( > > > > + // update the browser side if the social component is toggled > > > > > > I'm not sure what this is doing - isn't it updating the Social object in > > > Social.jsm? If so, I don't see why it is necessary (setting > > > SocialService.enabled sets social.enabled preference before sending the > > > social:pref-changed notification, and Social.enabled is implemented as just > > > getting the social.enabled pref.) > > > > There is a setter for Social.enabled. The issue is that Social is where we > > have knowledge of the current provider. SocialService has no concept of the > > current provider, and if we ever want to have a provider-per-window we > > shouldn't add one to it. Social.provider.enabled must be set when the > > social.enabled pref changes, and Social.enabled is the central place to do > > that. The Social class does not listen for the pref change, so I am > > updating where we do listen for it. One could argue that Social should > > listen for the pref change now, and that may be more logical, I am just > > avoiding adding more listeners. > > That still doesn't make sense to me - we have per-window code each setting > this global state. If SocialService is indirectly doing this via the > notification, it should just set the global. But as Gavin said, maybe just > nuke it completely? SocialService is in toolkit and Social, where that state is kept, is in browser chrome. Having SocialService set it directly requires toolkit code to have knowledge of browser chrome code, which to my understanding, shouldn't happen. I'm looking at changing enabled, but every further change like that delays getting the base patch in which would make it much easier for more than just myself to work on this. > > > Also, the comment seems misleading - I'm not sure what it means by "update > > > the browser side" when this already is in the browser side. > > > > The comment means that it is updating the browser side component Social, > > since the toolkit side does not do that. I'm sure it can be phrased better. > > Yeah - having each window try and do this doesn't make sense to me. agreed.

back to the megapatch. Social.active is now based on the providers. Leaving Social.enabled since there needs to be a way to disable social (all of it) without removing it.

Comment on attachment 688077 [details] [diff] [review] multiprovider Review of attachment 688077 [details] [diff] [review]: ----------------------------------------------------------------- looking good! ::: browser/base/content/browser-social.js @@ +79,3 @@ > case "social:pref-changed": > + // this will also "unset" if we're disabled > + Social.enabled = data == "enabled"; I still don't grok why every window needs to attempt to set this global, but I'll leave it to others :) @@ +100,5 @@ > + provider.errorState = "frameworker-error"; > + if (Social.provider == provider) > + SocialSidebar.setSidebarErrorMessage("frameworker-error"); > + } else { > + Cu.reportError("frameworker-error for "+data+" has no provider"); spaces around operators and the message looks confusing - something like " ... + data + " but no such provider found"? @@ +927,5 @@ > panel.openPopup(toolbarButtonImage, "bottomcenter topright", 0, 0, false, false); > }, > > setPanelErrorMessage: function SocialToolbar_setPanelErrorMessage(aNotificationFrame) { > + if (!aNotificationFrame || !aNotificationFrame.webNavigation) in what scenarios will webNavigation be false? ::: browser/base/content/test/head.js @@ +134,4 @@ > > // Now that we've set the UI's provider, enable the social functionality > + Social.enabled = true; > + ok(Social.provider.enabled, "Provider "+provider.origin+" is now enabled"); spaces around operators ::: browser/modules/Social.jsm @@ +93,3 @@ > } > + // once initialized, never do so again. > + this.init = function() {}.bind(this); interesting trick :) But is the .bind necessary given the body is empty? @@ +117,4 @@ > }, > > set enabled(val) { > + SocialService.enabled = val; Gavin suggested we get rid of SocialService.enabled and making the getter be just "this.provider && this.provider.enabled" (oh - and the safeMode check) - is there a reason this can't be done? ::: toolkit/components/social/SocialService.jsm @@ +141,5 @@ > throw new Error("SocialService.removeProvider: no provider with this origin exists!"); > > let provider = SocialServiceInternal.providers[origin]; > provider.enabled = false; > + Services.prefs.clearUserPref("social.active."+provider.prefName); spaces nit again :) @@ +169,5 @@ > * The SocialProvider object represents a social provider, and allows > * access to its FrameWorker (if it has one). > * > * @constructor > * @param {jsobj} object representing the manifest file describing this provider this comment no longer reflects the actual params @@ +189,5 @@ > } > > SocialProvider.prototype = { > + // active is whether this provider will be available to the user. > + get active() { 2x operator spaces nits

(In reply to Mark Hammond (:markh) from comment #49) > Comment on attachment 688077 [details] [diff] [review] > @@ +927,5 @@ > > panel.openPopup(toolbarButtonImage, "bottomcenter topright", 0, 0, false, false); > > }, > > > > setPanelErrorMessage: function SocialToolbar_setPanelErrorMessage(aNotificationFrame) { > > + if (!aNotificationFrame || !aNotificationFrame.webNavigation) > > in what scenarios will webNavigation be false? during toolkit tests where we test frameworker errors. it's because those tests are not fully initializing the browser chrome social code. > @@ +117,4 @@ > > }, > > > > set enabled(val) { > > + SocialService.enabled = val; > > Gavin suggested we get rid of SocialService.enabled and making the getter be > just "this.provider && this.provider.enabled" (oh - and the safeMode check) > - is there a reason this can't be done? We can evaluate that later. Right now enabled works well as-is.

updated with marks feedback

get the right patch. marks feedback added.

Comment on attachment 688385 [details] [diff] [review] multiprovider Review of attachment 688385 [details] [diff] [review]: ----------------------------------------------------------------- I didn't look too deeply into the test changes. ::: browser/app/profile/firefox.js @@ -1170,3 @@ > pref("social.sidebar.open", true); > pref("social.sidebar.unload_timeout_ms", 10000); > -pref("social.active", false); note to self, why does social.active pref need to be removed in this patch if social.facebook.active isn't added in this patch? ::: browser/base/content/browser-menubar.inc @@ +543,5 @@ > class="show-only-for-keyboard"/> > <menuseparator class="social-statusarea-separator"/> > <menuitem class="social-toggle-menuitem" command="Social:Toggle"/> > <menuitem class="social-remove-menuitem" command="Social:Remove"/> > + <menuseparator class="social-provider-menu" hidden="true"/> The mockup for multiprovider shows the networks towards the top of the menu (2c). Can you move those? Here's a link to the mockup, http://people.mozilla.com/~jboriss/temp/social_api_multiprovider/pasteboard2.png ::: browser/base/content/browser-social.js @@ +32,5 @@ > SocialSidebar.update(); > SocialChatBar.update(); > }); > > + // make sure Social.jsm is initialized This comment seems redundant. @@ +40,1 @@ > this.updateActiveBroadcaster(); Be careful about intializing window level UI that doesn't rely on a provider being enabled, as we just fixed a bug due to the updateActiveBroadcaster being called before the provider was ready. @@ +122,5 @@ > }, > > + // update when switching providers, activation, enabling or disabling social > + update: function() { > + this.updateActiveBroadcaster(); this.updateActiveBroadcaster() should be moved to the bottom of this function so the UI doesn't activate before the elements are populated. @@ +136,5 @@ > + }, > + > + updateToggleCommand: function SocialUI_updateToggleCommand() { > + let toggleCommand = this.toggleCommand; > + toggleCommand.setAttribute("hidden", Social.active ? "false" : "true"); toggleCommand.setAttribute("hidden", !Social.active); @@ +945,5 @@ > + for (let providerMenu of providerMenus) > + this.renderProviderMenu(providerMenu); > + }, > + > + renderProviderMenu: function SocialToolbar_renderProviderMenu(providerMenu) { s/renderProviderMenu/_renderProviderMenu/ since this doesn't look like it should be called external to this class. @@ -918,5 @@ > init: function SocialSidebar_init() { > let sbrowser = document.getElementById("social-sidebar-browser"); > this.errorListener = new SocialErrorListener("sidebar"); > this.configureSidebarDocShell(sbrowser.docShell); > - this.update(); note to self, if this is removed, when does it get "updated"? ::: toolkit/components/social/SocialService.jsm @@ +59,5 @@ > + // we need to set the active flag for our providers if socialapi was active > + // previously. > + let allActive = false; > + if (Services.prefs.prefHasUserValue("social.active")) { > + allActive = Services.prefs.getBoolPref("social.active"); I think we should set a pref to state that this migration has occurred. If a user upgrades from Fx17 to Fx23 they may get multiple providers activated that they had never agreed to before. So I'd like to see this migration *only* enable Facebook, and then set a pref named "social.apiVersion=1.1" or so, which we can then use for future migrations to know what steps need to be performed. @@ +169,5 @@ > * The SocialProvider object represents a social provider, and allows > * access to its FrameWorker (if it has one). > * > * @constructor > + * @param {string} string portion of prefName. e.g. social.manifest.prefName Naming this argument "prefName" is misleading, because it is only the suffix of the preference name. So maybe change it to prefSuffix?

(In reply to Jared Wein [:jaws] from comment #53) > ::: browser/app/profile/firefox.js > @@ -1170,3 @@ > > pref("social.sidebar.open", true); > > pref("social.sidebar.unload_timeout_ms", 10000); > > -pref("social.active", false); > > note to self, why does social.active pref need to be removed in this patch > if social.facebook.active isn't added in this patch? It is set at runtime in the provider class. > @@ +40,1 @@ > > this.updateActiveBroadcaster(); > > Be careful about intializing window level UI that doesn't rely on a provider > being enabled, as we just fixed a bug due to the updateActiveBroadcaster > being called before the provider was ready. not sure what the ask is here. > @@ -918,5 @@ > > init: function SocialSidebar_init() { > > let sbrowser = document.getElementById("social-sidebar-browser"); > > this.errorListener = new SocialErrorListener("sidebar"); > > this.configureSidebarDocShell(sbrowser.docShell); > > - this.update(); > > note to self, if this is removed, when does it get "updated"? Update happens when a provider is set. > ::: toolkit/components/social/SocialService.jsm > @@ +59,5 @@ > > + // we need to set the active flag for our providers if socialapi was active > > + // previously. > > + let allActive = false; > > + if (Services.prefs.prefHasUserValue("social.active")) { > > + allActive = Services.prefs.getBoolPref("social.active"); > > I think we should set a pref to state that this migration has occurred. If a > user upgrades from Fx17 to Fx23 they may get multiple providers activated > that they had never agreed to before. > > So I'd like to see this migration *only* enable Facebook, and then set a > pref named "social.apiVersion=1.1" or so, which we can then use for future > migrations to know what steps need to be performed. I don't want to call it apiVersion, since code/architecture change could happen independent of the APIs. But I have implemented this for the next patch.

ok, fixed provider removal and updated with other comments from Jared. I'm moving this to review, see if we can get it landed soon.

pushed a small try to verify tests https://tbpl.mozilla.org/?tree=Try&rev=1e5a5a6f3f86

Sorry, didn't mean to submit the "note to self" messages above.

Comment on attachment 688454 [details] [diff] [review] multiprovider Review of attachment 688454 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I'd still like Gavin to take a look at this before landing it. ::: browser/base/content/browser-social.js @@ +39,3 @@ > this.updateActiveBroadcaster(); > + SocialToolbar.init(); > + SocialSidebar.init(); What I meant in my previous feedback pass is that when rebasing this patch, we need to make sure that "this.updateActiveBroadcaster()" is called last, since in it's current order it will show an uninitialized toolbar while loading. ::: browser/base/content/test/browser_social_multiprovider.js @@ +32,5 @@ > + function waitForCorrectProviderUI(next) { > + let menu = document.getElementById("social-statusarea-popup"); > + waitForCondition(function() { > + let el = menu.getElementsByAttribute("origin", Social.provider.origin); > + info("has menu? "+el.length+" origin? "+(el.length>0?el[0].getAttribute("origin"):"NONE")); nit: spaces around operators. @@ +41,5 @@ > + waitForCondition(function() { > + return Social.provider.profile && Social.provider.profile.displayName; > + }, function() { > + // We have a profile for the selected provider - check it was used. > + let displayName, userName nit: add a trailing semicolon.

I'll address issues in comment 58 after further review by gavin.

Comment on attachment 688454 [details] [diff] [review] multiprovider In reviewing this I ended up with many comments - I decided to address them in patch form rather than detail them in length. I'll give an overview of what I changed. >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js > let SocialUI = { I like the idea of splitting things into "update" (from provider etc.) and "window init". We've not done a very good job of making sure the state changes are minimal during various state transitions (enable/disable, new window open, and now provider change), so we should probably try to improve that in the future. My patch introduces "_updateProvider" (for provider changes) and "_updateEnabledState" (for when we're toggling social on/off). Right now they mostly contain a mismash of calls needed to get everything working, which is not ideal, but I think we should try to clean that up in a followup. My patch keeps the _providerReady setup we already had for first-window load, but I'm not sure this makes much sense - we'll probably want to merge _providerReady with _updateProvider, which is a bit closer to what you have in this patch, I think. The social:provider-set stuff in my patch is just copied from yours essentially. I added a _matchesCurrentProvider(origin) helper for the provider-specific notifications. I made some changes with activation - moved the code to Social.activateFromOrigin/Social.deactiveFromOrigin, and had that handle setting global state (and returning the activated provider so the UI can notify). This also required having Social.jsm keep a cache of all installed providers, which it keeps in sync with SocialService via a registered "providerListener" system. This also involved a few changes to how Social.jsm handles provider switching and managing the "social.provider.current" pref. The provider menu stuff is all straight from your patch, though I refactored slightly (to use Social.providers) and renamed it populateProviderMenu. I changed the removal behavior so that "Remove from Firefox" also disables the feature entirely, rather than switching to the previous provider as in your patch. I think this matches the behavior Boriss expected when I chatted with her - there are some weird interactions we'll need to figure out (I left some XXX comments about how activation should work if there's already a provider active, for example). I made "active" persisting for providers use a JSON cache stored in prefs ("ActiveProvider" helper object), rather than in separate prefs per-provider. I reworked the test to add a little bit of additional coverage and refactored it to use some helper functions. I made runSocialTestWithProvider accept an array of manifests to install/activate, to help test multiprovider stuff. I had FrameWorker.jsm cache its "origin" so that it can retrieve its associated provider and set its errorState directly, rather than relying on the UI doing that via an observer. I made some other minor changes as I discovered issues in tests: - updateShareState was calling updateButtonHiddenState multiple times unnecessarily depending on the code path, so I introduced a boolean parameter to control that. Similarly, updateProfileInfo was calling updateShareState unnecessarily. - SocialSidebar.update() was sometimes not updating the "origin" attribute on its browser (when it returns early because the provider is in an error state). This caused trouble when switching to/from a failed provider, so I adjusted that code to always set the origin. - moved haveLoggedInUser to Social.jsm (from SocialUI) - I kept the behavior of setting the feature = false disabling all of the providers. This shouldn't matter much in practice, but it seemed slightly safer. - I updated test_SocialService.js to deal with providers now being disabled by default There are some XXX comments in here that I think may need to be cleaned up/addressed, but otherwise I think this is ready to go. I want to get as many eyes on this as possible, since I've made a non-trivial amount of changes. Please let me know what you think!

Oops, the last paragraph of comment 60 was meant to be used when I attached this patch.

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

Comment on attachment 690283 [details] [diff] [review] adjusted patch Review of attachment 690283 [details] [diff] [review]: ----------------------------------------------------------------- Lovely! ::: browser/base/content/browser-social.js @@ +47,3 @@ > }, > > + // Called once, after window load, once Social.jsm's provider has been set. As you mentioned, I think it would be better to merge _providerReady and _updateProvider - it's easier to follow and removes some hacks and explanatory comments from Social.jsm and from here. @@ +72,5 @@ > + SocialToolbar.updateProvider(); > + SocialSidebar.update(); > + }, > + > + // The entire feature is being turned on/off. Is this comment strictly accurate? "The entire feature" implies it is only handling Social.active going to false, whereas it looks like it is also handling Social.enabled going to false (in which case "the entire feature" doesn't seem accurate) @@ +218,3 @@ > description.value = message; > + // Set the origin being activated > + document.getElementById("social-undoactivation-button").setAttribute("origin", provider.origin); is there any reason the origin gets set on the button, then the button itself passed in via the XUL? Why not just grab the origin in the same way it is set (or just set it directly on the panel, which undoActivation needs a reference to anyway)? @@ +615,5 @@ > this.updateShareState(); > this.dismissUnsharePopup(); > }, > > + updateShareState: function SSB_updateShareState(updateHiddenState) { it looks as though updateShareState is called far less often with updateHiddenState=false - I wonder if this micro-optimization is worth it, or if the condition should be reversed so the more common (and safer) case is the default? ::: browser/modules/Social.jsm @@ +61,5 @@ > + > + this._provider = provider; > + > + if (this._provider) { > + if (this.enabled) A thought for a followup: Is it possible the provider will be set while !this.enabled? ISTM we could be close to having this.enabled be simply "this.provider && this.provider.enabled", in the same way this.active has been re-defined. @@ +162,5 @@ > return SocialService.enabled; > }, > > get active() { > + return this.provider && this.provider.active; similarly to the above, it seems a reasonable invariant that this.provider must always be active (ie, it should be impossible for this.provider.active to be false - this.provider should be null instead) ::: toolkit/components/social/FrameWorker.jsm @@ +473,5 @@ > this._pendingMessagesOutgoing = null; > } > } > + > +function notifyWorkerError(worker) { any reason not to notify with the origin instead of the worker URL? It would make this, plus the browser-social observer of this, saner and more consistent. ::: toolkit/components/social/SocialService.jsm @@ +59,5 @@ > + }, > + > + get _deferredTask() { > + delete this._deferredTask; > + return this._deferredTask = new DeferredTask(this._persist.bind(this), 100); what's magic about the 100? ::: toolkit/components/social/test/browser/browser_frameworker.js @@ +463,5 @@ > } > ioService.offline = true; > } > }, > trailing whitespace which wasn't introduced by this patch, but might as well be removed by it ;) ::: toolkit/content/Troubleshoot.jsm @@ +58,5 @@ > "plugin.", > "plugins.", > "print.", > "privacy.", > "security.", it might be nice to have a followup bug so this info can include the active/enabled provider(s)

Remove from firefox: I have 9 active providers, if I choose to "Remove from Firefox" it completely removes the ability for me to see any of them. I can't actually figure out what prefs to tweak to get it to come back, I had to re-activate facebook via their website to get my other providers back (figured out why, described later in this comment). The disable and remove functionality is basically wrong for multiprovider. It should be "disable social" and "remove ProviderName". "disable social" sets social.enabled=false, "remove ProviderName" sets provider.active=false. Regardless of the social.enabled setting, as long as there is one active provider, there should always be some UI present somewhere. If we disable social when removing a provider, then the UI should at a minimum allow me to select a different provider. Activation should (and does with your patch) make the activated provider the current provider, but undo should reset to the previous state which it no longer does. If I have facebook enabled, and I stumble across another provider and enable it, but change my mind and undo, I don't expect to also lose facebook (along with my other providers) and have social completely removed. activeProviders pref; while this is definitely cleaner, it makes manual configuration of a provider much more difficult. I have 9 providers configured, it took me three edits to get the pref right, I'm great at typo's :( I wanted to see the menu without facebook, but I don't seem to be able to deactivate facebook and keep social activated. Trying manually, if I remove facebook from the activeProviders pref, and reset the currentProvider pref, disable then enable social, I get a different provider appearing in the sidebar (I expected), but facebook still shows up in the menu as a selection. The menu should only show those providers that the user has activated. If I restart in this state, all of social is deactivated. I think we are not keeping track of currentProvider correctly, and on startup the "first" provider is made current rather than the "first active" provider. Having a list of providers in two places (SocialServiceInternal._providers and Social.providers) feels like added complexity to gain synchronous access. We could add SocialService.getProviderByOrigin and have it be synchronous, using SocialServiceInternal._providers, avoiding the data duplication. I'm concerned the data duplication could lead to bugs in the future.

Thanks Mark, great comments. (In reply to Mark Hammond (:markh) from comment #63) > As you mentioned, I think it would be better to merge _providerReady and > _updateProvider - it's easier to follow and removes some hacks and > explanatory comments from Social.jsm and from here. I think there's some value in keeping the "initialize once" bits separate from the "provider switch" stuff, but you may be right that the difference is negligible enough to not merit the complexity. I don't want to touch too much more in this patch, but let's do this in a followup. > Is this comment strictly accurate? "The entire feature" implies it is only > handling Social.active going to false, whereas it looks like it is also > handling Social.enabled going to false (in which case "the entire feature" > doesn't seem accurate) Hrm, as it is it only tracks changes to Social.enabled, which is what I consider the main switch. I haven't put much consideration to what Social.active should mean in this new world - changes to that state are now mostly handled by _updateProvider, IIRC. I think we will need to followup on this. > is there any reason the origin gets set on the button, then the button > itself passed in via the XUL? Why not just grab the origin in the same way > it is set (or just set it directly on the panel, which undoActivation needs > a reference to anyway)? No good reason - I'll switch to the panel. > it looks as though updateShareState is called far less often with > updateHiddenState=false - I wonder if this micro-optimization is worth it, > or if the condition should be reversed so the more common (and safer) case > is the default? OK, I just removed it. > A thought for a followup: Is it possible the provider will be set while > !this.enabled? ISTM we could be close to having this.enabled be simply > "this.provider && this.provider.enabled", in the same way this.active has > been re-defined. > similarly to the above, it seems a reasonable invariant that this.provider > must always be active (ie, it should be impossible for this.provider.active > to be false - this.provider should be null instead) Yep, these are good points, and I thought about this as well. Better documenting the invariants would help make this code more understandable. > any reason not to notify with the origin instead of the worker URL? It > would make this, plus the browser-social observer of this, saner and more > consistent. Yeah, this is an artifact of my only adding the "origin" property on the FrameWorker objects later on in patch development - I'll switch and update the tests accordingly. > > + return this._deferredTask = new DeferredTask(this._persist.bind(this), 100); > > what's magic about the 100? It's just an arbitrary timeout to coalesce multiple consecutive invocations of flush() (e.g. if you're setting .active on all providers in a loop). I guess it should be 0.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #66) > Thanks Mark, great comments. > > (In reply to Mark Hammond (:markh) from comment #63) > > As you mentioned, I think it would be better to merge _providerReady and > > _updateProvider - it's easier to follow and removes some hacks and > > explanatory comments from Social.jsm and from here. > > I think there's some value in keeping the "initialize once" bits separate > from the "provider switch" stuff, but you may be right that the difference > is negligible enough to not merit the complexity. To clarify, I agree the bits should be separate, but don't see why the "initialize once" stuff needs to be done via a callback - the "initialize once" stuff should be independent of a provider being setup, so should be able to be done "inline" or "sync" (what's the opposite of "via a callback?" :) > but let's do this in a followup. Sounds fine to me!

(In reply to Shane Caraveo (:mixedpuppy) from comment #64) > Remove from firefox: I have 9 active providers, if I choose to "Remove from > Activation should (and does with your patch) make the activated provider the I think I fixed these issues, let me know if you notice any weirdness. > activeProviders pref; while this is definitely cleaner, it makes manual > configuration of a provider much more difficult. I have 9 providers > configured, it took me three edits to get the pref right, I'm great at > typo's :( Sorry - I think we shouldn't optimize for hand-tweaking, though. I kind of like not "polluting" prefs and keeping the storage separate from the rest of the API. > I wanted to see the menu without facebook, but I don't seem to be able to > deactivate facebook and keep social activated. Trying manually, if I remove > facebook from the activeProviders pref, and reset the currentProvider pref, > disable then enable social, I get a different provider appearing in the > sidebar (I expected), but facebook still shows up in the menu as a > selection. The menu should only show those providers that the user has > activated. If I restart in this state, all of social is deactivated. I > think we are not keeping track of currentProvider correctly, and on startup > the "first" provider is made current rather than the "first active" provider. Not sure what was going on here, but as mentioned on IRC changing the "active"/"current" pref manually isn't really supported (or at least doesn't take effect without a restart). The menu does only show active providers, but I don't try to support "no provider selected", since you can't get into that state with our UI. I think the new patch should be better here. > Having a list of providers in two places (SocialServiceInternal._providers > and Social.providers) feels like added complexity to gain synchronous > access. We could add SocialService.getProviderByOrigin and have it be > synchronous, using SocialServiceInternal._providers, avoiding the data > duplication. I'm concerned the data duplication could lead to bugs in the > future. This seems unlikely since the listener thing keeps stuff in sync. But I agree that it's a bit weird. Having the social service expose both async and sync APIs is also a bit weird though, unless we transition to something like the search service's "async init, then everything is sync" model. Let's discuss that in a followup, I'm definitely open to it.

(In reply to Mark Hammond (:markh) from comment #67) > To clarify, I agree the bits should be separate, but don't see why the > "initialize once" stuff needs to be done via a callback Ah, quite true! Definitely should fix that in the followup, you're right.

This just shows what I changed to address the comments, in a more concise form.

Comment on attachment 690737 [details] [diff] [review] patch with comments addressed awesome!

Comment on attachment 690737 [details] [diff] [review] patch with comments addressed cool, I cannot make anything break now.

I will file the followups tomorrow! Thanks guys.

Followup fix to address a migration issue discovered by Mark: https://hg.mozilla.org/mozilla-central/rev/f6580fbf3b0b

Bug 832811 is a regression caused by this.