Bug 61862

Summary: EventSource should support CORS
Product: WebKit Reporter: goberman
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: abarth, adam.bergkvist, allan.jardine, ap, christoph.kaser, darren, dglazkov, donggwan.kim, d-r, evan, ian, japhet, levin+threading, mifenton, mike, ojan.autocc, ojan, per-erik.brodin, pf, rakuco, rwlbuis, sonny.piers, syoichi, tonikitoo, webkit.review.bot, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 65694    
Bug Blocks: 70112    
Attachments:
Description Flags
proposed patch
abarth: review-
updated patch
none
patch 3
none
patch 4
ap: review-, webkit.review.bot: commit-queue-
patch 5
ap: review-
patch 6
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
patch 7
none
patch 8 none

Description goberman 2011-06-01 08:13:38 PDT
I set "Access-Control-Allow-Origin" header to "*" in server response to enable cross-domain requests. 
This works for XHR, but does not work for EventSource. Can EventSource be enhanced to support cross-domain calls.

SERVER

package test;

import java.io.IOException;
import java.io.PrintWriter;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class WebFrameworkServletEventSource extends HttpServlet {

	public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
		System.out.println("Begin for session: " + request.getSession(true).getId() + " " + response.getWriter());
		
		response.setHeader("pragma", "no-cache,no-store");
		response.setHeader("cache-control", "no-cache,no-store,max-age=0,max-stale=0");
		response.setHeader("Connection", "close");
	    response.setHeader("Access-Control-Allow-Origin", "*");
		
		response.setContentType("text/event-stream");
		
		PrintWriter out = response.getWriter();
		
		int messagesSent = 0;
		while (true) {
			out.print("data: ");
			out.print("{\"m\":8448,\"r\":0,\"e\":1,\"s\":[\"BAC.\",0],\"f\":[[0,6,3993,2],[55,6,1185,2],[54,6,3218,2],[5,6,6617,2],[52,4,15],[12,6,1700,2]]}");
			out.print("\n\n");
			out.flush();
			
			messagesSent++;
			
			try {
				Thread.sleep(2000);
			} catch (InterruptedException e1) {
			}
		}
	}
}

CLIENT
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/1999/REC-html401-19991224/loose.dtd">
<html>
<head>
    <title>test</title>
</head>
<body>
    <input type="button" value="TEST" onclick="test(); return false;" />

    <script type="text/javascript">
        var domElement;
        var messagesProcessed = 0;
        var eventSrc;

        function test() {
            domElement = document.getElementsByTagName('body')[0];

            debug('connect');

            try {
                eventSrc = new EventSource('http://cross-domain-address/Test/Controller');
            }
            catch (e) {
                debug('exception');
            }

            eventSrc.onopen = function (event) {
                debug('open ' + event.type);
            };

            eventSrc.onerror = function (event) {
                debug('error ' + event.type);
                eventSrc.close();
            };

            eventSrc.onmessage = function (event) {
                //debug(event.type + ' ' + event.data);
            };
        }
        
        function debug(message) {
            var oText = document.createTextNode(message);
            var oDiv = document.createElement('div');
            oDiv.appendChild(oText);

            domElement.insertBefore(oDiv, domElement.hasChildNodes() ? domElement.childNodes[0] : null);
        }
    </script>

</body>
</html>
Comment 1 Alexey Proskuryakov 2011-06-01 08:40:22 PDT
EventSource specification doesn't allow this yet, and almost certainly won't until version 2. This seems to be a feature requested by many developers.
Comment 2 goberman 2011-06-01 08:49:29 PDT
I would appreciate if you keep this enhancement request open. I have not seen any other formal requests for this.
Thanks
Comment 3 goberman 2011-06-01 10:49:23 PDT
Alexey, sorry to bug you with this, but do you know where I can find road map for the EventSource 2 release? Also, I looked at http://www.w3.org/TR/2011/WD-eventsource-20110310/ draft and I do not see cross domain calls be a part of it. Do you know if they have a list of formal requests?
Thanks
Comment 4 Ian 'Hixie' Hickson 2011-06-01 10:54:04 PDT
I expect I'll update EventSource to support CORS sometime this year. If it's particularly urgent, please e-mail the WHATWG list with your use case.

