|
|
Created:
3 years, 8 months ago by yiyix Modified:
3 years, 7 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, blink-reviews-w3ctests_chromium.org, nhiroki, haraken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik, varkha Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFetch API: Update fetch request
According to the spec, Request#url should not exclude the URL
fragment (only Response#url excludes it).
BUG=624278
Review-Url: https://codereview.chromium.org/2783623003
Cr-Commit-Position: refs/heads/master@{#461323}
Committed: https://chromium.googlesource.com/chromium/src/+/7ea0952f2888c2de0cc0827743e63de9e7dc3778
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Messages
Total messages: 43 (31 generated)
The CQ bit was checked by yiyix@chromium.org 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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org 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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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 ========== temperary temperary BUG= ========== to ========== [WIP] Fetch API: Update fetch request Update the fetch request to include the fragment part in URL. BUG=624278 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by yiyix@chromium.org 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...
yiyix@chromium.org changed reviewers: + falken@chromium.org
Could you please take a look at this patch? Thank you.
Description was changed from ========== [WIP] Fetch API: Update fetch request Update the fetch request to include the fragment part in URL. BUG=624278 ========== to ========== Fetch API: Update fetch request Update the fetch request to include the fragment part in URL. BUG=624278 ==========
lgtm CL description suggestions: "Fetch API: Update fetch request" -> "Fetch API: Don't exclude the URL fragment in Request#url" And for the second sentence: "According to the spec, Request#url should not exclude the URL fragment (only Response#url excludes it)." https://codereview.chromium.org/2783623003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/2783623003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:438: return RequestString(test_page_url_ + fragment.c_str(), "navigate", nit: I don't think you need the c_str()? https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/fetch.js (right): https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/fetch.js:171: // flag set. There's no "fragment flag", it's just the "exclude fragment flag" from the URL spec: https://url.spec.whatwg.org/#url-serializing This comment can just deleted: it's a bit confusing to just point out that the "exclude fragment flag is not set" since that's the default. https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:694: // Strips off the fragment part of URL. nit: "Strips" -> "Strip" is more consistent with the codebase's comments. I'd also add a comment about why we're doing this. "So far, all users of WebServiceWorkerRequest expect the fragment to be excluded."
Description was changed from ========== Fetch API: Update fetch request Update the fetch request to include the fragment part in URL. BUG=624278 ========== to ========== Fetch API: Update fetch request According to the spec, Request#url should not exclude the URL fragment (only Response#url excludes it). BUG=624278 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yiyix@chromium.org changed reviewers: + horo@chromium.org
yiyix@chromium.org changed reviewers: + sky@chromium.org
Thank for reviewing @falken @horo, could you please review changes in third_party/WebKit/Source/modules/fetch/Request.cpp? @sky, could you please review changes in chrome/browser/chrome_service_worker_browsertest.cc? Thank you so much. https://codereview.chromium.org/2783623003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_service_worker_browsertest.cc (right): https://codereview.chromium.org/2783623003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_service_worker_browsertest.cc:438: return RequestString(test_page_url_ + fragment.c_str(), "navigate", On 2017/03/31 06:40:49, falken wrote: > nit: I don't think you need the c_str()? Done. https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/fetch.js (right): https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/fetch.js:171: // flag set. On 2017/03/31 06:40:49, falken wrote: > There's no "fragment flag", it's just the "exclude fragment flag" from the URL > spec: https://url.spec.whatwg.org/#url-serializing > > This comment can just deleted: it's a bit confusing to just point out that the > "exclude fragment flag is not set" since that's the default. True, the term "flag set" is not used in the spec. I will delete the comment. https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/Request.cpp (right): https://codereview.chromium.org/2783623003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/Request.cpp:694: // Strips off the fragment part of URL. On 2017/03/31 06:40:49, falken wrote: > nit: "Strips" -> "Strip" is more consistent with the codebase's comments. I'd > also add a comment about why we're doing this. "So far, all users of > WebServiceWorkerRequest expect the fragment to be excluded." Done.
lgtm Please add the links to the spec change commit and the spec discussion to the CL description. https://github.com/whatwg/fetch/commit/111da37ad6c6485d0b77cfa5437200b34b1f7f0e https://github.com/whatwg/fetch/issues/214
On 2017/03/31 08:28:01, horo wrote: > lgtm > > Please add the links to the spec change commit and the spec discussion to the CL > description. > > https://github.com/whatwg/fetch/commit/111da37ad6c6485d0b77cfa5437200b34b1f7f0e > https://github.com/whatwg/fetch/issues/214 nit: Also update the first line of the CL description to match the new issue title.
LGTM
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2783623003/#ps80001 (title: "address comments")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491051407876630, "parent_rev": "4b4a2c65e6ef9e8609a6a7b3bcd32c0438b5655a", "commit_rev": "7ea0952f2888c2de0cc0827743e63de9e7dc3778"}
Message was sent while issue was closed.
Description was changed from ========== Fetch API: Update fetch request According to the spec, Request#url should not exclude the URL fragment (only Response#url excludes it). BUG=624278 ========== to ========== Fetch API: Update fetch request According to the spec, Request#url should not exclude the URL fragment (only Response#url excludes it). BUG=624278 Review-Url: https://codereview.chromium.org/2783623003 Cr-Commit-Position: refs/heads/master@{#461323} Committed: https://chromium.googlesource.com/chromium/src/+/7ea0952f2888c2de0cc0827743e6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7ea0952f2888c2de0cc0827743e6...
Message was sent while issue was closed.
On 2017/04/01 at 13:52:04, commit-bot wrote: > Committed patchset #2 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7ea0952f2888c2de0cc0827743e6... GitHub PR created: https://github.com/w3c/web-platform-tests/pull/5324
Message was sent while issue was closed.
Created chrome status for this: https://www.chromestatus.com/feature/4753419730419712 |