Update and fix sendBeacon() redirect behavior.

Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in

 https://github.com/w3c/beacon/pull/33
 https://github.com/w3c/beacon/pull/34

In particular, correctly flag a CORS-disallowed redirect as not to
be followed by WebURLLoader.

R=
BUG=628762
Committed: https://crrev.com/3fe79278480bcf6aea4172056d5b8beb18aeb5a9
Cr-Commit-Position: refs/heads/master@{#408380}

[sof, 2016-07-27 15:16:00]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 15:16:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2016-07-27 16:04:33]

Description was changed from

==========
Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in
https://github.com/w3c/beacon/pull/{33, 34}

R=
BUG=628762
==========

to

==========
Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in

 https://github.com/w3c/beacon/pull/33
 https://github.com/w3c/beacon/pull/34

In particular, correctly flag a CORS-disallowed redirect as not to
be followed by WebURLLoader.

R=
BUG=628762
==========

[sof, 2016-07-27 16:06:26]

sigbjornf@opera.com changed reviewers:
+ japhet@chromium.org, tyoshino@chromium.org

[sof, 2016-07-27 16:06:27]

please take a look.

[sof, 2016-07-27 16:06:30]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 16:06:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 17:32:31]

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

[commit-bot: I haz the power, 2016-07-27 17:32:32]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-07-28 07:21:57]

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/beacon-cross-origin-redirect-blob.html
(right):

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/beacon-cross-origin-redirect-blob.html:4:
<script src="/js-test-resources/js-test.js"></script>
mkwst@ has sent a mail encouraging use of testharness.js for new tests and there
were basically no objection. Shall we follow that?

https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/testharne...
https://sites.google.com/a/chromium.org/dev/developers/testing/webkit-layout-...

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:36: virtual const
AtomicString getContentType() const = 0;
BeaconString returns the type including the charset parameter while the
multipart boundary parameter is omitted. I understand it's fine for checking
simpleness, but let's make them consistent. E.g. name it
getMIMETypeWithoutParameters() or just getMIMEType() and add a comment saying
that parameters are ignored, and then, make BeaconString's return only
"text/plain".

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:92: }
do the extraction in the constructor and use it saved in m_contentType here? or
there will be a dependency that getContentType() must be called after
serialize() which might cause a bug in the future.

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:129: const AtomicString
getContentType() const { return nullAtom; }
be consistent with L123-124?

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:267: passedNewRequest =
WebURLRequest();
This is ok but I wonder if you had to add this to make it work or just for
safety.

[sof, 2016-07-28 08:28:20]

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/beacon-cross-origin-redirect-blob.html
(right):

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/beacon-cross-origin-redirect-blob.html:4:
<script src="/js-test-resources/js-test.js"></script>
On 2016/07/28 07:21:57, tyoshino wrote:
> mkwst@ has sent a mail encouraging use of testharness.js for new tests and
there
> were basically no objection. Shall we follow that?
> 
>
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/testharne...
>
https://sites.google.com/a/chromium.org/dev/developers/testing/webkit-layout-...

No, I don't think we should for one beacon test.

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:36: virtual const
AtomicString getContentType() const = 0;
On 2016/07/28 07:21:57, tyoshino wrote:
> BeaconString returns the type including the charset parameter while the
> multipart boundary parameter is omitted. I understand it's fine for checking
> simpleness, but let's make them consistent. E.g. name it
> getMIMETypeWithoutParameters() or just getMIMEType() and add a comment saying
> that parameters are ignored, and then, make BeaconString's return only
> "text/plain".

getContentType() implements what
https://fetch.spec.whatwg.org/#concept-bodyinit-extract defines, so I think it
is fine to keep it like this. i.e., that we currently re-use an simple-header
predicate to check these values is an implementation detail, and not something
we should assume will be used always.

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:92: }
On 2016/07/28 07:21:57, tyoshino wrote:
> do the extraction in the constructor and use it saved in m_contentType here?
or
> there will be a dependency that getContentType() must be called after
> serialize() which might cause a bug in the future.

