Last Comment Bug 771877 - associate WorkerAPI object with SocialProvider for centralized management of the "Worker API" messages
: associate WorkerAPI object with SocialProvider for centralized management of ...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks: 765874 771826
  Show dependency treegraph
 
Reported: 2012-07-08 00:53 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-07-11 15:18 PDT (History)
3 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.86 KB, patch)
2012-07-08 01:39 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
mixedpuppy: review+
Details | Diff | Splinter Review
patch (8.03 KB, patch)
2012-07-08 18:27 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
jaws: review+
Details | Diff | Splinter Review
patch with comments addressed (9.51 KB, patch)
2012-07-09 00:22 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch with leak fix (9.83 KB, patch)
2012-07-09 17:50 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 00:53:35 PDT
Messages like social.initialize and social.ambient-notification-area need to be handled by the provider class itself, rather than left up to individual clients.

This is basically copying workerAPI.js from the add-on code.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 01:39:23 PDT
Created attachment 640033 [details] [diff] [review]
patch

This implements a basic provider.workerAPI object. In the future, we can add a link from the workerAPI to the provider object, so that it can expose properties (e.g. "username") via the provider. It might also make sense to expose some methods/properites on workerAPI itself - for now I just added "initialized", and invented a new "social.initialize.response" message which sets it, and which is the expected response to "social.initialize".
Comment 2 Shane Caraveo (:mixedpuppy) 2012-07-08 13:50:53 PDT
Comment on attachment 640033 [details] [diff] [review]
patch

+  terminate: function terminate() {

it might be nice to prefix the anon functions with something like SocialProvider_terminate or at least sp_terminate, but that's my own preference.


+WorkerAPI.prototype = {
+  _handleMessage: function (event) {

name the function?
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2012-07-08 15:33:03 PDT
Comment on attachment 640033 [details] [diff] [review]
patch

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

::: toolkit/components/social/WorkerAPI.jsm
@@ +14,5 @@
> +  if (!port)
> +    throw new Error("Can't initialize WorkerAPI with a null port");
> +
> +  this.port = port;
> +  this.port.onmessage = this._handleMessage.bind(this);

The new WorkerAPI.jsm now allows two different places to get a port from. These two ports are the same since each call to getFrameWorkerHandle increments the port number. Can you add an underscore on |port| so it is more obvious that it is internal?

@@ +40,5 @@
> +    }
> +  },
> +
> +  handlers: {
> +    "social.initialize.response": function (data) {

nit: the convention used by the API is to hyphenate the part following the first period, so this message name should be "social.initialize-response"

::: toolkit/components/social/test/browser/browser_workerAPI.js
@@ +25,5 @@
> +  port.onmessage = function onMessage(event) {
> +    let {topic, data} = event.data;
> +    if (topic == "test-initialization-complete") {
> +      is(provider.workerAPI.initialized, true, "workerAPI is now initialized");
> +      executeSoon(finish);

Why is the executeSoon here necessary?

@@ +28,5 @@
> +      is(provider.workerAPI.initialized, true, "workerAPI is now initialized");
> +      executeSoon(finish);
> +    }
> +  }
> +  port.postMessage({topic: "test-initialization"});

I'm not sure why the "test-initialization"/"test-initialization-complete" act here is necessary. As far as I can tell, initialization of the workerAPI doesn't happen because of this. It seems that this is merely here as a way to delay checking |provider.workerAPI.initialized|. Is this true? Should we just build something into WorkerAPI (maybe an event) so that code that interacts with it has a way of knowing when the WorkerAPI state is updated?

::: toolkit/components/social/test/browser/head.js
@@ +47,5 @@
>    runNextTest();
>  }
>  
> +function makeWorkerUrl(runner) {
> +  return "data:application/javascript," + encodeURI("let run=" + runner.toSource()) + ";run();"

Why is this function moved to head.js? I can only find usage of it within browser_frameworker.js. My guess is that it got moved to head.js while browser_workerAPI.js was being worked on but then the worker got split out to its own file.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 18:24:27 PDT
(In reply to Jared Wein [:jaws] from comment #3)
> nit: the convention used by the API is to hyphenate the part following the
> first period, so this message name should be "social.initialize-response"

Not sure there's much benefit to this convention.

> > +      executeSoon(finish);
> 
> Why is the executeSoon here necessary?

Just an old habit of mine, to let the stack unwind before moving on to the next test. This used to be necessary because the test harness didn't always do that, but I think it does now (since bug 528469, I think).

> It seems that this is merely here as a way
> to delay checking |provider.workerAPI.initialized|. Is this true?

Yep.

> Should we just build something into WorkerAPI (maybe an event) so that code that
> interacts with it has a way of knowing when the WorkerAPI state is updated?

I thought of that, but couldn't think of a model that made sense. We can certainly investigate in a followup.

> Why is this function moved to head.js?

Just a leftover cleanup from when I thought I'd use it in the other test.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 18:27:15 PDT
Created attachment 640117 [details] [diff] [review]
patch

Shane and Jared's comments addressed, and made one other tweak (check that getWorkerPort() is non-null rather than checking workerURL before creating workerAPI, since we'll probably want to make getWorkerPort() always return null when social functionality is disabled).
Comment 6 Shane Caraveo (:mixedpuppy) 2012-07-08 18:50:04 PDT
Comment on attachment 640117 [details] [diff] [review]
patch

Should terminate clear WorkerAPI from the SocialProvider class?  Not sure if the WorkerAPI needs a terminate to close the port.  Maybe function names for the anon functions in the workerapi handlers.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 21:28:52 PDT
(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Should terminate clear WorkerAPI from the SocialProvider class?  Not sure if
> the WorkerAPI needs a terminate to close the port.

I considered this, since the add-on module does it, but when looking into it further it seems unnecessary - terminating the worker terminates all of its ports, including the workerAPI's, so there's no need to close the workerAPI port explicitly. I guess this does bring up the fact that a provider needs to be re-usable after having been terminated, though, since I imagine we'll be using terminate() to shut down providers when you've toggled the global enable state, and that can be un-toggled.

I will file a followup for this.

> Maybe function names for the anon functions in the workerapi handlers.

Can't use "." in a function name, and seems unlikely that names would be useful for these, so I didn't bother with this.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-09 00:20:13 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> I will file a followup for this.

Bug 771980.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-09 00:22:08 PDT
Created attachment 640147 [details] [diff] [review]
patch with comments addressed

One more minor tweak for test bustage: had to make failure to create a FrameWorker non-fatal, since the Provider constructor now triggers the creation of one, and that fails in xpcshell where there is no hidden window.
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-09 00:23:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/034afcc038c9
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-09 17:50:59 PDT
Created attachment 640462 [details] [diff] [review]
patch with leak fix

The test needs to terminate its provider to avoid "leaking" a window until shutdown.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=223ca4f20f52
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-09 21:46:57 PDT
Take 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/4de086e95f6b
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-07-10 15:47:58 PDT
https://hg.mozilla.org/mozilla-central/rev/4de086e95f6b

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