make additions to the workerAPI to support toolbar UI

RESOLVED FIXED in Firefox 16

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 3 obsolete attachments)

The toolbar UI needs a bunch of stuff from the workerAPI to work properly, let's add that stuff!
Created attachment 641693 [details] [diff] [review]
Shane's patch

This is Shane's patch, split out from bug 771826.
Created attachment 641755 [details] [diff] [review]
Shane's patch

Oops, attached the wrong patch. I'm on a roll tonight :/
Attachment #641693 - Attachment is obsolete: true
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.
Assignee: nobody → mixedpuppy
(Assignee)

Comment 4

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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
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

5 years ago
Is this something that needs to make it in for v1?
Yes, it blocks bug 771826.

Updated

5 years ago
Whiteboard: [Fx16]
(Assignee)

Comment 8

5 years ago
Created attachment 642252 [details]
updated patch

updated patch with message name changes, part of the patch should be removed for landing (see comments in WorkerAPI.jsm)
Attachment #641755 - Attachment is obsolete: true
Attachment #642252 - Flags: review?(gavin.sharp)
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.
Attachment #642252 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 642289 [details] [diff] [review]
updated patch

removed trailing coma.  ready for you to commit to m-c
Attachment #642252 - Attachment is obsolete: true
Attachment #642289 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Blocks: 774003
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed137f37a3c2
Flags: in-testsuite+
Target Milestone: --- → Firefox 16
Comment on attachment 642289 [details] [diff] [review]
updated patch

I made a few tweaks (mostly adding rudimentary documentation) when pushing this to inbound.
Attachment #642289 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/ed137f37a3c2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #642289 - Attachment is patch: true

Updated

5 years ago
Depends on: 776554
You need to log in before you can comment on or make changes to this bug.