ServiceWorker: Change the return value of ServiceWorkerRegistration::unregister to boolean (1/4)

ServiceWorker: Change the return value of ServiceWorkerRegistration::unregister to boolean

1. [Blink] This patch
2. [Chromium] https://codereview.chromium.org/516823003/
3. [Blink] https://codereview.chromium.org/512163002/
4. [Chromium] https://codereview.chromium.org/509283003/

BUG=390894
TEST=N/A

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

[shimazu, 2014-08-28 09:38:12]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org, nhiroki@chromium.org

[nhiroki, 2014-08-28 10:13:21]

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:185: #ifdef
SERVICEWORKER_UNREGISTER_DISABLE_NEW_FEATURE
"NEW_FEATURE" would be ambiguous. How about "_UNREGISTER_RESOLVE_TO_BOOLEAN" or
something else?

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:45: return boolean;
*boolean ?

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:45: #define
SERVICEWORKER_UNREGISTER_DISABLE_NEW_FEATURE
Can you add FIXME comment and a link to the issue here?

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:60: typedef
WebServiceWorkerRegistrationCallbacks WebServiceWorkerUnRegistrationCallbacks;
nit: s/UnRegistration/Unregistration/

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:62: typedef WebCallbacks<bool,
WebServiceWorkerError> WebServiceWorkerUnRegistrationCallbacks;
ditto.

[shimazu, 2014-08-29 07:07:07]

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:185: #ifdef
SERVICEWORKER_UNREGISTER_DISABLE_NEW_FEATURE
On 2014/08/28 10:13:20, nhiroki wrote:
> "NEW_FEATURE" would be ambiguous. How about "_UNREGISTER_RESOLVE_TO_BOOLEAN"
or
> something else?

Done.

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/515083002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:45: return boolean;
On 2014/08/28 10:13:21, nhiroki wrote:
> *boolean ?

Done.

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:45: #define
SERVICEWORKER_UNREGISTER_DISABLE_NEW_FEATURE
On 2014/08/28 10:13:21, nhiroki wrote:
> Can you add FIXME comment and a link to the issue here?

Done.

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:60: typedef
WebServiceWorkerRegistrationCallbacks WebServiceWorkerUnRegistrationCallbacks;
On 2014/08/28 10:13:21, nhiroki wrote:
> nit: s/UnRegistration/Unregistration/

Done.

https://codereview.chromium.org/515083002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:62: typedef WebCallbacks<bool,
WebServiceWorkerError> WebServiceWorkerUnRegistrationCallbacks;
On 2014/08/28 10:13:21, nhiroki wrote:
> ditto.

Done.

[nhiroki, 2014-08-29 07:17:53]

LGTM with nits.

For readability, can you list the subsequent CLs in the description? For
example,

1. [Blink] This patch.
2. [Chromium] https://codereview.chromium.org/516823003/
3. [Blink] ...

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:153: }
nit: In blink, an empty function body seems to be inlined like this:

  static void dispose(WebType* boolean) { }

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:49: }
ditto.

[horo, 2014-08-29 07:18:35]

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:23: class
UndefinedValue {
I think the class definition of UndefinedValue and BooleanValue should be
wrapped in #ifdef.

[nhiroki, 2014-08-29 07:18:56]

fyi: you need to add another reviewer for
public/platform/WebServiceWorkerProvider.h

[shimazu, 2014-08-29 07:33:03]

shimazu@chromium.org changed reviewers:
+ tkent@chromium.org

[shimazu, 2014-08-29 07:33:03]

tkent@chromium.org: Please review changes in

[tkent, 2014-09-01 03:11:24]

https://codereview.chromium.org/515083002/diff/40001/public/platform/WebServi...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/515083002/diff/40001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:45: // FIXME(shimazu): Remove this
macro after the chromium side patch is applied
Please refer to #3 of http://www.chromium.org/blink/coding-style#TOC-Comments

[shimazu, 2014-09-01 05:13:47]

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:153: }
On 2014/08/29 07:17:53, nhiroki wrote:
> nit: In blink, an empty function body seems to be inlined like this:
> 
>   static void dispose(WebType* boolean) { }

Done.

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:23: class
UndefinedValue {
On 2014/08/29 07:18:35, horo wrote:
> I think the class definition of UndefinedValue and BooleanValue should be
> wrapped in #ifdef.

Done.

https://codereview.chromium.org/515083002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:49: }
On 2014/08/29 07:17:53, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/515083002/diff/40001/public/platform/WebServi...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/515083002/diff/40001/public/platform/WebServi...
public/platform/WebServiceWorkerProvider.h:45: // FIXME(shimazu): Remove this
macro after the chromium side patch is applied
On 2014/09/01 03:11:24, tkent wrote:
> Please refer to #3 of http://www.chromium.org/blink/coding-style#TOC-Comments

Done.

[tkent, 2014-09-01 05:27:22]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-09-01 05:27:23]

lgtm

[commit-bot: I haz the power, 2014-09-01 05:28:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/shimazu@chromium.org/515083002/60001

[commit-bot: I haz the power, 2014-09-01 06:30:38]

Committed patchset #4 (id:60001) as 181151