Stub implementation for PushRegistration.unregister().

Stub implementation for PushRegistration.unregister().

This is passing the ServiceWorkerRegistration to the PushRegistration
object and adds the unregister method in PushRegistration and
WebPushProvider. For now, the stub implementation always succeeds in
order to have a test that will not fail when the Chromium implementation
will be added.

This is a multi-sided CL.
Part 1: <this>
Part 2: https://codereview.chromium.org/793403002/
Part 3: https://codereview.chromium.org/800503002/

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

[mlamouri (slow - plz ping), 2014-12-10 21:57:40]

mlamouri@chromium.org changed reviewers:
+ mkwst@chromium.org, mvanouwerkerk@google.com

[mlamouri (slow - plz ping), 2014-12-10 21:57:41]

Note: this CL is based on a few CLs that are not landed yet.
Full list:
https://codereview.chromium.org/735293005/
https://codereview.chromium.org/784873004/
https://codereview.chromium.org/783983003/

[mlamouri (slow - plz ping), 2014-12-11 10:41:27]

mlamouri@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org
- mvanouwerkerk@google.com

[mlamouri (slow - plz ping), 2014-12-12 10:48:50]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2014-12-12 10:48:51]

+jochen@ given that Mike is currently sick.

[Michael van Ouwerkerk, 2014-12-12 11:48:01]

On 2014/12/10 21:57:41, Mounir Lamouri wrote:
> Note: this CL is based on a few CLs that are not landed yet.
> Full list:
> https://codereview.chromium.org/735293005/
> https://codereview.chromium.org/784873004/
> https://codereview.chromium.org/783983003/

These dependencies have landed now.

[Michael van Ouwerkerk, 2014-12-12 12:18:05]

Please also run the try bots for the next patch, they should work now that the
dependencies have landed.

https://codereview.chromium.org/793913002/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/unregister.html (right):

