Bug 14997 – Support for server-sent DOM events

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Henry Mason]

As currently specified in HTML 5's section 6.2, WebKit should support the mechanism for "Server-sent DOM events". This includes:

* A parser for the "application/x-dom-event-stream" stream format
* A mechanism for creating and dispatching Events after parsing is completed
* Possibly a set of test cases for this functionality?

[Henry Mason]

Created attachment 16003 [details]
Patch which implements server-sent DOM events

My proposed implementation for this feature.

[Matt Lilek]

Is this something that should go on feature-branch?

[Sam Weinig]

First of all, awesome patch! I am only going to comment on one part of the patch for now.

Regarding the changes to:

    WebCore/bindings/scripts/CodeGeneratorJS.pm
    WebCore/bindings/js/JSDocumentCustom.h - (new file)
    WebCore/bindings/js/JSDocumentCustom.cpp

    +Document* toDocument(KJS::JSValue* val)

A custom implementation of this function is not necessary.  You can add the [GenerateNativeConverter] extended attribute to Document.idl for the same code.

[Sam Weinig]

Some other bindings related issues.

In WebCore/html/HTMLEventSourceElement.idl

+module html {
+    interface [GenerateConstructor] HTMLEventSourceElement : HTMLElement {
+        attribute DOMString src;
+    };
+}

Please put spaces before and after the interface.  Also, if you are going to use the [GenerateConstructor] extended attribute (and therefore generate a JS constructor) please also add the constructor to DOMWindow.idl.  The same goes for the [GenerateConstructor] in WebCore/dom/MessageEvent.idl

[Adam Roben (:aroben)]

Comment on attachment 16003 [details]
Patch which implements server-sent DOM events

It's exciting to see an implementation of this part of HTML5!

Thanks for adding the new files to all 3 build systems!

Your patch has a mixture of LGPL and BSD licenses for the new files. Would you be willing to make them all BSD?

-				EnablePREfast="$(EnablePREfast)"

Please remove this change from your patch in the 3 places it occurs.

+static HashSet<String> *NonBubblingEventTypes()
+static HashSet<String> *NonCancelableEventTypes()

These functions should return a const HashSet<String>&. Their names should have a lowercase first letter. Any *s in this patch should be next to the type name, not the variable name.

+DOMEventStreamLoader::DOMEventStreamLoader(EventTargetNode *target, KURL& url)

The second parameter should be a const KURL&.

