891225 - implement status panel button

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

This button allows providers to have a status button with notifications that can be placed in the toolbar, or australis menu, that is not dependent on being the "selected" provider.  It supports a badged icon and contains a panel with iframe.  The iframe url is defined via the manifest.  The icon may be updated via the link rel=icon tag, similar to how the chat window, app tabs, etc. work.  

TBD:

We want to reduce reliance on having a worker for functionality, but need to flesh out how badge updates happen.

- The badge would be updated via a DOM event the iframe content sets.
- The badge would be updated via the worker.

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP

This WIP adds a button to the customization palette for each enabled social provider. If the user places an icon in a toolbar, the position will be remembered after a Firefox restart.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP statusbutton

adds new statusbutton class which manages all provider status buttons.  each button has a frame managed by SharedFrames so they can swap between windows.  Button position is cached and uninstall of the provider removes its cache entry.

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 777776 [details] [diff] [review]
WIP statusbutton

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

Overall this seems reasonable to me, but there may be issues I don't see due to missing context.

::: browser/base/content/browser-social.js
@@ +1047,5 @@
> +      if (frame.docShell) {
> +        frame.docShell.isActive = false;
> +        Social.setErrorListener(frame, this.setPanelErrorMessage.bind(this));
> +      }
> +      

Nit: Trailing whitespace.

@@ +1066,5 @@
> +      let notif = icons[iconNames[0]];
> +      if (!notif) {
> +        button.setAttribute("badge", "");
> +        button.setAttribute("aria-label", "");
> +        button.setAttribute("tooltiptext", "");

Did you mean removeAttribute instead of setting the value to ""?

@@ +1163,5 @@
> +    let panel = aNotificationFrame.parentNode;
> +    sizeSocialPanelToContent(panel, aNotificationFrame);
> +  },
> +
> +  peristPositions: function() {

Typo: I think you wanted "persist" rather than "perist".

@@ +1168,5 @@
> +    let positions;
> +    // Reload the previously saved value so that we don't lose the positions of
> +    // temporarily disabled providers.
> +    // TODO We likely want to forget the position for uninstalled providers
> +    // though.

Looks like this TODO comment has already been addressed.

Comment 4

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

Comment on attachment 777776 [details] [diff] [review]
WIP statusbutton

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

This is a shed-load of new code.  Are there any mockups for this?  I'm particularly interested in seeing how old and new providers look when they are together and see what scope there is to kill old toolbar code.

This was only a quick glance - I think the other parts of the multi-provider world is more time sensitive, so I'm putting off a detailed look.

::: browser/base/content/browser-social.js
@@ +986,5 @@
>      }
>    }
>  };
>  
> +SocialStatus = {

can we add some comments describing what a SocialStatus is?  A SocialToolbar or SocialSidebar is somewhat self-explanatory, but I've no clue from just the name what this is.

This almost seems a replacement for SocialToolbar - but I don't see SocialToolbar being removed :)  What's the plan for that?  It is really impossible to map the behaviour of "old" providers onto this?

Are there any UX mockups?

@@ +991,5 @@
> +  init: function() {
> +    this._dynamicResizer = new DynamicResizeWatcher();
> +    window.addEventListener("aftercustomization", this.peristPositions.bind(this));
> +
> +    // providers-changed does not give us the detail of which provider to change

can we fix providers-changed (or add a new similar notification)?  OTOH, why can't you find the keys of |positions| that don't appear in Social.providers manually?

@@ +1140,5 @@
> +        }, true);
> +      }
> +    });
> +
> +    let navBar = document.getElementById("nav-bar");

let className = navBar.getAttribute("mode") == "text" ? "toolbarbutton-text" : "toolbarbutton-badge-container";
let anchor = document.getAnonymousElementByAttribute(aToolbarButton, "class", className)

@@ +1187,5 @@
> +      // aftercustomization may have moved the button, which will loose the
> +      // badge, we need to update to get the badge back
> +      this.updateNotification(provider.origin);
> +    }
> +    Services.prefs.setCharPref(CACHE_STATUS_PREF, JSON.stringify(positions));

It's not clear to me that prefs is the right way to persist these (it might be, but I don't recall having seen that done in the past). We probably want some who's helped with Australis to review some of this.

Comment 5

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

Comment on attachment 777776 [details] [diff] [review]
WIP statusbutton

oops, sorry, didn't mean to f-, just to clear the flag.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #4)
> Comment on attachment 777776 [details] [diff] [review]
> WIP statusbutton
> 
> Review of attachment 777776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a shed-load of new code.  Are there any mockups for this?  I'm
> particularly interested in seeing how old and new providers look when they
> are together and see what scope there is to kill old toolbar code.

The mockups are in the meta bug 889427, which was created with Boriss, though she hasn't commented on the bug she did review it.  The original toolbarbutton remains in the interim for 2 reasons; to support "old" providers and to provide a way to change the sidebar until we figure out better ux.  Providers using the new statusURL in the manifest are not allowed to use the ambient icons in the old toolbarbutton.  We'll deprecate the old style via documentation and eventually remove support at some future point (probably fx28/29, but will do outreach to existing providers before locking that down).

> This was only a quick glance - I think the other parts of the multi-provider
> world is more time sensitive, so I'm putting off a detailed look.
> 
> ::: browser/base/content/browser-social.js
> @@ +986,5 @@
> >      }
> >    }
> >  };
> >  
> > +SocialStatus = {
> 
> can we add some comments describing what a SocialStatus is?  A SocialToolbar
> or SocialSidebar is somewhat self-explanatory, but I've no clue from just
> the name what this is.

Sure, and the class could probably be called SocialStatusButton as well.

> This almost seems a replacement for SocialToolbar - but I don't see
> SocialToolbar being removed :)  What's the plan for that?  It is really
> impossible to map the behaviour of "old" providers onto this?

