Add a status code to install event handled message from Service Worker

Add a status code to install event handled message from Service Worker

The Service Worker can reject an install event by passing a promise to
waitUntil that is rejected. This patch allows handling for this situation.

This patch depends on the Blink change:
https://codereview.chromium.org/210833004/

BUG=285976
R=jam@chromium.org, kinuko@chromium.org, tkent@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260356

[falken, 2014-03-25 10:29:44]

PTL

This will be a three-sided change:
1) Chrome: This patch
2) Blink: https://codereview.chromium.org/210833004/
3) Chrome: Re-enable the test and delete the old dispatchInstallEvent functions.

[kinuko, 2014-03-25 11:16:48]

looks good.

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:88: void
HandleInstallEventFinished(base::WeakPtr<ServiceWorkerVersion> version,
I think we'll want to use this too for activate event (since it aksi has
waitUntil). Can we not fork this from HandleEventFinished but just revise it for
now?  (In order to make it clearer that the method is for handling
InstallPhaseEvent family it might be better to rename it to
HandleInstallPhaseEvent too)

https://codereview.chromium.org/205563006/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/205563006/diff/1/content/renderer/service_wor...
content/renderer/service_worker/embedded_worker_context_client.cc:133: :
SERVICE_WORKER_INSTALL_EVENT_RESULT_COMPLETED);
It looks like we'll also need this rejected or completed concept for other
events, i.e. Activate and Fetch.  Does it make sense to have more generic enum
type in public/platform in blink (like kEventCompleted, kEventRejected) and use
it here and also in blink?

(As we chatted offline this could also be refactored using callback object which
has rejected() and completed() methods, but probably we want to do that later)

[kinuko, 2014-03-25 11:18:42]

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:84: int /*
active_version_embedded_worker_id */)
While you're there can we also rename this to active_version_id (throughout the
IPC code path)? (When I wrote this I was thinking about using 'embedded worker
id' for communication, but I think we'll probably just use 'service worker
version id' for that purpose)

[dominicc (has gone to gerrit), 2014-03-26 01:28:00]

DBC

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_browsertest.cc:221:
ServiceWorkerStatusCode expected) {
Would it be better to name this expected_status? It leaves room for passing
other expectations later.

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_types.h:29: // The service worker
passed a promise to waitUntil that rejected.
that rejected -> that was rejected? Or rejected needs an object... that rejected
food?

[falken, 2014-03-26 07:53:56]

Thanks for the reviews.

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_browsertest.cc:221:
ServiceWorkerStatusCode expected) {
On 2014/03/26 01:28:01, dominicc wrote:
> Would it be better to name this expected_status? It leaves room for passing
> other expectations later.

Done.

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/205563006/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:88: void
HandleInstallEventFinished(base::WeakPtr<ServiceWorkerVersion> version,
On 2014/03/25 11:16:49, kinuko wrote:
> I think we'll want to use this too for activate event (since it aksi has
> waitUntil). Can we not fork this from HandleEventFinished but just revise it
for
> now?  (In order to make it clearer that the method is for handling
> InstallPhaseEvent family it might be better to rename it to
> HandleInstallPhaseEvent too)

Done.

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:84: int /*
active_version_embedded_worker_id */)
On 2014/03/25 11:18:42, kinuko wrote:
> While you're there can we also rename this to active_version_id (throughout
the
> IPC code path)? (When I wrote this I was thinking about using 'embedded worker
> id' for communication, but I think we'll probably just use 'service worker
> version id' for that purpose)

Done.

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/205563006/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_types.h:29: // The service worker
passed a promise to waitUntil that rejected.
On 2014/03/26 01:28:01, dominicc wrote:
> that rejected -> that was rejected? Or rejected needs an object... that
rejected
> food?

Haha, right. I actually thought this was weird too but wrote it deliberately
because the algorithms spec reads "if the resulting promise rejects..." so it
seemed like the language of promises to say a promise rejects or fulfills.
However on further research it looks like not a common usage. I revised this
comment on the Blink-side patch.

https://codereview.chromium.org/205563006/diff/1/content/renderer/service_wor...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/205563006/diff/1/content/renderer/service_wor...
content/renderer/service_worker/embedded_worker_context_client.cc:133: :
SERVICE_WORKER_INSTALL_EVENT_RESULT_COMPLETED);
On 2014/03/25 11:16:49, kinuko wrote:
> It looks like we'll also need this rejected or completed concept for other
> events, i.e. Activate and Fetch.  Does it make sense to have more generic enum
> type in public/platform in blink (like kEventCompleted, kEventRejected) and
use
> it here and also in blink?
> 
> (As we chatted offline this could also be refactored using callback object
which
> has rejected() and completed() methods, but probably we want to do that later)

Done.

[kinuko, 2014-03-27 09:21:21]

lgtm

https://codereview.chromium.org/205563006/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/205563006/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:26: struct
HandleInstallPhaseEventFinishedParameters {
Ah this starts looking a bit ominous..?

[falken, 2014-03-27 11:13:17]

Thanks! Can OWNERS please review?

jam: content/
tsepez: messages/

This adds an exception to content/common/DEPS. Blink and Chromium share
WebServiceWorkerEventResult, and we want to pass this enum in IPC messages that
indicate event handling completed.

https://codereview.chromium.org/205563006/diff/110001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/205563006/diff/110001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:26: struct
HandleInstallPhaseEventFinishedParameters {
On 2014/03/27 09:21:22, kinuko wrote:
> Ah this starts looking a bit ominous..?

Yes... I don't yet see a better way for Install and Activate to share a function
like HandleInstallPhaseEventFinished. I added a comment for this struct.

[jam, 2014-03-27 17:10:23]

lgtm

[Tom Sepez, 2014-03-27 17:45:13]

Messages LGTM.

[falken, 2014-03-27 23:22:16]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:23:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/205563006/140001

[commit-bot: I haz the power, 2014-03-27 23:54:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:54:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[falken, 2014-03-28 00:01:46]

tkent: can you approve the DEPS change as public/platform OWNER?

[tkent, 2014-03-28 00:49:46]

https://codereview.chromium.org/205563006/diff/140001/content/common/DEPS
File content/common/DEPS (right):

https://codereview.chromium.org/205563006/diff/140001/content/common/DEPS#new...
content/common/DEPS:26:
"+third_party/WebKit/public/platform/WebServiceWorkerEventResult.h",
This is a simple enum header.  lgtm.

[falken, 2014-03-28 01:05:52]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-03-28 01:06:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/205563006/140001

[falken, 2014-03-28 21:45:19]

The CQ bit was unchecked by falken@chromium.org

[falken, 2014-03-28 21:45:44]

The CQ bit was checked by falken@chromium.org

[falken, 2014-03-29 04:09:09]

Committed patchset #5 manually as r260356 (presubmit successful).