Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(89)

Issue 2019613003: ServiceWorker: Bypass SW when the script doesn't have fetch handler (Closed)

Created:
4 years, 8 months ago by shimazu
Modified:
4 years, 8 months ago
CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, tzik
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ServiceWorker: Bypass SW when the script doesn't have fetch handler When the script doesn't have a fetch handler, a request will immediately go to the network after passed to SW in current implementation. However, if SW is down, this procedure will wait for waking SW up though SW doesn't have fetch handler. This patch enables the request to directly go to network bypassing SW in this situation. BUG=605844 TEST=/out/Release/content_unittest --gtest_filter="ServiceWorkerControlleeRequestHandlerTest.FallbackWithNoFetchHandler" Committed: https://crrev.com/8a5ede2ab476d57dbce99422d89333b71cd7dcab Cr-Commit-Position: refs/heads/master@{#396787}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Factor out the forwarding codes and fix the test and foreignfetch #

Total comments: 6

Patch Set 3 : Add a test for subresources and fix the nits #

Total comments: 6

Patch Set 4 : Change the struct to class and move codes out of the ctor #

Total comments: 4

Patch Set 5 : Fix the nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -66 lines) Patch
M content/browser/service_worker/service_worker_controllee_request_handler.cc View 1 2 3 4 4 chunks +25 lines, -7 lines 0 comments Download
M content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc View 1 2 3 4 10 chunks +90 lines, -59 lines 0 comments Download
M content/browser/service_worker/service_worker_version.h View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (9 generated)
shimazu
PTAL:)
4 years, 8 months ago (2016-05-27 05:58:42 UTC) #3
falken
This looks good. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_worker/service_worker_controllee_request_handler.cc File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_worker/service_worker_controllee_request_handler.cc#newcode366 content/browser/service_worker/service_worker_controllee_request_handler.cc:366: job_->ForwardToServiceWorker(); Can you factor out these ...
4 years, 8 months ago (2016-05-27 09:53:34 UTC) #4
Marijn Kruisselbrink
https://codereview.chromium.org/2019613003/diff/1/content/browser/service_worker/foreign_fetch_request_handler.cc File content/browser/service_worker/foreign_fetch_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_worker/foreign_fetch_request_handler.cc#newcode201 content/browser/service_worker/foreign_fetch_request_handler.cc:201: !active_version->has_fetch_handler()) { This is probably not correct. Foreign fetch ...
4 years, 8 months ago (2016-05-27 16:18:12 UTC) #6
shimazu
PTAL again. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_worker/foreign_fetch_request_handler.cc File content/browser/service_worker/foreign_fetch_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_worker/foreign_fetch_request_handler.cc#newcode201 content/browser/service_worker/foreign_fetch_request_handler.cc:201: !active_version->has_fetch_handler()) { On 2016/05/27 16:18:12, Marijn Kruisselbrink ...
4 years, 8 months ago (2016-05-30 05:41:59 UTC) #7
falken
What is the plan with foreignfetch? I guess since the worker had to call registerForeignFetch ...
4 years, 8 months ago (2016-05-30 06:11:29 UTC) #8
shimazu
As you said, I've understood this optimization is not necessary for the foreignfetch. Thanks. https://codereview.chromium.org/2019613003/diff/20001/content/browser/service_worker/service_worker_controllee_request_handler.cc ...
4 years, 8 months ago (2016-05-30 09:45:33 UTC) #9
falken
Thanks, this makes sense. Just some style nits. https://codereview.chromium.org/2019613003/diff/40001/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/40001/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc#newcode58 content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:58: struct ...
4 years, 8 months ago (2016-05-31 01:19:51 UTC) #10
shimazu
https://codereview.chromium.org/2019613003/diff/40001/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/40001/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc#newcode58 content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:58: struct ServiceWorkerRequestTestResources { On 2016/05/31 01:19:51, falken wrote: > ...
4 years, 8 months ago (2016-05-31 02:18:10 UTC) #11
falken
lgtm thanks https://codereview.chromium.org/2019613003/diff/60001/content/browser/service_worker/service_worker_controllee_request_handler.cc File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/60001/content/browser/service_worker/service_worker_controllee_request_handler.cc#newcode32 content/browser/service_worker/service_worker_controllee_request_handler.cc:32: ServiceWorkerVersion* version) { nit: const SWVersion* https://codereview.chromium.org/2019613003/diff/60001/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc ...
4 years, 8 months ago (2016-05-31 02:24:51 UTC) #12
shimazu
Thanks for your review! :) I'll commit it. https://codereview.chromium.org/2019613003/diff/60001/content/browser/service_worker/service_worker_controllee_request_handler.cc File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/60001/content/browser/service_worker/service_worker_controllee_request_handler.cc#newcode32 content/browser/service_worker/service_worker_controllee_request_handler.cc:32: ServiceWorkerVersion* ...
4 years, 8 months ago (2016-05-31 04:15:10 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019613003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2019613003/80001
4 years, 8 months ago (2016-05-31 04:15:34 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019613003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2019613003/80001
4 years, 8 months ago (2016-05-31 04:57:29 UTC) #19
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 8 months ago (2016-05-31 05:28:55 UTC) #21
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/8a5ede2ab476d57dbce99422d89333b71cd7dcab Cr-Commit-Position: refs/heads/master@{#396787}
4 years, 8 months ago (2016-05-31 05:30:15 UTC) #23
shimazu
4 years, 7 months ago (2016-06-21 06:55:38 UTC) #24
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2080793003/ by shimazu@chromium.org.

The reason for reverting is: See: http://crbug.com/621788.

Powered by Google App Engine
This is Rietveld 408576698