Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2077843002:
Service worker: Remove unneeded early stop in FinishRequest (Closed)
|
Created:
4 years, 6 months ago by falken Modified:
4 years, 6 months ago Reviewers:
nhiroki CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, kinuko+serviceworker, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionService worker: Remove unneeded early stop in FinishRequest
This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb
Previously Doom() called StopIfIdle() rather than Stop(). Now (as of
https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one
exception when DevTools is attached. In that case, we stop immediately once
DevTools is detached.
There are two callsites that set status to REDUNDANT directly. This patch
changes one to call Stop() first, as per spec. It's not necessary to call
Doom() because the new version has already been stored which puts the old
version's resources in the purged list.
This is minor cleanup in anticipation of some changes for
https://crbug.com/616331
BUG=616331, 448670
TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html
Committed: https://crrev.com/0276ebb1eae96b10ade52b189ecbaa01ffc8009f
Cr-Commit-Position: refs/heads/master@{#400636}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix a callsiet #
Messages
Total messages: 16 (8 generated)
Description was changed from ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in <https://crrev.com/0c171c6df9b7fbb>. Previously Doom() called StopIfIdle() rather than Stop(). Now (as of <https://crrev.com/b34f1e5b97667e>) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. This is minor cleanup in anticipation of some changes for <https://crbug.com/616331>. BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ========== to ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. This is minor cleanup in anticipation of some changes for <https://crbug.com/616331>. BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ==========
Description was changed from ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. This is minor cleanup in anticipation of some changes for <https://crbug.com/616331>. BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ========== to ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ==========
falken@chromium.org changed reviewers: + nhiroki@chromium.org
r? cq?
lgtm https://codereview.chromium.org/2077843002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2077843002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:602: StopWorkerIfIdle(); Should we remove this, too?
On 2016/06/17 21:28:22, nhiroki (slow until Jun 20) wrote: > lgtm > > https://codereview.chromium.org/2077843002/diff/1/content/browser/service_wor... > File content/browser/service_worker/service_worker_version.cc (right): > > https://codereview.chromium.org/2077843002/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_version.cc:602: > StopWorkerIfIdle(); > Should we remove this, too? Eh we have a direct SetStatus(REDUNDANT) callsite that I overlooked. When a worker goes from installing -> waiting and there is already a waiting worker. I think this one should be StopWorker(), SetStatus(REDUNDANT) just like the other direct REDUNDANT callsite for waiting -> active and there is an active worker. But now I'm wondering why both these aren't just using Doom().
Description was changed from ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ========== to ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDADNT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ==========
PTAL
Description was changed from ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDADNT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ========== to ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDANT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ==========
lgtm
The CQ bit was checked by falken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077843002/20001
Message was sent while issue was closed.
Description was changed from ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDANT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ========== to ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDANT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDANT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html ========== to ========== Service worker: Remove unneeded early stop in FinishRequest This is a vestige of older code added in https://crrev.com/0c171c6df9b7fbb Previously Doom() called StopIfIdle() rather than Stop(). Now (as of https://crrev.com/b34f1e5b97667e) Doom() terminates immediately, with the one exception when DevTools is attached. In that case, we stop immediately once DevTools is detached. There are two callsites that set status to REDUNDANT directly. This patch changes one to call Stop() first, as per spec. It's not necessary to call Doom() because the new version has already been stored which puts the old version's resources in the purged list. This is minor cleanup in anticipation of some changes for https://crbug.com/616331 BUG=616331,448670 TEST=third_party/WebKit/Tools/Scripts/run-webkit-tests --repeat-each=50 http/tests/serviceworker/register-same-scope-different-script-url.html Committed: https://crrev.com/0276ebb1eae96b10ade52b189ecbaa01ffc8009f Cr-Commit-Position: refs/heads/master@{#400636} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0276ebb1eae96b10ade52b189ecbaa01ffc8009f Cr-Commit-Position: refs/heads/master@{#400636} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
