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

Issue 571843003: [ServivceWorker] Treat rejecting respondWith as a Network Error (Closed)

Created:
6 years, 3 months ago by horo
Modified:
6 years, 3 months ago
CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, mkwst+moarreviews_chromium.org, falken, kinuko+serviceworker, horo+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

[ServivceWorker] Treat rejecting respondWith as a Network Error Chromium side change is this: https://codereview.chromium.org/574643003/ BUG=411173 TEST=layouttest http/tests/serviceworker/ Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182147

Patch Set 1 #

Patch Set 2 : #

Total comments: 7

Patch Set 3 : incorporated Mike's comment #

Total comments: 8

Patch Set 4 : incorporated yhirano's comment #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -39 lines) Patch
M LayoutTests/http/tests/serviceworker/fetch-event.html View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M Source/modules/serviceworkers/RespondWithObserver.h View 1 chunk +0 lines, -4 lines 0 comments Download
M Source/modules/serviceworkers/RespondWithObserver.cpp View 1 2 3 3 chunks +17 lines, -16 lines 5 comments Download
M Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h View 1 2 3 2 chunks +5 lines, -3 lines 0 comments Download
M Source/web/ServiceWorkerGlobalScopeClientImpl.h View 2 chunks +3 lines, -1 line 0 comments Download
M Source/web/ServiceWorkerGlobalScopeClientImpl.cpp View 1 chunk +6 lines, -13 lines 0 comments Download

Messages

Total messages: 24 (6 generated)
horo
yhirano@
6 years, 3 months ago (2014-09-16 09:01:42 UTC) #2
horo
yhirano@ Could you please review?
6 years, 3 months ago (2014-09-16 09:01:53 UTC) #3
Mike West
Drive-by LGTM with a few nits. You'll still need a modules/serviceworkers/ and web/ OWNER to ...
6 years, 3 months ago (2014-09-16 11:01:38 UTC) #5
horo
Thank you for your comments. https://codereview.chromium.org/571843003/diff/20001/Source/modules/serviceworkers/RespondWithObserver.cpp File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/571843003/diff/20001/Source/modules/serviceworkers/RespondWithObserver.cpp#newcode99 Source/modules/serviceworkers/RespondWithObserver.cpp:99: if (!executionContext()) On 2014/09/16 ...
6 years, 3 months ago (2014-09-16 11:17:41 UTC) #6
yhirano
lgtm with nits. https://codereview.chromium.org/571843003/diff/40001/LayoutTests/http/tests/serviceworker/fetch-event.html File LayoutTests/http/tests/serviceworker/fetch-event.html (right): https://codereview.chromium.org/571843003/diff/40001/LayoutTests/http/tests/serviceworker/fetch-event.html#newcode109 LayoutTests/http/tests/serviceworker/fetch-event.html:109: 'Response should NetworkError'); s/should/should be a/ ...
6 years, 3 months ago (2014-09-17 02:25:31 UTC) #7
horo
https://codereview.chromium.org/571843003/diff/40001/LayoutTests/http/tests/serviceworker/fetch-event.html File LayoutTests/http/tests/serviceworker/fetch-event.html (right): https://codereview.chromium.org/571843003/diff/40001/LayoutTests/http/tests/serviceworker/fetch-event.html#newcode109 LayoutTests/http/tests/serviceworker/fetch-event.html:109: 'Response should NetworkError'); On 2014/09/17 02:25:31, yhirano wrote: > ...
6 years, 3 months ago (2014-09-17 04:45:20 UTC) #8
horo
tkent@ Could you please review Source/web/ServiceWorkerGlobalScopeClientImpl.*?
6 years, 3 months ago (2014-09-17 04:46:53 UTC) #10
tkent
On 2014/09/17 04:46:53, horo wrote: > tkent@ > Could you please review Source/web/ServiceWorkerGlobalScopeClientImpl.*? Please take ...
6 years, 3 months ago (2014-09-17 04:54:38 UTC) #11
horo
> Please take an l-g-t-m by a Service Worker engineer before asking owner > approval. ...
6 years, 3 months ago (2014-09-17 05:15:22 UTC) #12
horo
nhiroki@ Could you please review this?
6 years, 3 months ago (2014-09-17 05:15:47 UTC) #14
nhiroki
Looks good. Let me ask a question about handling of the execution context... https://codereview.chromium.org/571843003/diff/60001/Source/modules/serviceworkers/RespondWithObserver.cpp File ...
6 years, 3 months ago (2014-09-17 05:42:46 UTC) #15
horo
https://codereview.chromium.org/571843003/diff/60001/Source/modules/serviceworkers/RespondWithObserver.cpp File Source/modules/serviceworkers/RespondWithObserver.cpp (right): https://codereview.chromium.org/571843003/diff/60001/Source/modules/serviceworkers/RespondWithObserver.cpp#newcode78 Source/modules/serviceworkers/RespondWithObserver.cpp:78: ASSERT(executionContext()); On 2014/09/17 05:42:46, nhiroki wrote: > Is this ...
6 years, 3 months ago (2014-09-17 07:45:26 UTC) #16
tkent
lgtm
6 years, 3 months ago (2014-09-17 08:30:40 UTC) #17
nhiroki
On 2014/09/17 07:45:26, horo wrote: > https://codereview.chromium.org/571843003/diff/60001/Source/modules/serviceworkers/RespondWithObserver.cpp > File Source/modules/serviceworkers/RespondWithObserver.cpp (right): > > https://codereview.chromium.org/571843003/diff/60001/Source/modules/serviceworkers/RespondWithObserver.cpp#newcode78 > ...
6 years, 3 months ago (2014-09-17 08:46:29 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/571843003/60001
6 years, 3 months ago (2014-09-17 09:38:21 UTC) #20
commit-bot: I haz the power
Committed patchset #4 (id:60001) as 182147
6 years, 3 months ago (2014-09-17 10:41:13 UTC) #21
horo
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/580993002/ by horo@chromium.org. ...
6 years, 3 months ago (2014-09-18 00:01:40 UTC) #22
kinuko
6 years, 3 months ago (2014-09-23 16:44:39 UTC) #24
Message was sent while issue was closed.
https://codereview.chromium.org/571843003/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/571843003/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/RespondWithObserver.cpp:78:
ASSERT(executionContext());
On 2014/09/17 07:45:26, horo wrote:
> On 2014/09/17 05:42:46, nhiroki wrote:
> > Is this always true? Who ensures this condition?
> 
> In the constructor of RespondWithObserver, ContextLifecycleObserver is
> initialized with the ServiceWorkerGlobalScope object.
> So executionContext is cleared only when ServiceWorkerGlobalScope is deleted.
> But ServiceWorkerGlobalScope is deleted in WorkerThread::cleanup() after the
> worker thread is stopped.
> So executionContext() must always returns non-NULL.

Drive-by nit: if executionContext() can never be null it'd be probably better to
stop inheriting this from ContextLifecycleObserver and add a comment about
lifetime issue?

Powered by Google App Engine
This is Rietveld 408576698