Last Comment Bug 773530 - make additions to the workerAPI to support toolbar UI
: make additions to the workerAPI to support toolbar UI
Status: RESOLVED FIXED
[Fx16]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
:
Mentors:
Depends on: 776554
Blocks: 771826 774003
  Show dependency treegraph
 
Reported: 2012-07-12 19:16 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-07-23 08:40 PDT (History)
2 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Shane's patch (11.59 KB, patch)
2012-07-12 19:17 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
Shane's patch (11.29 KB, patch)
2012-07-12 23:37 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
updated patch (8.38 KB, text/plain)
2012-07-14 11:24 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
gavin.sharp: review+
Details
updated patch (9.00 KB, patch)
2012-07-14 16:36 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
gavin.sharp: review+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-12 19:16:47 PDT
The toolbar UI needs a bunch of stuff from the workerAPI to work properly, let's add that stuff!
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-12 19:17:26 PDT
Created attachment 641693 [details] [diff] [review]
Shane's patch

This is Shane's patch, split out from bug 771826.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-12 23:37:14 PDT
Created attachment 641755 [details] [diff] [review]
Shane's patch

Oops, attached the wrong patch. I'm on a roll tonight :/
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-12 23:45:32 PDT
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) [on leave 5/16-7/16] 2012-07-13 01:25:29 PDT
(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
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-13 01:31:57 PDT
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] 2012-07-13 11:02:13 PDT
Is this something that needs to make it in for v1?
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-13 11:03:14 PDT
Yes, it blocks bug 771826.
Comment 8 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-14 11:24:27 PDT
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)
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-14 13:10:33 PDT
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) [on leave 5/16-7/16] 2012-07-14 16:36:44 PDT
Created attachment 642289 [details] [diff] [review]
updated patch

removed trailing coma.  ready for you to commit to m-c
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-14 17:49:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed137f37a3c2
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-14 17:50:21 PDT
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] 2012-07-15 10:20:36 PDT
https://hg.mozilla.org/mozilla-central/rev/ed137f37a3c2

Note You need to log in before you can comment on or make changes to this bug.