Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(199)

Issue 2715663002: ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver. (Closed)

Created:
3 years, 10 months ago by zino
Modified:
3 years, 9 months ago
Reviewers:
haraken, falken, shimazu, nhiroki
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, nhiroki, blink-reviews, kinuko+serviceworker, horo+watch_chromium.org, falken+watch_chromium.org, tzik, kinuko+watch
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver. To implement respondWith() in PaymentRequestEvent, it's better to factor out the FetchEvent related logics from RespondWithObserver and then make it as common class to share common logic between FetchEvent and PaymentRequestEvent. This CL doesn't change any existing behavior. See the other CLs in this series: [1/3] This patch [2/3] https://codereview.chromium.org/2705293010/ (blink side) [3/3] https://codereview.chromium.org/2718013004/ (content side and test) BUG=661608 Review-Url: https://codereview.chromium.org/2715663002 Cr-Commit-Position: refs/heads/master@{#456760} Committed: https://chromium.googlesource.com/chromium/src/+/87bcdeadc1c7bdeb590472d36fc1458e9641274d

Patch Set 1 #

Patch Set 2 : RespondWithObserver #

Total comments: 19

Patch Set 3 : ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver. #

Total comments: 4

Patch Set 4 : ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver. #

Total comments: 4

Patch Set 5 : ServiceWorker: Factor out FetchEvent related logics from RespondWithObserver. #

Messages

Total messages: 33 (20 generated)
zino
PTAL
3 years, 9 months ago (2017-03-07 15:51:39 UTC) #8
shimazu
drive-by comments: https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp (left): https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp#oldcode233 third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:233: ServiceWorkerGlobalScopeClient::from(getExecutionContext()) Could you add a virtual method ...
3 years, 9 months ago (2017-03-08 01:39:03 UTC) #12
nhiroki
Looks good overall. Added some comments about layering. https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp File third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp (right): https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp#newcode144 third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:144: FetchRespondWithObserver::~FetchRespondWithObserver() ...
3 years, 9 months ago (2017-03-08 01:49:04 UTC) #13
falken
https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp (left): https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp#oldcode233 third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:233: ServiceWorkerGlobalScopeClient::from(getExecutionContext()) On 2017/03/08 01:39:03, shimazu (slow) wrote: > Could ...
3 years, 9 months ago (2017-03-09 04:59:00 UTC) #15
zino
Thank you for input and reviews! I addressed your comments. PTAL. https://codereview.chromium.org/2715663002/diff/20001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp File third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp (right): ...
3 years, 9 months ago (2017-03-10 17:57:18 UTC) #20
falken
lgtm https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp File third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp (right): https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp#newcode1 third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:1: // Copyright 2017 The Chromium Authors. All rights ...
3 years, 9 months ago (2017-03-14 07:33:20 UTC) #21
haraken
LGTM
3 years, 9 months ago (2017-03-14 09:01:10 UTC) #22
zino
Thank you for review. https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp File third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp (right): https://codereview.chromium.org/2715663002/diff/40001/third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp#newcode1 third_party/WebKit/Source/modules/serviceworkers/FetchRespondWithObserver.cpp:1: // Copyright 2017 The Chromium ...
3 years, 9 months ago (2017-03-14 14:10:05 UTC) #23
please use gerrit instead
No payments related changes in here, so I'm removing myself from reviewers list.
3 years, 9 months ago (2017-03-14 14:11:39 UTC) #24
nhiroki
lgtm https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right): https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h#newcode22 third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:22: class FetchRespondWithObserver; This is not necessary because FetchRespondWithObserver.h ...
3 years, 9 months ago (2017-03-14 15:10:38 UTC) #26
zino
Thank you for review. https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h File third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h (right): https://codereview.chromium.org/2715663002/diff/60001/third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h#newcode22 third_party/WebKit/Source/modules/serviceworkers/FetchEvent.h:22: class FetchRespondWithObserver; On 2017/03/14 15:10:38, ...
3 years, 9 months ago (2017-03-14 15:56:15 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2715663002/80001
3 years, 9 months ago (2017-03-14 15:57:01 UTC) #30
commit-bot: I haz the power
3 years, 9 months ago (2017-03-14 18:03:42 UTC) #33
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/87bcdeadc1c7bdeb590472d36fc1...

Powered by Google App Engine
This is Rietveld 408576698