+void DOMEventStreamLoader::open()
+{
+    if (!m_isOpen) {

Please reverse the condition of this if and make it an early return. This avoids needless indentation.

+        RefPtr<SubresourceLoader> loader = SubresourceLoader::create(m_target->ownerDocument()->frame(), this, request, true, true);
+        m_loader = loader.release();

There's no need for the local loader variable. You can assign straight into m_loader.

+void DOMEventStreamLoader::close()
+{
+    if (m_isOpen) {

Please make this an early return.

+void DOMEventStreamLoader::handleEvent(Event* event, bool isWindowEvent)

Please remove the unused parameter name (release builds will break otherwise). You also don't need braces around the body of the if.

+RefPtr<EventTargetNode> DOMEventStreamLoader::getNodeFromFieldValue(String& fieldValue)

The return type should be a PassRefPtr (see <http://webkit.org/coding/RefPtr.html> for an explanation of the uses of RefPtr vs. PassRefPtr). The parameter should be a const String&.

+    if (fieldValue.isEmpty())
+        return 0;
+    else if (fieldValue[0] == '#')
+        return m_target->ownerDocument()->getElementById(fieldValue.substring(1, fieldValue.length()-1));
+    else if (fieldValue == "Document")
+        return m_target->ownerDocument();
+    else
+        return 0;

You can remove all instances of the else keyword from these lines, since they all follow return statements. You also don't need to pass a second argument to String::substring (the default argument will do the same thing for you).

+Event* DOMEventStreamLoader::createEventFromFields(HashMap<String, String>& fields)

The return type should be a PassRefPtr<Event>. The parameter should be a const HashMap<String, String>&.

+    String eventType = fields.contains("Event") ? fields.get("Event") : "message";

+    bool bubbles, cancelable;

Please declare these on separate lines (preferably right above the if-else where each is initialized).

+    Event *event;
+    PassRefPtr<WebCore::Event> eventRefPtr;

There's no need for two variables here. You should just use a single RefPtr<Event> event. (If event is not a RefPtr, you will crash!)

+        else if (eventType == messageEvent)
+            event = new MessageEvent;

There's no need for this branch (the else below will accomplish the same thing).

+    String data = fields.contains("data") ? fields.get("data") : "";
+    int detail = fields.contains("detail") ? fields.get("detail").toInt() : 0;
+    bool ctrlKey = fields.contains("ctrlKey") ? (fields.get("ctrlKey") != "false") : false;

You should make helper functions like getStringFieldValue, getIntFieldValue, getBooleanFieldValue instead of repeating these ternary operators over and over.

+    EventTargetNode* relatedTarget;
+    if (fields.contains("relatedTarget")) {
+        String relatedTargetStr = fields.get("relatedTarget");
+        RefPtr<EventTargetNode> relatedTargetRef = getNodeFromFieldValue(relatedTargetStr);
+        relatedTarget = relatedTargetRef ? relatedTargetRef.get() : 0;
+    } else 
+        relatedTarget = 0;

relatedTarget should be a RefPtr. Once you change that you won't need the relatedTargetRef local variable. You also don't need the relatedTargetStr local variable. If you initialize relatedTarget to 0 you won't need the else at all. These comments also apply to the relatedNode block below it.

+    // TODO: These fields should be used in cross-domain messaging when it is implemented

Should be a FIXME (we don't use TODO generally).

+    Document* source(0);

We would normally use = 0 instead of the constructor syntax here.

+        (static_cast<UIEvent*>(event))->initUIEvent(eventType, bubbles, cancelable, view, detail);

Don't need the parentheses around the static_cast.

+        (static_cast<SVGZoomEvent*>(event))->setNewScale(newScale);
+        (static_cast<SVGZoomEvent*>(event))->setPreviousScale(previousScale);

I think a local variable is preferable to casting twice.

+void DOMEventStreamLoader::willSendRequest(SubresourceLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse)

Please remove the unused request parameter name.

+    if (loader != m_loader) return;

Put the return on the next line. (This happens elsewhere as well)

+        /* FIXME:
+           There is a rare unhandled case here. If we get the first byte of a CRLF ("\r\n")
+           new line in one data chunk, when we get the second chunk with an '\n' byte at the
+           start, we will wrongly parse an empty event. 
+        */

Please use C++ style comments.

+        int newLinePos, nextParseStartPos;

+        } else {
+            // No newline chars, need more data!
+            break;
+        }

I think it would be good to do:

if (lfPos < 0 && crPos < 0)
   break;

right after the calls to find(). Then this else won't be needed.

+            RefPtr<EventTargetNode> eventTarget;
+            
+            if (fields.contains("Target")) {
+                String target = fields.get("Target");
+                RefPtr<EventTargetNode> otherTarget = getNodeFromFieldValue(target);
+                eventTarget = otherTarget ? otherTarget : m_target;
+            } else
+                eventTarget = m_target;

Would be clearer/simpler as:

RefPtr<EventTargetNode> eventTarget = 0;
           
if (fields.contains("Target"))
    eventTarget = getNodeFromFieldValue(fields.get("Target"));

if (!eventTarget)
    eventTarget = m_target;

+            Event* event = createEventFromFields(fields);

event needs to be a RefPtr.

+            // Dispatch completed (or not), clear the fields and start parsing the next event

I don't think this comment adds any information the code doesn't already say.

+        } else if ((m_unparsedData[parseStartPos] != ';') && (m_unparsedData[parseStartPos] != ':')) {

You can reverse this and turn it into an early continue.

+            String fieldName, fieldContent;

Please declare these on separate lines.

+                fields.set(fieldName, (fields.get(fieldName) + String("\n") + fieldContent));

It seems a shame to keep appending all these strings as we go along. There must be a more efficient way to do this.

+    m_reloadTimer.startOneShot(12.0);

The 12.0 constant should go in a static const double at the top of the file.

+#include "EventListener.h"
+#include "EventTargetNode.h"
+#include "KURL.h"
+#include "SubresourceLoaderClient.h"
+#include "TextResourceDecoder.h"
+#include "Timer.h"
+
+#ifndef DOMEventStreamLoader_h
+#define DOMEventStreamLoader_h

The header guards should go above the #includes. You don't need to #include  EventTargetNode.h, you can just forward declare EventTargetNode.

+    class DOMEventStreamLoader : public EventListener, private SubresourceLoaderClient {

"private" is not needed.

+        DOMEventStreamLoader(EventTargetNode *target, KURL& url);

Please remove the parameter names as they don't add any information beyond the types.

+        void setTarget(PassRefPtr<EventTargetNode> target) { m_target = target; }

The parameter type should be an EventTargetNode*.

+        KURL *URL() { return &m_url; }

This should be:

const KURL& url() { return m_url; }

+} // namespace

Please name the namespace here.

+void EventTargetNode::addEventSource(String& src)
+{    
+    
+    if (m_eventSources.contains(src)) {

You've got an extra newline above the if. The parameter should be a const String& (ditto for removeEventSource).

+    if (m_eventSources.contains(src)) {

Please reverse this and make it an early return.

 #include "EventTarget.h"
+#include "RemoteEventTarget.h"
 #include "Node.h"
+#include "DOMEventStreamLoader.h"

Please keep the #includes lexically sorted. You should also be able to forward-declare DOMEventStreamLoader instead of #including the header.

+ * This file is part of the DOM implementation for KDE.

Please remove this line from any new files.

+ * Copyright (C) 2001 Peter Kelly ([e-mail])
+ * Copyright (C) 2001 Tobias Anton ([e-mail])
+ * Copyright (C) 2006 Samuel Weinig ([e-mail])
+ * Copyright (C) 2007 Henry Mason ([e-mail])
+ * Copyright (C) 2003, 2005, 2006 Apple Computer, Inc.

I don't think all these old copyrights apply to your new MessageEvent.cpp file.

+    protected:
+        
+
+    private:    

The protected: is not needed since there's nothing in it.

+        virtual void addEventSource(String& src);
+        virtual void removeEventSource(String& src);

Perhaps these should be pure virtual functions?

+    DOMEventStreamLoader *m_loader;

Please make this an OwnPtr<DOMEventStreamLoader>.

+    m_loader = new DOMEventStreamLoader((EventTargetNode*)this, kurl);

Use static_cast instead of a C-style case.

+
+#include "EventTargetNode.h"
+

I don't see why this is needed in HTTPParsers.h.

Index: LayoutTests/http/tests/domeventsource/event-source.html

It would be great to add tests of invalid values for the various fields defined in HTML5.

Index: LayoutTests/http/tests/domeventsource/remote-event-target-expected.txt

This should go inside a "resources" directory. Then run-webkit-tests will know that it's not a test itself and won't generate results for it.

\ No newline at end of file

Please add a newline.

r- so that the style can be cleaned up and the RefPtr problems can be addressed. I'm sure other people will have more comments on this patch as well.

[Henry Mason]

Created attachment 16038 [details]
Updated Patch

This patch addresses most of the issues brought up so far, along with some others:

- More sane use of RefPtr stuff
- Now supports adding an event source directly to a Document without crashing
- EventTargetNode now only stores a pointer to its internal URL->Stream Loader hash map, thus making nodes which don't need event sources require much less memory
- Refactored the field parsing code
- Proper handling of split CR+LF packets
- Switched to BSD license for all new code

Is there a fast way to concat strings? The + operator is what's used in KJS's array join() method...

[Anne van Kesteren]

http://tc.labs.opera.com/html/event-source/ and http://tc.labs.opera.com/html/parsing/event-source/ have tests.

[Adam Roben (:aroben)]

Comment on attachment 16038 [details]
Updated Patch

Sorry it's taken so long for me to get back to this patch. This is looking a lot nicer!

A couple more stylistic comments:

+static HashSet<String>& nonBubblingEventTypes()
+static HashSet<String>& nonCancelableEventTypes()

The return types should be const.

+DOMEventStreamLoader::DOMEventStreamLoader(EventTargetNode *target, const KURL& url)

The * should be next to EventTargetNode, not target.

+PassRefPtr<EventTargetNode> DOMEventStreamLoader::getNodeFromFieldValue(const String& fieldValue)

I was wrong before in saying that this should return a PassRefPtr. Since we're not transferring ownership, the correct return type is EventTargetNode* (as you had it originally).

+        /* Not supported in WebCore yet....
+        else if (eventType == DOMElementNamedChangedEvent || eventType == DOMAttributeNameChangeEvent)
+            event = new MutationNameEvent; */

We don't like to have commented-out code checked in to our tree. I think it would be more appropriate to have a FIXME comment here linking to a bug about supporting these event types.

+    return event;

(at the end of createEventFromFields) To be slightly more efficient, you can do:

return event.releaseRef();

+    if (loader != m_loader) return;
+    if (len < 1) return;

Please put the return on its own line, here and elsewhere.

+        const KURL& URL() { return m_url; }

In my previous comment I forgot to say that this should be a const method.

+    private:
+    
+        String getStringFieldValue(const HashMap<String, String>&, const String&, const String& defaultValue = "");
+        int getIntFieldValue(const HashMap<String, String>&, const String&, const int defaultValue = 0);
+        unsigned getUnsignedFieldValue(const HashMap<String, String>&, const String&, const unsigned defaultValue = 0);
+        unsigned short getUnsignedShortFieldValue(const HashMap<String, String>&, const String&, const unsigned short defaultValue = 0);
+        bool getBoolFieldValue(const HashMap<String, String>&, const String&, const bool defaultValue = false);
+        float getFloatFieldValue(const HashMap<String, String>&, const String&, const float defaultValue = 0.0);
+        EventTargetNode* getEventTargetNodeFieldValue(const HashMap<String, String>&, const String&, EventTargetNode* defaultValue = 0);
+        
+        PassRefPtr<EventTargetNode> getNodeFromFieldValue(const String& fieldValue);
+        PassRefPtr<Event> createEventFromFields(const HashMap<String, String>& fields);

I think these can all be static helper functions in the .cpp file, rather than being private instance methods.

+    HashMap<String, EventSourceEntry>* m_eventSources;

You could put this in an OwnPtr if you want so that you can avoid having to remember to delete it.

+#include <wtf/Forward.h>

(in RemoteEventTarget.h) I don't think this include is necessary.

I don't think this is quite ready to go in yet, but we're making nice progress!

[Adam Roben (:aroben)]

(In reply to comment #6)
> Is there a fast way to concat strings? The + operator is what's used in KJS's
> array join() method...

One method we sometimes use is to create a Vector<UChar> and do all of the appending work on that. Then, once we're done appending, we create a String from Vector<UChar>::data().

[Adam Roben (:aroben)]

(In reply to comment #8)
> +    return event;
> 
> (at the end of createEventFromFields) To be slightly more efficient, you can
> do:
> 
> return event.releaseRef();

Sorry, that should have been:

return event.release();

[Adam Roben (:aroben)]

(In reply to comment #9)
> Then, once we're done appending, we create a String from Vector<UChar>::data().

What I really meant was that we pass the Vector into String::adopt().

[sideshowbarker]

URL should be updated to http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#server-sent-events

Note that descript ion for this issue is stale in that MIME type is now text/event-stream -
http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#parsing-an-event-stream

[Adam Bergkvist]

Created attachment 29828 [details]
patch

Here's an implementation that is up to date with the current spec.

[Adam Bergkvist]

Created attachment 30050 [details]
proposed patch

Updated patch since the previous one no longer applies.

[Alexey Proskuryakov]

Is there is a consensus about server-sent DOM events between browser vendors and other parties? I remember it raised concerns about potentially breaking NATs by using more connections per client machine on average, but I do not know how serious this concern is.

[Adam Bergkvist]

(In reply to comment #15)

I'm aware of these concerns. This initial implementation is meant to promote further discussions in the WHATWG, W3C, and IETF.

[sideshowbarker]

(In reply to comment #16)
> This initial implementation is meant to promote
> further discussions in the WHATWG, W3C, and IETF.

Adam, (if you don't know this already) as far as promoting it for further discussion in the W3C, the place to do that would probably best be the [e-mail] mailing list. http://lists.w3.org/Archives/Public/public-webapps/

[Eric Seidel (no email)]

Comment on attachment 30050 [details]
proposed patch

We need to add test cases for this.  They can also be disabled/skipped by default of course.  The http tests should make this possible.

[Eric Seidel (no email)]

(In reply to comment #18)
> (From update of attachment 30050 [details] [review])
> We need to add test cases for this.  They can also be disabled/skipped by
> default of course.  The http tests should make this possible.

Sorry, my comment may not apply.  It was mostly drive-by.  Reading more of the history this appears to be more of a speculative, framework implementation.  It will still of course need testing, but maybe you already have that planned...  Still could you some explanation about testing in the ChangeLogs.  In fact, the ChangeLog in general needs some lovin... like a link to the spec and this bug at least.

[Maciej Stachowiak]

Comment on attachment 30050 [details]
proposed patch

Thanks for the patch! It will be good to finally have this feature. A few things need to be addressed first:

1) I think it would be ok to enable this by default for testing, unless there is some critical problem with it.

2) It would be nice if constructors (real ones like this, not fake ones like Element) could be autogenerated. I guess that is not a new issue with your patch, so you don't have to fix this right away, just noticing.

3) might be good to file a follow-up bug on this: // FIXME: should support cross-origin requests

4) EventSource::parseEventStream looks a little large for a single function. Is there a way to break it up into smaller functions for understandability?

5) A couple of times the code removes UChars from the beginning of the m_receiveBuf vector:  m_receiveBuf.remove(0, removeLen);

Removing from the start of a Vector is very inefficient. It copies almost all the contents of the vector every time. I would consider using a different data structure here. One possibility is to keep each incoming chunk of text as a separate Vector or String, and just track where you are in the current segment, and drop it once the whole thing has been processed. Another possibility is to use a Deque, which allows for relatively efficient prepend or remove from start.

6) This patch needs regression tests. I think it would be good to add http tests including both normal Window versions and Worker versions. Tests should ideally cover both normal event delivery, and handling of malformed EventSource streams, with particular attention to edge cases.

The tests are the most critical piece of feedback here. We definitely need regression tests to cover this new feature.

r- for test cases and other feedback. Please resubmit once these issues have been addressed.

[Adam Bergkvist]

Created attachment 30778 [details]
updated patch

(In reply to comment #20)

Thanks for your feedback.

> 1) I think it would be ok to enable this by default for testing, unless there
> is some critical problem with it.
Done. Note that I have only updated the GTK build system since it is the platform I am testing on.

> 2) It would be nice if constructors (real ones like this, not fake ones like
> Element) could be autogenerated. I guess that is not a new issue with your
> patch, so you don't have to fix this right away, just noticing.
I am not sure how to automatically generate constructors that take arguments. This could perhaps be fixed later.

> 3) might be good to file a follow-up bug on this: // FIXME: should support
> cross-origin requests
Sure, will do that as soon as this patch is landed.

> 4) EventSource::parseEventStream looks a little large for a single function. Is
> there a way to break it up into smaller functions for understandability?
Line parsing in EventSource::parseEventStream has been extracted to a separate function (EventSource::parseEventStreamLine).

> 5) A couple of times the code removes UChars from the beginning of the
> m_receiveBuf vector:  m_receiveBuf.remove(0, removeLen);
> 
> Removing from the start of a Vector is very inefficient. It copies almost all
> the contents of the vector every time. I would consider using a different data
> structure here. One possibility is to keep each incoming chunk of text as a
> separate Vector or String, and just track where you are in the current segment,
> and drop it once the whole thing has been processed. Another possibility is to
> use a Deque, which allows for relatively efficient prepend or remove from
> start.
Removing from the beginning of the Vector is now reduced to being done when all lines in one chunk has been parsed.

> 6) This patch needs regression tests. I think it would be good to add http
> tests including both normal Window versions and Worker versions. Tests should
> ideally cover both normal event delivery, and handling of malformed EventSource
> streams, with particular attention to edge cases.
Added various tests.

[Michael Nordman]

This is a nice feature :) Some drive-by-comments.

About the implementation, I was under the impression that the intent was to be able to reuse the same http connection to a given event source for multiple pages. With the goal being fewer distinct hanging GETs lingering around. It looks like this patch makes a distinct request for each instance of an EventSource in all cases. Is that right? If so, maybe a FIXME (and maybe not turn it on by default quite yet?)

[Maciej Stachowiak]

(In reply to comment #22)
> This is a nice feature :) Some drive-by-comments.
> 
> About the implementation, I was under the impression that the intent was to be
> able to reuse the same http connection to a given event source for multiple
> pages. With the goal being fewer distinct hanging GETs lingering around. It
> looks like this patch makes a distinct request for each instance of an
> EventSource in all cases. Is that right? If so, maybe a FIXME (and maybe not
> turn it on by default quite yet?)
> 

I don't see anything like that in the spec: <http://www.w3.org/TR/eventsource/>. The event-stream format has no support for multiplexing or suggestion that it's possible, as far as I can tell.

[Michael Nordman]

You're right. I'm not sure why I was under that impression? Although, now I have a greater appreciation of the concerns alluded to earlier about exceeding connection limits :)

I very much like that this feature formalizes support for the commonly performed 'hanging get', and alleviates some of the webapp busy work involved to do so.

> This initial implementation is meant to promote
> further discussions in the WHATWG, W3C, and IETF.

Maybe when that discussion continues, ways to reduce the cost of long lived connections in some use cases can be a part of it.

[Adam Bergkvist]

Created attachment 31160 [details]
updated patch

Updated patch to reflect recent changes to ThreadableLoader.

Also replaced remove() with clear() when removing all elements from the m_receiveBuf vector.

[Sam Weinig]

Comment on attachment 31160 [details]
updated patch


> +static JSObject* constructEventSource(ExecState* exec, JSObject* constructor, const ArgList& args)
> +{
> +    if (args.size() == 0)
> +        return throwError(exec, SyntaxError, "Not enough arguments");

Out approach is ussually to just all the the toString conversion go to work on the undefined value instead of checking the number of args here. Is this called out for specifically in the spec?

One thing we should definitely look into doing is writing a fuzzer for this new parser.  Otherwise, this looks great, thanks for keeping it up to date and hanging in there for this ridiculous long time. r=me.

[Adam Barth]

Will land.

[Adam Barth]

Comment on attachment 31160 [details]
updated patch

I tried to land this, but the patch did not apply cleanly to a top-of-tree checkout:

1 out of 3 hunks FAILED -- saving rejects to file configure.ac.rej

I haven't looked whether the merge is trivial.  Can you post an updated patch that applies cleanly?  Thanks.

[Alexey Proskuryakov]

I think EventSource code should call nonCacheRequestInFlight(), similarly to how XMLHttpRequest does it. If it does not, other connections may stall indefinitely with some network back-ends.

[Alexey Proskuryakov]

+        void disconnect();

I believe this doesn't match the current spec - disconnect has been renamed to close. I do not know if there were more spec changes in the meanwhile.

[Adam Bergkvist]

Created attachment 34139 [details]
updated patch

Updated patch to reflect recent changes in the spec as well as in WebKit.

Fixes:
* Renamed disconnect to close according to spec changes (and updated tests)
* Added connection limit counting fix (nonCacheRequestInFlight/Complete)
* Updated calls to set/unsetPendingActivity according to new garbage collection rules in the spec
* Added EventSource to the Qt build system

Since r45558 events are delayed until 512 bytes have been received with the Soup backend due to content sniffing always being enabled (see https://bugs.webkit.org/show_bug.cgi?id=27981).

[Sam Weinig]

Comment on attachment 34139 [details]
updated patch

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 46804)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,12 @@
> [phone]  Adam Bergkvist  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] Added EventSource to the build (default on).
> +        https://bugs.webkit.org/show_bug.cgi?id=14997
> +
> +        * configure.ac:
> +
>  2009-08-05  Jan Michael Alonzo  <[e-mail]>
>  
>          Reviewed by Xan Lopez.
> Index: configure.ac
> ===================================================================
> --- configure.ac	(revision 46804)
> +++ configure.ac	(working copy)
> @@ -348,6 +348,14 @@ AC_ARG_ENABLE(database,
>                [],[enable_database="yes"])
>  AC_MSG_RESULT([$enable_database])
>  
> +# check whether to build with server-sent events support
> +AC_MSG_CHECKING([whether to enable HTML5 server-sent events support])
> +AC_ARG_ENABLE(eventsource,
> +              AC_HELP_STRING([--enable-eventsource],
> +                             [enable HTML5 server-sent events support [default=yes]]),
> +              [],[enable_eventsource="yes"])
> +AC_MSG_RESULT([$enable_eventsource])
> +
>  # check whether to build with icon database support
>  AC_MSG_CHECKING([whether to enable icon database support])
>  AC_ARG_ENABLE(icon_database,
> @@ -702,6 +710,7 @@ AM_CONDITIONAL([ENABLE_JAVASCRIPT_DEBUGG
>  AM_CONDITIONAL([ENABLE_OFFLINE_WEB_APPLICATIONS],[test "$enable_offline_web_applications" = "yes"])
>  AM_CONDITIONAL([ENABLE_DOM_STORAGE],[test "$enable_dom_storage" = "yes"])
>  AM_CONDITIONAL([ENABLE_DATABASE],[test "$enable_database" = "yes"])
> +AM_CONDITIONAL([ENABLE_EVENTSOURCE],[test "$enable_eventsource" = "yes"])
>  AM_CONDITIONAL([ENABLE_ICONDATABASE],[test "$enable_icon_database" = "yes"])
>  AM_CONDITIONAL([ENABLE_XPATH],[test "$enable_xpath" = "yes"])
>  AM_CONDITIONAL([ENABLE_XSLT],[test "$enable_xslt" = "yes"])
> @@ -758,6 +767,7 @@ Features:
>   HTML5 client-side session and persistent storage support : $enable_dom_storage
>   HTML5 client-side database storage support               : $enable_database
>   HTML5 ruby support                                       : $enable_ruby
> + HTML5 server-sent events support                         : $enable_eventsource
>   HTML5 video element support                              : $enable_video
>   Icon database support                                    : $enable_icon_database
>   SharedWorkers support                                    : $enable_shared_workers
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 46804)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,87 @@
> [phone]  Adam Bergkvist  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added implementation of the EventSource object that enables
> +        server-sent events from HTML5.
> +        http://dev.w3.org/html5/eventsource/
> +        https://bugs.webkit.org/show_bug.cgi?id=14997
> +
> +        Tests: fast/eventsource/eventsource-attribute-listeners.html
> +               fast/eventsource/eventsource-constructor.html
> +               http/tests/eventsource/eventsource-bad-mime-type.html
> +               http/tests/eventsource/eventsource-parse-event-stream.html
> +               http/tests/eventsource/eventsource-reconnect.html
> +               http/tests/eventsource/eventsource-status-code-states.html
> +               http/tests/eventsource/workers/eventsource-simple.html
> +
> +        * GNUmakefile.am:
> +        * WebCore.pro:
> +        * bindings/js/JSDOMWindowCustom.cpp:
> +        (WebCore::JSDOMWindow::eventSource):
> +        * bindings/js/JSEventSourceConstructor.cpp: Added.
> +        (WebCore::):
> +        (WebCore::JSEventSourceConstructor::JSEventSourceConstructor):
> +        (WebCore::JSEventSourceConstructor::scriptExecutionContext):
> +        (WebCore::constructEventSource):
> +        (WebCore::JSEventSourceConstructor::getConstructData):
> +        (WebCore::JSEventSourceConstructor::mark):
> +        * bindings/js/JSEventSourceConstructor.h: Added.
> +        (WebCore::JSEventSourceConstructor::classInfo):
> +        * bindings/js/JSEventSourceCustom.cpp: Added.
> +        (WebCore::JSEventSource::mark):
> +        (WebCore::JSEventSource::addEventListener):
> +        (WebCore::JSEventSource::removeEventListener):
> +        * bindings/js/JSEventTarget.cpp:
> +        (WebCore::toJS):
> +        (WebCore::toEventTarget):
> +        * bindings/js/JSWorkerContextCustom.cpp:
> +        (WebCore::JSWorkerContext::eventSource):
> +        * dom/EventNames.h:
> +        * dom/EventTarget.cpp:
> +        (WebCore::EventTarget::toEventSource):
> +        * dom/EventTarget.h:
> +        * page/DOMWindow.idl:
> +        * page/EventSource.cpp: Added.
> +        (WebCore::EventSource::EventSource):
> +        (WebCore::EventSource::~EventSource):
> +        (WebCore::EventSource::connect):
> +        (WebCore::EventSource::endRequest):
> +        (WebCore::EventSource::scheduleReconnect):
> +        (WebCore::EventSource::reconnectTimerFired):
> +        (WebCore::EventSource::url):
> +        (WebCore::EventSource::readyState):
> +        (WebCore::EventSource::close):
> +        (WebCore::EventSource::scriptExecutionContext):
> +        (WebCore::EventSource::addEventListener):
> +        (WebCore::EventSource::removeEventListener):
> +        (WebCore::EventSource::dispatchEvent):
> +        (WebCore::EventSource::didReceiveResponse):
> +        (WebCore::EventSource::didReceiveData):
> +        (WebCore::EventSource::didFinishLoading):
> +        (WebCore::EventSource::didFail):
> +        (WebCore::EventSource::didFailRedirectCheck):
> +        (WebCore::EventSource::parseEventStream):
> +        (WebCore::EventSource::parseEventStreamLine):
> +        (WebCore::EventSource::dispatchGenericEvent):
> +        (WebCore::EventSource::dispatchMessageEvent):
> +        (WebCore::EventSource::stop):
> +        * page/EventSource.h: Added.
> +        (WebCore::EventSource::create):
> +        (WebCore::EventSource::):
> +        (WebCore::EventSource::setOnopen):
> +        (WebCore::EventSource::onopen):
> +        (WebCore::EventSource::setOnmessage):
> +        (WebCore::EventSource::onmessage):
> +        (WebCore::EventSource::setOnerror):
> +        (WebCore::EventSource::onerror):
> +        (WebCore::EventSource::toEventSource):
> +        (WebCore::EventSource::eventListeners):
> +        (WebCore::EventSource::refEventTarget):
> +        (WebCore::EventSource::derefEventTarget):
> +        * page/EventSource.idl: Added.
> +        * workers/WorkerContext.idl:
> +
>  2009-08-05  Andras Becsi  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> Index: WebCore/GNUmakefile.am
> ===================================================================
> --- WebCore/GNUmakefile.am	(revision 46804)
> +++ WebCore/GNUmakefile.am	(working copy)
> @@ -237,6 +237,7 @@ IDL_BINDINGS += \
>  	WebCore/page/Coordinates.idl \
>  	WebCore/page/DOMSelection.idl \
>  	WebCore/page/DOMWindow.idl \
> +	WebCore/page/EventSource.idl \
>  	WebCore/page/Geolocation.idl \
>  	WebCore/page/Geoposition.idl \
>  	WebCore/page/History.idl \
> @@ -339,6 +340,9 @@ webcore_sources += \
>  	WebCore/bindings/js/JSEventCustom.cpp \
>  	WebCore/bindings/js/JSEventListener.cpp \
>  	WebCore/bindings/js/JSEventListener.h \
> +	WebCore/bindings/js/JSEventSourceConstructor.cpp \
> +	WebCore/bindings/js/JSEventSourceConstructor.h \
> +	WebCore/bindings/js/JSEventSourceCustom.cpp \
>  	WebCore/bindings/js/JSEventTarget.cpp \
>  	WebCore/bindings/js/JSEventTarget.h \
>  	WebCore/bindings/js/JSGeolocationCustom.cpp \
> @@ -1263,6 +1267,8 @@ webcore_sources += \
>  	WebCore/page/EditorClient.h \
>  	WebCore/page/EventHandler.cpp \
>  	WebCore/page/EventHandler.h \
> +	WebCore/page/EventSource.cpp \
> +	WebCore/page/EventSource.h \
>  	WebCore/page/FocusController.cpp \
>  	WebCore/page/FocusController.h \
>  	WebCore/page/FocusDirection.h \
> @@ -2114,6 +2120,20 @@ webcore_cppflags += -DENABLE_DATABASE=0
>  endif # END ENABLE_DATABASE
>  
>  # ----
> +# HTML5 server-sent events
> +# ----
> +if !ENABLE_EVENTSOURCE
> +global_cppflags += -DENABLE_EVENTSOURCE=0
> +endif
> +
> +if ENABLE_EVENTSOURCE
> +FEATURE_DEFINES_JAVASCRIPT += ENABLE_EVENTSOURCE=1
> +
> +webcore_cppflags += \
> +	-DENABLE_EVENTSOURCE=1
> +endif # ENABLE_EVENTSOURCE
> +
> +# ----
>  # HTML5 client-side session and persistent storage
>  # ----
>  if ENABLE_DOM_STORAGE
> Index: WebCore/WebCore.pro
> ===================================================================
> --- WebCore/WebCore.pro	(revision 46804)
> +++ WebCore/WebCore.pro	(working copy)
> @@ -106,6 +106,7 @@ contains(DEFINES, ENABLE_SINGLE_THREADED
>  
>  !contains(DEFINES, ENABLE_JAVASCRIPT_DEBUGGER=.): DEFINES += ENABLE_JAVASCRIPT_DEBUGGER=1
>  !contains(DEFINES, ENABLE_DATABASE=.): DEFINES += ENABLE_DATABASE=1
> +!contains(DEFINES, ENABLE_EVENTSOURCE=.): DEFINES += ENABLE_EVENTSOURCE=1
>  !contains(DEFINES, ENABLE_OFFLINE_WEB_APPLICATIONS=.): DEFINES += ENABLE_OFFLINE_WEB_APPLICATIONS=1
>  !contains(DEFINES, ENABLE_DOM_STORAGE=.): DEFINES += ENABLE_DOM_STORAGE=1
>  !contains(DEFINES, ENABLE_ICONDATABASE=.): DEFINES += ENABLE_ICONDATABASE=1
> @@ -425,6 +426,7 @@ IDL_BINDINGS += \
>      page/Coordinates.idl \
>      page/DOMSelection.idl \
>      page/DOMWindow.idl \
> +    page/EventSource.idl \
>      page/Geolocation.idl \
>      page/Geoposition.idl \
>      page/History.idl \
> @@ -486,6 +488,8 @@ SOURCES += \
>      bindings/js/JSDOMWindowShell.cpp \
>      bindings/js/JSElementCustom.cpp \
>      bindings/js/JSEventCustom.cpp \
> +    bindings/js/JSEventSourceConstructor.cpp \
> +    bindings/js/JSEventSourceCustom.cpp \
>      bindings/js/JSEventTarget.cpp \
>      bindings/js/JSGeolocationCustom.cpp \
>      bindings/js/JSHTMLAllCollection.cpp \
> @@ -916,6 +920,7 @@ SOURCES += \
>      page/NavigatorBase.cpp \
>      page/DragController.cpp \
>      page/EventHandler.cpp \
> +    page/EventSource.cpp \
>      page/FocusController.cpp \
>      page/Frame.cpp \
>      page/FrameTree.cpp \
> @@ -1162,6 +1167,7 @@ HEADERS += \
>      bindings/js/JSDOMWindowCustom.h \
>      bindings/js/JSDOMWindowShell.h \
>      bindings/js/JSEventListener.h \
> +    bindings/js/JSEventSourceConstructor.h \
>      bindings/js/JSEventTarget.h \
>      bindings/js/JSHistoryCustom.h \
>      bindings/js/JSHTMLAllCollection.h \
> @@ -1586,6 +1592,7 @@ HEADERS += \
>      page/DOMWindow.h \
>      page/DragController.h \
>      page/EventHandler.h \
> +    page/EventSource.h \
>      page/FocusController.h \
>      page/Frame.h \
>      page/FrameTree.h \
> @@ -2264,6 +2271,10 @@ contains(DEFINES, ENABLE_DATAGRID=1) {
>      FEATURE_DEFINES_JAVASCRIPT += ENABLE_DATAGRID=1
>  }
>  
> +contains(DEFINES, ENABLE_EVENTSOURCE=1) {
> +    FEATURE_DEFINES_JAVASCRIPT += ENABLE_EVENTSOURCE=1
> +}
> +
>  contains(DEFINES, ENABLE_SQLITE=1) {
>      !system-sqlite:exists( $${SQLITE3SRCDIR}/sqlite3.c ) {
>              # Build sqlite3 into WebCore from source
> Index: WebCore/bindings/js/JSDOMWindowCustom.cpp
> ===================================================================
> --- WebCore/bindings/js/JSDOMWindowCustom.cpp	(revision 46804)
> +++ WebCore/bindings/js/JSDOMWindowCustom.cpp	(working copy)
> @@ -38,6 +38,7 @@
>  #include "JSDOMWindowShell.h"
>  #include "JSEvent.h"
>  #include "JSEventListener.h"
> +#include "JSEventSourceConstructor.h"
>  #include "JSHTMLCollection.h"
>  #include "JSHistory.h"
>  #include "JSImageConstructor.h"
> @@ -424,6 +425,13 @@ JSValue JSDOMWindow::event(ExecState* ex
>      return toJS(exec, event);
>  }
>  
> +#if ENABLE(EVENTSOURCE)
> +JSValue JSDOMWindow::eventSource(ExecState* exec) const
> +{
> +    return getDOMConstructor<JSEventSourceConstructor>(exec, this);
> +}
> +#endif
> +
>  JSValue JSDOMWindow::image(ExecState* exec) const
>  {
>      return getDOMConstructor<JSImageConstructor>(exec, this);
> Index: WebCore/bindings/js/JSEventSourceConstructor.cpp
> ===================================================================
> --- WebCore/bindings/js/JSEventSourceConstructor.cpp	(revision 0)
> +++ WebCore/bindings/js/JSEventSourceConstructor.cpp	(revision 0)
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (C) 2009 Ericsson AB
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer
> + *    in the documentation and/or other materials provided with the
> + *    distribution.
> + * 3. Neither the name of Ericsson nor the names of its contributors
> + *    may be used to endorse or promote products derived from this
> + *    software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +
> +#if ENABLE(EVENTSOURCE)
> +
> +#include "JSEventSourceConstructor.h"
> +
> +#include "EventSource.h"
> +#include "ExceptionCode.h"
> +#include "JSEventSource.h"
> +#include "ScriptExecutionContext.h"
> +
> +using namespace JSC;
> +
> +namespace WebCore {
> +
> +ASSERT_CLASS_FITS_IN_CELL(JSEventSourceConstructor);
> +
> +const ClassInfo JSEventSourceConstructor::s_info = { "EventSourceContructor", 0, 0, 0 };
> +
> +JSEventSourceConstructor::JSEventSourceConstructor(ExecState* exec, JSDOMGlobalObject* globalObject)
> +    : DOMObject(JSEventSourceConstructor::createStructure(exec->lexicalGlobalObject()->objectPrototype()))

This should inherit from DOMConstructorObject

> +    , m_globalObject(globalObject)
> +{
> +    putDirect(exec->propertyNames().prototype, JSEventSourcePrototype::self(exec, exec->lexicalGlobalObject()), None);

This should use the passed in global object instead of the lexical global object.

> +    putDirect(exec->propertyNames().length, jsNumber(exec, 1), ReadOnly|DontDelete|DontEnum);
> +}
> +
> +ScriptExecutionContext* JSEventSourceConstructor::scriptExecutionContext() const
> +{
> +    return m_globalObject->scriptExecutionContext();
> +}
> +
> +static JSObject* constructEventSource(ExecState* exec, JSObject* constructor, const ArgList& args)
> +{
> +    if (args.size() == 0)
> +        return throwError(exec, SyntaxError, "Not enough arguments");
> +
> +    UString url = args.at(0).toString(exec);
> +    if (exec->hadException())
> +        return 0;
> +
> +    ScriptExecutionContext* context = static_cast<JSEventSourceConstructor*>(constructor)->scriptExecutionContext();
> +    if (!context)
> +        return throwError(exec, ReferenceError, "EventSource constructor associated document is unavailable");
> +
> +    ExceptionCode ec = 0;
> +    RefPtr<EventSource> eventSource = EventSource::create(url, context, ec);
> +    setDOMException(exec, ec);
> +
> +    return asObject(toJS(exec, eventSource.release()));
> +}
> +
> +ConstructType JSEventSourceConstructor::getConstructData(ConstructData& constructData)
> +{
> +    constructData.native.function = constructEventSource;
> +    return ConstructTypeHost;
> +}
> +
> +void JSEventSourceConstructor::mark()
> +{
> +    DOMObject::mark();
> +    if (!m_globalObject->marked())
> +        m_globalObject->mark();
> +}

This method will not be necessary if you inherit from DOMConstructorObject.


> +#if ENABLE(EVENTSOURCE)
> +    if (EventSource* eventSource = target->toEventSource())
> +        return getCachedDOMObjectWrapper(exec->globalData(), eventSource);

This should use toJS(exec, globalObject, eventSource);


In addition, the new files need to be added to all the project files (xcode, vcproj, etc.) and DerivedSources.make needs to be updated.

r-

[Adam Bergkvist]

Created attachment 34665 [details]
updated patch

Thank you for the quick review and sorry for not noticing the recent addition of the DOMConstructorObject class.
Here is an updated patch that also adds EventSource to the mac and win builds (untested).

[Eric Seidel (no email)]

Comment on attachment 34665 [details]
updated patch

Testing 11056 test cases.
fast/dom/prototype-inheritance-2.html -> failed
fast/dom/prototype-inheritance.html -> failed
fast/dom/Window/window-properties.html -> failed

This needs more tests updated.  Either the patch needs to be updated, or someone needs to do that when landing.

[Adam Bergkvist]

Created attachment 34732 [details]
updated patch

Updated expected results of the affected tests.

Index: LayoutTests/fast/dom/prototype-inheritance-2-expected.txt
===================================================================
--- LayoutTests/fast/dom/prototype-inheritance-2-expected.txt	(revision 47105)
+++ LayoutTests/fast/dom/prototype-inheritance-2-expected.txt	(working copy)
@@ -77,6 +77,7 @@ PASS DataGridColumnListConstructor from 
 PASS DataGridColumnListPrototype from inner.document.activeElement.childNodes.14.columns.__proto__
 PASS DocumentPrototype from inner.document.__proto__.__proto__
 PASS ElementPrototype from inner.document.activeElement.lastElementChild.__proto__.__proto__
+PASS EventSourceContructor from inner.EventSource
 FAIL Function from inner.document.location.pathname.constructor
 PASS HTMLAnchorElement from inner.document.activeElement.firstElementChild
 PASS HTMLAnchorElementConstructor from inner.document.activeElement.firstElementChild.constructor
Index: LayoutTests/fast/dom/prototype-inheritance-expected.txt
===================================================================
--- LayoutTests/fast/dom/prototype-inheritance-expected.txt	(revision 47105)
+++ LayoutTests/fast/dom/prototype-inheritance-expected.txt	(working copy)
@@ -77,6 +77,8 @@ PASS inner.Event.isInner is true
 PASS inner.Event.constructor.isInner is true
 PASS inner.EventException.isInner is true
 PASS inner.EventException.constructor.isInner is true
+PASS inner.EventSource.isInner is true
+PASS inner.EventSource.constructor.isInner is true
 PASS inner.File.isInner is true
 PASS inner.File.constructor.isInner is true
 PASS inner.FileList.isInner is true
Index: LayoutTests/fast/dom/Window/window-properties-expected.txt
===================================================================
--- LayoutTests/fast/dom/Window/window-properties-expected.txt	(revision 47105)
+++ LayoutTests/fast/dom/Window/window-properties-expected.txt	(working copy)
@@ -857,6 +857,15 @@ window.EventException [object EventExcep
 window.EventException.UNSPECIFIED_EVENT_TYPE_ERR [number]
 window.EventException.prototype [object EventExceptionPrototype]
 window.EventException.prototype.UNSPECIFIED_EVENT_TYPE_ERR [number]
+window.EventSource [object EventSourceContructor]
+window.EventSource.prototype [object EventSourcePrototype]
+window.EventSource.prototype.CLOSED [number]
+window.EventSource.prototype.CONNECTING [number]
+window.EventSource.prototype.OPEN [number]
+window.EventSource.prototype.addEventListener [function]
+window.EventSource.prototype.close [function]
+window.EventSource.prototype.dispatchEvent [function]
+window.EventSource.prototype.removeEventListener [function]
 window.File [object FileConstructor]
 window.File.prototype [object FilePrototype]
 window.FileList [object FileListConstructor]

[Sam Weinig]

I will land this.

[Sam Weinig]

Fix landed in r47323.

[Sam Weinig]

Landed missing GTK piece in r47324.