DO NOT REVIEW Bindings and plumbing for push messaging API.

DO NOT REVIEW
Bindings and plumbing for push messaging API.

The Chromium counterpart is 186023002.

Class diagrams:
https://www.lucidchart.com/documents/view/4336-0378-530494f3-9ce0-1a5e0a005ed1
https://www.lucidchart.com/documents/view/4ffc-b120-5305ebfa-9d0a-31410a005610

Intent to implement thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/GLXyXABvA_0

BUG=350378

[Peter Beverloo, 2014-03-10 13:59:28]

Thanks Michael! A few general comments:

  * Please make sure that a BUG is attached to patches like this before it
lands. Also be sure to tune to commit message before proceeding; it would be
nice to mention the Intent to Implement.
  * What is the plan for testing? Will there be a MockPushClient which handles
(fake) registrations?

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/NavigatorPushManager.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/NavigatorPushManager.h:21: PushManager*
pushManager() const;
I don't think it's good to mark this method as const :(. If it were just to ref
and deref the RefPtr it'd be fine, but it actually creates a PushManager
instance if it doesn't exist yet. Naming it ensurePushManager() and have it be
non-const would be a lot better, but this seems to be used more often :/.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushController.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushController.cpp:31: return 0;
nit: nullptr is now preferred in Blink.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushController.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushController.h:31: blink::WebPushClient* client()
{ return m_client; }
nit: can be const

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushManager.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.cpp:32: return adoptRef(new
PushManager());
adoptRefWillBeNoop(). Can probably be moved to the header file as well.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushManager.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.h:16: class PushManager FINAL : public
RefCounted<PushManager>, public ScriptWrappable {
This should use RefCountedWillBeGarbageCollectedFinalized<PushManager>.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.h:18: static PassRefPtr<PushManager>
create();
PassRefPtrWillBeRawPtr<PushManager> create();

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushManager.idl (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.idl:8: [CallWith=ExecutionContext,
ImplementedAs=registerPushMessaging] Promise register(DOMString channelname);
Add a FIXME (possibly with assigned bug) about the rest of the API needing
implementation, specifically unregister() and registrations()?

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.idl:8: [CallWith=ExecutionContext,
ImplementedAs=registerPushMessaging] Promise register(DOMString channelname);
I thought we were dropping the |channelName| attribute as it's just syntactic
sugar?

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushMessagingError.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushMessagingError.cpp:21: }
micro nit: new line, optionally "// namespace WebCore", but it's quite a short
file.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushMessagingError.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushMessagingError.h:18: static
PassRefPtr<DOMError> from(WebType* webErrorRaw)
RefPtrWillBeRawPtr<DOMError>

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushMessagingError.h:27:
WTF_MAKE_NONCOPYABLE(PushMessagingError);
micro nit: I would place a blank line above this, as lines 27 and 28 are
removing functionality rather than adding it. Personal preference though :-).

https://codereview.chromium.org/152763002/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:413: providePushControllerTo(*m_page,
m_client->webPushClient());
Perhaps you could check the value of the runtime flag here, to avoid creating a
PushController if experimental features are disabled? It won't be accessed
anyway because of ASSERTs elsewhere. Otherwise you could consolidate this block
with the one on lines 418-421, to avoid an extra branch.

https://codereview.chromium.org/152763002/diff/100001/public/platform/WebPush...
File public/platform/WebPushMessagingError.h (right):

https://codereview.chromium.org/152763002/diff/100001/public/platform/WebPush...
public/platform/WebPushMessagingError.h:16: ErrorTypeLast = UnknownError
ErrorTypeLast isn't used anywhere, what do we need it for?

[Michael van Ouwerkerk, 2014-03-10 15:12:04]

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/NavigatorPushManager.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/NavigatorPushManager.h:21: PushManager*
pushManager() const;
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> I don't think it's good to mark this method as const :(. If it were just to
ref
> and deref the RefPtr it'd be fine, but it actually creates a PushManager
> instance if it doesn't exist yet. Naming it ensurePushManager() and have it be
> non-const would be a lot better, but this seems to be used more often :/.

Maybe I'm doing the old thing because I looked at the Geolocation code, among
others.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushController.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushController.cpp:31: return 0;
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> nit: nullptr is now preferred in Blink.

"return nullptr" does not compile. How is that not compatible?

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushController.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushController.h:31: blink::WebPushClient* client()
{ return m_client; }
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> nit: can be const

Done.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushManager.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.cpp:32: return adoptRef(new
PushManager());
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> adoptRefWillBeNoop(). Can probably be moved to the header file as well.

Moved to header.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushManager.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.h:16: class PushManager FINAL : public
RefCounted<PushManager>, public ScriptWrappable {
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> This should use RefCountedWillBeGarbageCollectedFinalized<PushManager>.

Maybe I can leave that for a later patch. It would take a bit of time to read up
on how Oilpan works.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.h:18: static PassRefPtr<PushManager>
create();
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> PassRefPtrWillBeRawPtr<PushManager> create();

Same.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushManager.idl (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.idl:8: [CallWith=ExecutionContext,
ImplementedAs=registerPushMessaging] Promise register(DOMString channelname);
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> I thought we were dropping the |channelName| attribute as it's just syntactic
> sugar?

Maybe, it wasn't clear to me whether we'd partially or completely remove it from
the API.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushManager.idl:8: [CallWith=ExecutionContext,
ImplementedAs=registerPushMessaging] Promise register(DOMString channelname);
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> Add a FIXME (possibly with assigned bug) about the rest of the API needing
> implementation, specifically unregister() and registrations()?

I think it's implied that one patch does not implement the whole API. The API
spec will be referenced from the bug.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushMessagingError.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushMessagingError.cpp:21: }
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> micro nit: new line, optionally "// namespace WebCore", but it's quite a short
> file.

Done.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
File Source/modules/pushmessaging/PushMessagingError.h (right):

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushMessagingError.h:18: static
PassRefPtr<DOMError> from(WebType* webErrorRaw)
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> RefPtrWillBeRawPtr<DOMError>

Punting.

https://codereview.chromium.org/152763002/diff/100001/Source/modules/pushmess...
Source/modules/pushmessaging/PushMessagingError.h:27:
WTF_MAKE_NONCOPYABLE(PushMessagingError);
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> micro nit: I would place a blank line above this, as lines 27 and 28 are
> removing functionality rather than adding it. Personal preference though :-).

Done.

https://codereview.chromium.org/152763002/diff/100001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/152763002/diff/100001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:413: providePushControllerTo(*m_page,
m_client->webPushClient());
On 2014/03/10 13:59:28, Peter Beverloo wrote:
> Perhaps you could check the value of the runtime flag here, to avoid creating
a
> PushController if experimental features are disabled? It won't be accessed
> anyway because of ASSERTs elsewhere. Otherwise you could consolidate this
block
> with the one on lines 418-421, to avoid an extra branch.

Done.

[Michael van Ouwerkerk, 2014-03-10 15:14:06]

Adam, this needs to be broken into smaller pieces, and tests. Could you please
look at it from a general perspective though?