872470 - add EventSource to frameworker

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: add eventsource to frameworker

EventSource is a simple mechanism for handling server push events as dom events, and is defined to be a part of WorkerUtils per the W3C draft.  The WebRTC Apps group needs use of EventSource.

http://www.w3.org/TR/2009/WD-eventsource-20091222/

Comment 1

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

Comment on attachment 749816 [details] [diff] [review]
add eventsource to frameworker

>diff --git a/toolkit/components/social/test/browser/worker_eventsource.js b/toolkit/components/social/test/browser/worker_eventsource.js

>+function fn_onmessage(e) {
>+  fn_onmessage.msg_ok = true;
>+}

>+function doTest() {

>+  setTimeout(function() {
>+    ok(fn_onmessage.msg_ok, "eventsource onmessage test");
>+    ok(esListener.msg_ok, "listener ok");
>+    es.close();
>+    port.postMessage({topic: "pong"});
>+  }, 3000);

Why a 3 second timeout? Looks fishy. Can't fn_onmessage just end the test when it is called?

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: add eventsource to frameworker

feedback implemented.  anyone can review, it's simple.

Comment 3

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

Are there any plans to add EventSource to real workers?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 750380 [details] [diff] [review]
add eventsource to frameworker

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

This looks OK to me.  It seems EventSource and WebSockets are a logical pair, both have real use-cases inside workers, neither are in the worker spec as it currently stands but both are supported in workers for chrome.

So I'll take the easy way out - r+ as the impl and tests looks fine, but leave the final decision on *whether* it should be done to gavin :)  Shane, it might help if you indicated if you have a real and current use-case for this (eg, WebRTC or a provider)

::: toolkit/components/social/test/browser/browser_frameworker.js
@@ +635,5 @@
> +
> +  testEventSource: function(cbnext) {
> +    let worker = getFrameWorkerHandle("https://example.com/browser/toolkit/components/social/test/browser/worker_eventsource.js", undefined, "testEventSource");
> +    worker.port.onmessage = function(e) {
> +      let m = e.data;

even though 'm' is defined here, the block still uses e.data in a couple of places - it should use 'm'

Comment 5

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

Comment on attachment 750380 [details] [diff] [review]
add eventsource to frameworker

If I understand http://weblog.bocoup.com/javascript-creating-an-eventsource-within-a-worker/ correctly, Webkit/Chrome already support EventSource in workers.

From asking on IRC, seems like we don't. Can you file a bug on supporting it?

Comment 6

[Shane Caraveo (:mixedpuppy)]

bug 876498 added for supporting eventsource in real workers.

Comment 7

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=9e1e7039d68b

Comment 8

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f37696472d4

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 750380 [details] [diff] [review]
add eventsource to frameworker

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: feature needed for talkilla
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low if any, only affects social
String or IDL/UUID changes made by this patch: none

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f37696472d4

Comment 11

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c29e7c0e357

Comment 12

[u279076]

Looks like this landed with tests so I'm deprioritizing fix verification. If there's some testing needed please drop the [qa-] tag and add the verifyme keyword. Thank you.