Move fetch-related predicates to core/fetch.

Move fetch-related predicates to core/fetch.

Currently core/fetch, core/xml and modules/serviceworkers have predicates
defined in http://fetch.spec.whatwg.org/ such as "simple method".
This CL moves these predicates to core/fetch/FetchUtils.h and
renames some predicates.

BUG=NONE

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179692

[sof, 2014-07-17 08:18:09]

(Cc: +abarth)

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:97: static const
ForbiddenHeaderNames* initializeForbiddenHeaderNames()
Unless there are reasons not to, should we move this and the other static
function into ForbiddenHeaderNames as static methods?

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:99: // Uses dummy to avoid
warnings about an unused variable.
I know this is code that you moved into here, but it is used? Switch to a better
name (and remove comment)?

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:313: // "A simple method is a
method that is `GET`, `HEAD`, or `POST`."
Anchor this with a spec href?

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:319: // "A simple header is a
header whose name is either one of `Accept`,
This is also something with a settled spec reference by now; add one?

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:357: // "A forbidden method is a
method that is a byte case-insensitive match"
I think it is a good idea to share definitions like this and a worthwhile
stopping point to have them in CrossOriginAccessControl, but Fetch/XHR
predicates are best kept separate and somewhere else? They're not cross-origin
related, per se.

This is another definition that could do with a spec xref.