Good idea, much better; done.

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:129: const AtomicString
getContentType() const { return nullAtom; }
On 2016/07/28 07:21:57, tyoshino wrote:
> be consistent with L123-124?

No, that would make ArrayBuffer-backed follow CORS, which it shouldn't per spec.

I'd be prepared to align with the spec and not set a Content-Type request
header.. but I'd prefer to make that change separately from this one, lest there
is trouble.

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:267: passedNewRequest =
WebURLRequest();
On 2016/07/28 07:21:57, tyoshino wrote:
> This is ok but I wonder if you had to add this to make it work or just for
> safety.

It won't work without it (a longer-standing bug.)

[sof, 2016-07-28 08:28:42]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 08:29:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 09:37:12]

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

[commit-bot: I haz the power, 2016-07-28 09:37:13]

Dry run: This issue passed the CQ dry run.

[tyoshino (SeeGerritForStatus), 2016-07-28 12:03:21]

lgtm

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/beacon-cross-origin-redirect-blob.html
(right):

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/beacon-cross-origin-redirect-blob.html:4:
<script src="/js-test-resources/js-test.js"></script>
On 2016/07/28 08:28:19, sof wrote:
> On 2016/07/28 07:21:57, tyoshino wrote:
> > mkwst@ has sent a mail encouraging use of testharness.js for new tests and
> there
> > were basically no objection. Shall we follow that?
> > 
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/testharne...
> >
>
https://sites.google.com/a/chromium.org/dev/developers/testing/webkit-layout-...
> 
> No, I don't think we should for one beacon test.

For consistency with existing tests? Then, ok.

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/BeaconLoader.cpp (right):

https://codereview.chromium.org/2177383006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/BeaconLoader.cpp:129: const AtomicString
getContentType() const { return nullAtom; }
On 2016/07/28 08:28:19, sof wrote:
> On 2016/07/28 07:21:57, tyoshino wrote:
> > be consistent with L123-124?
> 
> No, that would make ArrayBuffer-backed follow CORS, which it shouldn't per
spec.
> 
> I'd be prepared to align with the spec and not set a Content-Type request
> header.. but I'd prefer to make that change separately from this one, lest
there
> is trouble.

OK

[sof, 2016-07-28 13:19:50]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-07-28 13:20:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 13:23:01]

Description was changed from

==========
Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in

 https://github.com/w3c/beacon/pull/33
 https://github.com/w3c/beacon/pull/34

In particular, correctly flag a CORS-disallowed redirect as not to
be followed by WebURLLoader.

R=
BUG=628762
==========

to

==========
Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in

 https://github.com/w3c/beacon/pull/33
 https://github.com/w3c/beacon/pull/34

In particular, correctly flag a CORS-disallowed redirect as not to
be followed by WebURLLoader.

R=
BUG=628762
==========

[commit-bot: I haz the power, 2016-07-28 13:23:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-28 13:24:18]

Description was changed from

==========
Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in

 https://github.com/w3c/beacon/pull/33
 https://github.com/w3c/beacon/pull/34

In particular, correctly flag a CORS-disallowed redirect as not to
be followed by WebURLLoader.

R=
BUG=628762
==========

to

==========
Update and fix sendBeacon() redirect behavior.

Refresh the implementation to follow the specification changes in

 https://github.com/w3c/beacon/pull/33
 https://github.com/w3c/beacon/pull/34

In particular, correctly flag a CORS-disallowed redirect as not to
be followed by WebURLLoader.

R=
BUG=628762

Committed: https://crrev.com/3fe79278480bcf6aea4172056d5b8beb18aeb5a9
Cr-Commit-Position: refs/heads/master@{#408380}
==========

[commit-bot: I haz the power, 2016-07-28 13:24:19]

Patchset 3 (id:??) landed as
https://crrev.com/3fe79278480bcf6aea4172056d5b8beb18aeb5a9
Cr-Commit-Position: refs/heads/master@{#408380}