Inform the client when the Service Worker rejects an install event

Inform the client when the Service Worker rejects an install event

BUG=285976
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170073

[falken, 2014-03-25 10:30:14]

Blink-side of https://codereview.chromium.org/205563006/

[kinuko, 2014-03-25 11:26:50]

almost looks good

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/WaitUntilObserver.h (right):

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/WaitUntilObserver.h:47: bool m_error;
m_hasError might be better? (Otherwise this var name feels it records actual
error code etc)

https://codereview.chromium.org/210833004/diff/1/public/web/WebServiceWorkerC...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/210833004/diff/1/public/web/WebServiceWorkerC...
public/web/WebServiceWorkerContextClient.h:84: virtual void
didHandleInstallEvent(int installEventID, bool rejected) { }
If you're going to make yet another blink patch (which needs to land before your
first chrome patch) you can include this public interface change code into it
(and probably you can skip the second chrome patch instead)


1st patch (blink): add another virtual void didHandleInstallEvent(...) { } which
internally calls old (single-arg) didHandleInstallEvent()

2nd patch (chrome): implement the new didHandleInstallEvent without disabling
chrome test

3rd patch (blink): remove the old didHandleInstallEvent() interface

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

DBC:

There's a subtle domain shift that happens from "has error" to "is rejected", by
passing a boolean through. Could that be confusing? Enums would be more
explicit, although maybe it is too belt-and-suspenders? WDYT?

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right):

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:49: virtual void
didHandleInstallEvent(int installEventID, bool rejected) = 0;
Would it be better to introduce an enum for rejected?

[falken, 2014-03-26 07:26:36]

Thanks for the comments! I uploaded a new patch that uses an enum in
public/platform, as suggested in the Chromium-side review.

Now the three-sided landing plan is:
1) Blink: this patch
2) Chrome: https://codereview.chromium.org/205563006/
3) Blink: remove the old didHandleInstallEvent()

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h (right):

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.h:49: virtual void
didHandleInstallEvent(int installEventID, bool rejected) = 0;
On 2014/03/26 01:23:15, dominicc wrote:
> Would it be better to introduce an enum for rejected?

Done.

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/WaitUntilObserver.h (right):

https://codereview.chromium.org/210833004/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/WaitUntilObserver.h:47: bool m_error;
On 2014/03/25 11:26:51, kinuko wrote:
> m_hasError might be better? (Otherwise this var name feels it records actual
> error code etc)

Done, replaced with the new enum.

https://codereview.chromium.org/210833004/diff/1/public/web/WebServiceWorkerC...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/210833004/diff/1/public/web/WebServiceWorkerC...
public/web/WebServiceWorkerContextClient.h:84: virtual void
didHandleInstallEvent(int installEventID, bool rejected) { }
On 2014/03/25 11:26:51, kinuko wrote:
> If you're going to make yet another blink patch (which needs to land before
your
> first chrome patch) you can include this public interface change code into it
> (and probably you can skip the second chrome patch instead)
> 
> 
> 1st patch (blink): add another virtual void didHandleInstallEvent(...) { }
which
> internally calls old (single-arg) didHandleInstallEvent()
> 
> 2nd patch (chrome): implement the new didHandleInstallEvent without disabling
> chrome test
> 
> 3rd patch (blink): remove the old didHandleInstallEvent() interface
> 

Thanks for the idea. Taking this approach now.

[falken, 2014-03-26 07:55:23]

+tkent for public/ OWNER review

https://codereview.chromium.org/210833004/diff/40002/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/210833004/diff/40002/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:87:
didHandleInstallEvent(installEventID);
Note: Using the old didHandleInstallEvent is needed here just because this patch
is part of a three-sided patch. It will be cleaned up on the third patch.

[tkent, 2014-03-26 08:46:03]

lgtm

https://codereview.chromium.org/210833004/diff/40002/public/platform/WebServi...
File public/platform/WebServiceWorkerEventResult.h (right):

https://codereview.chromium.org/210833004/diff/40002/public/platform/WebServi...
public/platform/WebServiceWorkerEventResult.h:2: 
The blank line should be removed.

[kinuko, 2014-03-26 12:53:56]

lgtm w/ one comment

https://codereview.chromium.org/210833004/diff/40002/Source/modules/servicewo...
File Source/modules/serviceworkers/WaitUntilObserver.cpp (right):

https://codereview.chromium.org/210833004/diff/40002/Source/modules/servicewo...
Source/modules/serviceworkers/WaitUntilObserver.cpp:87: ,
m_result(blink::WebServiceWorkerEventResultCompleted)
nit: it feels a bit weird to me to initialize and keep it as 'Completed', since
we have another pending activity counter to watch completion.  (In that sense
'hasError' boolean might have been better while we're in this inner class)

[falken, 2014-03-26 14:38:32]

Thanks!

https://codereview.chromium.org/210833004/diff/40002/Source/modules/servicewo...
File Source/modules/serviceworkers/WaitUntilObserver.cpp (right):

https://codereview.chromium.org/210833004/diff/40002/Source/modules/servicewo...
Source/modules/serviceworkers/WaitUntilObserver.cpp:87: ,
m_result(blink::WebServiceWorkerEventResultCompleted)
On 2014/03/26 12:53:56, kinuko wrote:
> nit: it feels a bit weird to me to initialize and keep it as 'Completed',
since
> we have another pending activity counter to watch completion.  (In that sense
> 'hasError' boolean might have been better while we're in this inner class)

Makes sense to me. I switched to m_hasError.

https://codereview.chromium.org/210833004/diff/40002/public/platform/WebServi...
File public/platform/WebServiceWorkerEventResult.h (right):

https://codereview.chromium.org/210833004/diff/40002/public/platform/WebServi...
public/platform/WebServiceWorkerEventResult.h:2: 
On 2014/03/26 08:46:03, tkent wrote:
> The blank line should be removed.

Done.

[falken, 2014-03-26 14:39:44]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-03-26 14:39:54]

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

[commit-bot: I haz the power, 2014-03-26 15:45:50]

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

[commit-bot: I haz the power, 2014-03-26 15:45:50]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[falken, 2014-03-26 15:47:02]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-03-26 15:47:08]

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

[commit-bot: I haz the power, 2014-03-26 17:54:49]

Change committed as 170073