https://codereview.chromium.org/379113002/diff/180001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/379113002/diff/180001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:123: if
((m_options.preflightPolicy == ConsiderPreflight &&
CrossOriginAccessControl::isSimpleOrForbiddenRequest(request.httpMethod(),
request.httpHeaderFields())) || m_options.preflightPolicy == PreventPreflight) {
Could we keep this as isSimpleRequest() ?

[horo, 2014-07-18 03:03:28]

lgtm

[horo, 2014-07-18 03:03:28]

lgtm

lgtm

[yhirano, 2014-08-06 08:48:03]

Sorry for the delay. sof@, can you take a look again?

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:97: static const
ForbiddenHeaderNames* initializeForbiddenHeaderNames()
On 2014/07/17 08:18:09, sof wrote:
> Unless there are reasons not to, should we move this and the other static
> function into ForbiddenHeaderNames as static methods?

Done.

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:99: // Uses dummy to avoid
warnings about an unused variable.
On 2014/07/17 08:18:09, sof wrote:
> I know this is code that you moved into here, but it is used? Switch to a
better
> name (and remove comment)?

Done.

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:313: // "A simple method is a
method that is `GET`, `HEAD`, or `POST`."
On 2014/07/17 08:18:09, sof wrote:
> Anchor this with a spec href?

Done.

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:319: // "A simple header is a
header whose name is either one of `Accept`,
On 2014/07/17 08:18:09, sof wrote:
> This is also something with a settled spec reference by now; add one?

Done.

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:357: // "A forbidden method is a
method that is a byte case-insensitive match"
On 2014/07/17 08:18:09, sof wrote:
> I think it is a good idea to share definitions like this and a worthwhile
> stopping point to have them in CrossOriginAccessControl, but Fetch/XHR
> predicates are best kept separate and somewhere else? They're not cross-origin
> related, per se.
> 
> This is another definition that could do with a spec xref.

I didn't come up with a great name, but moved these code to FetchUtil class.

https://codereview.chromium.org/379113002/diff/180001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/379113002/diff/180001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:123: if
((m_options.preflightPolicy == ConsiderPreflight &&
CrossOriginAccessControl::isSimpleOrForbiddenRequest(request.httpMethod(),
request.httpHeaderFields())) || m_options.preflightPolicy == PreventPreflight) {
On 2014/07/17 08:18:09, sof wrote:
> Could we keep this as isSimpleRequest() ?

I think we can't.
Blink sometimes insert forbidden headers internally. We should permit them here
(Previously isSimpleRequest allowed that, but I changed the behavior in this
CL). Forbidden headers from the user is rejected in front.

[sof, 2014-08-06 19:48:15]

lgtm, but a related test is currently failing?

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/379113002/diff/180001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:357: // "A forbidden method is a
method that is a byte case-insensitive match"
On 2014/08/06 08:48:02, yhirano wrote:
> On 2014/07/17 08:18:09, sof wrote:
> > I think it is a good idea to share definitions like this and a worthwhile
> > stopping point to have them in CrossOriginAccessControl, but Fetch/XHR
> > predicates are best kept separate and somewhere else? They're not
cross-origin
> > related, per se.
> > 
> > This is another definition that could do with a spec xref.
> 
> I didn't come up with a great name, but moved these code to FetchUtil class.

Yes, "Utils" classes can grow to become a disparate collection of methods, but
just something to look out for as we go along.

https://codereview.chromium.org/379113002/diff/240001/Source/core/fetch/Fetch...
File Source/core/fetch/FetchUtils.cpp (right):

https://codereview.chromium.org/379113002/diff/240001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.cpp:19: struct ForbiddenHeaderNames {
nit: might as well make it 'class'?

https://codereview.chromium.org/379113002/diff/240001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.cpp:95: // Preflight is required for MIME types
that can not be sent via form submission.
Move this comment to the call site in isSimpleRequest() ?

[yhirano, 2014-08-07 03:21:50]

Fixed the test failure.

https://codereview.chromium.org/379113002/diff/240001/Source/core/fetch/Fetch...
File Source/core/fetch/FetchUtils.cpp (right):

https://codereview.chromium.org/379113002/diff/240001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.cpp:19: struct ForbiddenHeaderNames {
On 2014/08/06 19:48:15, sof wrote:
> nit: might as well make it 'class'?

Done.

https://codereview.chromium.org/379113002/diff/240001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.cpp:95: // Preflight is required for MIME types
that can not be sent via form submission.
On 2014/08/06 19:48:15, sof wrote:
> Move this comment to the call site in isSimpleRequest() ?

Done.

[yhirano, 2014-08-07 03:22:42]

+falken for modules/serviceworker
+tkent for web

[tkent, 2014-08-07 03:39:25]

lgtm

https://codereview.chromium.org/379113002/diff/260001/Source/core/fetch/Fetch...
File Source/core/fetch/FetchUtils.h (right):

https://codereview.chromium.org/379113002/diff/260001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.h:8: #include "wtf/text/WTFString.h"
Use wtf/Forward.h instead.

https://codereview.chromium.org/379113002/diff/260001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.h:24: };
Please add

private:
    FetchUtils();

or FetchUtils should be a namespace.

[falken, 2014-08-07 04:05:28]

great! lgtm

[yhirano, 2014-08-07 05:20:01]

https://codereview.chromium.org/379113002/diff/260001/Source/core/fetch/Fetch...
File Source/core/fetch/FetchUtils.h (right):

https://codereview.chromium.org/379113002/diff/260001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.h:8: #include "wtf/text/WTFString.h"
On 2014/08/07 03:39:25, tkent wrote:
> Use wtf/Forward.h instead.

Done.

https://codereview.chromium.org/379113002/diff/260001/Source/core/fetch/Fetch...
Source/core/fetch/FetchUtils.h:24: };
On 2014/08/07 03:39:25, tkent wrote:
> Please add
> 
> private:
>     FetchUtils();
> 
> or FetchUtils should be a namespace.

Done.

[yhirano, 2014-08-07 05:20:08]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-08-07 05:20:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/379113002/280001

[commit-bot: I haz the power, 2014-08-07 06:42:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21495)

[commit-bot: I haz the power, 2014-08-07 07:55:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 07:55:48]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[yhirano, 2014-08-07 07:58:52]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-08-07 07:59:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/379113002/280001

[commit-bot: I haz the power, 2014-08-07 08:14:56]

Change committed as 179692