ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"

Committed: https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e
Cr-Commit-Position: refs/heads/master@{#402710}

[shimazu, 2016-06-02 10:11:57]

shimazu@chromium.org changed reviewers:
+ falken@chromium.org, keishi@chromium.org, palmer@chromium.org,
rkaplow@chromium.org

[shimazu, 2016-06-02 10:11:58]

palmer@chromium.org: Please review changes in IPC

keishi@chromium.org: Please review changes in third_party/WebKit/public/web

rkaplow@chromium.org: Please review changes in histograms.xml

falken@chromium.org: Please review changes in this CL overall

[rkaplow, 2016-06-02 14:37:09]

lgtm

histogram lgtm

[Marijn Kruisselbrink, 2016-06-02 16:58:54]

mek@chromium.org changed reviewers:
+ mek@chromium.org

[Marijn Kruisselbrink, 2016-06-02 16:58:55]

just some random thoughts on possible ways this could be done without having
SWVersion know about fetch details (or at least explaining what I was thinking
when I wrote the SWVersion dispatchEvent related logic).

https://codereview.chromium.org/2034663002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.h:260: void
DispatchFetchEvent(int response_request_id,
It seems very unfortunate to go back to having code specific for certain events
in ServiceWorkerVersion....

First of all I don't understand why you need two request ids? It is perfectly
fine from SWVersions point of view to have multiple replies for a single
request. The main limitation you'd run into is that currently DispatchEvent (and
SWVersion::EventResponseHandler) assume all replies for a particular request id
are the same type of IPC message. One hacky way (but maybe not worse than adding
fetch specific logic to SWVersion) would thus be to use one IPC message for both
the waitUntil and the respondWith result (you'd need to add some kind of field
to the message then to be able to distinguish the two).

Another way would be to add a lower-level overload of DispatchEvent that would
allow specifying a custom EmbeddedWorkerInstance::Listener instance rather than
have SWVersion creates its own EventResponseHandler. That custom listener would
have to duplicate some of the code in EventResponseHandler, which might be
unfortunate though. Or SWVersion could expose EventResponseHandler as an
abstract (non-templated) base-class that just deals with the add/remove listener
part and checking of request_id, with a separate internal
BasicEventResponseHandler that adds the dispatch-to-callback bit it currently
has (and for fetch you'd then have a different subclass that has slightly more
complicated dispatch logic to support multiple IPC types).

Or some other way DispatchEvent could be made more flexible to allow multiple
IPC types (perhaps as simple as an overload to support exactly two IPC types;
which should be fairly straight forward).

[shimazu, 2016-06-03 04:48:40]

Thanks! I also think it's unfortunate to create the fetch-specific method, so
that's very helpful.

The reason why I didn't use one IPC callback and I wanted to integrated the
dispatch method into SWVersion is that SWFetchDispatcher will die just after it
gets the response from fetch event. The lifecycle of fetch event itself can be
extended until SWVersion terminates. 
This lifecycle issue is the cause of breaking abstraction in this CL, so
creating and passing a custom EventResponseHandler which has different lifecycle
from SWFetchDispatcher looks good. 


Ah, I'm now thinking it might be an another good way to separate dispatching a
request and registering callbacks to SWVersion. In this case, SWVersion
internally creates EventResponseHandler for each callback, and there is no need
to expose EventResponseHandler.
This enables us to easily use one-request multiple-response IPC, and it won't
inject any event specific codes into SWVersion. I'll implement it.


On 2016/06/02 16:58:55, Marijn Kruisselbrink wrote:
> just some random thoughts on possible ways this could be done without having
> SWVersion know about fetch details (or at least explaining what I was thinking
> when I wrote the SWVersion dispatchEvent related logic).
> 
>
https://codereview.chromium.org/2034663002/diff/1/content/browser/service_wor...
> File content/browser/service_worker/service_worker_version.h (right):
> 
>
https://codereview.chromium.org/2034663002/diff/1/content/browser/service_wor...
> content/browser/service_worker/service_worker_version.h:260: void
> DispatchFetchEvent(int response_request_id,
> It seems very unfortunate to go back to having code specific for certain
events
> in ServiceWorkerVersion....
> 
> First of all I don't understand why you need two request ids? It is perfectly
> fine from SWVersions point of view to have multiple replies for a single
> request. The main limitation you'd run into is that currently DispatchEvent
(and
> SWVersion::EventResponseHandler) assume all replies for a particular request
id
> are the same type of IPC message. One hacky way (but maybe not worse than
adding
> fetch specific logic to SWVersion) would thus be to use one IPC message for
both
> the waitUntil and the respondWith result (you'd need to add some kind of field
> to the message then to be able to distinguish the two).
> 
> Another way would be to add a lower-level overload of DispatchEvent that would
> allow specifying a custom EmbeddedWorkerInstance::Listener instance rather
than
> have SWVersion creates its own EventResponseHandler. That custom listener
would
> have to duplicate some of the code in EventResponseHandler, which might be
> unfortunate though. Or SWVersion could expose EventResponseHandler as an
> abstract (non-templated) base-class that just deals with the add/remove
listener
> part and checking of request_id, with a separate internal
> BasicEventResponseHandler that adds the dispatch-to-callback bit it currently
> has (and for fetch you'd then have a different subclass that has slightly more
> complicated dispatch logic to support multiple IPC types).
> 
> Or some other way DispatchEvent could be made more flexible to allow multiple
> IPC types (perhaps as simple as an overload to support exactly two IPC types;
> which should be fairly straight forward).

[shimazu, 2016-06-03 10:59:10]

Following mek@'s comment, I omitted DispatchFetchEvent and  separated
DispatchEvent into two parts:
 - RegisterRequestCallback (called multiple times)
 - DispatchEvent (called once)

I expect this provides a simple way to implement one-request/multiple-reply IPC.

PTAL again! :)

[shimazu, 2016-06-03 11:01:17]

Description was changed from

==========
ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests
--gtest_filter="ServiceWorkerURLRequestJobTest.*"
==========

to

==========
ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests
--gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"
==========

[falken, 2016-06-06 10:01:40]

One round of comments. Can you rebase the patch with ToT? I want to load it in
my editor and play around but there were merge conflicts.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:234: int
finish_request_id,
These names should match the ones in the header file.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:916:
int finish_request_id_;
Can you initialize these?

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:922:
SetUpWithHelper(helper);
(We should have made SetUpWithHelper take a unique_ptr as it takes ownership.
But we can fix that later.)

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:939:
EXPECT_EQ(std::string(), info->response_cache_storage_cache_name());
Does it make sense to factor out this block of expectations into a function? It
looks copypasted from the above tests, with the addition of the cache_storage
expectations.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:244: const
RequestIDs&... rest);
Does RequestIDs need to be a template type? I'd rather keep this dump and simple
if possible.
Can you just have one-int and two-int overloaded functions? Or take in a vector
of ints?

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:415: bool is_dispatched;
Need to initialize this.