The main reason it's not done already is that I'm waiting for implementation feedback on the various other parts of the platform that use CORS — primarily XMLHttpRequest, but also now <img src> and <video src>/<audio src>/<source>/<track>. I don't want to add too many dependencies on CORS before we are confident it works.
Comment 5 goberman 2011-06-01 10:57:04 PDT
OK, it is fair. But how would I know when it is implemented? Is there some request I can subscribe to to receive notification of progress for this? My concern is that it will be implemented and I would not know anything about it. 
Or you will update this request with progress...
Thanks
Comment 6 Ian 'Hixie' Hickson 2011-06-01 11:00:35 PDT
I expect this bug will be updated when it's implemented in WebKit. I don't know of a good way to keep track of it for all browsers. Maybe you should suggest to http://caniuse.com/ that there should be a way to subscribe for updates or something. :-) Let me know if you do find something.
Comment 7 goberman 2011-06-01 11:25:27 PDT
OK, sent request to whatwg@lists.whatwg.org
Comment 8 goberman 2011-06-02 07:32:10 PDT
Got a response from Jonas Sicking:
I absolutely think we should do this. I was in fact quite surprised to
find that this wasn't already the case.
 
Getting event streams from other sites seems like a prime use case for
EventSource.
Comment 9 goberman 2011-06-10 09:30:38 PDT
It looks like a large number of people want to have it happen, but people responsible for the EventSource draft document (whoever they are) have not updated it to reflect it. A similar request was sent to WHATWG half a year ago.
I am not sure what would be the best way to keep moving it forward. I am out of ideas personally.
Thanks
Comment 10 Ian 'Hixie' Hickson 2011-06-10 12:09:54 PDT
The only reason the spec doesn't have this yet is that I'm rolling out CORS support in the WHATWG spec slowly over time, so as to collect implementation experience and not make a bunch of mistakes we can't fix. I recently added CORS support for <img> and <video>; once we have implementation experience on that I'll move on to adding it to one of the other features that could use it, such as EventSource or Workers.
Comment 11 goberman 2011-06-13 13:18:33 PDT
I appreciate it. Thanks
Comment 12 Per-Erik Brodin 2011-06-19 14:37:46 PDT
Created attachment 97728 [details]
proposed patch

Had to make a few assumptions. Feedback sent to WHATWG:  http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-June/032158.html
Comment 13 Adam Barth 2011-06-20 01:52:30 PDT
Comment on attachment 97728 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97728&action=review

> Source/WebCore/loader/CrossOriginAccessControl.cpp:52
>      if (equalIgnoringCase(name, "accept")
>          || equalIgnoringCase(name, "accept-language")
> +        || equalIgnoringCase(name, "cache-control")
>          || equalIgnoringCase(name, "content-language")
> +        || equalIgnoringCase(name, "last-event-id")
>          || equalIgnoringCase(name, "origin"))
>          return true;

This list comes from the CORS spec.  It's not clear we should change it unilaterally.

> Source/WebCore/page/EventSource.cpp:71
> -    , m_origin(context->securityOrigin()->toString())
> +    , m_origin(SecurityOrigin::createFromString(url)->toString())

I don't understand this part of the change.  Why don't the origin come from the context?
Comment 14 Adam Barth 2011-06-20 01:52:48 PDT
*** Bug 62949 has been marked as a duplicate of this bug. ***
Comment 15 Adam Barth 2011-06-20 01:53:52 PDT
Comment on attachment 97728 [details]
proposed patch

