[webkit-reviews] review requested: [Bug 65694] Make it possible to explicitly prevent a preflight via ThreadableLoaderOptions : [Attachment 103070] patch 3

[bugzilla-daemon at webkit.org]

Per-Erik Brodin <per-erik.brodin at ericsson.com> has asked  for review:
Bug 65694: Make it possible to explicitly prevent a preflight via
ThreadableLoaderOptions
https://bugs.webkit.org/show_bug.cgi?id=65694

Attachment 103070: patch 3
https://bugs.webkit.org/attachment.cgi?id=103070&action=review

------- Additional Comments from Per-Erik Brodin <per-erik.brodin at ericsson.com>
(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.