https://codereview.chromium.org/2034663002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2034663002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:138: void
respondFetchEvent(
This seems grammatically incorrect. respondToFetchEvent?

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:134: void
ServiceWorkerGlobalScopeProxy::dispatchFetchEvent(int responseEventID, int
finishEventID, const WebServiceWorkerRequest& webRequest)
I feel these names could be improved. responseEvent/finishEvent read to me like
fetchEvent/installEvent/activateEvent.

Maybe just responseID and eventFinishID?

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:78: void
dispatchForeignFetchEvent(int, int, const WebServiceWorkerRequest&) override;
we need variable names for these ints now that it's ambiguous which is which

[shimazu, 2016-06-07 02:27:41]

Thanks for your review. I rebased and replied to some comments.
For the remaining, I'll fix after your 2nd time comments.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:939:
EXPECT_EQ(std::string(), info->response_cache_storage_cache_name());
On 2016/06/06 10:01:40, falken wrote:
> Does it make sense to factor out this block of expectations into a function?
It
> looks copypasted from the above tests, with the addition of the cache_storage
> expectations.

I thought this should work completely the same with "Simple", so I put just the
same expectations. But, yes, I also think this is not so much valuable for this
test.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:244: const
RequestIDs&... rest);
On 2016/06/06 10:01:40, falken wrote:
> Does RequestIDs need to be a template type? I'd rather keep this dump and
simple
> if possible.
> Can you just have one-int and two-int overloaded functions? Or take in a
vector
> of ints?

Yes, vector<int> and two-int arguments will also work, but I thought
variable-length arguments is better because it's more general. This can be used
for any number of responses. Moreover, practically we will use this for only two
or three replies, so I thought template is more handy than vector<int> for the
users of this function.

[falken, 2016-06-07 07:56:17]

Thanks for rebasing, I actually don't have further comments now.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:244: const
RequestIDs&... rest);
On 2016/06/07 02:27:40, makoto wrote:
> On 2016/06/06 10:01:40, falken wrote:
> > Does RequestIDs need to be a template type? I'd rather keep this dump and
> simple
> > if possible.
> > Can you just have one-int and two-int overloaded functions? Or take in a
> vector
> > of ints?
> 
> Yes, vector<int> and two-int arguments will also work, but I thought
> variable-length arguments is better because it's more general. This can be
used
> for any number of responses. Moreover, practically we will use this for only
two
> or three replies, so I thought template is more handy than vector<int> for the
> users of this function.

I'd prefer either vector<int> or two-int over the template. It'll keep the code
and stacktraces simple. There's not many uses of typename... in our codebase.

[shimazu, 2016-06-09 05:44:21]

Updated the patch.
PTAL again! :)

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:234: int
finish_request_id,
On 2016/06/06 10:01:40, falken wrote:
> These names should match the ones in the header file.

