RESOLVED FIXED 61862
EventSource should support CORS 
https://bugs.webkit.org/show_bug.cgi?id=61862
Summary EventSource should support CORS
goberman
Reported 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>
Attachments
proposed patch (9.47 KB, patch)
2011-06-19 14:37 PDT, Per-Erik Brodin 		abarth: review-
DetailsFormatted DiffDiff
updated patch (10.46 KB, patch)
2011-08-17 11:39 PDT, Per-Erik Brodin 		no flags
DetailsFormatted DiffDiff
patch 3 (10.43 KB, patch)
2011-08-24 13:45 PDT, Per-Erik Brodin 		no flags
DetailsFormatted DiffDiff
patch 4 (13.11 KB, patch)
2011-10-17 12:20 PDT, Per-Erik Brodin 		ap: review-
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
patch 5 (44.90 KB, patch)
2011-12-22 06:18 PST, Per-Erik Brodin 		ap: review-
DetailsFormatted DiffDiff
patch 6 (36.89 KB, patch)
2012-05-17 10:13 PDT, Per-Erik Brodin 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ec2-cr-linux-03 (480.24 KB, application/zip)
2012-05-17 11:06 PDT, WebKit Review Bot 		no flags
Details
patch 7 (38.76 KB, patch)
2012-12-13 11:17 PST, Per-Erik Brodin 		no flags
DetailsFormatted DiffDiff
patch 8 (38.80 KB, patch)
2012-12-18 14:15 PST, Per-Erik Brodin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 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.
goberman
Comment 2 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
goberman
Comment 3 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
Ian 'Hixie' Hickson
Comment 4 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.
goberman
Comment 5 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
Ian 'Hixie' Hickson
Comment 6 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.
goberman
Comment 7 2011-06-01 11:25:27 PDT
OK, sent request to whatwg@lists.whatwg.org
goberman
Comment 8 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.
goberman
Comment 9 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
Ian 'Hixie' Hickson
Comment 10 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.
goberman
Comment 11 2011-06-13 13:18:33 PDT
I appreciate it. Thanks
Per-Erik Brodin
Comment 12 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
Adam Barth
Comment 13 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?
Adam Barth
Comment 14 2011-06-20 01:52:48 PDT
*** Bug 62949 has been marked as a duplicate of this bug. ***
Adam Barth
Comment 15 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.
Per-Erik Brodin
Comment 16 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.
Adam Barth
Comment 17 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.
Per-Erik Brodin
Comment 18 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)
Per-Erik Brodin
Comment 19 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).
goberman
Comment 20 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
Adam Barth
Comment 21 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+
Adam Barth
Comment 22 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?
Ian 'Hixie' Hickson
Comment 23 2011-10-13 15:04:00 PDT
It's in. Sorry I forgot to update this bug about it, my bad.
Adam Barth
Comment 24 2011-10-13 15:05:31 PDT
Awesome. Thanks.
Per-Erik Brodin
Comment 25 2011-10-17 12:20:18 PDT
Created attachment 111298 [details] patch 4
WebKit Review Bot
Comment 26 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.
WebKit Review Bot
Comment 27 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
goberman
Comment 28 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
Adam Barth
Comment 29 2011-12-02 10:46:38 PST
ap should probably review this patch.
Alexey Proskuryakov
Comment 30 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.
Ian 'Hixie' Hickson
Comment 31 2011-12-02 14:58:02 PST
> The spec says to always use credentials when reconnecting, but that looks like a mistake. Fixed.
Per-Erik Brodin
Comment 32 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.
Alexey Proskuryakov
Comment 33 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.
Adam Barth
Comment 34 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.
Adam Barth
Comment 35 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.
Per-Erik Brodin
Comment 36 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.
Per-Erik Brodin
Comment 37 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.
Alexey Proskuryakov
Comment 38 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?)
Per-Erik Brodin
Comment 39 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
Per-Erik Brodin
Comment 40 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.
WebKit Review Bot
Comment 41 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
WebKit Review Bot
Comment 42 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
Alexey Proskuryakov
Comment 43 2012-09-11 11:01:49 PDT
*** Bug 96368 has been marked as a duplicate of this bug. ***
Dominik Röttsches (drott)
Comment 44 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?
Per-Erik Brodin
Comment 45 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
Alexey Proskuryakov
Comment 46 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).
Per-Erik Brodin
Comment 47 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.
WebKit Review Bot
Comment 48 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>
WebKit Review Bot
Comment 49 2012-12-18 16:20:57 PST
All reviewed patches have been landed. Closing bug.
Darren Cook
Comment 50 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.)
Note You need to log in before you can comment on or make changes to this bug.