Per above.  We did not want to support "old" providers in the new buttons since we could easily end up with multiple long-ish toolbarbuttons. 

> Are there any UX mockups?
> 
> @@ +991,5 @@
> > +  init: function() {
> > +    this._dynamicResizer = new DynamicResizeWatcher();
> > +    window.addEventListener("aftercustomization", this.peristPositions.bind(this));
> > +
> > +    // providers-changed does not give us the detail of which provider to change
> 
> can we fix providers-changed (or add a new similar notification)?  OTOH, why
> can't you find the keys of |positions| that don't appear in Social.providers
> manually?

The other issue that I realized when working on this, but didn't update the comment, is that providers-changed happens when a provider is disabled/enabled (via about:addons).  We don't want to remove the persisted position on disable, only on uninstall.  Using an addonmanager observer is the way to catch the uninstall.

> @@ +1187,5 @@
> > +      // aftercustomization may have moved the button, which will loose the
> > +      // badge, we need to update to get the badge back
> > +      this.updateNotification(provider.origin);
> > +    }
> > +    Services.prefs.setCharPref(CACHE_STATUS_PREF, JSON.stringify(positions));
> 
> It's not clear to me that prefs is the right way to persist these (it might
> be, but I don't recall having seen that done in the past). We probably want
> some who's helped with Australis to review some of this.

There may well be a better mechanism, it may simply be the persist attribute, I'll take a better look at that.

Comment 7

[Florian Quèze [:florian]]

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

> > @@ +1187,5 @@
> > > +      // aftercustomization may have moved the button, which will loose the
> > > +      // badge, we need to update to get the badge back
> > > +      this.updateNotification(provider.origin);
> > > +    }
> > > +    Services.prefs.setCharPref(CACHE_STATUS_PREF, JSON.stringify(positions));
> > 
> > It's not clear to me that prefs is the right way to persist these (it might
> > be, but I don't recall having seen that done in the past). We probably want
> > some who's helped with Australis to review some of this.
> 
> There may well be a better mechanism, it may simply be the persist
> attribute, I'll take a better look at that.

This is coming from attachment 773956 [details] [diff] [review]. The reason why I handled position saving with a pref is that the normal way to persist customized button positions won't work for the status buttons because they are generated dynamically after the customizations are applied. We could put the string that I saved in a pref in a persisted attribute of a random XUL element (that is guaranteed to always be in the DOM of the main browser window) instead, but I don't think that's better than a pref.

Comment 8

[Shane Caraveo (:mixedpuppy)]