Done.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:916:
int finish_request_id_;
On 2016/06/06 10:01:40, falken wrote:
> Can you initialize these?

Done.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:922:
SetUpWithHelper(helper);
On 2016/06/06 10:01:40, falken wrote:
> (We should have made SetUpWithHelper take a unique_ptr as it takes ownership.
> But we can fix that later.)

Acknowledged.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:244: const
RequestIDs&... rest);
On 2016/06/07 07:56:17, falken wrote:
> On 2016/06/07 02:27:40, makoto wrote:
> > On 2016/06/06 10:01:40, falken wrote:
> > > Does RequestIDs need to be a template type? I'd rather keep this dump and
> > simple
> > > if possible.
> > > Can you just have one-int and two-int overloaded functions? Or take in a
> > vector
> > > of ints?
> > 
> > Yes, vector<int> and two-int arguments will also work, but I thought
> > variable-length arguments is better because it's more general. This can be
> used
> > for any number of responses. Moreover, practically we will use this for only
> two
> > or three replies, so I thought template is more handy than vector<int> for
the
> > users of this function.
> 
> I'd prefer either vector<int> or two-int over the template. It'll keep the
code
> and stacktraces simple. There's not many uses of typename... in our codebase.

I see. I don't have so much preference for the template. As you said, vector<>
is simpler stacktrace and codes.
Done.

https://codereview.chromium.org/2034663002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.h:415: bool is_dispatched;
On 2016/06/06 10:01:40, falken wrote:
> Need to initialize this.

Done.

