Define push_messaging module with PushManager skeleton.

Define push_messaging module with PushManager skeleton.

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

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

[Michael van Ouwerkerk, 2014-03-26 19:30:22]

This is a small subset of this larger prototyping patch:
https://codereview.chromium.org/152763002/

Peter: as feature reviewer
Adam: as owner

Please take a look, thanks!

[Peter Beverloo, 2014-03-26 21:35:35]

This overall looks fine to me, thanks Michael!

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/NavigatorPushManager.h (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/NavigatorPushManager.h:15: class
NavigatorPushManager FINAL : public Supplement<Navigator> {
WillBeHeapSupplement? Then include the WILL_BE_USING_GARBAGE_COLLECTED_MIXIN
define, use RefPtrWillBeMember<> instead of RefPtr<>, and so on. I don't think
there's a very strong reason not to do this yet, and there are plenty of
examples in other modules around.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/NavigatorPushManager.h:27: mutable
RefPtr<PushManager> m_pushManager;
While there is some precedence, not declaring pushManager() as const is cleaner,
since it's not. That'd allow you to get rid of the mutable here.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:30: return promise;
It looks a bit odd that we create a ScriptPromiseResolver instance and then free
it at the end of the method, but that's because we're not actually doing
anything here yet. Could you add a FIXME saying that the actual functionality
still has to be implemented?

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.h:10: 
#include "wtf/RefCounted.h"

(IWYU)

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.h:14: class ScriptPromise;
Can we really forward declare ScriptPromise since it's being returned as a value
in registerPushMessaging()? Surely the size needs to be known?

https://codereview.chromium.org/213013006/diff/1/Source/platform/RuntimeEnabl...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/213013006/diff/1/Source/platform/RuntimeEnabl...
Source/platform/RuntimeEnabledFeatures.in:83: PushMessaging status=experimental
Should we postpone moving this to experimental until at least some of the
functionality is working?

Since there is no Web content relying on this yet it's probably fine either way.

[abarth-chromium, 2014-03-27 03:38:57]

lgtm on creating the module

You might want to write some basic tests for this class even at this stage.

Please make sure beverloo is happy before landing.  (I'd also appreciate being
CCed on these changes for a bit to make sure you're starting off on the right
foot.)

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:27:
ASSERT(RuntimeEnabledFeatures::pushMessagingEnabled());
We usually skip these and trust the IDL

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:30: return promise;
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> It looks a bit odd that we create a ScriptPromiseResolver instance and then
free
> it at the end of the method, but that's because we're not actually doing
> anything here yet. Could you add a FIXME saying that the actual functionality
> still has to be implemented?

Maybe we should reject the promise for now?

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.idl (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.idl:6: NoInterfaceObject,
Can you add a RuntimeEnabled attribute to this interface too?

https://codereview.chromium.org/213013006/diff/1/Source/platform/RuntimeEnabl...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/213013006/diff/1/Source/platform/RuntimeEnabl...
Source/platform/RuntimeEnabledFeatures.in:83: PushMessaging status=experimental
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> Should we postpone moving this to experimental until at least some of the
> functionality is working?

+1

Let's start with status=test.  We can graduate to status=experimental when we're
ready for folks to start experimenting with it.

[Michael van Ouwerkerk, 2014-03-27 19:15:23]

Thanks! PTAL. Acknowledged there are no tests yet, I did a manual test.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/NavigatorPushManager.h (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/NavigatorPushManager.h:15: class
NavigatorPushManager FINAL : public Supplement<Navigator> {
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> WillBeHeapSupplement? Then include the WILL_BE_USING_GARBAGE_COLLECTED_MIXIN
> define, use RefPtrWillBeMember<> instead of RefPtr<>, and so on. I don't think
> there's a very strong reason not to do this yet, and there are plenty of
> examples in other modules around.

Done.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/NavigatorPushManager.h:27: mutable
RefPtr<PushManager> m_pushManager;
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> While there is some precedence, not declaring pushManager() as const is
cleaner,
> since it's not. That'd allow you to get rid of the mutable here.

Done.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.cpp (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:27:
ASSERT(RuntimeEnabledFeatures::pushMessagingEnabled());
On 2014/03/27 03:38:57, abarth wrote:
> We usually skip these and trust the IDL

Done.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:30: return promise;
On 2014/03/27 03:38:57, abarth wrote:
> On 2014/03/26 21:35:35, Peter Beverloo wrote:
> > It looks a bit odd that we create a ScriptPromiseResolver instance and then
> free
> > it at the end of the method, but that's because we're not actually doing
> > anything here yet. Could you add a FIXME saying that the actual
functionality
> > still has to be implemented?
> 
> Maybe we should reject the promise for now?

Done.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.cpp:30: return promise;
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> It looks a bit odd that we create a ScriptPromiseResolver instance and then
free
> it at the end of the method, but that's because we're not actually doing
> anything here yet. Could you add a FIXME saying that the actual functionality
> still has to be implemented?

Done.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.h (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.h:10: 
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> #include "wtf/RefCounted.h"
> 
> (IWYU)

Done.

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.h:14: class ScriptPromise;
On 2014/03/26 21:35:35, Peter Beverloo wrote:
> Can we really forward declare ScriptPromise since it's being returned as a
value
> in registerPushMessaging()? Surely the size needs to be known?

Not sure. How about these precedents?
https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20ScriptProm...

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
File Source/modules/push_messaging/PushManager.idl (right):

https://codereview.chromium.org/213013006/diff/1/Source/modules/push_messagin...
Source/modules/push_messaging/PushManager.idl:6: NoInterfaceObject,
On 2014/03/27 03:38:57, abarth wrote:
> Can you add a RuntimeEnabled attribute to this interface too?

Done.

[Peter Beverloo, 2014-03-28 13:52:20]

lgtm

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

https://codereview.chromium.org/213013006/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.h:17: class PushManager FINAL : public
RefCounted<PushManager>, public ScriptWrappable {
s/RefCounted/RefCountedWillBeRefCountedGarbageCollected/.

https://codereview.chromium.org/213013006/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.idl (right):

https://codereview.chromium.org/213013006/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.idl:7: RuntimeEnabled=PushMessaging,
+WillBeGarbageCollected

[Michael van Ouwerkerk, 2014-03-28 16:08:17]

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

https://codereview.chromium.org/213013006/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.h:17: class PushManager FINAL : public
RefCounted<PushManager>, public ScriptWrappable {
On 2014/03/28 13:52:21, Peter Beverloo wrote:
> s/RefCounted/RefCountedWillBeRefCountedGarbageCollected/.

Done.

https://codereview.chromium.org/213013006/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushManager.idl (right):

https://codereview.chromium.org/213013006/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushManager.idl:7: RuntimeEnabled=PushMessaging,
On 2014/03/28 13:52:21, Peter Beverloo wrote:
> +WillBeGarbageCollected

Done.

[Michael van Ouwerkerk, 2014-03-28 16:26:51]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:26:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/213013006/1...

[commit-bot: I haz the power, 2014-03-28 17:00:43]

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

[commit-bot: I haz the power, 2014-03-28 17:00:44]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Michael van Ouwerkerk, 2014-03-28 17:03:25]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-03-28 17:03:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/213013006/1...

[commit-bot: I haz the power, 2014-03-28 17:15:01]

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

[commit-bot: I haz the power, 2014-03-28 17:15:02]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Michael van Ouwerkerk, 2014-03-31 14:17:01]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-03-31 14:17:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/213013006/1...

[Michael van Ouwerkerk, 2014-03-31 14:26:42]

The CQ bit was unchecked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2014-03-31 14:31:39]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-03-31 14:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/213013006/1...

[commit-bot: I haz the power, 2014-03-31 15:39:06]

Change committed as 170454