Closed Bug 774520 Opened 9 years ago Closed 9 years ago

display multiple providers in toolbar ui

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 809694

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx18][strings:1] )

Attachments

(3 files, 8 obsolete files)

need to have the ability to switch between providers in the ui
Attached patch multiprovider.patch (obsolete) — Splinter Review
Attachment #642785 - Flags: feedback?(jaws)
Attachment #642785 - Flags: feedback?(gavin.sharp)
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Fx17]
Version: unspecified → Trunk
Comment on attachment 642785 [details] [diff] [review]
multiprovider.patch

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

On a side-note: can you update your .hgrc to include 8 lines of context for your changes? I'm not sure exactly what is changing within the browser.css file unless I apply the patch. This section provides some basic defaults for hgrc files that will include more context on the changes <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>

::: browser/base/content/browser-social.js
@@ +296,5 @@
> +  
> +  renderProviderMenu: function SocialToolbar_renderProviderMenu()
> +  {
> +    // Put the list of providers in a submenu:
> +    Cu.import("resource://gre/modules/SocialService.jsm");

Is there a better way to do this? I don't think we should be loading the JSM each time this function is called.

@@ +320,5 @@
> +          menuitem.setAttribute("checked", "true");
> +        } else {
> +          menuitem.setAttribute("oncommand", "Social.setProvider(this.getAttribute('origin'))");
> +        }
> +        subMenu.appendChild(menuitem);//insertBefore(menuitem, before);

I know this is a feedback pass, but just wanted to let you know about this commented out line that looks like it could just be deleted.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +653,4 @@
>  toolbar button -->
>  <!ENTITY markupButton.accesskey          "M">
>  
> +<!ENTITY socialProviderMenu.label   "Switch social network">

I'm not sure about this terminology here. Not all types of providers may consider themselves "social networks".

::: browser/modules/Social.jsm
@@ +16,4 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "SocialService",
>    "resource://gre/modules/SocialService.jsm");
>  
> +var _provider;

It looks like this gets added to the global scope (I could be wrong). Can you move this variable to within the Social object?

