Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by sof Modified:
4 years, 4 months ago Reviewers:
tyoshino (SeeGerritForStatus), Nate Chapin CC:
chromium-reviews, blink-reviews, loading-reviews_chromium.org, tyoshino+watch_chromium.org, Nate Chapin, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate 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}
Patch Set 1 #Patch Set 2 : comment spelling #
Total comments: 12
Patch Set 3 : improve content type initialization #
Created: 4 years, 4 months ago
Messages
Total messages: 22 (15 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
sigbjornf@opera.com changed reviewers: + japhet@chromium.org, tyoshino@chromium.org
please take a look.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3fe79278480bcf6aea4172056d5b8beb18aeb5a9 Cr-Commit-Position: refs/heads/master@{#408380} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