@Gijs regarding the persisting of button locations for socialapi buttons (see comment #7), do you have any better suggestions?

Comment 9

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> @Gijs regarding the persisting of button locations for socialapi buttons
> (see comment #7), do you have any better suggestions?

In current trunk the currentset attributes on toolbars already keep track of the location of items. You should be able to look at those to figure out where to insert your item if it's dynamically generated, as they will be persisted (though you would need to manually document.persist the currentset attribute if you're inserting it directly into a toolbar yourself for the first time (ie, it's not yet anywhere in a currentset attribute). When you insert it, use toolbar.insertItem instead of insertBefore and things should in theory Just Work, even in Australis.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > @Gijs regarding the persisting of button locations for socialapi buttons
> > (see comment #7), do you have any better suggestions?
> 
> In current trunk the currentset attributes on toolbars already keep track of
> the location of items. You should be able to look at those to figure out
> where to insert your item if it's dynamically generated, as they will be
> persisted (though you would need to manually document.persist the currentset
> attribute if you're inserting it directly into a toolbar yourself for the
> first time (ie, it's not yet anywhere in a currentset attribute). When you
> insert it, use toolbar.insertItem instead of insertBefore and things should
> in theory Just Work, even in Australis.

@Gijs I was looking into this, the problem is that on startup, we don't load social until well after the toolbars have already updated currentset.  Hence our persisted IDs have been removed from currentset since our buttons did not exist yet.  Not sure where to go with this now.  Any thoughts?

Comment 11

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> > > @Gijs regarding the persisting of button locations for socialapi buttons
> > > (see comment #7), do you have any better suggestions?
> > 
> > In current trunk the currentset attributes on toolbars already keep track of
> > the location of items. You should be able to look at those to figure out
> > where to insert your item if it's dynamically generated, as they will be
> > persisted (though you would need to manually document.persist the currentset
> > attribute if you're inserting it directly into a toolbar yourself for the
> > first time (ie, it's not yet anywhere in a currentset attribute). When you
> > insert it, use toolbar.insertItem instead of insertBefore and things should
> > in theory Just Work, even in Australis.
> 
> @Gijs I was looking into this, the problem is that on startup, we don't load
> social until well after the toolbars have already updated currentset.  Hence
> our persisted IDs have been removed from currentset since our buttons did
> not exist yet.  Not sure where to go with this now.  Any thoughts?

Currentset shouldn't be removing non-existing IDs. Are you sure it's doing that? The expected behaviour would be that it would not automatically insert your button when you add it to the DOM, but that you'd have to go and look where the user moved it (iterate over the toolbars, check the currentset attribute, then insert in the right place). But the id should remain in the currentset even if the button isn't there on startup.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #11)

> > @Gijs I was looking into this, the problem is that on startup, we don't load
> > social until well after the toolbars have already updated currentset.  Hence
> > our persisted IDs have been removed from currentset since our buttons did
> > not exist yet.  Not sure where to go with this now.  Any thoughts?
> 
> Currentset shouldn't be removing non-existing IDs. Are you sure it's doing
> that? The expected behaviour would be that it would not automatically insert
> your button when you add it to the DOM, but that you'd have to go and look
> where the user moved it (iterate over the toolbars, check the currentset
> attribute, then insert in the right place). But the id should remain in the
> currentset even if the button isn't there on startup.


The currentset setter/getter in toolbar.xml only keeps those buttons that exist in xul (palette or otherwise) prior to the toolbar._init being run.  Specifically, this code requires the button to already exist:

https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#272

Since we do not initialize anything in social until well after the toolbars have initialized, the persisted id in currentset is lost.  Either we'll need to initialize social before the toolbars (early in startup) or we need a separate persistence mechanism (or there is something else I could do that I am not aware of).

Comment 13

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> The currentset setter/getter in toolbar.xml only keeps those buttons that
> exist in xul (palette or otherwise) prior to the toolbar._init being run. 
> Specifically, this code requires the button to already exist:
> 
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/
> toolbar.xml#272
> 
> Since we do not initialize anything in social until well after the toolbars
> have initialized, the persisted id in currentset is lost.  Either we'll need
> to initialize social before the toolbars (early in startup) or we need a
> separate persistence mechanism (or there is something else I could do that I
> am not aware of).

Right, so this is the property setter. The *attribute* should still have it, provided that you set it and persisted it, which is essentially what consumers do as far as I know. However, you may want to consult with Dao if what I'm saying is correct and the best way to go about this.

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #12)

> Right, so this is the property setter. The *attribute* should still have it,
> provided that you set it and persisted it, which is essentially what
> consumers do as far as I know. However, you may want to consult with Dao if
> what I'm saying is correct and the best way to go about this.

wonderful, switching to using the attribute got this working for me.  My concern here is whether something else will (in the future, or perhaps an addon) modify the attribute prior to social starting up.  I guess I could grab the attribute early in startup and keep a copy for social startup.

Comment 15

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> 
> > Right, so this is the property setter. The *attribute* should still have it,
> > provided that you set it and persisted it, which is essentially what
> > consumers do as far as I know. However, you may want to consult with Dao if
> > what I'm saying is correct and the best way to go about this.
> 
> wonderful, switching to using the attribute got this working for me.  My
> concern here is whether something else will (in the future, or perhaps an
> addon) modify the attribute prior to social starting up.  I guess I could
> grab the attribute early in startup and keep a copy for social startup.

Etiquette dictates that you modify it in a way that keeps everyone else in there (ie, only add/remove your own stuff, dynamically), so that shouldn't be a problem... :-)

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

add toolbar helper class, which will also be used by the socialmark button in the patch for bug 891219.  This uses currentset on the toolbar for persisting button locations.

Comment 17

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

Comment on attachment 789209 [details] [diff] [review]
statusbutton

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

Looks OK - I stopped reading browser-social carefully after 'onEnbaled' made it obvious it was still a WIP :)

::: browser/base/content/browser-social.js
@@ +1407,5 @@
> +  this._createButton = createButtonFn;
> +  this._type = type;
> +
> +  // providers-changed does not give us the detail we need to manage the
> +  // buttons, so we rely on the addonmanager to give us those signals.

how hard is it to have our observers give the signals we need?  Or alternatvely, of dropping some of our observers that can be made redundant by the Addon manager notifications?  I'm not that keen on a kindof mismatch of different ways to handle various aspects.

@@ +1410,5 @@
> +  // providers-changed does not give us the detail we need to manage the
> +  // buttons, so we rely on the addonmanager to give us those signals.
> +  let tbh = this;
> +  this.listener = {
> +    onInstalled: function(addon) {

where are these notifications restricted to coming just from social providers?  ie, how do we know addon.manifest exists and is from one of our providers?

@@ +1416,5 @@
> +      // button id to the toolbar currentset attribute. When the addon is
> +      // enabled we'll call populatePalette which will make the button appear.
> +      tbh.setPersistentPosition(tbh.idFromOrgin(addon.manifest.origin));
> +    },
> +    onEnbaled: function(addon) {

typo

@@ +1451,5 @@
> +    try {
> +      // etld doesn't work with some schemes
> +      let etld = Components.classes["@mozilla.org/network/effective-tld-service;1"].getService(Ci.nsIEffectiveTLDService);
> +      domain = etld.getBaseDomainFromHost(domain);
> +    } catch(e) {}

I wasn't aware we are using the TLD in places, but are we sure we want to prevent (say) tumblr.com from having multiple providers?  Also, are we sure we want the port passed in here - it says "FromHost" which implies the port might not be expected (it returns the port in the result, which doesn't sound right as a TLD).

But ultimately, I don't see why origin can't be used?

@@ +1483,5 @@
> +    if (pos >= 1) {
> +      currentset.splice(pos-1, 0, id);
> +    } else {
> +      currentset.push(id);
> +    }

This looks off-by-one to me.  if pos==0 here aren't we going to stick the new element to the right, and otherwise we will be sticking it such that there is 1 element between the primary button and the new one?

ie, shouldn't it just be currentset.splice(pos, 0, id) (and possibly a .push if index == -1, or maybe that's an error?

@@ +1492,5 @@
> +  removeProviderButton: function(provider) {
> +    if (!provider.statusURL)
> +      return;
> +    // this will remove the button from the palette or the toolbar
> +    let id = this._type + "-" + provider.domain;

why not idFromOrigin?

@@ +1497,5 @@
> +    let button = this._getExistingButton(id);
> +    if (button)
> +      button.parentNode.removeChild(button);
> +  },
> +  

trailing space

@@ +1522,5 @@
> +
> +  // should be called on startup of each window, otherwise the addon manager
> +  // listener will handle new activations, or enable/disabling of a provider
> +  // XXX we currently call more regularily, will fix during refactoring
> +  populatePalette: function() {

does this need to be called as social is disabled?

@@ +1553,5 @@
> +        let next = document.getElementById(pset[pi]);
> +        parent.insertItem(id, next, null, false);
> +      }
> +    }
> +  } 

whitespace

@@ +1630,5 @@
> +      if (frame.docShell) {
> +        frame.docShell.isActive = false;
> +        Social.setErrorListener(frame, this.setPanelErrorMessage.bind(this));
> +      }
> +      

whitespace

::: toolkit/components/social/SocialService.jsm
@@ +502,5 @@
>      // workerURL, sidebarURL is required and must be same-origin
>      // iconURL and name are required
>      // iconURL may be a different origin (CDN or data url support) if this is
>      // a whitelisted or directory listed provider
> +    let providerHasFeatures = [url for (url of sameOriginRequired) if (data[url])].length > 0;

isn't this going to fail for old providers?

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #17)
> Comment on attachment 789209 [details] [diff] [review]
> statusbutton
> 
> Review of attachment 789209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK - I stopped reading browser-social carefully after 'onEnbaled' made
> it obvious it was still a WIP :)

yup

> ::: browser/base/content/browser-social.js
> @@ +1407,5 @@
> > +  this._createButton = createButtonFn;
> > +  this._type = type;
> > +
> > +  // providers-changed does not give us the detail we need to manage the
> > +  // buttons, so we rely on the addonmanager to give us those signals.
> 
> how hard is it to have our observers give the signals we need?  Or
> alternatvely, of dropping some of our observers that can be made redundant
> by the Addon manager notifications?  I'm not that keen on a kindof mismatch
> of different ways to handle various aspects.

I looked into using our observers first, but this was a more trivial approach.  I think there is room for refactoring some of the notification/observer/listener stuff we have when we hit the refactoring patches.

> @@ +1483,5 @@
> > +    if (pos >= 1) {
> > +      currentset.splice(pos-1, 0, id);
> > +    } else {
> > +      currentset.push(id);
> > +    }
> 
> This looks off-by-one to me.  

yeah, was still playing around with that, but am just going to append rather than trying to be smart about it.  

> @@ +1522,5 @@
> > +
> > +  // should be called on startup of each window, otherwise the addon manager
> > +  // listener will handle new activations, or enable/disabling of a provider
> > +  // XXX we currently call more regularily, will fix during refactoring
> > +  populatePalette: function() {
> 
> does this need to be called as social is disabled?

are you saying if or when social is disabled?  this should never be called if social is disabled.  it is called when social is enabled/disabled since set-provider is called.  that will change slightly in the later refactoring we do.


> ::: toolkit/components/social/SocialService.jsm
> @@ +502,5 @@
> >      // workerURL, sidebarURL is required and must be same-origin
> >      // iconURL and name are required
> >      // iconURL may be a different origin (CDN or data url support) if this is
> >      // a whitelisted or directory listed provider
> > +    let providerHasFeatures = [url for (url of sameOriginRequired) if (data[url])].length > 0;
> 
> isn't this going to fail for old providers?

older providers had workerURL and sidebarURL, so it should work fine.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

updated w/feedback

Comment 20

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

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

> > how hard is it to have our observers give the signals we need?  Or
> > alternatvely, of dropping some of our observers that can be made redundant
> > by the Addon manager notifications?  I'm not that keen on a kindof mismatch
> > of different ways to handle various aspects.
> 
> I looked into using our observers first, but this was a more trivial
> approach.  I think there is room for refactoring some of the
> notification/observer/listener stuff we have when we hit the refactoring
> patches.

Can you elaborate a little on this?  ie, I can't imagine our existing observers are far from satisfying this use-case and mixing up observers in this way makes me uneasy as it gets much harder to track what happens when.

> > @@ +1522,5 @@
> > > +
> > > +  // should be called on startup of each window, otherwise the addon manager
> > > +  // listener will handle new activations, or enable/disabling of a provider
> > > +  // XXX we currently call more regularily, will fix during refactoring
> > > +  populatePalette: function() {
> > 
> > does this need to be called as social is disabled?
> 
> are you saying if or when social is disabled?  this should never be called
> if social is disabled.  it is called when social is enabled/disabled since
> set-provider is called.

I'm not with you here - you first say it's never called if social is disabled, but then say it is?

The first line of that was explicitly checking 'if (!Social.enabled)', so it almost certainly needs to be called as social toggles from enabled to disabled, but I didn't notice a call for that case.  I might have missed it though.

> 
> > ::: toolkit/components/social/SocialService.jsm
> > @@ +502,5 @@
> > >      // workerURL, sidebarURL is required and must be same-origin
> > >      // iconURL and name are required
> > >      // iconURL may be a different origin (CDN or data url support) if this is
> > >      // a whitelisted or directory listed provider
> > > +    let providerHasFeatures = [url for (url of sameOriginRequired) if (data[url])].length > 0;
> > 
> > isn't this going to fail for old providers?
> 
> older providers had workerURL and sidebarURL, so it should work fine.

Yeah, sorry, I thought things in sameOriginRequired were considered compulsory, but I see now they are not.

Comment 21

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #20)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> 
> > > how hard is it to have our observers give the signals we need?  Or
> > > alternatvely, of dropping some of our observers that can be made redundant
> > > by the Addon manager notifications?  I'm not that keen on a kindof mismatch
> > > of different ways to handle various aspects.
> > 
> > I looked into using our observers first, but this was a more trivial
> > approach.  I think there is room for refactoring some of the
> > notification/observer/listener stuff we have when we hit the refactoring
> > patches.
> 
> Can you elaborate a little on this?  ie, I can't imagine our existing
> observers are far from satisfying this use-case and mixing up observers in
> this way makes me uneasy as it gets much harder to track what happens when.

install/uninstall are not handled in the social observers or the listener callbacks. I considered adding those however.... 

For provider-added/removed, we'd have to import SocialService and use the listener interface, and there are issues with provider-removed (e.g. the removed provider is gone by the time the listener is called, as well, it doesn't actually let you know which provider was removed, you get a list of the remaining providers).  provider-set and providers-changed observers are not sufficient for handling any of the four events we need to deal with.


> > > @@ +1522,5 @@
> > > > +
> > > > +  // should be called on startup of each window, otherwise the addon manager
> > > > +  // listener will handle new activations, or enable/disabling of a provider
> > > > +  // XXX we currently call more regularily, will fix during refactoring
> > > > +  populatePalette: function() {
> > > 
> > > does this need to be called as social is disabled?
> > 
> > are you saying if or when social is disabled?  this should never be called
> > if social is disabled.  it is called when social is enabled/disabled since
> > set-provider is called.
> 
> I'm not with you here - you first say it's never called if social is
> disabled, but then say it is?

it is not called if social is in a disabled state.  it should be called when there is a transition between disabled and enabled or the provider list changes.

Comment 22

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

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

> install/uninstall are not handled in the social observers or the listener
> callbacks

providers-changed conveys much of this.  I'm not bothered by this being (relatively) inefficient (ie, by needing to do a little more work due to not knowing exactly what the change was)

> For provider-added/removed, we'd have to import SocialService and use the
> listener interface,

Can't we just have SocialService have its addon listener send whatever social-specific notifications we actually need?

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #22)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> 
> > install/uninstall are not handled in the social observers or the listener
> > callbacks
> 
> providers-changed conveys much of this.  I'm not bothered by this being
> (relatively) inefficient (ie, by needing to do a little more work due to not
> knowing exactly what the change was)

providers-changed is not enough for install/uninstall.  A disabled provider being uninstalled will not trigger providers-changed, or provider-added/removed.  The onInstalled callback from the addon manager happens prior to the provider being enabled, which is a great time to add the button ids to currentset for new installs, something we also cannot currently get from the other mechanisms.

> > For provider-added/removed, we'd have to import SocialService and use the
> > listener interface,
> 
> Can't we just have SocialService have its addon listener send whatever
> social-specific notifications we actually need?

We could go that route, but then we're just adding a layer of indirection.  I'll do a little investigation on it.

Comment 24

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #23)
> (In reply to Mark Hammond (:markh) from comment #22)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > 
> > > install/uninstall are not handled in the social observers or the listener
> > > callbacks
> > 
> > providers-changed conveys much of this.  I'm not bothered by this being
> > (relatively) inefficient (ie, by needing to do a little more work due to not
> > knowing exactly what the change was)
> 
> providers-changed is not enough for install/uninstall.  A disabled provider
> being uninstalled will not trigger providers-changed, or
> provider-added/removed.  The onInstalled callback from the addon manager
> happens prior to the provider being enabled, which is a great time to add
> the button ids to currentset for new installs, something we also cannot
> currently get from the other mechanisms.

Aren't we going to want to do the button dance on a provider being disabled/enabled too?

> > > For provider-added/removed, we'd have to import SocialService and use the
> > > listener interface,
> > 
> > Can't we just have SocialService have its addon listener send whatever
> > social-specific notifications we actually need?
> 
> We could go that route, but then we're just adding a layer of indirection. 

It's a layer of abstraction.  Tightly coupling to the addon manager implementation will make someone hate us a few years down the track :)

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

This version adds new notifications for install/uninstall/enable/disable and moves to using that rather than the addon manager listener.  It also disables the status buttons if allowMultipleProviders == false.

Comment 26

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

Comment on attachment 793114 [details] [diff] [review]
statusbutton

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

IIRC, some tests are using the "addons" listener and they could now use these new listeners?  If so, I think they should also change to the new notifications (partly to keep things clean, and partly to test the new ones)

::: browser/base/content/browser-social.js
@@ +81,5 @@
>  
>    observe: function SocialUI_observe(subject, topic, data) {
>      // Exceptions here sometimes don't get reported properly, report them
>      // manually :(
>      try {

I think it's probably worth a comment here saying something like "in all social:provider-* notifications, when |data| is supplied it is the origin of the affected provider"

@@ +1491,5 @@
> +  },
> +
> +  // should be called on startup of each window, otherwise the addon manager
> +  // listener will handle new activations, or enable/disabling of a provider
> +  // XXX we currently call more regularily, will fix during refactoring

typo in "regularly" - and are you planning on refactoring before requesting review or as a followup?

@@ +1523,5 @@
> +        let next = document.getElementById(pset[pi]);
> +        parent.insertItem(id, next, null, false);
> +      }
> +    }
> +  } 

trailing space

@@ +1738,5 @@
> +    sizeSocialPanelToContent(panel, aNotificationFrame);
> +  },
> +
> +};
> +

*lots* of new code, but I guess it's hard to avoid.  I haven't read this huge new block carefully yet though.

::: browser/modules/Social.jsm
@@ +170,5 @@
>      // Register an observer for changes to the provider list
>      SocialService.registerProviderListener(function providerListener(topic, data) {
>        // An engine change caused by adding/removing a provider should notify.
>        // any providers we receive are enabled in the AddonsManager
> +      if (["provider-enabled", "provider-disabled", "provider-installed", "provider-uninstalled"].indexOf(topic) >= 0) {

Would these if statements now be better as a switch/case?

::: toolkit/components/social/SocialService.jsm
@@ +406,5 @@
>      SocialServiceInternal.providers[provider.origin] = provider;
>      ActiveProviders.add(provider.origin);
>  
>      this.getOrderedProviderList(function (providers) {
> +      this._notifyProviderListeners("providers-changed", providers);

why this change? Isn't provider-added more accurate?

@@ +442,4 @@
>      }
>  
>      this.getOrderedProviderList(function (providers) {
> +      this._notifyProviderListeners("providers-changed", providers);

ditto here?

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #26)
> Comment on attachment 793114 [details] [diff] [review]
> statusbutton
> 
> Review of attachment 793114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> IIRC, some tests are using the "addons" listener and they could now use
> these new listeners?  If so, I think they should also change to the new
> notifications (partly to keep things clean, and partly to test the new ones)

browser_addons.js, the only place using an addon manager listener, is largely about testing integration with AddonManager.  We have to be sure that we are sending the notifications that AddonManager listeners wait on so other things (e.g. about:addons) update correctly.  As I recall, there was some nuance in getting those notifications right, so I'd like to keep the tests.

> @@ +1491,5 @@
> > +  },
> > +
> > +  // should be called on startup of each window, otherwise the addon manager
> > +  // listener will handle new activations, or enable/disabling of a provider
> > +  // XXX we currently call more regularily, will fix during refactoring
> 
> typo in "regularly" - and are you planning on refactoring before requesting
> review or as a followup?

as a followup.  I can change the comment.

> ::: browser/modules/Social.jsm
> @@ +170,5 @@
> >      // Register an observer for changes to the provider list
> >      SocialService.registerProviderListener(function providerListener(topic, data) {
> >        // An engine change caused by adding/removing a provider should notify.
> >        // any providers we receive are enabled in the AddonsManager
> > +      if (["provider-enabled", "provider-disabled", "provider-installed", "provider-uninstalled"].indexOf(topic) >= 0) {
> 
> Would these if statements now be better as a switch/case?

sure

> ::: toolkit/components/social/SocialService.jsm
> @@ +406,5 @@
> >      SocialServiceInternal.providers[provider.origin] = provider;
> >      ActiveProviders.add(provider.origin);
> >  
> >      this.getOrderedProviderList(function (providers) {
> > +      this._notifyProviderListeners("providers-changed", providers);
> 
> why this change? Isn't provider-added more accurate?

provider-enabled/disabled are more accurate than provider-added/removed (I also want to change the function names later).  provider-added/removed didn't indicate a provider but rather a list of providers, and was used in one specific place to update the provider cache and any multiprovider UI.  The combination of changes here are now accurate.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

slightly better provider notifications.  tests added.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

small fix in populatePalette.

Comment 30

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

Comment on attachment 794255 [details] [diff] [review]
statusbutton

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

Looks good, but I wouldn't mind another look once comments are addressed, mainly at the browser-social and test changes

::: browser/base/content/browser-social.js
@@ +1080,5 @@
> +    // If the provider uses the new SocialStatus button, then they do not get
> +    // to use the ambient icons in the old toolbar button.  Since the status
> +    // button depends on multiple workers, if not enabled we will ignore this
> +    // limitation.
> +    if (Social.provider.statusURL && Social.allowMultipleWorkers)

I thought we'd expect providers to not supply both ambient notifications and a statusURL, so I'm not sure why this block depends on allowMultipleWorkers?

@@ +1421,5 @@
>  }
>  
> +// this helper class is used by removable/customizable buttons to handle
> +// location persistence and insertion into palette and/or toolbars
> +function ToolbarHelper(type, createButtonFn) {

can we get some more detail in comments here about the general principle of how the toolbars work?

eg, something like:

When a provider is installed, we allocate an ID for its button but don't actually make it visible.  This button is then available in the "customize" panel, so the user can move it blah blah.

This is managed via a "palette" attribute on the toolbox.  This palette is responsible for managing the customization (ie, whether a button actually appears, and if so, where), so all we need to do it allocate the ID and tell the palette about it.

(or something - obviously I just made that up and probably have it wrong - which is why I think we need some note for people who aren't yet familiar with the new world order)

@@ +1439,5 @@
> +      return button;
> +    let palette = document.getElementById("navigator-toolbox").palette;
> +    let paletteItem = palette.firstChild;
> +    while (paletteItem) {
> +      if (paletteItem.id == id) {

remove braces here - this file doesn't have braces for single-line ifs

@@ +1481,5 @@
> +
> +  // if social is entirely disabled, we need to clear the palette, but leave
> +  // the persisted id's in place
> +  clearPalette: function() {
> +    for (let provider of Social.providers) {

single line for == no braces

@@ +1488,5 @@
> +  },
> +
> +  // should be called on startup of each window, otherwise the addon manager
> +  // listener will handle new activations, or enable/disabling of a provider
> +  // XXX we currently call more regularily, will fix during refactoring

typo - regularly

@@ +1498,5 @@
> +    let persisted = document.querySelectorAll("*[currentset]");
> +    let persistedById = {};
> +    for (let pent of persisted) {
> +      let pset = pent.getAttribute("currentset").split(',');
> +      for (let id of pset) {

braces

@@ +1523,5 @@
> +}
> +
> +SocialStatus = {
> +  populateToolbarPalette: function() {
> +    if (!Social.allowMultipleWorkers)

can't we just avoid creating the object if this is false?  Having almost every function guarded by this condition seems wrong.

@@ +1561,5 @@
> +    return this._dynamicResizer;
> +  },
> +
> +  _createButton: function(provider) {
> +    if (!provider.statusURL)

similarly to the above, can't we just avoid making this object for the provider if they don't support a statusURL?

@@ +1606,5 @@
> +          "src": provider.statusURL
> +        }
> +      );
> +
> +      if (frame.socialErrorListener) {

braces

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

can we reformat this line?  eg, put all the "null" params on their own line or use a temp for the string?

::: browser/base/content/test/social/browser_social_status.js
@@ +50,5 @@
> +    let activationURL = manifest2.origin + "/browser/browser/base/content/test/social/social_activate.html"
> +    addTab(activationURL, function(tab) {
> +      let doc = tab.linkedBrowser.contentDocument;
> +      Social.installProvider(doc, manifest2, function(addonManifest) {
> +          // at this point, we should have a button id in the currentset for our provider

can we add another check what when 'manifest' is used there is *no* button id for the provider?

@@ +51,5 @@
> +    addTab(activationURL, function(tab) {
> +      let doc = tab.linkedBrowser.contentDocument;
> +      Social.installProvider(doc, manifest2, function(addonManifest) {
> +          // at this point, we should have a button id in the currentset for our provider
> +          let id = "social-status-button-"+manifest2.origin;

spaces around operator

@@ +70,5 @@
> +  testButtonOnEnable: function(next) {
> +    // enable the provider now
> +    SocialService.addBuiltinProvider(manifest2.origin, function(provider) {
> +      is(Social.providers[1], provider, "provider is installed");
> +      let id = "social-status-button-"+manifest2.origin;

spaces around operator

@@ +87,5 @@
> +    let id = "social-status-button-" + provider.origin;
> +    let btn = document.getElementById(id)
> +    ok(btn, "got a status button");
> +    let port = provider.getWorkerPort();
> +    ok(btn, "got a port");

I think this should be: ok(port, "got a port");  (although it could possibly just be removed - if port is null, the error caused by "port.onmessage" would probably make things obvious)

::: toolkit/components/social/SocialService.jsm
@@ +477,5 @@
>    unregisterProviderListener: function unregisterProviderListener(listener) {
>      this._providerListeners.delete(listener);
>    },
>  
> +  _notifyProviderListeners: function (topic, origin, providers) {

I think this change is good, but browser_addons uses this notification and accepts only 2 params (but uses neither IIUC).  For the sake of completeness, I think the new param should be added there or the existing params removed completely.  browser_blocklist.js also uses this but takes no params, which I think is ok.

Comment 31

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #30)
> Comment on attachment 794255 [details] [diff] [review]
> statusbutton
> 
> Review of attachment 794255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I wouldn't mind another look once comments are addressed,
> mainly at the browser-social and test changes
> 
> ::: browser/base/content/browser-social.js
> @@ +1080,5 @@
> > +    // If the provider uses the new SocialStatus button, then they do not get
> > +    // to use the ambient icons in the old toolbar button.  Since the status
> > +    // button depends on multiple workers, if not enabled we will ignore this
> > +    // limitation.
> > +    if (Social.provider.statusURL && Social.allowMultipleWorkers)
> 
> I thought we'd expect providers to not supply both ambient notifications and
> a statusURL, so I'm not sure why this block depends on allowMultipleWorkers?

This is useful for "upgrading".  e.g. the demo provider changes (that I have not pushed to git yet) has both.  Without multiple workers (necessary for the individual status buttons) the "old" button is still used.  Once you turn on multiple workers, the "new" button is used.

> @@ +1523,5 @@
> > +}
> > +
> > +SocialStatus = {
> > +  populateToolbarPalette: function() {
> > +    if (!Social.allowMultipleWorkers)
> 
> can't we just avoid creating the object if this is false?  Having almost
> every function guarded by this condition seems wrong.

It is a static object that handles the buttons for all providers, we're not creating an object per provider.  I could avoid creating the object if allowMultipleWorkers is false, but then I'd need to check for its existence everyplace we call into it.  Once we can remove allowMultipleWorkers these checks become unnecessary.

> @@ +1561,5 @@
> > +    return this._dynamicResizer;
> > +  },
> > +
> > +  _createButton: function(provider) {
> > +    if (!provider.statusURL)
> 
> similarly to the above, can't we just avoid making this object for the
> provider if they don't support a statusURL?

The toolbarhelper class calls into this.  The helper is generic (will work with the new marks button in bug 891219).  It is simpler to have this callback do the check than configure the toolbarhelper to understand what configuration elements any given button might need.

> >  
> > +  _notifyProviderListeners: function (topic, origin, providers) {
> 
> I think this change is good, but browser_addons uses this notification and
> accepts only 2 params (but uses neither IIUC).  For the sake of
> completeness, I think the new param should be added there or the existing
> params removed completely.  browser_blocklist.js also uses this but takes no
> params, which I think is ok.

Both should be checking the topic, so I can just add the params.

Comment 32

[Shane Caraveo (:mixedpuppy)]

> ::: browser/base/content/test/social/browser_social_status.js
> @@ +50,5 @@
> > +    let activationURL = manifest2.origin + "/browser/browser/base/content/test/social/social_activate.html"
> > +    addTab(activationURL, function(tab) {
> > +      let doc = tab.linkedBrowser.contentDocument;
> > +      Social.installProvider(doc, manifest2, function(addonManifest) {
> > +          // at this point, we should have a button id in the currentset for our provider
> 
> can we add another check what when 'manifest' is used there is *no* button
> id for the provider?

good catch, this pointed to a flaw where an id was always being added even if the provider did not have a statusURL.  It did mean that I had to change the provider listener arguments to pass the manifest.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

Comment 34

[Shane Caraveo (:mixedpuppy)]

BTW, some modifications in attachment 794845 [details] [diff] [review] are also in the patch I'm testing to fix bug 847124.  e.g. the uninstall callback

Comment 35

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

Comment on attachment 794845 [details] [diff] [review]
statusbutton

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

Looking good, but as discussed on IRC, there's some rework of the notifications necessary.

::: browser/base/content/browser-social.js
@@ +84,5 @@
>      // manually :(
>      try {
>        switch (topic) {
> +        case "social:provider-installed":
> +          // some of our tests bypass install, thus we have no manifest

I don't think these notifications should ever be called without an indication of what the provider is.  We should guarantee the origin is supplied and expose a method to fetch a manifest given the origin (which I thought we already had?)

@@ +1085,5 @@
> +
> +    // If the provider uses the new SocialStatus button, then they do not get
> +    // to use the ambient icons in the old toolbar button.  Since the status
> +    // button depends on multiple workers, if not enabled we will ignore this
> +    // limitation.

please update this comment to reflect the fact this is temporary, and only for providers who supply both in order to work both now and after multi-providers are fully enabled.

@@ +1554,5 @@
> +    tbh.setPersistentPosition(tbh.idFromOrgin(manifest.origin));
> +  },
> +
> +  removePosition: function(manifest) {
> +    if (!manifest.statusURL || !Social.allowMultipleWorkers)

I don't thing these manifest checks should be done on uninstall, as removePersistence will still do the right thing if it was never previously added.  Ditto all "remove" functions.

@@ +1709,5 @@
> +    });
> +
> +    panel.addEventListener("popupshown", function onpopupshown() {
> +      panel.removeEventListener("popupshown", onpopupshown);
> +      // This attribute is needed on both the button and the

just to make sure this doesn't get stale/misleading over time, s/This attribute/The "open" attribute/

::: browser/base/content/test/social/browser_addons.js
@@ -306,2 @@
>              gBrowser.removeTab(tab);
> -            next();

hrm - this was removed rather than moved?  Sounds strange.

::: toolkit/components/social/SocialService.jsm
@@ +587,5 @@
>          aAddon.userDisabled = false;
>        }
>        schedule(function () {
> +        this._installProvider(aDOMDocument, data, (aManifest) => {
> +          this._notifyProviderListeners("provider-installed", aManifest);

I think we want to go back to 'origin' if we can't absolutely guarantee a non-null manifest for the origin.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

this patch is now on top of the patch in bug 891219

feedback should all be addressed now

Comment 37

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

Comment on attachment 795655 [details] [diff] [review]
statusbutton

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

Looks good - thanks!

::: browser/base/content/test/social/browser_blocklist.js
@@ +163,2 @@
>            SocialService.getProvider(provider.origin, function(p) {
> +            ok(p==null, "blocklisted provider disabled");

spaces around operator (I missed that last time, sorry!)

::: browser/base/content/test/social/browser_social_status.js
@@ +95,5 @@
> +          let id = "social-status-button-" + manifest2.origin;
> +          let toolbar = document.getElementById("nav-bar");
> +
> +          waitForCondition(function() {
> +                              let currentset = toolbar.getAttribute("currentset").split(',');

3 space indentation should be replaced with 2 (and a few lines down too)

@@ +146,5 @@
> +            port.postMessage({topic: "test-ambient-notification", data: icon});
> +            port.close();
> +            waitForCondition(function() { return btn.getAttribute("badge"); },
> +                       function() {
> +                        is(btn.style.listStyleImage, "url(\"" + icon.iconURL + "\")", "notification icon updated");

ditto here (only 1 space currently!)

::: toolkit/components/social/SocialService.jsm
@@ +586,5 @@
>          aAddon.cancelUninstall();
>          aAddon.userDisabled = false;
>        }
>        schedule(function () {
> +        this._installProvider(aDOMDocument, data, (aManifest) => {

no params around aManifest needed here.

@@ +674,5 @@
>      if (ActiveProviders.has(manifest.origin)) {
>        let provider = new SocialProvider(manifest);
>        SocialServiceInternal.providers[provider.origin] = provider;
>        // update the cache and ui, reload provider if necessary
> +      this.getOrderedProviderList((providers) => {

ditto here re parens

Comment 38

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

patch with review feedback

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

Comment 39

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

fixed persisting of the currentSet for the toolbar, which caused oranges in previous try

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

Comment 40

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

Comment on attachment 798106 [details] [diff] [review]
statusbutton

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

I just had a look at the interdiff and it lgtm

Comment 41

[Shane Caraveo (:mixedpuppy)]

Attachment: statusbutton

testing toolbar.currentSet was a little more tricky, but getting greens now.

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

Comment 42

[Shane Caraveo (:mixedpuppy)]

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

Comment 43

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

https://hg.mozilla.org/mozilla-central/rev/58a67d0f50bc

Comment 44

[Asa Dotzler [:asa]]

Shane, can you help out with a release note here? Is this something like "Firefox Social API now supports multiple active and visible providers" or something like that?

Comment 45

[Shane Caraveo (:mixedpuppy)]

(In reply to Asa Dotzler [:asa] from comment #44)
> Shane, can you help out with a release note here? Is this something like
> "Firefox Social API now supports multiple active and visible providers" or
> something like that?

As a note, the status button will not be available until we pref on multiple workers (bug 906839).  In general there are a few features enabled by that:

- each provider can send notifications/alerts to their toolbar button
- each provider can have a toolbar button that will present a page with notifications or other interesting content from that provider
- you can chat on more than one provider at a time

I think the wording you have above is fine but doesn't capture the nuances, this is wordy, but more how I think:

"You can now run more than one service at a time with Firefox SocialAPI, allowing you to receive notifications, chat and more with all your favorite services."