ServiceWorker: register() should return the existing registration object if available

ServiceWorker: register() should return the existing registration object if available

Before this patch, register() always returns new registration object
even if the same registration already exists. This patch makes
register() return the existing object if it's available.

Chromium-side change: https://codereview.chromium.org/470843002/

NOTE: ServiceWorkerRegistration is still disabled and this change does
not affect the existing code/behavior.


BUG=396400
TEST=https://codereview.chromium.org/466723002/
NOTRY=true

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

[nhiroki, 2014-08-14 04:33:26]

Hi falken@, can you review this? Thanks!

[nhiroki, 2014-08-14 04:36:02]

On 2014/08/14 04:33:26, nhiroki wrote:
> Hi falken@, can you review this? Thanks!

Added a missing file (WebServiceWorkerRegistrationProxy.cpp)

[falken, 2014-08-14 05:04:35]

lgtm

nit: "has been existed" -> "already exists"

[nhiroki, 2014-08-14 05:17:05]

On 2014/08/14 05:04:35, falken wrote:
> lgtm
> 
> nit: "has been existed" -> "already exists"

Done. Thanks!

[nhiroki, 2014-08-14 05:19:16]

tkent@: Can you review {Source,public}/platform/*? Thanks!

[tkent, 2014-08-14 05:33:15]

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

https://codereview.chromium.org/473743002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerRegistrationProxy.h:8: #include "WebCommon.h"
should be "public/platform/WebCommon.h"

https://codereview.chromium.org/473743002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerRegistrationProxy.h:34: BLINK_PLATFORM_EXPORT
ServiceWorkerRegistration* unwrap() const;
We usually define |operator ServiceWorkerRegistration*() const| instead of
defining a normal function.

[nhiroki, 2014-08-14 06:15:19]

Thanks! Updated.

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

https://codereview.chromium.org/473743002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerRegistrationProxy.h:8: #include "WebCommon.h"
On 2014/08/14 05:33:15, tkent wrote:
> should be "public/platform/WebCommon.h"

Done.

https://codereview.chromium.org/473743002/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerRegistrationProxy.h:34: BLINK_PLATFORM_EXPORT
ServiceWorkerRegistration* unwrap() const;
On 2014/08/14 05:33:15, tkent wrote:
> We usually define |operator ServiceWorkerRegistration*() const| instead of
> defining a normal function.

Done.

[tkent, 2014-08-14 06:28:03]

lgtm

[nhiroki, 2014-08-14 06:44:59]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-14 06:45:32]

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

[nhiroki, 2014-08-14 08:52:08]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2014-08-14 08:52:20]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-14 08:53:46]

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

[commit-bot: I haz the power, 2014-08-14 08:55:21]

Committed patchset #4 (60001) as 180251

[sof, 2014-08-14 08:59:49]

https://codereview.chromium.org/473743002/diff/60001/public/platform/WebServi...
File public/platform/WebServiceWorkerRegistrationProxy.h (right):

https://codereview.chromium.org/473743002/diff/60001/public/platform/WebServi...
public/platform/WebServiceWorkerRegistrationProxy.h:38:
ServiceWorkerRegistration* m_private;
This is a GCed object; can you guarantee that the object is otherwise referenced
and kept alive for the duration of this proxy object?

(I guess what I'm really asking is why this isn't wrapped in a WebPrivatePtr<>.)

[nhiroki, 2014-08-15 00:05:54]

https://codereview.chromium.org/473743002/diff/60001/public/platform/WebServi...
File public/platform/WebServiceWorkerRegistrationProxy.h (right):

https://codereview.chromium.org/473743002/diff/60001/public/platform/WebServi...
public/platform/WebServiceWorkerRegistrationProxy.h:38:
ServiceWorkerRegistration* m_private;
On 2014/08/14 08:59:49, sof wrote:
> This is a GCed object; can you guarantee that the object is otherwise
referenced
> and kept alive for the duration of this proxy object?
> 
> (I guess what I'm really asking is why this isn't wrapped in a
WebPrivatePtr<>.)

Thank you for pointing out! I think our subsequent CLs will ensure that the
proxy is never accessed after it stops, and I'll consider the WebPrivatePtr<>
way (sorry, I don't really understand that pointer yet).


Following lists are just my memo about the lifetime of the proxy object:

1) ServiceWorkerRegistration is not GCed until ServiceWorkerRegistration::stop()
is called (see https://codereview.chromium.org/467133002/).
2) After the registration is stoped, |m_private| is no longer accessed
(CallbackPromiseAdapter doesn't call ServiceWorkerRegistration::take()).
3) When the proxy stops, the embedder is notified via
WebServiceWorkerRegistration::proxyStopped() and stops using the proxy object
(I'm making the chromium-side CL for this).