809709 - multiprovider: activation

Status: RESOLVED DUPLICATE of bug 809694

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: activation.patch

This fixes the activation handler to deal with multiple providers.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Keeping this separate since I think activation may need a bit more work

Comment 2

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

Comment on attachment 679485 [details] [diff] [review]
activation.patch

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

Some trailing whitespace, but in general it looks fine - although as you mention, I suspect additional work might be needed around discovery (or alternatively, if we really are going to ship with all supported manifests builtin to Firefox, the whitelist seems unnecessary)

Comment 3

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

Comment on attachment 679485 [details] [diff] [review]
activation.patch

Why does this remove the event throttling?

The "undo" button no longer really "undoes" with this, right? If social was already active but with a different provider, it will be disabled after undo AFAICT. Also, nothing seems to re-set Social.provider to its previous value.

The callback also needs to null-check the provider, in case the whitelist doesn't match the set of built-in providers (though as Mark suggests, maybe the whitelist is not needed now).

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: activation.patch WIP

work on activation is still in progress, but I wanted to get this out for early feedback, you should be able to see the direction I'm taking it.

Comment 5

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

Comment on attachment 685958 [details] [diff] [review]
activation.patch WIP

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

::: 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;
> +  

whitespace

@@ +202,5 @@
> +      description.value = message;
> +  
> +      SocialUI.notificationPanel.hidden = false;
> +  
> +      setTimeout(function () {

Not directly related to this patch, but I wonder if it is worthwhile guarding against multiple copies of the popup being open?  Eg, imagine the same provider accidentally firing the event multiple times > 1 sec apart, and even 2 different providers both attempting to enable at the same time via different tabs.  Both are unlikely, but not impossible I guess...

@@ +216,5 @@
>    undoActivation: function SocialUI_undoActivation() {
> +    // restore the previous state of the socialapi
> +    Social.provider.active = false;
> +    Social.provider = this._previousProvider;
> +    Social.active = this._previouslyActivated;

I wonder if we also need some way of recording the user previously declined the activation?  Eg, imagine an annoying provider who attempted activation without prompting the user and the user doesn't want it.

As a strawman, the activation dialog could get a "never ask to activate this provider in the future", and if the user checks it, the provider gets "blacklisted".  Sadly that then leaves the question of how the user could remove it from the blacklist later...  I guess we can avoid this until we allow arbitrary providers to activate though.

Also, provider.active will need to be persisted somewhere - I guess that's coming in a later version?

Comment 6

[Shane Caraveo (:mixedpuppy)]

this patch has rolled up into bug 809694