Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2019613003:
ServiceWorker: Bypass SW when the script doesn't have fetch handler (Closed)
|
Created:
4 years, 7 months ago by shimazu Modified:
4 years, 6 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. |
DescriptionServiceWorker: 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 #
Messages
Total messages: 24 (9 generated)
Description was changed from ========== 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" ASSERT -> EXPECT and removed overwrapped tests Add a test for has_fetch_handler and add NOTREACHED() Change the default behavior of construction of the ServiceWorkerStatusCode Use SWVersion::DispatchEvent instead of SWVersion::DispatchInstallEvent on SWRegisterJob::DispatchInstallEvent Fix my mistake ServiceWorker: Store the existence of fetch event handler 2/2 true is stored as |data.has_fetch_handler| on current implementation. This information is needed for skipping SW when no fetch handler is registered. BUG=605844 TEST=./out/Releasee/content_browsertests --gtest_filter='*ServiceWorker*' ========== to ========== 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" ==========
shimazu@chromium.org changed reviewers: + falken@chromium.org
PTAL:)
This looks good. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler.cc:366: job_->ForwardToServiceWorker(); Can you factor out these three blocks into a helper function? (Another option may be to skip the SW machinery completely by having ServiceWorkerProviderHost::CreateRequestHandler check if the controlling version has a fetch handler. That may be faster than creating this job that will just bail out. However, if we do that, the update scheduler in ~ServiceWorkerControlleeRequestHandler won't run for subresources. So possibly the page load time is hurt by the update job. That could be worked around by having CreateRequestHandler call DeferScheduleUpdate though, but that's getting a bit ugly.) https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:299: registration_->SetWaitingVersion(version_); SetActiveVersion? https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:330: EXPECT_TRUE(version_->HasControllee()); Maybe add a check that provider_host_->controlling_version == version. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:334: EXPECT_TRUE(version_->update_timer_.IsRunning()); Don't need to test this for this test.
mek@chromium.org changed reviewers: + mek@chromium.org
https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... File content/browser/service_worker/foreign_fetch_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/foreign_fetch_request_handler.cc:201: !active_version->has_fetch_handler()) { This is probably not correct. Foreign fetch events get delivered to the "foreignfetch" event, not the "fetch" event.
PTAL again. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... File content/browser/service_worker/foreign_fetch_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/foreign_fetch_request_handler.cc:201: !active_version->has_fetch_handler()) { On 2016/05/27 16:18:12, Marijn Kruisselbrink wrote: > This is probably not correct. Foreign fetch events get delivered to the > "foreignfetch" event, not the "fetch" event. Sorry, it's true! Done. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler.cc:366: job_->ForwardToServiceWorker(); On 2016/05/27 09:53:34, falken wrote: > Can you factor out these three blocks into a helper function? > > (Another option may be to skip the SW machinery completely by having > ServiceWorkerProviderHost::CreateRequestHandler check if the controlling version > has a fetch handler. That may be faster than creating this job that will just > bail out. However, if we do that, the update scheduler in > ~ServiceWorkerControlleeRequestHandler won't run for subresources. So possibly > the page load time is hurt by the update job. That could be worked around by > having CreateRequestHandler call DeferScheduleUpdate though, but that's getting > a bit ugly.) I roughly checked the performance improvements by implementing this and browsing some pages w/o fetch handler with DevTools, but there seems to be no visible improvements of performance, so I think it's better to take cleanness of codes. Done. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:299: registration_->SetWaitingVersion(version_); On 2016/05/27 09:53:34, falken wrote: > SetActiveVersion? Done. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:330: EXPECT_TRUE(version_->HasControllee()); On 2016/05/27 09:53:34, falken wrote: > Maybe add a check that provider_host_->controlling_version == version. Done. https://codereview.chromium.org/2019613003/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:334: EXPECT_TRUE(version_->update_timer_.IsRunning()); On 2016/05/27 09:53:34, falken wrote: > Don't need to test this for this test. Done.
What is the plan with foreignfetch? I guess since the worker had to call registerForeignFetch to get a foreign fetch scope, it's not that useful to check for a foreignfetch event handler? https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:276: "Forwarded to the ServiceWorker"); nit: can you update this trace to say whether it was forwarded or skipped? https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:331: EXPECT_EQ(version_, provider_host_->controlling_version()); Would it be possible to add a test for the subresource case? https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:332: } We usually add a blink line here, but I won't complain if "git-cl format" formats it without.
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... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:276: "Forwarded to the ServiceWorker"); On 2016/05/30 06:11:29, falken wrote: > nit: can you update this trace to say whether it was forwarded or skipped? Done. https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:331: EXPECT_EQ(version_, provider_host_->controlling_version()); On 2016/05/30 06:11:29, falken wrote: > Would it be possible to add a test for the subresource case? Done. Could you take a look if this null check is enough? https://codereview.chromium.org/2019613003/diff/20001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:332: } On 2016/05/30 06:11:29, falken wrote: > We usually add a blink line here, but I won't complain if "git-cl format" > formats it without. git-cl format accepts w/ and w/o a blank line. Added it.
Thanks, this makes sense. Just some style nits. https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:58: struct ServiceWorkerRequestTestResources { The style guide is to use structs only for "passive objects" that carry data. This object is doing some heavy work with MaybeCreateJob(). It's probably more suitable as a class. https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:73: } Alternatively, you don't need Create and use the ctor directly. The uses below don't seem to demand a pointer. https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:95: job(handler->MaybeCreateJob(request.get(), The coding style says to not do heavy work in constructors. Could you move this MaybeCreateJob() call out of the constructor. Maybe GetJob() can change to MaybeCreateJob()?
https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:58: struct ServiceWorkerRequestTestResources { On 2016/05/31 01:19:51, falken wrote: > The style guide is to use structs only for "passive objects" that carry data. > This object is doing some heavy work with MaybeCreateJob(). It's probably more > suitable as a class. Done. https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:73: } On 2016/05/31 01:19:51, falken wrote: > Alternatively, you don't need Create and use the ctor directly. The uses below > don't seem to demand a pointer. Done. https://codereview.chromium.org/2019613003/diff/40001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:95: job(handler->MaybeCreateJob(request.get(), On 2016/05/31 01:19:51, falken wrote: > The coding style says to not do heavy work in constructors. Could you move this > MaybeCreateJob() call out of the constructor. Maybe GetJob() can change to > MaybeCreateJob()? Done.
lgtm thanks https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... 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... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:88: void RemoveHandler() { handler_.reset(nullptr); } nit: better is ResetHandler()
Thanks for your review! :) I'll commit it. https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler.cc (right): https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler.cc:32: ServiceWorkerVersion* version) { On 2016/05/31 02:24:51, falken wrote: > nit: const SWVersion* Done. https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc (right): https://codereview.chromium.org/2019613003/diff/60001/content/browser/service... content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc:88: void RemoveHandler() { handler_.reset(nullptr); } On 2016/05/31 02:24:51, falken wrote: > nit: better is ResetHandler() Done.
The CQ bit was checked by shimazu@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/2019613003/#ps80001 (title: "Fix the nits")
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
The CQ bit was unchecked by shimazu@chromium.org
The CQ bit was checked by shimazu@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== 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" ========== to ========== 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" ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== 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" ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8a5ede2ab476d57dbce99422d89333b71cd7dcab Cr-Commit-Position: refs/heads/master@{#396787}
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