https://codereview.chromium.org/2034663002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2034663002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:138: void
respondFetchEvent(
On 2016/06/06 10:01:40, falken wrote:
> This seems grammatically incorrect. respondToFetchEvent?

Done.

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:134: void
ServiceWorkerGlobalScopeProxy::dispatchFetchEvent(int responseEventID, int
finishEventID, const WebServiceWorkerRequest& webRequest)
On 2016/06/06 10:01:40, falken wrote:
> I feel these names could be improved. responseEvent/finishEvent read to me
like
> fetchEvent/installEvent/activateEvent.
> 
> Maybe just responseID and eventFinishID?

Done.

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/2034663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:78: void
dispatchForeignFetchEvent(int, int, const WebServiceWorkerRequest&) override;
On 2016/06/06 10:01:40, falken wrote:
> we need variable names for these ints now that it's ambiguous which is which

Done.

[falken, 2016-06-09 19:28:48]

I'm still a bit concerned with the naming. We should choose a sensible and
consistent one so we don't get confused.

It looks like for the first time, we're saying an "event" can have multiple
"requests".

In the case of the fetch event, we're saying there is a "response request" and
an "event finish request".

I'm not sure I like that. Can we do better?

I think previously we used request and event interchangeably. Maybe "request"
isn't the best word here.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:164: int
event_request_id,
There is some inconsistency throughout this patch of "event_request_id" vs
"finish_request_id".

Of the two, I think prefer "finish_request_id".

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:98: FETCH_WAITUNTIL =
16,
It's unfortunate that WAITUNTIL is not an "event" since this is an EventType.
Should this be RequestType? I'm not sure the naming is making sense.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:236: // callbacks
associated with the |request_ids| are called.
Let's mention these are the error callbacks from StartRequest. It's a bit
confusing there are callbacks passed to both StartRequest and
RegisterRequestCallback.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:237: // This method
assumes you already registered callbacks to receive the IPCs.
The reader might think "to receive the IPCs" means on the renderer-side, since
this function takes an IPC::Message.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:239: // response
callback which is registered beforehand.
...and I think this is now somewhat redundant with the previous sentence.

Maybe it's clearer to say "Use RegisterRequestCallback or RegisterSimpleRequest
to register a callback to receive messages sent back in response to this event.
The callback must be registered before  DispatchEvent is called."

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:245: // receives and
handles the message sent back from this SWVersion.
It maybe a bit vague what "corresponding" means and what "the message" is. Also,
SWVersion is a browser-side concept. Maybe:

"Registers a callback to receive messages sent back from the service worker in
response to |request_id|."

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:255: // a
blink::WebServiceWorkerEventResult field. The result is converted to
It's a bit hard to follow this: it's not clear that the third sentence is
describing what the "simple callback" from the first sentence does.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:265: // DispatchEvent.
For simple events where the full functionality of
The "integrating..." part is a bit awkward wording. I'd maybe say "A wrapper
method equivalent to one RegisterSimpleRequest and one DispatchEvent".

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
(right):

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:73:
// Calling respondFetchEvent without response means no response was
respondToFetchEvent

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:78: void
dispatchForeignFetchEvent(int responseID, int eventFinishID, const
WebServiceWorkerRequest&) override;
This naming seems inconsistent with the browserside "response_request_id" and
"finish_request_id"/"event_request_id".

[shimazu, 2016-06-10 10:26:50]

Thanks for your review!
PTAL again.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:164: int
event_request_id,
On 2016/06/09 19:28:48, falken wrote:
> There is some inconsistency throughout this patch of "event_request_id" vs
> "finish_request_id".
> 
> Of the two, I think prefer "finish_request_id".

Done.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:98: FETCH_WAITUNTIL =
16,
On 2016/06/09 19:28:48, falken wrote:
> It's unfortunate that WAITUNTIL is not an "event" since this is an EventType.
> Should this be RequestType? I'm not sure the naming is making sense.

Yes, I know. Hmm, I don't have good name now. 
It's mostly used as "EventType". EventType is currently used for
StartRequest(WithCustomTimer) and RecordStartWorkerResult.
RecordStartWorker is called in StartWorker, RunAfterStartWorker,
DidEnsureLiveRegistrationForStartWorker, and I fell it's a bit odd for
StartWorker to get a RequestType because StartWorker itself doesn't request a
message sent back from the service worker.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:236: // callbacks
associated with the |request_ids| are called.
On 2016/06/09 19:28:48, falken wrote:
> Let's mention these are the error callbacks from StartRequest. It's a bit
> confusing there are callbacks passed to both StartRequest and
> RegisterRequestCallback.

Done.

I think it's a bit confusing, too. It looks better to integrate StartRequest
into DispatchEvent, but probably StartRequest needs to have the error callback
because BackgroundSync is using Mojo instead of using DispatchEvent.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:237: // This method
assumes you already registered callbacks to receive the IPCs.
On 2016/06/09 19:28:48, falken wrote:
> The reader might think "to receive the IPCs" means on the renderer-side, since
> this function takes an IPC::Message.

Done.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:239: // response
callback which is registered beforehand.
On 2016/06/09 19:28:48, falken wrote:
> ...and I think this is now somewhat redundant with the previous sentence.
> 
> Maybe it's clearer to say "Use RegisterRequestCallback or
RegisterSimpleRequest
> to register a callback to receive messages sent back in response to this
event.
> The callback must be registered before  DispatchEvent is called."

Done.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:245: // receives and
handles the message sent back from this SWVersion.
On 2016/06/09 19:28:48, falken wrote:
> It maybe a bit vague what "corresponding" means and what "the message" is.
Also,
> SWVersion is a browser-side concept. Maybe:
> 
> "Registers a callback to receive messages sent back from the service worker in
> response to |request_id|."

Done.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:255: // a
blink::WebServiceWorkerEventResult field. The result is converted to
On 2016/06/09 19:28:48, falken wrote:
> It's a bit hard to follow this: it's not clear that the third sentence is
> describing what the "simple callback" from the first sentence does.

Done.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:265: // DispatchEvent.
For simple events where the full functionality of
On 2016/06/09 19:28:48, falken wrote:
> The "integrating..." part is a bit awkward wording. I'd maybe say "A wrapper
> method equivalent to one RegisterSimpleRequest and one DispatchEvent".

Done.

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h
(right):

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:73:
// Calling respondFetchEvent without response means no response was
On 2016/06/09 19:28:48, falken wrote:
> respondToFetchEvent

Done.

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:78: void
dispatchForeignFetchEvent(int responseID, int eventFinishID, const
WebServiceWorkerRequest&) override;
On 2016/06/09 19:28:48, falken wrote:
> This naming seems inconsistent with the browserside "response_request_id" and
> "finish_request_id"/"event_request_id".

Done.
I think so too. I think the other eventIDs are also named as request_id in
chromium-side. Is it okay? or should I rename them too?

[palmer, 2016-06-10 19:23:17]

lgtm

[Marijn Kruisselbrink, 2016-06-15 12:37:07]

I like this approach, just some minor nits

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:102:
ServiceWorkerMetrics::EventType::FETCH_WAITUNTIL,
There should probably be separate FETCH_WAITUNTIL and FOREIGNFETCH_WAITUNTIL
event types? Or maybe combining them in stats doesn't matter, but then the
histogram description probably should be updated to mention both fetch and
foreign fetch.

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:582:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Now this code is in the .cc file you could use RunSoon rather than the explicit
base::ThreadTaskRunnerHandle::Get()->PostTask() thing.

[falken, 2016-06-16 06:23:12]

This is looking good mod mek's comments and some naming inconsistencies.

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:98: FETCH_WAITUNTIL =
16,
On 2016/06/10 10:26:49, makoto (ooo until Jun 20) wrote:
> On 2016/06/09 19:28:48, falken wrote:
> > It's unfortunate that WAITUNTIL is not an "event" since this is an
EventType.
> > Should this be RequestType? I'm not sure the naming is making sense.
> 
> Yes, I know. Hmm, I don't have good name now. 
> It's mostly used as "EventType". EventType is currently used for
> StartRequest(WithCustomTimer) and RecordStartWorkerResult.
> RecordStartWorker is called in StartWorker, RunAfterStartWorker,
> DidEnsureLiveRegistrationForStartWorker, and I fell it's a bit odd for
> StartWorker to get a RequestType because StartWorker itself doesn't request a
> message sent back from the service worker.

I don't think it's so odd, since StartWorker itself doesn't dispatch an event
either. But this enum is also not really exactly "request" since it also splits
up fetch into MAIN_FRAME, SUB_FRAME, etc. for UMA purposes.

Anyway, I think we can go ahead and just put a comment explaining what EventType
is and how it's mostly events but also requests, since events typically have a
1:1 correspondence with requests except for the fetch event.

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:78: void
dispatchForeignFetchEvent(int responseID, int eventFinishID, const
WebServiceWorkerRequest&) override;
On 2016/06/10 10:26:49, makoto (ooo until Jun 20) wrote:
> On 2016/06/09 19:28:48, falken wrote:
> > This naming seems inconsistent with the browserside "response_request_id"
and
> > "finish_request_id"/"event_request_id".
> 
> Done.
> I think so too. I think the other eventIDs are also named as request_id in
> chromium-side. Is it okay? or should I rename them too?

I'd like the fetch/foreignfetch names to be consistent between chromium and
blink side. The other events can have the event/request discrepancy for now.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:919: 
Can you add a lightweight comment explaining what this tests?

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:231: // StartRequest are
called.
"the error callbacks that were associated with |request_ids| via StartRequest
are called".

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:251: // handler
associated with the request_id which is registered at StartRequest.
"registered by"

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:1473:
ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_NETWORK;  // dummy
Lately it's common to use status = SERVICE_WORKER_ERROR_MAX_VALUE for this (then
you don't need "dummy")

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:1477:
version_->StartWorker(ServiceWorkerMetrics::EventType::SYNC,
nit: I'd use UNKNOWN for a dummy event type.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:1536:
EXPECT_TRUE(version_->FinishRequest(request_id2, true));
Nice test!

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
content/common/service_worker/service_worker_messages.h:257: int /* request_id
*/,
response_id

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
content/common/service_worker/service_worker_messages.h:261: int /* request_id
*/,
event_finish_id

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
content/common/service_worker/service_worker_messages.h:474: int /*
finish_request_id */,
I think in your naming system, this is actually "int response_id, int
event_finish_id" (based on embedded_worker_test_helper.h)

https://codereview.chromium.org/2034663002/diff/120001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2034663002/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:142:
blink::WebServiceWorkerEventResult result) override;
WebServiceWorkerContextClient is using responseID and eventFinishID for these.

https://codereview.chromium.org/2034663002/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:203: int
finish_request_id,
Probably this should be response_id and event_finish_id
(.cc file needs changes too)

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:64: //
WaitUntilObserver::Thenfunction but RespondWith needs to have a strong
ThenFunction

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:149:
waitUntilObserver->didDispatchEvent(dispatchResult !=
DispatchEventResult::NotCanceled);
Why do we need to check if NotCanceled? What's the consequence of passing true
vs false to didDispatchEvent here?

Also, is waitUntil(Promise.resolve()) any different than
waitUntil(Promise.reject()) for a fetch event? I think there shouldn't be a
difference. Can we have a test case for that?

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
(right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h:60:
virtual void dispatchForeignFetchEvent(int responseEventID, int finishEventID,
const WebServiceWorkerRequest& webRequest) = 0;
responseID and eventFinishID for consistency with the rest of the patch

[shimazu, 2016-06-21 02:43:38]

Thanks for the review. PTAL again:)

rkaplow@, I modified the UMA part again, so could you take a look?

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:98: FETCH_WAITUNTIL =
16,
On 2016/06/16 06:23:09, falken wrote:
> On 2016/06/10 10:26:49, makoto (ooo until Jun 20) wrote:
> > On 2016/06/09 19:28:48, falken wrote:
> > > It's unfortunate that WAITUNTIL is not an "event" since this is an
> EventType.
> > > Should this be RequestType? I'm not sure the naming is making sense.
> > 
> > Yes, I know. Hmm, I don't have good name now. 
> > It's mostly used as "EventType". EventType is currently used for
> > StartRequest(WithCustomTimer) and RecordStartWorkerResult.
> > RecordStartWorker is called in StartWorker, RunAfterStartWorker,
> > DidEnsureLiveRegistrationForStartWorker, and I fell it's a bit odd for
> > StartWorker to get a RequestType because StartWorker itself doesn't request
a
> > message sent back from the service worker.
> 
> I don't think it's so odd, since StartWorker itself doesn't dispatch an event
> either. But this enum is also not really exactly "request" since it also
splits
> up fetch into MAIN_FRAME, SUB_FRAME, etc. for UMA purposes.
> 
> Anyway, I think we can go ahead and just put a comment explaining what
EventType
> is and how it's mostly events but also requests, since events typically have a
> 1:1 correspondence with requests except for the fetch event.

Done.

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/2034663002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:78: void
dispatchForeignFetchEvent(int responseID, int eventFinishID, const
WebServiceWorkerRequest&) override;
On 2016/06/16 06:23:10, falken wrote:
> On 2016/06/10 10:26:49, makoto (ooo until Jun 20) wrote:
> > On 2016/06/09 19:28:48, falken wrote:
> > > This naming seems inconsistent with the browserside "response_request_id"
> and
> > > "finish_request_id"/"event_request_id".
> > 
> > Done.
> > I think so too. I think the other eventIDs are also named as request_id in
> > chromium-side. Is it okay? or should I rename them too?
> 
> I'd like the fetch/foreignfetch names to be consistent between chromium and
> blink side. The other events can have the event/request discrepancy for now.

Acknowledged.

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:102:
ServiceWorkerMetrics::EventType::FETCH_WAITUNTIL,
On 2016/06/15 12:37:07, Marijn Kruisselbrink wrote:
> There should probably be separate FETCH_WAITUNTIL and FOREIGNFETCH_WAITUNTIL
> event types? Or maybe combining them in stats doesn't matter, but then the
> histogram description probably should be updated to mention both fetch and
> foreign fetch.

Done.

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/2034663002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:582:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/06/15 12:37:07, Marijn Kruisselbrink wrote:
> Now this code is in the .cc file you could use RunSoon rather than the
explicit
> base::ThreadTaskRunnerHandle::Get()->PostTask() thing.

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:919: 
On 2016/06/16 06:23:10, falken wrote:
> Can you add a lightweight comment explaining what this tests?

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:231: // StartRequest are
called.
On 2016/06/16 06:23:10, falken wrote:
> "the error callbacks that were associated with |request_ids| via StartRequest
> are called".

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:251: // handler
associated with the request_id which is registered at StartRequest.
On 2016/06/16 06:23:10, falken wrote:
> "registered by"

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:1473:
ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_NETWORK;  // dummy
On 2016/06/16 06:23:10, falken wrote:
> Lately it's common to use status = SERVICE_WORKER_ERROR_MAX_VALUE for this
(then
> you don't need "dummy")

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:1477:
version_->StartWorker(ServiceWorkerMetrics::EventType::SYNC,
On 2016/06/16 06:23:11, falken wrote:
> nit: I'd use UNKNOWN for a dummy event type.

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version_unittest.cc:1536:
EXPECT_TRUE(version_->FinishRequest(request_id2, true));
On 2016/06/16 06:23:11, falken wrote:
> Nice test!

Thanks！

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
content/common/service_worker/service_worker_messages.h:257: int /* request_id
*/,
On 2016/06/16 06:23:11, falken wrote:
> response_id

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
content/common/service_worker/service_worker_messages.h:261: int /* request_id
*/,
On 2016/06/16 06:23:11, falken wrote:
> event_finish_id

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/common/service...
content/common/service_worker/service_worker_messages.h:474: int /*
finish_request_id */,
On 2016/06/16 06:23:11, falken wrote:
> I think in your naming system, this is actually "int response_id, int
> event_finish_id" (based on embedded_worker_test_helper.h)

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2034663002/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:142:
blink::WebServiceWorkerEventResult result) override;
On 2016/06/16 06:23:11, falken wrote:
> WebServiceWorkerContextClient is using responseID and eventFinishID for these.

Done.

https://codereview.chromium.org/2034663002/diff/120001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.h:203: int
finish_request_id,
On 2016/06/16 06:23:11, falken wrote:
> Probably this should be response_id and event_finish_id
> (.cc file needs changes too)

Done.

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:64: //
WaitUntilObserver::Thenfunction but RespondWith needs to have a strong
On 2016/06/16 06:23:11, falken wrote:
> ThenFunction

Done.

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:149:
waitUntilObserver->didDispatchEvent(dispatchResult !=
DispatchEventResult::NotCanceled);
On 2016/06/16 06:23:11, falken wrote:
> Why do we need to check if NotCanceled? What's the consequence of passing true
> vs false to didDispatchEvent here?
> 
> Also, is waitUntil(Promise.resolve()) any different than
> waitUntil(Promise.reject()) for a fetch event? I think there shouldn't be a
> difference. Can we have a test case for that?

There is no difference between the two because a receiver of didDispatchEvent
for FetchEvent is SWUtils::NoOpStatusCallback.

The spec says "the rejected state of any of the promise in extend lifetime
promises to suggest operation failure" originally, so I thought an argument of
didDispatchEvent should be the same with other ExtendableEvents, and the
browser-side callback should treat it as they want.

[1]
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h
(right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextProxy.h:60:
virtual void dispatchForeignFetchEvent(int responseEventID, int finishEventID,
const WebServiceWorkerRequest& webRequest) = 0;
On 2016/06/16 06:23:11, falken wrote:
> responseID and eventFinishID for consistency with the rest of the patch

Done.

[falken, 2016-06-21 04:12:32]

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:149:
waitUntilObserver->didDispatchEvent(dispatchResult !=
DispatchEventResult::NotCanceled);
On 2016/06/21 02:43:38, makoto wrote:
> On 2016/06/16 06:23:11, falken wrote:
> > Why do we need to check if NotCanceled? What's the consequence of passing
true
> > vs false to didDispatchEvent here?
> > 
> > Also, is waitUntil(Promise.resolve()) any different than
> > waitUntil(Promise.reject()) for a fetch event? I think there shouldn't be a
> > difference. Can we have a test case for that?
> 
> There is no difference between the two because a receiver of didDispatchEvent
> for FetchEvent is SWUtils::NoOpStatusCallback.
> 
> The spec says "the rejected state of any of the promise in extend lifetime
> promises to suggest operation failure" originally, so I thought an argument of
> didDispatchEvent should be the same with other ExtendableEvents, and the
> browser-side callback should treat it as they want.
> 
> [1]
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...

Hm, the problem I have is WaitUntilObserver::didDispatchEvent takes a "bool
errorOccurred" which is different than  canceled. The other ExtendableEvents
don't look at DispatchEventResult. dispatchExtendableEvent just tracks whether
an runtime error occurred.

I think this might be an existing bug that dispatchFetchEvent doesn't care about
runtime errors. What happens if you do:
onfetch = e => {
  ...
  e.respondWith(response);
  throw 'error';
};

The spec seems to say the respondWith shouldn't work.

I don't think we need to block this patch on this though. Can you change it to
waitUntilObserver->didDispatchEvent(false /* errorOccurred */) and add a TODO
that dispatchFetchEvent should track whether a runtime error occurred, along
with a comment that this is OK for now because waitUntil for fetch event doesn't
care about promise rejection or runtime errors?

[shimazu, 2016-06-21 10:00:36]

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:149:
waitUntilObserver->didDispatchEvent(dispatchResult !=
DispatchEventResult::NotCanceled);
On 2016/06/21 04:12:32, falken wrote:
> On 2016/06/21 02:43:38, makoto wrote:
> > On 2016/06/16 06:23:11, falken wrote:
> > > Why do we need to check if NotCanceled? What's the consequence of passing
> true
> > > vs false to didDispatchEvent here?
> > > 
> > > Also, is waitUntil(Promise.resolve()) any different than
> > > waitUntil(Promise.reject()) for a fetch event? I think there shouldn't be
a
> > > difference. Can we have a test case for that?
> > 
> > There is no difference between the two because a receiver of
didDispatchEvent
> > for FetchEvent is SWUtils::NoOpStatusCallback.
> > 
> > The spec says "the rejected state of any of the promise in extend lifetime
> > promises to suggest operation failure" originally, so I thought an argument
of
> > didDispatchEvent should be the same with other ExtendableEvents, and the
> > browser-side callback should treat it as they want.
> > 
> > [1]
> >
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ex...
> 
> Hm, the problem I have is WaitUntilObserver::didDispatchEvent takes a "bool
> errorOccurred" which is different than  canceled. The other ExtendableEvents
> don't look at DispatchEventResult. dispatchExtendableEvent just tracks whether
> an runtime error occurred.
> 
> I think this might be an existing bug that dispatchFetchEvent doesn't care
about
> runtime errors. What happens if you do:
> onfetch = e => {
>   ...
>   e.respondWith(response);
>   throw 'error';
> };
> 
> The spec seems to say the respondWith shouldn't work.
> 
> I don't think we need to block this patch on this though. Can you change it to
> waitUntilObserver->didDispatchEvent(false /* errorOccurred */) and add a TODO
> that dispatchFetchEvent should track whether a runtime error occurred, along
> with a comment that this is OK for now because waitUntil for fetch event
doesn't
> care about promise rejection or runtime errors?

Done.

I'll file the bug.

[Marijn Kruisselbrink, 2016-06-21 17:32:15]

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:149:
waitUntilObserver->didDispatchEvent(dispatchResult !=
DispatchEventResult::NotCanceled);
Note that there is also a discussion about what throwing in onfetch should do at
https://github.com/slightlyoff/ServiceWorker/issues/896, where it seems perhaps
the current chrome behavior rather than the speced behavior is what is desired.

[falken, 2016-06-22 01:27:32]

just some nits and one question

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:149:
waitUntilObserver->didDispatchEvent(dispatchResult !=
DispatchEventResult::NotCanceled);
On 2016/06/21 17:32:15, Marijn Kruisselbrink wrote:
> Note that there is also a discussion about what throwing in onfetch should do
at
> https://github.com/slightlyoff/ServiceWorker/issues/896, where it seems
perhaps
> the current chrome behavior rather than the speced behavior is what is
desired.

Aha! Thanks for pointing that out.

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:84: // has only one
request to start event and one response to finish event, but
grammar nit: "events has" -> "events have", "request to start event" -> "request
that starts the event", "response to finish event" -> "response that finishes
the event"

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:233: // to receive
messages sent back in response to this event.
Should probably mention that the callback should be registered before
DispatchEvent is called.

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:158:
m_observer->decrementPendingActivity();
Where is the incrementPendingActivity this balances out? Are we sure it's
correct to decrement here?

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:148: // work
when the script throws some error, etc.
I think we can just remove this TODO based on the bug mek pointed out.

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:152: // promise
rejection.
about promise rejection or an uncaught runtime script error.

[shimazu, 2016-06-22 05:21:58]

Patchset #11 (id:200001) has been deleted

[shimazu, 2016-06-22 05:41:27]

Thanks, fix it. PTAL again.

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:84: // has only one
request to start event and one response to finish event, but
On 2016/06/22 01:27:31, falken wrote:
> grammar nit: "events has" -> "events have", "request to start event" ->
"request
> that starts the event", "response to finish event" -> "response that finishes
> the event"

Done.

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2034663002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_version.h:233: // to receive
messages sent back in response to this event.
On 2016/06/22 01:27:31, falken wrote:
> Should probably mention that the callback should be registered before
> DispatchEvent is called.

Done.

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:158:
m_observer->decrementPendingActivity();
On 2016/06/22 01:27:32, falken wrote:
> Where is the incrementPendingActivity this balances out? Are we sure it's
> correct to decrement here?

m_observer is non-null only when respondWith is in-flight at this point 
because didDispatchEvent decrements the counter and clears m_observer if there
is no respondWith, so this might be not a problem. 

Anyway, I found another bug; there is an excess decrementPendingActivity at the
last of didDispatchEvent... I removed it.

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:148: // work
when the script throws some error, etc.
On 2016/06/22 01:27:32, falken wrote:
> I think we can just remove this TODO based on the bug mek pointed out.

Done.

https://codereview.chromium.org/2034663002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:152: // promise
rejection.
On 2016/06/22 01:27:32, falken wrote:
> about promise rejection or an uncaught runtime script error.

Done.

[falken, 2016-06-22 05:53:33]

LGTM!

https://codereview.chromium.org/2034663002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:84: // have only one
request that starts event and one response that finishes
nit: s/starts event/starts the event and s/finishes event/finishes the event

https://codereview.chromium.org/2034663002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2034663002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:157:
DCHECK(m_state == Pending);
nit: DCHECK_EQ gives a nicer error message.

[shimazu, 2016-06-22 07:13:39]

https://codereview.chromium.org/2034663002/diff/220001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/2034663002/diff/220001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.h:84: // have only one
request that starts event and one response that finishes
On 2016/06/22 05:53:33, falken wrote:
> nit: s/starts event/starts the event and s/finishes event/finishes the event

Done.

https://codereview.chromium.org/2034663002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2034663002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:157:
DCHECK(m_state == Pending);
On 2016/06/22 05:53:33, falken wrote:
> nit: DCHECK_EQ gives a nicer error message.

Done.

[shimazu, 2016-06-24 01:30:01]

rkaplow@: I modified the UMA part again, so could you take a look?

keishi@: could you take another look at blink/web/?

[keishi, 2016-06-24 12:24:00]

LGTM Source/web/

[rkaplow, 2016-06-28 14:33:02]

lgtm

[shimazu, 2016-06-29 01:03:05]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-06-29 01:03:06]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2034663002/#ps280001
(title: "Add FOREIGH_FETCH_WAITUNTIL to histograms.xml")

[shimazu, 2016-06-29 01:03:09]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2016-06-29 01:46:00]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-06-29 01:46:01]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
keishi@chromium.org, palmer@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2034663002/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-29 01:47:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 04:12:52]

Description was changed from

==========
ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests
--gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"
==========

to

==========
ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests
--gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"
==========

[commit-bot: I haz the power, 2016-06-29 04:12:54]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2016-06-29 04:15:01]

Description was changed from

==========
ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests
--gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"
==========

to

==========
ServiceWorker: Keep the worker alive until FetchEvent.waitUntil settles

Currently ServiceWorker doesn't track if FetchEvent.waitUntil is finished, so
worker will be shutted down by idle timeout though a heavy task is running.
This patch adds WaitUntilObserver and makes RespondWith collaborate with it
in order to keep the worker alive.

BUG=601095
TEST=./out/Release/content_unittests
--gtest_filter="ServiceWorkerURLRequestJobTest.*"
TEST=./out/Release/content_unittests --gtest_filter="ServiceWorkerVersionTest.*"

Committed: https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e
Cr-Commit-Position: refs/heads/master@{#402710}
==========

[commit-bot: I haz the power, 2016-06-29 04:15:03]

Patchset 15 (id:??) landed as
https://crrev.com/3f122ea68eb653a175edc74ce5dbf9e8ba344e5e
Cr-Commit-Position: refs/heads/master@{#402710}