Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2473263003:
Set "Service-Worker-Navigation-Preload" header (Closed)
|
Created:
4 years, 1 month ago by horo Modified:
4 years, 1 month ago Reviewers:
falken CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet "Service-Worker-Navigation-Preload" header for the navigation requests.
https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270
BUG=649558
Committed: https://crrev.com/754e439177beb1271aeb6b0153e598fc2eb6d084
Cr-Commit-Position: refs/heads/master@{#430559}
Patch Set 1 #
Total comments: 8
Patch Set 2 : incorporated falken's comment #
Messages
Total messages: 24 (17 generated)
The CQ bit was checked by horo@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...
Description was changed from ========== set header BUG=649558 ========== to ========== Set "Service-Worker-Navigation-Preload" header. BUG=649558 ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) 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 ========== Set "Service-Worker-Navigation-Preload" header. BUG=649558 ========== to ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. BUG=649558 ==========
Description was changed from ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. BUG=649558 ========== to ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 BUG=649558 ==========
horo@chromium.org changed reviewers: + falken@chromium.org
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Please review this.
falken@ Please review this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1596: " 'var channel = new MessageChannel();\\n' +\n" Sorry, I remember now we don't need to use MessageChannel. We can do: navigator.serviceWorker.controller.postMessage('hi from page'); navigator.serviceWorker.onmessage = e => { }; event.source.postMessage('hi from worker'); self.addEventListener('message', e => { }); https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1653: " return new Response('<title>' + tite + '</title>' + body,\n" title https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1690: // When the nagitation started, the navigation preload was not enabled yet. s/nagigation/navigation (throughout file) https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1721: request_log_[kPageUrl3][0].headers[kNavigationPreloadHeaderName]); could these helpers make it easier to read? HasNavigationPreloadHeader() { return request.all_headers.find(kNavigationPreloadHeaderName) != std::string::npos; } GetNavigationPreloadHeader() { ASSERT_TRUE(HasNavigationPreloadHeader(request)); return request.headers[kNaviagtionPreloadHeaderName]; }
https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1596: " 'var channel = new MessageChannel();\\n' +\n" On 2016/11/07 06:09:39, falken wrote: > Sorry, I remember now we don't need to use MessageChannel. > > We can do: > navigator.serviceWorker.controller.postMessage('hi from page'); > navigator.serviceWorker.onmessage = e => { }; > > event.source.postMessage('hi from worker'); > self.addEventListener('message', e => { }); Done. https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1653: " return new Response('<title>' + tite + '</title>' + body,\n" On 2016/11/07 06:09:38, falken wrote: > title Done. https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1690: // When the nagitation started, the navigation preload was not enabled yet. On 2016/11/07 06:09:38, falken wrote: > s/nagigation/navigation (throughout file) Done. https://codereview.chromium.org/2473263003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:1721: request_log_[kPageUrl3][0].headers[kNavigationPreloadHeaderName]); On 2016/11/07 06:09:38, falken wrote: > could these helpers make it easier to read? > > HasNavigationPreloadHeader() { > return request.all_headers.find(kNavigationPreloadHeaderName) != > std::string::npos; > } > > GetNavigationPreloadHeader() { > ASSERT_TRUE(HasNavigationPreloadHeader(request)); > return request.headers[kNaviagtionPreloadHeaderName]; > } Done.
The CQ bit was checked by horo@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/2473263003/#ps60001 (title: "incorporated falken's comment")
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 ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 BUG=649558 ========== to ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 BUG=649558 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 BUG=649558 ========== to ========== Set "Service-Worker-Navigation-Preload" header for the navigation requests. https://github.com/w3c/ServiceWorker/issues/920#issuecomment-251150270 BUG=649558 Committed: https://crrev.com/754e439177beb1271aeb6b0153e598fc2eb6d084 Cr-Commit-Position: refs/heads/master@{#430559} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/754e439177beb1271aeb6b0153e598fc2eb6d084 Cr-Commit-Position: refs/heads/master@{#430559} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
