[Webkit-unassigned] [Bug 61862] EventSource should support CORS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61862


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #120316|review?                     |review-
               Flag|                            |




--- Comment #33 from Alexey Proskuryakov <ap at webkit.org>  2011-12-22 11:30:08 PST ---
(From update of attachment 120316)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.