@@ +49,5 @@
> +    try {
> +      let current = Services.prefs.getCharPref("social.provider.current");
> +      SocialService.getProvider(current, function(provider) {
> +        this.provider = provider;
> +        if (callback)

I think this should also check to see if |provider| is truthy, since the callback may be depending on it.

@@ +61,1 @@
>          callback();

This changes the previous behavior, which only executed the callback if provider.length > 0.

::: browser/themes/pinstripe/browser.css
@@ +3351,5 @@
>    -moz-appearance: none;
>    margin: 0;
>    padding: 2px;
> +  min-width: 20px;
> +  min-height: 20px;

Why make these changes as part of this patch? Should there be similar changes for the other themes, or should these style changes be part of a different bug?
Attachment #642785 - Flags: feedback?(jaws)
Comment on attachment 642785 [details] [diff] [review]
multiprovider.patch

I think we should defer this for the moment, and focus on the 16-targeted work items.
Attachment #642785 - Flags: feedback?(gavin.sharp) → feedback-
Attached patch multiprovider.patch (obsolete) — Splinter Review
just keeping patch updated.
Attachment #642785 - Attachment is obsolete: true
Attached patch rebase on top of bug 777176 (obsolete) — Splinter Review
Assignee: nobody → mixedpuppy
Attachment #648116 - Attachment is obsolete: true
Depends on: 777176, 779360
Attached patch rbased on top of 777176,779360 (obsolete) — Splinter Review
Attachment #651603 - Attachment is obsolete: true
Attached patch multiprovider.patch with tests (obsolete) — Splinter Review
Attachment #651913 - Attachment is obsolete: true
Attachment #652627 - Flags: feedback?(mhammond)
Attachment #652627 - Flags: feedback?(felipc)
Comment on attachment 652627 [details] [diff] [review]
multiprovider.patch with tests

+        SocialToolbar.renderProviderMenu();

I can't see that new method?

+    try {
+      let current = Services.prefs.getCharPref("social.provider.current");
+      SocialService.getProvider(current, function(provider) {
+        this.provider = provider;
+        if (callback)
+          callback();
+      }.bind(this));
+    } catch(e) {
+      SocialService.getProviderList(function (providers) {
+        if (providers.length)
+          this.provider = providers[0];
+        if (callback)
+          callback();
+      }.bind(this));

This exception handler seems overly broad.  I guess you are just guarding against the pref not being set?  If so, I'd suggest just catching that.
Attachment #652627 - Flags: feedback?(mhammond) → feedback+
Attached patch multiprovider.patch with tests (obsolete) — Splinter Review
fixes patch with missing method
Attachment #652627 - Attachment is obsolete: true
Attachment #652627 - Flags: feedback?(felipc)
Attachment #653619 - Flags: feedback?(mhammond)
Attachment #653619 - Flags: feedback?(jaws)
Whiteboard: [Fx17] → [Fx17][strings:1]
Attached patch multiprovider.patch (obsolete) — Splinter Review
rebased
Attachment #653619 - Attachment is obsolete: true
Attachment #653619 - Flags: feedback?(mhammond)
Attachment #653619 - Flags: feedback?(jaws)
Blocks: 786131
Summary: support multiple providers → dislpay multiple providers in toolbar ui
Comment on attachment 655782 [details] [diff] [review]
multiprovider.patch

> // If the last event was received < 1s ago, ignore this one

I think we should kill that code - Gavin says he remembers adding it but doesn't think it is adding any value at the moment.

+    ["social-notification-box",
+     "social-status-iconbox"].forEach(function removeChildren(parentId) {

Most existing code seems to be using the pattern:

  for (let parentId of ["social-...", ...]) {

+        if (!provider.enabled) continue;

If a disabled provider later becomes enabled (or vice-versa), it doesn't appear the UI reflect that fact?  We probably need to fire a "social:providers-updated" notification when this happens.  Plus test coverage for this.

+		 browser_social_multiprovider.js \

appears to be mixing tabs and spaces?
Oops - I meant to add that the feedback above is in addition to the previous feedback from jaws and myself.
Whiteboard: [Fx17][strings:1] → [Fx18][strings:1]
Attached patch Updated patch (obsolete) — Splinter Review
Minor updates to Shane's patch:

* Ensure the profile info on the "share" button is updated.
* Moar tests!
* All feedback comments from previous patch addressed.
Attachment #655782 - Attachment is obsolete: true
This patch is on top of the previous patch and is more about cleanup rather than fixing things.

* The various "init" methods of the social elements now perform initialization that should happen exactly once, rather than things which depend on the current provider.  These are now always called, even if no provider is current, and should address Gavin's comments in bug 781386 comment 0.

* The code for handling "preference changed", "provider changed" and "provider profile changed" has been split into new methods, and is called both at initialize time (assuming a current provider etc exists at that time) and as we observer notifications telling us about the changes.

* The Social:Toggle element is updated as we change providers - however, as per bug 789672, this element will probably need more radical changes in a multi-provider world.

* There is some additional refactoring that could be done - eg, SocialToolbar methods updateButton, updateButtonHiddenState and updateProfile are all doing similar work), but I've left them alone for now.
Attachment #660293 - Flags: feedback?(gavin.sharp)
Comment on attachment 660293 [details] [diff] [review]
refactor initialization of social components

This looks like a nice cleanup, but this seems like the wrong bug to be tracking the work in (we could land this patch as a precursor to "display multiple providers in toolbar UI").

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

>+  _onPrefChanged: function SocialUI_onPrefChanged() {

I noticed you re-ordered the calls in this when copying from the pref-changed observer - was that meaningful or just tidying up?

>   // Called once Social.jsm's provider has been set
>+  _postInit: function SocialUI_postInit() {

>+    if (Social.provider) {
>+      this._onProviderChanged();

We're now breaking some of the core assumptions this code had. From the comment above: Social.provider would previously always be set in this method, so the null check here wouldn't be needed. If we're changing that, we'll need to change the comments accordingly.

>   // Called once, after window load, when the Social.provider object is initialized
>   init: function SSB_init() {

Should just remove this if it's going to be empty.

>   init: function SocialToolbar_init() {
>-    document.getElementById("social-provider-image").setAttribute("image", Social.provider.iconURL);

This gets removed but not re-added anywhere?
Attachment #660293 - Flags: feedback?(gavin.sharp) → feedback+
Summary: dislpay multiple providers in toolbar ui → display multiple providers in toolbar ui
Attached file Updated to current tip
This is a refresh against the current tip.  It hasn't been extensively tested or self-reviewed, but does basically work.  I'll get back to this and the other "refactor" patch asap.
Attachment #660291 - Attachment is obsolete: true
Attached file v3
fixes menus and error state for sidebar
I am closing this bug in favor of a string of bugs with smaller patches.  The string of bugs starts with bug 809694
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 809694
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.