Marking R- because I'd like those questions answered.  If you think the current patch is correct, please feel free to re-nominate.
Comment 16 Per-Erik Brodin 2011-06-20 03:13:11 PDT
(In reply to comment #13)
> (From update of attachment 97728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97728&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:52
> >      if (equalIgnoringCase(name, "accept")
> >          || equalIgnoringCase(name, "accept-language")
> > +        || equalIgnoringCase(name, "cache-control")
> >          || equalIgnoringCase(name, "content-language")
> > +        || equalIgnoringCase(name, "last-event-id")
> >          || equalIgnoringCase(name, "origin"))
> >          return true;
> 
> This list comes from the CORS spec.  It's not clear we should change it unilaterally.

Note that Last-Event-ID is in the simple request header list in the CORS spec, but it doesn't make sense to add only that since then a preflight request will be made anyway.

> 
> > Source/WebCore/page/EventSource.cpp:71
> > -    , m_origin(context->securityOrigin()->toString())
> > +    , m_origin(SecurityOrigin::createFromString(url)->toString())
> 
> I don't understand this part of the change.  Why don't the origin come from the context?

This is the origin set on the dispatched message events, "the origin of the event stream's URL" in the spec. I should probably rename this to m_eventStreamOrigin or similar. This was the same as the script's origin when only same origin requests were supported. I should drop 'fromString' as well since url is passed as a KURL to the constructor.

I agree that we shouldn't land anything until the questions on the spec(s) get answered.
Comment 17 Adam Barth 2011-06-20 10:06:09 PDT
> This is the origin set on the dispatched message events, "the origin of the event stream's URL" in the spec. I should probably rename this to m_eventStreamOrigin or similar. This was the same as the script's origin when only same origin requests were supported.

Makes sense, but don't we need to get that value from the final response in the redirect chain rather than the first?  In either case, having a clearer name would be very helpful.
Comment 18 Per-Erik Brodin 2011-08-17 11:39:46 PDT
Created attachment 104203 [details]
updated patch

(In reply to comment #13)
> (From update of attachment 97728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97728&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:52
> >      if (equalIgnoringCase(name, "accept")
> >          || equalIgnoringCase(name, "accept-language")
> > +        || equalIgnoringCase(name, "cache-control")
> >          || equalIgnoringCase(name, "content-language")
> > +        || equalIgnoringCase(name, "last-event-id")
> >          || equalIgnoringCase(name, "origin"))
> >          return true;
> 
> This list comes from the CORS spec.  It's not clear we should change it unilaterally.

This change is no longer needed since the check is skipped altogether.

(In reply to comment #17)
> > This is the origin set on the dispatched message events, "the origin of the event stream's URL" in the spec. I should probably rename this to m_eventStreamOrigin or similar. This was the same as the script's origin when only same origin requests were supported.
> 
> Makes sense, but don't we need to get that value from the final response in the redirect chain rather than the first?  In either case, having a clearer name would be very helpful.

m_origin renamed to m_eventStreamOrigin and now set from the response URL.
(makes no difference currently since it seems that WebKit doesn't support CORS redirects)
Comment 19 Per-Erik Brodin 2011-08-24 13:45:44 PDT
Created attachment 105060 [details]
patch 3

Fixed an issue with the test (the idea was to make EventSource reconnect to test that the Last-Event-ID header doesn't trigger a preflight but a new EventSource was created instead and the test passed anyway due to incorrect logic).
Comment 20 goberman 2011-10-13 13:23:15 PDT
It has been a while since this request was entered. Is there a chance to promote the fix anytime soon?
Thanks
Comment 21 Adam Barth 2011-10-13 14:15:32 PDT
Comment on attachment 105060 [details]
patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=105060&action=review

> Source/WebCore/page/EventSource.cpp:246
> +        String message = "EventSource cannot load " + error.failingURL() + ". " + error.localizedDescription();

Please use makeString rather than operator+
Comment 22 Adam Barth 2011-10-13 14:16:33 PDT
This implementation looks reasonable.  There's a question of whether we should implement before the spec is written.

@Hixie: Any thoughts on when you're going to add this feature to the spec?
Comment 23 Ian 'Hixie' Hickson 2011-10-13 15:04:00 PDT
It's in. Sorry I forgot to update this bug about it, my bad.
Comment 24 Adam Barth 2011-10-13 15:05:31 PDT
Awesome.  Thanks.
Comment 25 Per-Erik Brodin 2011-10-17 12:20:18 PDT
Created attachment 111298 [details]
patch 4
Comment 26 WebKit Review Bot 2011-10-17 12:22:59 PDT
Attachment 111298 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
	M	Source/WebCore/GNUmakefile.list.am
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/platform/network/chromium/ResourceRequest.h
	M	Source/WebCore/WebCore.gypi
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Source/WebCore/loader/cache/CachedResourceLoader.cpp
	M	Source/WebCore/loader/cache/CachedResourceLoader.h
	M	Source/WebCore/loader/cache/CachedResource.cpp
	M	Source/WebCore/loader/cache/CachedResourceClient.h
	M	Source/WebCore/loader/cache/CachedResource.h
	A	Source/WebCore/loader/cache/CachedCues.cpp
	M	Source/WebCore/loader/cache/CachedResourceRequest.cpp
	A	Source/WebCore/loader/cache/CachedCues.h
r97637 = a24babbf6336686dc5c919f00b81e650219e531e (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 WebKit Review Bot 2011-10-17 18:03:37 PDT
Comment on attachment 111298 [details]
patch 4

Attachment 111298 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10120056

New failing tests:
http/tests/eventsource/eventsource-cors-no-server.html
Comment 28 goberman 2011-12-02 10:06:50 PST
Can someone resolve whatever is holding the progress on this item? It has been half a year since the request was entered.
Thanks
Comment 29 Adam Barth 2011-12-02 10:46:38 PST
ap should probably review this patch.
Comment 30 Alexey Proskuryakov 2011-12-02 11:40:45 PST
Comment on attachment 111298 [details]
patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=111298&action=review

Looks like the spec changed substantially since the patch was posted.

> Source/WebCore/page/EventSource.cpp:119
>      options.allowCredentials = AllowStoredCredentials;

The spec says that credentials should not be sent cross-origin unless constructor is called with a second argument telling it to allow credentials:

    4. Let CORS mode be Anonymous.

    5. If the second argument is present, and the withCredentials dictionary member has the value true, then set CORS mode to Use Credentials and initialize the new EventSource object's withCredentials attribute to true.

There should also be a readonly withCredentials attribute on EventSource object.

The spec says to always use credentials when reconnecting, but that looks like a mistake.

> Source/WebCore/page/EventSource.cpp:256
> +    bool failedResourceSharingCheck = error.domain() == errorDomainWebKitInternal;

That's terribly fragile. We need a better way to communicate this to EventSource code.
Comment 31 Ian 'Hixie' Hickson 2011-12-02 14:58:02 PST
> The spec says to always use credentials when reconnecting, but that looks like a mistake.

Fixed.
Comment 32 Per-Erik Brodin 2011-12-22 06:18:33 PST
Created attachment 120316 [details]
patch 5

> (From update of attachment 111298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111298&action=review
> 
> Looks like the spec changed substantially since the patch was posted.
> 
> > Source/WebCore/page/EventSource.cpp:119
> >      options.allowCredentials = AllowStoredCredentials;
> 
> The spec says that credentials should not be sent cross-origin unless constructor is called with a second argument telling it to allow credentials:

Fixed.

> There should also be a readonly withCredentials attribute on EventSource object.

Fixed.

> > Source/WebCore/page/EventSource.cpp:256
> > +    bool failedResourceSharingCheck = error.domain() == errorDomainWebKitInternal;
> 
> That's terribly fragile. We need a better way to communicate this to EventSource code.

Yes, that doesn't even work anymore since now errorDomainWebKitInternal is used for other error conditions as well.
Fixed.
Comment 33 Alexey Proskuryakov 2011-12-22 11:30:08 PST
Comment on attachment 120316 [details]
patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review

> Source/WebCore/bindings/js/JSEventSourceCustom.cpp:53
> +    JSValue initializerValue = exec->argument(1);
> +    if (!initializerValue.isUndefinedOrNull()) {

This logic is somewhat surprising, and entirely untested. The spec just says "If the second argument is present". Not sure what the implied behavior for non-dictionary second argument is, but special casing null and undefined appears clearly wrong.

Please make sure that the behavior is correct (Adam can advise on current IDL semantics), and add tests.

> Source/WebCore/bindings/js/JSEventSourceCustom.cpp:54
> +        JSDictionary dictionary(exec, initializerValue.toObject(exec));

I think that this can raise an exception.

> Source/WebCore/bindings/v8/custom/V8EventSourceCustom.cpp:57
> +    if (args.Length() >= 2) {

This is different from JSC counterpart.

> Source/WebCore/loader/ThreadableLoaderClient.h:50
> +        virtual void didFailAccessControlCheck(const ResourceError&) { }

It seems that the rule here is that anyone who cares about didFail should also care about didFailAccessControlCheck. Nothing enforces that (and it's not easy to even check that XMLHttpRequest is the only other client that needed this).

Perhaps we should have a base class behavior of calling didFail() here.

> Source/WebCore/loader/ThreadableLoaderClient.h:51
>          virtual void didFailRedirectCheck() { }

This existing code also looks suspicious, for the same reason as above.

> Source/WebCore/page/EventSource.cpp:286
> +    scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), 0);

Aren't there default arguments for missing line number etc? Per discussion in bug 74075, the line should likely be 0, not 1.

> Source/WebCore/xml/XMLHttpRequest.h:163
>      virtual void didFail(const ResourceError&);
> +    virtual void didFailAccessControlCheck(const ResourceError&);

I've been thinking of adding a more direct reason indication to ResourceError (similar to isCancellation()), but this also looks OK (similar to didFailRedirectCheck). We'll probably need to clean this up one day.
Comment 34 Adam Barth 2011-12-22 11:50:07 PST
Comment on attachment 120316 [details]
patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review

>> Source/WebCore/bindings/js/JSEventSourceCustom.cpp:54
>> +        JSDictionary dictionary(exec, initializerValue.toObject(exec));
> 
> I think that this can raise an exception.

Why is this using a dictionary at all?  /me is confused.

> Source/WebCore/page/EventSource.idl:36
> +        CustomConstructor,

Why does this need a custom constructor?  That seems unlikely.
Comment 35 Adam Barth 2011-12-22 11:51:40 PST
Comment on attachment 120316 [details]
patch 5

View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review

> Source/WebCore/page/EventSource.idl:38
> +        Constructor(in DOMString url, in [Optional] EventSourceInit eventSourceInit),

Ah, I see.  The spec uses EventSourceInit here.  Yeah, the code generator should be able to handle all of this for you.  No custom bindings needed.
Comment 36 Per-Erik Brodin 2011-12-22 14:31:01 PST
(In reply to comment #33)
> (From update of attachment 120316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review
> 
> > Source/WebCore/bindings/js/JSEventSourceCustom.cpp:53
> > +    JSValue initializerValue = exec->argument(1);
> > +    if (!initializerValue.isUndefinedOrNull()) {
> 
> This logic is somewhat surprising, and entirely untested. The spec just says "If the second argument is present". Not sure what the implied behavior for non-dictionary second argument is, but special casing null and undefined appears clearly wrong.

This is inspired by the code that is generated for the event constructor bindings. I think the idea is that everything that passes this check can be turned into a dictionary.

> > Source/WebCore/bindings/js/JSEventSourceCustom.cpp:54
> > +        JSDictionary dictionary(exec, initializerValue.toObject(exec));
> 
> I think that this can raise an exception.

I think the check above will ensure that this cannot raise an exception.

> > Source/WebCore/bindings/v8/custom/V8EventSourceCustom.cpp:57
> > +    if (args.Length() >= 2) {
> 
> This is different from JSC counterpart.

Yes, but the behavior should be the same. If there is a second argument that is an object that has a property 'withCredentials' that is true, then the EventSourceInit dictionary member 'withCredentials' will be true, otherwise it will be false. As I said, this is similar to the code that is generated for the event constructor bindings.

> > Source/WebCore/loader/ThreadableLoaderClient.h:50
> > +        virtual void didFailAccessControlCheck(const ResourceError&) { }
> 
> It seems that the rule here is that anyone who cares about didFail should also care about didFailAccessControlCheck. Nothing enforces that (and it's not easy to even check that XMLHttpRequest is the only other client that needed this).

didFailAccessControlCheck will only be called when CORS is used so it's good that there's a default/empty implementation for it. I used an ASSERT_NOT_REACHED() here to test that didFailAccessControlCheck is implemented wherever it's needed, but then I removed it since everything else here has completely empty implementations. Perhaps I should add it back?

> > Source/WebCore/page/EventSource.cpp:286
> > +    scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), 0);
> 
> Aren't there default arguments for missing line number etc? Per discussion in bug 74075, the line should likely be 0, not 1.

I guess I should use addConsoleMessage instead, since it's used elsewhere in EventSource.cpp and also in XMLHttpRequest.cpp. Then I don't have to supply the line number. It seems that addConsoleMessage is using line number 1 though.

> > Source/WebCore/xml/XMLHttpRequest.h:163
> >      virtual void didFail(const ResourceError&);
> > +    virtual void didFailAccessControlCheck(const ResourceError&);
> 
> I've been thinking of adding a more direct reason indication to ResourceError (similar to isCancellation()), but this also looks OK (similar to didFailRedirectCheck). We'll probably need to clean this up one day.

I wonder how this can be done without making ResourceError aware of web platform concepts like CORS.
Comment 37 Per-Erik Brodin 2011-12-22 14:34:05 PST
(In reply to comment #35)
> (From update of attachment 120316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review
> 
> > Source/WebCore/page/EventSource.idl:38
> > +        Constructor(in DOMString url, in [Optional] EventSourceInit eventSourceInit),
> 
> Ah, I see.  The spec uses EventSourceInit here.  Yeah, the code generator should be able to handle all of this for you.  No custom bindings needed.

EventSourceInit is a WebIDL dictionary. I don't think that this is supported by the code generators other than specifically for the event constructors, using a "constructor template". In case I'm mistaken, could you advise me on the proper WebKit WebIDL syntax to have the bindings automatically generated? It would be great if we could avoid the custom bindings.
Comment 38 Alexey Proskuryakov 2011-12-22 14:43:18 PST
> didFailAccessControlCheck will only be called when CORS is used so it's good that there's a default/empty implementation for it. 

Understood why it cannot be "= 0". I don't understand why it's good for it to be empty. 

> I used an ASSERT_NOT_REACHED() here to test that didFailAccessControlCheck is implemented wherever it's needed, but then I removed it

I would still prefer a compile time guarantee. Would it be a mistake to fall back to didFail()?

> I wonder how this can be done without making ResourceError aware of web platform concepts like CORS.

Yes, you're right. This wasn't a good idea. This still feels quite messy (e.g. CORS will be doing redirect checks too, so why is there a separate callback with a different signature for didFailRedirectCheck?)
Comment 39 Per-Erik Brodin 2012-05-17 10:13:16 PDT
Created attachment 142495 [details]
patch 6

Updated patch:
- The constructor bindings are now generated (this wasn't possible until recently)
- Added testing of the new second argument to the constructor
- addConsoleMessage now used instead of addMessage (it doesn't output a line number anymore)
- didFail is now called from ThreadableLoaderClient in case didFailAccessControlCheck is not overridden
- Fixed EventSource.h indentation
Comment 40 Per-Erik Brodin 2012-05-17 10:15:08 PDT
(In reply to comment #38)
> This still feels quite messy (e.g. CORS will be doing redirect checks too, so why is there a separate callback with a different signature for didFailRedirectCheck?)

I agree that this could probably be improved, but I'd rather not touch didFailRedirectCheck in this patch.
Comment 41 WebKit Review Bot 2012-05-17 11:05:59 PDT
Comment on attachment 142495 [details]
patch 6

Attachment 142495 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12717588

New failing tests:
fast/eventsource/eventsource-constructor.html
Comment 42 WebKit Review Bot 2012-05-17 11:06:05 PDT
Created attachment 142505 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 43 Alexey Proskuryakov 2012-09-11 11:01:49 PDT
*** Bug 96368 has been marked as a duplicate of this bug. ***
Comment 44 Dominik Röttsches (drott) 2012-12-12 08:38:49 PST
(In reply to comment #39)
> Created an attachment (id=142495) [details]
> patch 6

Any plans to update this patch? Or would you like to have someone take over?
Comment 45 Per-Erik Brodin 2012-12-13 11:17:05 PST
Created attachment 179305 [details]
patch 7

Updated patch:
- Changed layoutTestController to testRunner in new tests
- Removed DefaultIsUndefined from the 'Optional' extended IDL attribute since that is now the default for dictionaries
- Removed LogMessageType argument that has been removed from addConsoleMessage()
- Simplified constructor argument test
- Updated more platform specific expected results for the constructor length test
- Replaced enum with static const members to avoid style checker errors
- Removed 'int' from 'unsigned int' in method signature to avoid style checker error
Comment 46 Alexey Proskuryakov 2012-12-17 13:45:05 PST
Comment on attachment 179305 [details]
patch 7

View in context: https://bugs.webkit.org/attachment.cgi?id=179305&action=review

Looks good to me, except for failing test. Sorry for the delay.

> LayoutTests/http/tests/eventsource/eventsource-cors-no-server-expected.txt:3
> +FAIL: got error event but readyState is CONNECTING

Is this OK?

> Source/WebCore/page/EventSource.cpp:124
> +    options.allowCredentials = origin->canRequest(m_url) || m_withCredentials ? AllowStoredCredentials : DoNotAllowStoredCredentials;

I would put braces around the condition for clarity (and because this makes it look more like an if () condition).
Comment 47 Per-Erik Brodin 2012-12-18 14:15:22 PST
Created attachment 180024 [details]
patch 8

(In reply to comment #46)
> > LayoutTests/http/tests/eventsource/eventsource-cors-no-server-expected.txt:3
> > +FAIL: got error event but readyState is CONNECTING
> 
> Is this OK?
The failing test was originally for bug 70112 and my plan was to update the expected result when fixing that one. However, the spec changed recently so the current behavior is correct.
I have updated the test accordingly.

> 
> > Source/WebCore/page/EventSource.cpp:124
> > +    options.allowCredentials = origin->canRequest(m_url) || m_withCredentials ? AllowStoredCredentials : DoNotAllowStoredCredentials;
> 
> I would put braces around the condition for clarity (and because this makes it look more like an if () condition).

Fixed.
Comment 48 WebKit Review Bot 2012-12-18 16:20:48 PST
Comment on attachment 180024 [details]
patch 8

Clearing flags on attachment: 180024

Committed r138083: <http://trac.webkit.org/changeset/138083>
Comment 49 WebKit Review Bot 2012-12-18 16:20:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 50 Darren Cook 2013-05-30 20:32:26 PDT
Does "RESOLVED FIXED" mean it became live and released in Dec 2012? EventSource using CORS does not appear to be working. (I get "SecurityError: DOM Exception 18" in Chromium 25, on Ubuntu 10.04; same code is working with Firefox 20, and Opera 12.15) (Chromium 25 was released Feb 21st 2013.)

(I'm using "Access-Control-Allow-Origin: *" on the server, not using credentials.)