https://codereview.chromium.org/793913002/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/unregister.html:28: return
pushRegistration.unregister();
nit for ultimate completeness:
assert_inherits(pushRegistration, 'unregister', 'The unregister method must be
exposed.');

https://codereview.chromium.org/793913002/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/unregister.html:31:
assert_true(unregistration_result);
nit: add 'Unregistration must succeed' as second argument, otherwise assert_true
will have nothing meaningful to say when it fails.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistration.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistration.h:24: typedef WebPushRegistration
WebType;
It seems you don't need this for CallbackPromiseAdapter anymore.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistration.h:41:
Member<ServiceWorkerRegistration> m_registration;
Please rename this to m_serviceWorkerRegistration to distinguish from |this|.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallback.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:25: class
PushRegistrationCallback final
Please rename to PushRegistrationCallbacks. It's an awful name but I'd like to
be consistent with the WebCallbacks it inherits from.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:26: : public
WebCallbacks<WebPushRegistration, WebPushError> {
Just write WebPushRegistrationCallbacks

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:28: explicit
PushRegistrationCallback(PassRefPtr<ScriptPromiseResolver>,
ServiceWorkerRegistration*);
omit explicit as there are two arguments

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:29: virtual
~PushRegistrationCallback();
Hmmz, I guess we should mark these with override instead of virtual.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:38:
Persistent<ServiceWorkerRegistration> m_registration;
Please rename to m_serviceWorkerRegistration to distinguish from that other
registration.

[jochen (gone - plz use gerrit), 2014-12-12 14:26:12]

https://codereview.chromium.org/793913002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/CallbackPromiseAdapter.h:199:
WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
blink style is to have this right after the "class" line

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallback.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:26: : public
WebCallbacks<WebPushRegistration, WebPushError> {
should be on previous line (no line length limit)

[mlamouri (slow - plz ping), 2014-12-15 10:55:07]

Review comments applied. PTAL.

https://codereview.chromium.org/793913002/diff/20001/LayoutTests/http/tests/p...
File LayoutTests/http/tests/push_messaging/unregister.html (right):

https://codereview.chromium.org/793913002/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/unregister.html:28: return
pushRegistration.unregister();
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> nit for ultimate completeness:
> assert_inherits(pushRegistration, 'unregister', 'The unregister method must be
> exposed.');

Done.

https://codereview.chromium.org/793913002/diff/20001/LayoutTests/http/tests/p...
LayoutTests/http/tests/push_messaging/unregister.html:31:
assert_true(unregistration_result);
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> nit: add 'Unregistration must succeed' as second argument, otherwise
assert_true
> will have nothing meaningful to say when it fails.

Done.

https://codereview.chromium.org/793913002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/CallbackPromiseAdapter.h:199:
WTF_MAKE_NONCOPYABLE(CallbackPromiseAdapter);
On 2014/12/12 14:26:11, jochen (slow) wrote:
> blink style is to have this right after the "class" line

Yeah. I was actually following the style of the file. I've updated the file then
;)

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistration.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistration.h:24: typedef WebPushRegistration
WebType;
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> It seems you don't need this for CallbackPromiseAdapter anymore.

Done.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistration.h:41:
Member<ServiceWorkerRegistration> m_registration;
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> Please rename this to m_serviceWorkerRegistration to distinguish from |this|.

Done.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallback.h (right):

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:25: class
PushRegistrationCallback final
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> Please rename to PushRegistrationCallbacks. It's an awful name but I'd like to
> be consistent with the WebCallbacks it inherits from.

Sic :(

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:26: : public
WebCallbacks<WebPushRegistration, WebPushError> {
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> Just write WebPushRegistrationCallbacks

Hmm, I don't think so. It would require including WebPushProvider.h to get the
typedef.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:28: explicit
PushRegistrationCallback(PassRefPtr<ScriptPromiseResolver>,
ServiceWorkerRegistration*);
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> omit explicit as there are two arguments

Done.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:29: virtual
~PushRegistrationCallback();
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> Hmmz, I guess we should mark these with override instead of virtual.

Done.

https://codereview.chromium.org/793913002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallback.h:38:
Persistent<ServiceWorkerRegistration> m_registration;
On 2014/12/12 12:18:04, Michael van Ouwerkerk wrote:
> Please rename to m_serviceWorkerRegistration to distinguish from that other
> registration.

Done.

[Michael van Ouwerkerk, 2014-12-15 11:42:36]

lgtm

[Peter Beverloo, 2014-12-15 11:45:36]

peter@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2014-12-15 11:45:37]

https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallbacks.h (right):

https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallbacks.h:33: void
trace(Visitor*);
What calls this?

[mlamouri (slow - plz ping), 2014-12-15 12:04:38]

https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallbacks.h (right):

https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallbacks.h:33: void
trace(Visitor*);
On 2014/12/15 11:45:36, Peter Beverloo wrote:
> What calls this?

Oilpan.

[Peter Beverloo, 2014-12-15 12:17:44]

On 2014/12/15 12:04:38, Mounir Lamouri wrote:
>
https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
> File Source/modules/push_messaging/PushRegistrationCallbacks.h (right):
> 
>
https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
> Source/modules/push_messaging/PushRegistrationCallbacks.h:33: void
> trace(Visitor*);
> On 2014/12/15 11:45:36, Peter Beverloo wrote:
> > What calls this?
> 
> Oilpan.

Oilpan does not magically call trace() methods, it needs either an explicit call
in the class having the object as a member (which could be a wrapper class) or
have the object be a GC root.

It's the Persistent<> object that is the GC root, not PushRegistrationCallbacks,
so that will have its trace() method called. This one is not necessary.

(PersistentWillBeMember does have this requirement.)

[Mike West, 2014-12-15 12:39:47]

On 2014/12/15 12:17:44, Peter Beverloo wrote:
> On 2014/12/15 12:04:38, Mounir Lamouri wrote:
> >
>
https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
> > File Source/modules/push_messaging/PushRegistrationCallbacks.h (right):
> > 
> >
>
https://codereview.chromium.org/793913002/diff/60001/Source/modules/push_mess...
> > Source/modules/push_messaging/PushRegistrationCallbacks.h:33: void
> > trace(Visitor*);
> > On 2014/12/15 11:45:36, Peter Beverloo wrote:
> > > What calls this?
> > 
> > Oilpan.
> 
> Oilpan does not magically call trace() methods, it needs either an explicit
call
> in the class having the object as a member (which could be a wrapper class) or
> have the object be a GC root.
> 
> It's the Persistent<> object that is the GC root, not
PushRegistrationCallbacks,
> so that will have its trace() method called. This one is not necessary.
> 
> (PersistentWillBeMember does have this requirement.)

I'm pretty sure Peter is correct here.

LGTM otherwise.

[mlamouri (slow - plz ping), 2014-12-15 12:47:29]

mlamouri@chromium.org changed reviewers:
+ sof@opera.com
- peter@chromium.org

[mlamouri (slow - plz ping), 2014-12-15 12:47:29]

mlamouri@chromium.org changed required reviewers:
+ mkwst@chromium.org, mvanouwerkerk@chromium.org

[sof, 2014-12-15 13:27:29]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-12-15 13:27:30]

+Cc: oilpan-reviews re: tool support.

https://codereview.chromium.org/793913002/diff/80001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallbacks.cpp (right):

https://codereview.chromium.org/793913002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallbacks.cpp:47:
visitor->trace(m_serviceWorkerRegistration);
Hmm, I wonder if this is worth some tool support (i.e., clang GC plugin) -- with
Oilpan enabled, you're defining a trace() method, but not deriving from a GCed
class. If not an error, but a warning.

(Given that the class doesn't derive from GarbageCollected<> (or variants
thereof), this method will not be invoked, as others have pointed out.)

https://codereview.chromium.org/793913002/diff/80001/Source/modules/push_mess...
File Source/modules/push_messaging/PushRegistrationCallbacks.h (right):

https://codereview.chromium.org/793913002/diff/80001/Source/modules/push_mess...
Source/modules/push_messaging/PushRegistrationCallbacks.h:38:
Persistent<ServiceWorkerRegistration> m_serviceWorkerRegistration;
A WebCallbacks instance is held onto by the embedder, right? In which case we
need to have a strong reference to this GCed object for the duration of its
lifetime as the Oilpan garbage collector will not be able to locate the pointer
to this object on the embedder side -- Oilpan handles Blink (for now.)

Hence, having a Persistent makes sense which registers the pointer as a global
root -- one that will be automatically trace()d during a GC.

In short, Persistent<> here makes sense.

[mlamouri (slow - plz ping), 2014-12-15 13:35:48]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-12-15 13:36:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793913002/100001

[commit-bot: I haz the power, 2014-12-15 15:52:18]

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

[commit-bot: I haz the power, 2014-12-15 15:52:19]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/35526)

[mlamouri (slow - plz ping), 2014-12-15 15:53:23]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-12-15 15:53:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793913002/100001

[commit-bot: I haz the power, 2014-12-15 17:01:55]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187156