Closed Bug 809709 Opened 12 years ago Closed 12 years ago

multiprovider: activation

Categories

(Firefox Graveyard :: SocialAPI, defect)

18 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 809694

People

(Reporter: mixedpuppy, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch activation.patch (obsolete) — Splinter Review
This fixes the activation handler to deal with multiple providers.
Attachment #679485 - Flags: feedback?(mhammond)
Attachment #679485 - Flags: feedback?(jaws)
Keeping this separate since I think activation may need a bit more work
Depends on: 809694
No longer depends on: 809701
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)
Attachment #679485 - Flags: feedback?(mhammond) → feedback+
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).
Attachment #679485 - Flags: feedback?(jaws) → feedback-
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.
Attachment #679485 - Attachment is obsolete: true
Attachment #685958 - Flags: feedback?(mhammond)
Attachment #685958 - Flags: feedback?(gavin.sharp)
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?
Attachment #685958 - Flags: feedback?(mhammond) → feedback+
this patch has rolled up into bug 809694
Status: NEW → RESOLVED
Closed: 12 years ago
No longer depends on: 809694
Resolution: --- → DUPLICATE
Attachment #685958 - Flags: feedback?(gavin.sharp)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: