Service Worker: (Re-commit) Make ServiceWorkerRegistration.update() return a promise. (Blink 1/3) (issue 1260833003 by jungkee.song@samsung.com)

[jungke...@samsung.com]

Reviewers: tkent, nhiroki, falken, kinuko, michaeln, zino, lgombos,

Message:
tkent@, nhiroki@, zino@,

The commit from the original CL was reverted with a leak report:
https://codereview.chromium.org/1264143002/. Here's the analysis I carried
out:
https://codereview.chromium.org/1267703003/#msg30. And here's a solution
that
zino@ suggested, and I incorporated that in this snapshot as such. PTAL.

Description:
Service Worker: (Re-commit) Make ServiceWorkerRegistration.update() return a
promise. (Blink 1/3)

* The original CL was committed but reverted due to a leak report:
https://codereview.chromium.org/1267703003/#msg29.
This CL re-commits the original CL with a patch for the leak. This change
will
also be existed temporarily until the chromium-side patch will land.

--
As per the resolution of f2f, ServiceWorkerRegistration.update() should
return a
promise that transforms the promise returned by Update algorithm.

In this CL, ServiceWorkerContextCore::UpdateServiceWorker method has been
overloaded to cover two invocation paths: a scheduled update without a
provider_host and a callback (Soft Update in the spec) and a call initiated
from
the script surface using ServiceWorkerRegistration.update().

This is a web-exposed API change. But it has no compatibility risk because
existing user code doesn't expect to receive anything from update().

Spec:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-registration-update
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/311

Companion CL (Chromium): https://codereview.chromium.org/1270513002/
Companion CL (Blink layout test):
https://codereview.chromium.org/1268663003/

BUG=513655

Please review this at https://codereview.chromium.org/1260833003/

Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Affected files (+23, -6 lines):
M Source/modules/serviceworkers/ServiceWorkerRegistration.h
M Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
M Source/modules/serviceworkers/ServiceWorkerRegistration.idl
M public/platform/WebServiceWorkerRegistration.h


Index: Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
diff --git a/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
b/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
index
7c701b4998cbf5cca150e04625530d6aebd0c36b..999a62087e8ec9937a1d6ab6e218ad57691abf82
100644
--- a/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
+++ b/Source/modules/serviceworkers/ServiceWorkerRegistration.cpp
@@ -86,13 +86,18 @@ String ServiceWorkerRegistration::scope() const
return m_outerRegistration->scope().string();
}

-void ServiceWorkerRegistration::update(ScriptState* scriptState,
ExceptionState& exceptionState)
+ScriptPromise ServiceWorkerRegistration::update(ScriptState* scriptState)
{
+ RefPtrWillBeRawPtr<ScriptPromiseResolver> resolver =
ScriptPromiseResolver::create(scriptState);
+ ScriptPromise promise = resolver->promise();
+
if (!m_provider) {
- exceptionState.throwDOMException(InvalidStateError, "Failed to
update a ServiceWorkerRegistration: No associated provider is available.");
- return;
+ resolver->reject(DOMException::create(InvalidStateError, "Failed
to update a ServiceWorkerRegistration: No associated provider is
available."));
+ return promise;
}
- m_outerRegistration->update(m_provider);
+
+ m_outerRegistration->update(m_provider, new
CallbackPromiseAdapter<void, ServiceWorkerError>(resolver));
+ return promise;
}

ScriptPromise ServiceWorkerRegistration::unregister(ScriptState*
scriptState)
Index: Source/modules/serviceworkers/ServiceWorkerRegistration.h
diff --git a/Source/modules/serviceworkers/ServiceWorkerRegistration.h
b/Source/modules/serviceworkers/ServiceWorkerRegistration.h
index
3e4acdce99e475e55ba56c2686907a394e6ed2e1..3f032d68e1597a1101be0ba5408e2f92b7453c7a
100644
--- a/Source/modules/serviceworkers/ServiceWorkerRegistration.h
+++ b/Source/modules/serviceworkers/ServiceWorkerRegistration.h
@@ -55,7 +55,7 @@ public:

WebServiceWorkerRegistration* webRegistration() { return
m_outerRegistration.get(); }

- void update(ScriptState*, ExceptionState&);
+ ScriptPromise update(ScriptState*);
ScriptPromise unregister(ScriptState*);

DEFINE_ATTRIBUTE_EVENT_LISTENER(updatefound);
Index: Source/modules/serviceworkers/ServiceWorkerRegistration.idl
diff --git a/Source/modules/serviceworkers/ServiceWorkerRegistration.idl
b/Source/modules/serviceworkers/ServiceWorkerRegistration.idl
index
c88cb42366a65c4624a7a5a91d8bd6c7e0a79849..982e93a48612018a5105cf7b10a24cf018b36a84
100644
--- a/Source/modules/serviceworkers/ServiceWorkerRegistration.idl
+++ b/Source/modules/serviceworkers/ServiceWorkerRegistration.idl
@@ -15,7 +15,7 @@

readonly attribute USVString scope;

- [CallWith=ScriptState, RaisesException] void update();
+ [CallWith=ScriptState] Promise<void> update();
[CallWith=ScriptState] Promise<boolean> unregister();

attribute EventHandler onupdatefound;
Index: public/platform/WebServiceWorkerRegistration.h
diff --git a/public/platform/WebServiceWorkerRegistration.h
b/public/platform/WebServiceWorkerRegistration.h
index
53790805cf2833d0e2d71d629bfa72f71e610844..51303a4531f604b1a5ae35d8fddf3a79ce6f3fdd
100644
--- a/public/platform/WebServiceWorkerRegistration.h
+++ b/public/platform/WebServiceWorkerRegistration.h
@@ -18,6 +18,7 @@ class WebServiceWorkerRegistration {
public:
virtual ~WebServiceWorkerRegistration() { }

+ using WebServiceWorkerUpdateCallbacks = WebCallbacks<void,
WebServiceWorkerError*>;
using WebServiceWorkerUnregistrationCallbacks = WebCallbacks<bool*,
WebServiceWorkerError*>;

virtual void setProxy(WebServiceWorkerRegistrationProxy*) { }
@@ -25,7 +26,18 @@ public:
virtual void proxyStopped() { }

virtual WebURL scope() const { return WebURL(); }
+ // TODO(jungkees):
+ // void update(p) remains temporarily before the chromium-side
companion
+ // patch lands: https://codereview.chromium.org/1270513002/.
+ // Before the chromium-side patch lands, the new update(p, c) will
+ // internally call this->update(p) to work with the existing code base.
+ // The final changes will be incorporated with the Blink layout
patches.
virtual void update(WebServiceWorkerProvider*) { }
+ virtual void update(WebServiceWorkerProvider* provider,
WebServiceWorkerUpdateCallbacks* callback)
+ {
+ this->update(provider);
+ callback->onSuccess();
+ }
virtual void unregister(WebServiceWorkerProvider*,
WebServiceWorkerUnregistrationCallbacks*) { }
};

[tk...@chromium.org]

lgtm




https://codereview.chromium.org/1260833003/diff/1/public/platform/WebServiceWorkerRegistration.h
File public/platform/WebServiceWorkerRegistration.h (right):

https://codereview.chromium.org/1260833003/diff/1/public/platform/WebServiceWorkerRegistration.h#newcode33
public/platform/WebServiceWorkerRegistration.h:33: // internally call

this->update(p) to work with the existing code base.

nit: |this->update(p)| -> |update(provider)|

https://codereview.chromium.org/1260833003/diff/1/public/platform/WebServiceWorkerRegistration.h#newcode38
public/platform/WebServiceWorkerRegistration.h:38:
this->update(provider);
nit: remove |this->|

https://codereview.chromium.org/1260833003/

[nhi...@chromium.org]

lgtm



https://codereview.chromium.org/1260833003/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1260833003/20001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1260833003/20001


https://codereview.chromium.org/1260833003/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199875

https://codereview.chromium.org/1260833003/

[jungke...@samsung.com]

johnme@, I checked it from the flakiness dashboard that this commit still
causes
http/tests/serviceworker/update.html to fail on WebKit Linux Leak and WebKit
Linux Oilpan Leak. But I have also checked locally using layout test's
--enable-leak-detection option that this failure goes away when the
chromium-side companion patch is applied. (That must be because the resolver
object is properly consumed and destroyed whereas our temporary patch might
miss
something. I'm still looking at it.)

I'm just waiting for this patch is rolled into the chromium repo and
planning to
commit the chromium-side patch as soon as it can go in. So, if possible, I'd
like to try what I just explained here.

/cc tkent@, nhiroki@, zino@.

https://codereview.chromium.org/1260833003/

[joh...@chromium.org]

Sounds good, let's hope that does the trick.

https://codereview.chromium.org/1260833003/

[jungke...@samsung.com]

The chromium-side patch has just landed:
https://codereview.chromium.org/1270513002/#msg39. I'll keep an eye on the
flakiness dashboard.

https://codereview.chromium.org/1260833003/

[jungke...@samsung.com]

On 2015/08/03 12:15:54, jungkees wrote:
> The chromium-side patch has just landed:
> https://codereview.chromium.org/1270513002/#msg39. I'll keep an eye on the
> flakiness dashboard.

Yes, it starts passing again! :)

https://codereview.chromium.org/1260833003/

[joh...@chromium.org]

On 2015/08/03 13:28:33, jungkees wrote:
> On 2015/08/03 12:15:54, jungkees wrote:
> > The chromium-side patch has just landed:
> > https://codereview.chromium.org/1270513002/#msg39. I'll keep an eye on
> the
> > flakiness dashboard.

> Yes, it starts passing again! :)

Thanks for following up :)

https://codereview.chromium.org/1260833003/