[Webkit-unassigned] [Bug 65694] Make it possible to explicitly prevent a preflight via ThreadableLoaderOptions

[bugzilla-daemon at webkit.org]

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


Per-Erik Brodin <per-erik.brodin at ericsson.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #102966|0                           |1
        is obsolete|                            |
 Attachment #103070|                            |review?, commit-queue?
               Flag|                            |




--- Comment #14 from Per-Erik Brodin <per-erik.brodin at ericsson.com>  2011-08-05 07:20:17 PST ---
Created an attachment (id=103070)
 --> (https://bugs.webkit.org/attachment.cgi?id=103070&action=review)
patch 3

(In reply to comment #13)
> (From update of attachment 102966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102966&action=review
> 
> r=me assuming build fix and a better name for enum value.
> 
Fixed chromium issue.

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:101
> > +    if ((m_options.preflightPolicy == ConditionPreflight && isSimpleCrossOriginAccessRequest(crossOriginRequest->httpMethod(), crossOriginRequest->httpHeaderFields())) || m_options.preflightPolicy == PreventPreflight)
> 
> I'd order this differently to avoid calling into isSimpleCrossOriginAccessRequest() when PreventPreflight is specified.
> 
The first evaluation will ensure that isSimpleCrossOriginAccessRequest() is not called if preflightPolicy is ForcePreflight or PreventPreflight. That is the default case and probably the most common one so that's why I put it first.

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:115
> > +    ASSERT(m_options.preflightPolicy == PreventPreflight || isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields()));
> 
> Please add ASSERT(m_options.preflightPolicy != ForcePreflight) for completeness.
> 
Fixed.

> > Source/WebCore/loader/ThreadableLoader.h:60
> > +        ConditionPreflight,
> 
> Condition as a verb has a different meaning than you want here. Maybe "MayUsePreflight" or "AllowPreflight"?

I wasn't entirely happy with that name but I still would like the option to reflect that CORS rules will determine whether a preflight is made when that policy is set. My new proposal is 'ConsiderPreflight'. If you don't like that name I have no problem changing to one of the two that you suggested.

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