773530 - make additions to the workerAPI to support toolbar UI

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

The toolbar UI needs a bunch of stuff from the workerAPI to work properly, let's add that stuff!

Comment 1

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

Attachment: Shane's patch

This is Shane's patch, split out from bug 771826.

Comment 2

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

Attachment: Shane's patch

Oops, attached the wrong patch. I'm on a roll tonight :/

Comment 3

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

Comment on attachment 641755 [details] [diff] [review]
Shane's patch

>diff --git a/toolkit/components/social/SocialProvider.jsm b/toolkit/components/social/SocialProvider.jsm

> function SocialProvider(input, enabled) {

We should add an empty "profile" object here by default.

What are "ambientNotificationIcons"? Comments would be helpful.

>diff --git a/toolkit/components/social/WorkerAPI.jsm b/toolkit/components/social/WorkerAPI.jsm

>+    "social.ambient-notification-area": function (data) {

Can we get rid of these "backward compat" cases? It would be nice to not be constrained by the existing implementation right from the get go - it's only going to get worse once more and more providers are on board. "ambient-notification-area" isn't very descriptive or intuitive.

>+    "social.ambient-notification-update": function (data) {

>+          data.iconURL = /url\((['"]?)(.*)(\1)\)/.exec(data.background)[2];

Some explanation of this regex goop in a comment would be useful.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> Comment on attachment 641755 [details] [diff] [review]
> Shane's patch
> 
> >diff --git a/toolkit/components/social/SocialProvider.jsm b/toolkit/components/social/SocialProvider.jsm
> 
> > function SocialProvider(input, enabled) {
> 
> We should add an empty "profile" object here by default.
> 
> What are "ambientNotificationIcons"? Comments would be helpful.

They are the status icons in the toolbar button.

> >diff --git a/toolkit/components/social/WorkerAPI.jsm b/toolkit/components/social/WorkerAPI.jsm
> 
> >+    "social.ambient-notification-area": function (data) {
> 
> Can we get rid of these "backward compat" cases?

Preferably we can keep them around perhaps for the next month, until we can get our live provider fixed.

> 
> >+    "social.ambient-notification-update": function (data) {
> 
> >+          data.iconURL = /url\((['"]?)(.*)(\1)\)/.exec(data.background)[2];
> 
> Some explanation of this regex goop in a comment would be useful.

The original design was to take css from the provider and apply it to an html div (the ui was all mocked up in html).  When I rewrote it to xul, I switched to image, and wanted a real url.  The regex is grabbing the data url from the providers message, but it is deprecated in place of iconURL.

I would like to change these notification names as well since the implementation has changed from the original concept, but we need transition time for that as well.  Preferably we would have:

social.provider-data - basic provider data, primary button for the toolbar button, etc

social.user-profile - bulk of the social.ambient-notification-area message is really about the user-profile

social.ambient-notification - replacing social.ambient-notification-update

Comment 5

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

OK, we can file followups for those API changes. Let's add some comments to the patch where indicated and I think we can land this with r=me.

Comment 6

[Asa Dotzler [:asa]]

Is this something that needs to make it in for v1?

Comment 7

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

Yes, it blocks bug 771826.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: updated patch

updated patch with message name changes, part of the patch should be removed for landing (see comments in WorkerAPI.jsm)

Comment 9

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

Comment on attachment 642252 [details]
updated patch

>diff --git a/toolkit/components/social/SocialProvider.jsm b/toolkit/components/social/SocialProvider.jsm

>+  setAmbientNotification: function(notification) {
>+    this.ambientNotificationIcons[notification.name] = notification;
>+
>+    Services.obs.notifyObservers(null, "social:ambient-notification-changed", this.origin);
>+  },
>+
> }

Ultra-nit: remove that trailing comma, because Komodo warns about it :)

Nice cleanup, I like it.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: updated patch

removed trailing coma.  ready for you to commit to m-c

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed137f37a3c2

Comment 12

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

Comment on attachment 642289 [details] [diff] [review]
updated patch

I made a few tweaks (mostly adding rudimentary documentation) when pushing this to inbound.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed137f37a3c2