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

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

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

BUG=390894
TEST=N/A

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

[shimazu, 2014-08-28 09:48:27]

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

[shimazu, 2014-08-28 09:48:28]

nhiroki@chromium.org: Please review changes in 

horo@chromium.org: Please review changes in

[horo, 2014-08-28 10:25:47]

On 2014/08/28 09:48:28, makoto wrote:
> mailto:nhiroki@chromium.org: Please review changes in 
> 
> mailto:horo@chromium.org: Please review changes in

I think you have to change the test files like
LayoutTests/http/tests/serviceworker/unregister.html in this patch.

[horo, 2014-08-29 07:37:29]

lgtm with nit

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:31: // FIXME: This value
must be false in spec, but the implementation is wrong.
nit: indent please

[shimazu, 2014-09-01 01:18:02]

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

[shimazu, 2014-09-01 01:18:03]

tkent@chromium.org: Please review changes in
public/platform/WebServiceWorkerProvider.h

This is 3-sided patch related to 
1/4 https://codereview.chromium.org/515083002/
2/4 https://codereview.chromium.org/516823003/
4/4 https://codereview.chromium.org/509283003/

[nhiroki, 2014-09-01 01:45:12]

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:31: // FIXME: This value
must be false in spec, but the implementation is wrong.
"... is wrong" seems to be negative. How about "... is not supported yet" or
other more mild phrase?

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:34: 'unregistering twice
should resolve with false, but now this will be true');
We should probably delete this check for now since the check will be broken when
you fix the chromium-side and you need to disable this test until the
chromium-side patch lands.

[nhiroki, 2014-09-01 01:46:56]

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:31: // FIXME: This value
must be false in spec, but the implementation is wrong.
On 2014/09/01 01:45:12, nhiroki wrote:
> "... is wrong" seems to be negative. How about "... is not supported yet" or
> other more mild phrase?

(because this implementation was correct before :)

[shimazu, 2014-09-01 05:29:42]

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:31: // FIXME: This value
must be false in spec, but the implementation is wrong.
On 2014/08/29 07:37:29, horo wrote:
> nit: indent please

Done.

https://codereview.chromium.org/512163002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:31: // FIXME: This value
must be false in spec, but the implementation is wrong.
On 2014/09/01 01:45:12, nhiroki wrote:
> "... is wrong" seems to be negative. How about "... is not supported yet" or
> other more mild phrase?

Done.

[nhiroki, 2014-09-01 08:43:03]

lgtm

https://codereview.chromium.org/512163002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/512163002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:31: // FIXME: This value
must be false in spec, but this is not supported now.
Can you wrap this at 80 columns?

https://codereview.chromium.org/512163002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/unregister.html:34: 'unregistering twice
should resolve with false, but now this will be true');
ditto.

(IMHO, just "unregistering twice should resolve with true" would ok because I
think an assert description should mention only the current expected behavior
and the FIXME comment mentions the ideal behavior instead.)

[shimazu, 2014-09-03 06:39:00]

tkent@: Could you review WebServiceWorkerProvider.h?

[tkent, 2014-09-03 06:46:59]

lgtm

[shimazu, 2014-09-03 06:51:07]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-09-03 06:51:30]

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

[commit-bot: I haz the power, 2014-09-03 06:51:47]

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

[commit-bot: I haz the power, 2014-09-03 06:51:48]

Failed to apply patch for
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/serviceworkers/ServiceWorkerContainer.cpp
  Hunk #1 FAILED at 124.
  Hunk #2 succeeded at 154 (offset -1 lines).
  Hunk #3 succeeded at 181 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/modules/serviceworkers/ServiceWorkerContainer.cpp.rej

Patch:       Source/modules/serviceworkers/ServiceWorkerContainer.cpp
Index: Source/modules/serviceworkers/ServiceWorkerContainer.cpp
diff --git a/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
b/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
index
6caea6e9807a3696ef5977e0be78239aa29bb227..d2ce8c6af97146b56ccf7319dfd1094ee47faa68
100644
--- a/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
+++ b/Source/modules/serviceworkers/ServiceWorkerContainer.cpp
@@ -124,25 +124,6 @@ ScriptPromise
ServiceWorkerContainer::registerServiceWorker(ScriptState* scriptS
     return promise;
 }
 
-#ifdef DISABLE_SERVICEWORKER_UNREGISTER_RESOLVE_TO_BOOLEAN
-class UndefinedValue {
-public:
-    typedef WebServiceWorkerRegistration WebType;
-    static V8UndefinedType take(ScriptPromiseResolver* resolver, WebType*
registration)
-    {
-        ASSERT(!registration); // Anything passed here will be leaked.
-        return V8UndefinedType();
-    }
-    static void dispose(WebType* registration)
-    {
-        ASSERT(!registration); // Anything passed here will be leaked.
-    }
-
-private:
-    UndefinedValue();
-};
-#else
-
 class BooleanValue {
 public:
     typedef bool WebType;
@@ -155,7 +136,6 @@ public:
 private:
     BooleanValue();
 };
-#endif
 
 ScriptPromise ServiceWorkerContainer::unregisterServiceWorker(ScriptState*
scriptState, const String& pattern)
 {
@@ -183,11 +163,7 @@ ScriptPromise
ServiceWorkerContainer::unregisterServiceWorker(ScriptState* scrip
         resolver->reject(DOMException::create(SecurityError, "The scope must
match the current origin."));
         return promise;
     }
-#ifdef DISABLE_SERVICEWORKER_UNREGISTER_RESOLVE_TO_BOOLEAN
-    m_provider->unregisterServiceWorker(patternURL, new
CallbackPromiseAdapter<UndefinedValue, ServiceWorkerError>(resolver));
-#else
     m_provider->unregisterServiceWorker(patternURL, new
CallbackPromiseAdapter<BooleanValue, ServiceWorkerError>(resolver));
-#endif
     return promise;
 }

[shimazu, 2014-09-03 07:08:06]

Patchset #7 (id:120001) has been deleted

[shimazu, 2014-09-03 07:09:26]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-09-03 07:10:01]

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

[commit-bot: I haz the power, 2014-09-03 08:10:18]

Committed patchset #7 (id:140001) as 181295