Bug 61862 - EventSource should support CORS
: EventSource should support CORS
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
: 65694
: 70112
  Show dependency treegraph
 
Reported: 2011-06-01 08:13 PST by
Modified: 2013-05-30 20:32 PST (History)


Attachments
proposed patch (9.47 KB, patch)
2011-06-19 14:37 PST, Per-Erik Brodin
abarth: review-
Review Patch | Details | Formatted Diff | Diff
updated patch (10.46 KB, patch)
2011-08-17 11:39 PST, Per-Erik Brodin
no flags Review Patch | Details | Formatted Diff | Diff
patch 3 (10.43 KB, patch)
2011-08-24 13:45 PST, Per-Erik Brodin
no flags Review Patch | Details | Formatted Diff | Diff
patch 4 (13.11 KB, patch)
2011-10-17 12:20 PST, Per-Erik Brodin
ap: review-
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch 5 (44.90 KB, patch)
2011-12-22 06:18 PST, Per-Erik Brodin
ap: review-
Review Patch | Details | Formatted Diff | Diff
patch 6 (36.89 KB, patch)
2012-05-17 10:13 PST, Per-Erik Brodin
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (480.24 KB, application/zip)
2012-05-17 11:06 PST, WebKit Review Bot
no flags Details
patch 7 (38.76 KB, patch)
2012-12-13 11:17 PST, Per-Erik Brodin
no flags Review Patch | Details | Formatted Diff | Diff
patch 8 (38.80 KB, patch)
2012-12-18 14:15 PST, Per-Erik Brodin
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-06-01 08:13:38 PST
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 From 2011-06-01 08:40:22 PST -------
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 From 2011-06-01 08:49:29 PST -------
I would appreciate if you keep this enhancement request open. I have not seen any other formal requests for this.
Thanks
------- Comment #3 From 2011-06-01 10:49:23 PST -------
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 From 2011-06-01 10:54:04 PST -------
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 From 2011-06-01 10:57:04 PST -------
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 From 2011-06-01 11:00:35 PST -------
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 From 2011-06-01 11:25:27 PST -------
OK, sent request to whatwg@lists.whatwg.org
------- Comment #8 From 2011-06-02 07:32:10 PST -------
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 From 2011-06-10 09:30:38 PST -------
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 From 2011-06-10 12:09:54 PST -------
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 From 2011-06-13 13:18:33 PST -------
I appreciate it. Thanks
------- Comment #12 From 2011-06-19 14:37:46 PST -------
Created an attachment (id=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 From 2011-06-20 01:52:30 PST -------
(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.

> 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 From 2011-06-20 01:52:48 PST -------
*** Bug 62949 has been marked as a duplicate of this bug. ***
------- Comment #15 From 2011-06-20 01:53:52 PST -------
(From update of attachment 97728 [details])
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 From 2011-06-20 03:13:11 PST -------
(In reply to comment #13)
> (From update of attachment 97728 [details] [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 From 2011-06-20 10:06:09 PST -------
> 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 From 2011-08-17 11:39:46 PST -------
Created an attachment (id=104203) [details]
updated patch

(In reply to comment #13)
> (From update of attachment 97728 [details] [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 From 2011-08-24 13:45:44 PST -------
Created an attachment (id=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 From 2011-10-13 13:23:15 PST -------
It has been a while since this request was entered. Is there a chance to promote the fix anytime soon?
Thanks
------- Comment #21 From 2011-10-13 14:15:32 PST -------
(From update of attachment 105060 [details])
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 From 2011-10-13 14:16:33 PST -------
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 From 2011-10-13 15:04:00 PST -------
It's in. Sorry I forgot to update this bug about it, my bad.
------- Comment #24 From 2011-10-13 15:05:31 PST -------
Awesome.  Thanks.
------- Comment #25 From 2011-10-17 12:20:18 PST -------
Created an attachment (id=111298) [details]
patch 4
------- Comment #26 From 2011-10-17 12:22:59 PST -------
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 From 2011-10-17 18:03:37 PST -------
(From update of attachment 111298 [details])
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 From 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 From 2011-12-02 10:46:38 PST -------
ap should probably review this patch.
------- Comment #30 From 2011-12-02 11:40:45 PST -------
(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:

    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 From 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 From 2011-12-22 06:18:33 PST -------
Created an attachment (id=120316) [details]
patch 5

> (From update of attachment 111298 [details] [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 From 2011-12-22 11:30:08 PST -------
(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.

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 From 2011-12-22 11:50:07 PST -------
(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: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 From 2011-12-22 11:51:40 PST -------
(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.
------- Comment #36 From 2011-12-22 14:31:01 PST -------
(In reply to comment #33)
> (From update of attachment 120316 [details] [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 From 2011-12-22 14:34:05 PST -------
(In reply to comment #35)
> (From update of attachment 120316 [details] [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 From 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 From 2012-05-17 10:13:16 PST -------
Created an attachment (id=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 From 2012-05-17 10:15:08 PST -------
(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 From 2012-05-17 11:05:59 PST -------
(From update of attachment 142495 [details])
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 From 2012-05-17 11:06:05 PST -------
Created an attachment (id=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 From 2012-09-11 11:01:49 PST -------
*** Bug 96368 has been marked as a duplicate of this bug. ***
------- Comment #44 From 2012-12-12 08:38:49 PST -------
(In reply to comment #39)
> Created an attachment (id=142495) [details] [details]
> patch 6

Any plans to update this patch? Or would you like to have someone take over?
------- Comment #45 From 2012-12-13 11:17:05 PST -------
Created an attachment (id=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 From 2012-12-17 13:45:05 PST -------
(From update of attachment 179305 [details])
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 From 2012-12-18 14:15:22 PST -------
Created an attachment (id=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 From 2012-12-18 16:20:48 PST -------
(From update of attachment 180024 [details])
Clearing flags on attachment: 180024

Committed r138083: <http://trac.webkit.org/changeset/138083>
------- Comment #49 From 2012-12-18 16:20:57 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #50 From 2013-05-30 20:32:26 PST -------
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.)