Closed Bug 872470 Opened 6 years ago Closed 6 years ago

add EventSource to frameworker

Categories

(Firefox Graveyard :: SocialAPI, defect)

22 Branch
x86
macOS
defect
Not set

Tracking

(firefox23 fixed, firefox24 fixed)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox23 --- fixed
firefox24 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch add eventsource to frameworker (obsolete) — Splinter Review
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/
Attachment #749816 - Flags: review?(felipc)
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?
feedback implemented.  anyone can review, it's simple.
Assignee: nobody → mixedpuppy
Attachment #749816 - Attachment is obsolete: true
Attachment #749816 - Flags: review?(felipc)
Attachment #750380 - Flags: review?(mhammond)
Attachment #750380 - Flags: review?(gavin.sharp)
Attachment #750380 - Flags: review?(felipc)
Are there any plans to add EventSource to real workers?
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'
Attachment #750380 - Flags: superreview?(gavin.sharp)
Attachment #750380 - Flags: review?(mhammond)
Attachment #750380 - Flags: review?(gavin.sharp)
Attachment #750380 - Flags: review?(felipc)
Attachment #750380 - Flags: review+
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?
Attachment #750380 - Flags: superreview?(gavin.sharp) → superreview+
bug 876498 added for supporting eventsource in real workers.
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
Attachment #750380 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3f37696472d4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Attachment #750380 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: in-testsuite+
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.