Push API: send and receive IPC messages for registration.

Push API: send and receive IPC messages for registration.

BUG=350378

[Michael van Ouwerkerk, 2014-03-31 17:10:43]

Peter as feature reviewer, owner reviewer TBD.

[Peter Beverloo, 2014-04-01 14:53:18]

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:23:
IPC_MESSAGE_HANDLER(PushMessagingHostMsg_Register, OnRegister)
Please indent the IPC_MESSAGE_HANDLER and IPC_MESSAGE_UNHANDLED macros by one
block.

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:32: // TODO: implement.
Please attribute your TODOs (this is different from Blink's FIXMEs, which are
unattributed).

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:32: // TODO: implement.
Should we perhaps post a task to call DidRegister with an error code?  That
would at least allow a round-trip between processes, which can be convenient for
testing at this stage.

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
content/public/common/push_messaging_status.h:10: // Generic push messaging
operation statuses.
nit: This comment doesn't add a lot of value.

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
content/public/common/push_messaging_status.h:16: // Profile not signed in.
Can you add a TODO for removing this value once we no longer require users to be
logged in when registering with GCM?

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
content/public/common/push_messaging_status.h:31: const char*
PushMessagingStatusToString(PushMessagingStatus status);
Do you expect this will be used outside of push_messaging_dispatcher.cc? If we
won't, it'd be better to just define this as a function in an anonymous
namespace in that file.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:10: #include
"third_party/WebKit/public/platform/WebString.h"
WebString.h is already included from the header file.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:22:
IPC_MESSAGE_HANDLER(PushMessagingMsg_RegisterSuccess, OnRegisterSuccess)
Please indent the IPC_MESSAGE_HANDLER and IPC_MESSAGE_UNHANDLED macros another
block.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:44: DCHECK(callbacks);
Can we just use CHECK here? If we receive an IPC with an invalid callback Id
then either the IPC is bogus (bad), or there's a bug in the sending side of the
browser process (bad). Both should be severe enough to fix with a high priority,
rather than silently ignoring here.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:48: callbacks->onSuccess(new
blink::WebPushRegistration(
We leak the WebPushRegistration object here. Can you put this in a scoped_ptr<>
before invoking |onSuccess|?

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.h:16: 
+#include "ipc/ipc_message.h"

[Michael van Ouwerkerk, 2014-04-02 14:25:14]

Thanks Peter! Please take another look.

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:23:
IPC_MESSAGE_HANDLER(PushMessagingHostMsg_Register, OnRegister)
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Please indent the IPC_MESSAGE_HANDLER and IPC_MESSAGE_UNHANDLED macros by one
> block.

Done.

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:32: // TODO: implement.
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Please attribute your TODOs (this is different from Blink's FIXMEs, which are
> unattributed).

Done.

https://codereview.chromium.org/219653002/diff/1/content/browser/push_messagi...
content/browser/push_messaging_message_filter.cc:32: // TODO: implement.
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Should we perhaps post a task to call DidRegister with an error code?  That
> would at least allow a round-trip between processes, which can be convenient
for
> testing at this stage.

Done.

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
content/public/common/push_messaging_status.h:10: // Generic push messaging
operation statuses.
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> nit: This comment doesn't add a lot of value.

Done.

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
content/public/common/push_messaging_status.h:16: // Profile not signed in.
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Can you add a TODO for removing this value once we no longer require users to
be
> logged in when registering with GCM?

Done.

https://codereview.chromium.org/219653002/diff/1/content/public/common/push_m...
content/public/common/push_messaging_status.h:31: const char*
PushMessagingStatusToString(PushMessagingStatus status);
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Do you expect this will be used outside of push_messaging_dispatcher.cc? If we
> won't, it'd be better to just define this as a function in an anonymous
> namespace in that file.

Done.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:10: #include
"third_party/WebKit/public/platform/WebString.h"
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> WebString.h is already included from the header file.

Done.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:22:
IPC_MESSAGE_HANDLER(PushMessagingMsg_RegisterSuccess, OnRegisterSuccess)
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Please indent the IPC_MESSAGE_HANDLER and IPC_MESSAGE_UNHANDLED macros another
> block.

Done.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:44: DCHECK(callbacks);
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> Can we just use CHECK here? If we receive an IPC with an invalid callback Id
> then either the IPC is bogus (bad), or there's a bug in the sending side of
the
> browser process (bad). Both should be severe enough to fix with a high
priority,
> rather than silently ignoring here.

Done.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.cc:48: callbacks->onSuccess(new
blink::WebPushRegistration(
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> We leak the WebPushRegistration object here. Can you put this in a
scoped_ptr<>
> before invoking |onSuccess|?

Done.

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/219653002/diff/1/content/renderer/push_messag...
content/renderer/push_messaging_dispatcher.h:16: 
On 2014/04/01 14:53:18, Peter Beverloo wrote:
> +#include "ipc/ipc_message.h"

Done.

[Michael van Ouwerkerk, 2014-04-02 14:25:37]

Jochen, can you please take a look as owner?

[Michael van Ouwerkerk, 2014-04-02 14:30:49]

Chris as IPC message reviewer.

[Peter Beverloo, 2014-04-02 14:46:04]

This seems good to me, modulo the following comments. What is your plan for
testing this?

https://codereview.chromium.org/219653002/diff/20001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/20001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:34:
BrowserThread::PostTask(BrowserThread::UI,
mmm this doesn't look nice, but I think it still beats not being able to test
the complete round-trip :(.

https://codereview.chromium.org/219653002/diff/20001/content/public/common/pu...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/20001/content/public/common/pu...
content/public/common/push_messaging_status.h:6: #define
CONTENT_PUBLIC_COMMON_PUSH_MESSAGING_PUSH_MESSAGING_STATUS_H_
nit: double PUSH_MESSAGING doesn't reflect the actual file name of this file.

https://codereview.chromium.org/219653002/diff/20001/content/public/common/pu...
content/public/common/push_messaging_status.h:10: enum PushMessagingStatus {
nit: maybe add a comment that this is based on the GCMClient status enumeration?

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.cc:77:
blink::WebString::fromUTF8(registration_id)));
You could use "using blink::WebString" to avoid a bunch of explicit blink::
qualifiers here. Especially if you expect to need more in the future it could
reduce the noise a bit.

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.cc:78:
callbacks->onSuccess(registration.release());
You want to use .get() here, since .release() will still cause us to leak the
instance (same on line 94).

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.cc:86: DCHECK(callbacks);
Same comment as I left on line 70 in my previous review: Let's use a CHECK here.
The if-clauses on lines 71 and 87 aren't needed anymore, because we'd have
crashed by then if callbacks == null.

[Michael van Ouwerkerk, 2014-04-02 15:49:30]

Thanks Peter! Let's make a plan for testing this separately. I'm trying to avoid
making these patches overly large, and testing these partial implementations is
tricky.

Please take another look.

https://codereview.chromium.org/219653002/diff/20001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/20001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:34:
BrowserThread::PostTask(BrowserThread::UI,
On 2014/04/02 14:46:05, Peter Beverloo wrote:
> mmm this doesn't look nice, but I think it still beats not being able to test
> the complete round-trip :(.

Acknowledged. Also, this is what you asked for.

https://codereview.chromium.org/219653002/diff/20001/content/public/common/pu...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/20001/content/public/common/pu...
content/public/common/push_messaging_status.h:6: #define
CONTENT_PUBLIC_COMMON_PUSH_MESSAGING_PUSH_MESSAGING_STATUS_H_
On 2014/04/02 14:46:05, Peter Beverloo wrote:
> nit: double PUSH_MESSAGING doesn't reflect the actual file name of this file.

Done.

https://codereview.chromium.org/219653002/diff/20001/content/public/common/pu...
content/public/common/push_messaging_status.h:10: enum PushMessagingStatus {
On 2014/04/02 14:46:05, Peter Beverloo wrote:
> nit: maybe add a comment that this is based on the GCMClient status
enumeration?

Done.

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
File content/renderer/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.cc:77:
blink::WebString::fromUTF8(registration_id)));
On 2014/04/02 14:46:05, Peter Beverloo wrote:
> You could use "using blink::WebString" to avoid a bunch of explicit blink::
> qualifiers here. Especially if you expect to need more in the future it could
> reduce the noise a bit.

Done.

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.cc:78:
callbacks->onSuccess(registration.release());
On 2014/04/02 14:46:05, Peter Beverloo wrote:
> You want to use .get() here, since .release() will still cause us to leak the
> instance (same on line 94).

Done.

https://codereview.chromium.org/219653002/diff/20001/content/renderer/push_me...
content/renderer/push_messaging_dispatcher.cc:86: DCHECK(callbacks);
On 2014/04/02 14:46:05, Peter Beverloo wrote:
> Same comment as I left on line 70 in my previous review: Let's use a CHECK
here.
> The if-clauses on lines 71 and 87 aren't needed anymore, because we'd have
> crashed by then if callbacks == null.

Done.

[Peter Beverloo, 2014-04-03 16:03:19]

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:46: void
PushMessagingMessageFilter::DidRegister(int routing_id,
Split this up in DidRegister and DidRegistrationFailed, to not have the
conditional on lines 52:57?

https://codereview.chromium.org/219653002/diff/40001/content/public/common/pu...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/40001/content/public/common/pu...
content/public/common/push_messaging_status.h:10: // This is based on the
GCMClient::Result enum.
Can we remove this enum altogether? Including the value-to-string function? The
spec doesn't force us to expose a specific reason at all (beyond AbortError),
and it adds quite some overhead.

[palmer, 2014-04-03 18:56:13]

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:54: routing_id, callbacks_id,
endpoint, registration_id));
Nit: indentation

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.h:28: const std::string&
sender_id);
What is the structure of this string?

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.h:32: const std::string& endpoint,
And of these strings?

[Michael van Ouwerkerk, 2014-04-04 10:39:55]

Thanks for the reviews! Please take another look.

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:46: void
PushMessagingMessageFilter::DidRegister(int routing_id,
On 2014/04/03 16:03:19, Peter Beverloo wrote:
> Split this up in DidRegister and DidRegistrationFailed, to not have the
> conditional on lines 52:57?

Having a single method works well with GCMClient, which uses a single method and
a callback. I'm inclined to leave this as is until there's a real need to change
it.

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.cc:54: routing_id, callbacks_id,
endpoint, registration_id));
On 2014/04/03 18:56:13, Chromium Palmer wrote:
> Nit: indentation

Done.

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.h:28: const std::string&
sender_id);
On 2014/04/03 18:56:13, Chromium Palmer wrote:
> What is the structure of this string?

It's an opaque token from the perspective of this class. It looks like it might
actually be a number e.g. 670330094152, but GCM receives it as a string:
http://developer.android.com/reference/com/google/android/gms/gcm/GoogleCloud...


For more details about the registration mechanism please see:
http://developer.android.com/google/gcm/gs.html
http://developer.android.com/google/gcm/client.html

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.h:32: const std::string& endpoint,
On 2014/04/03 18:56:13, Chromium Palmer wrote:
> And of these strings?

The endpoint will be a url unless we change the W3C specification. "the absolute
URL exposed by the push server where the application server can send push
notifications to":
https://dvcs.w3.org/hg/push/raw-file/tip/index.html#idl-def-PushRegistration

The registration id is again an opaque string from the perspective of this
class. It is returned by GCM's register method:
http://developer.android.com/reference/com/google/android/gms/gcm/GoogleCloud...

https://codereview.chromium.org/219653002/diff/40001/content/public/common/pu...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/40001/content/public/common/pu...
content/public/common/push_messaging_status.h:10: // This is based on the
GCMClient::Result enum.
On 2014/04/03 16:03:19, Peter Beverloo wrote:
> Can we remove this enum altogether? Including the value-to-string function?
The
> spec doesn't force us to expose a specific reason at all (beyond AbortError),
> and it adds quite some overhead.

Done. I didn't like the enum either.

[palmer, 2014-04-04 17:57:16]

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.h:32: const std::string& endpoint,
> The endpoint will be a url unless we change the W3C specification. "the
absolute
> URL exposed by the push server where the application server can send push
> notifications to":
> https://dvcs.w3.org/hg/push/raw-file/tip/index.html#idl-def-PushRegistration

Ahh. Use a GURL then.

[Michael van Ouwerkerk, 2014-04-07 14:43:26]

Thanks Chris! Please take another look.

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/219653002/diff/40001/content/browser/push_mes...
content/browser/push_messaging_message_filter.h:32: const std::string& endpoint,
On 2014/04/04 17:57:16, Chromium Palmer wrote:
> > The endpoint will be a url unless we change the W3C specification. "the
> absolute
> > URL exposed by the push server where the application server can send push
> > notifications to":
> > https://dvcs.w3.org/hg/push/raw-file/tip/index.html#idl-def-PushRegistration
> 
> Ahh. Use a GURL then.

Done.

[Peter Beverloo, 2014-04-07 14:52:55]

lgtm modulo removing push_messaging_status.h.

https://codereview.chromium.org/219653002/diff/100001/content/browser/push_me...
File content/browser/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/219653002/diff/100001/content/browser/push_me...
content/browser/push_messaging_message_filter.cc:32: // TODO(mvanouwerkerk):
Really implement, the below simply returns an error.
It might be nice to clarify why this code should temporarily be here. Something
like "TODO(mvanouwerkerk): Create a new registration with GCM. Forcefully fail
the registration for now, which will result in the web-facing Promise to be
rejected" could work, but might be a tad verbose.

https://codereview.chromium.org/219653002/diff/100001/content/browser/push_me...
File content/browser/push_messaging_message_filter.h (right):

https://codereview.chromium.org/219653002/diff/100001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:11: #include "url/gurl.h"
nit: you could forward declare GURL here.

https://codereview.chromium.org/219653002/diff/100001/content/browser/push_me...
content/browser/push_messaging_message_filter.h:34: bool error);
nit: the only use of this is "!error", so maybe "success" might be a better fit?

https://codereview.chromium.org/219653002/diff/100001/content/public/common/p...
File content/public/common/push_messaging_status.h (right):

https://codereview.chromium.org/219653002/diff/100001/content/public/common/p...
content/public/common/push_messaging_status.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Remove this file.

[palmer, 2014-04-07 20:23:21]

IPC security LGTM.

[jochen (gone - plz use gerrit), 2014-04-08 07:42:17]

i would recommend to move the new files to a separate sub directory
content/*/push_messaging or similar

lgtm with nits

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:12: #include "ipc/ipc_message.h"
can be forward declared?

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:14: #include
"third_party/WebKit/public/platform/WebPushError.h"
not needed in the header?

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:15: #include
"third_party/WebKit/public/platform/WebString.h"
can be forward declared?

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:16: #include "url/gurl.h"
can be forward declared?

[Michael van Ouwerkerk, 2014-04-08 13:27:00]

Thanks for the reviews!

Jochen: I think it's not necessary to create directories for this feature in
content, they would contain only 1 or 2 files. I expect most of the
implementation to reside in chrome/browser and the blink module. If things work
out differently, I'll introduce a directory.

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
File content/renderer/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:12: #include "ipc/ipc_message.h"
On 2014/04/08 07:42:18, jochen wrote:
> can be forward declared?

Done.

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:14: #include
"third_party/WebKit/public/platform/WebPushError.h"
On 2014/04/08 07:42:18, jochen wrote:
> not needed in the header?

Done.

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:15: #include
"third_party/WebKit/public/platform/WebString.h"
On 2014/04/08 07:42:18, jochen wrote:
> can be forward declared?

Done.

https://codereview.chromium.org/219653002/diff/140001/content/renderer/push_m...
content/renderer/push_messaging_dispatcher.h:16: #include "url/gurl.h"
On 2014/04/08 07:42:18, jochen wrote:
> can be forward declared?

Done.

[Michael van Ouwerkerk, 2014-04-08 13:27:08]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-04-08 13:27:17]

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

[commit-bot: I haz the power, 2014-04-08 15:39:41]

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

[commit-bot: I haz the power, 2014-04-08 15:39:42]

Couldn't make sense out of svn commit message:
Adding         content/browser/push_messaging_message_filter.cc
Adding         content/browser/push_messaging_message_filter.h
Sending        content/browser/renderer_host/render_process_host_impl.cc
Sending        content/common/content_message_generator.h
Adding         content/common/push_messaging_messages.h
Sending        content/content_browser.gypi
Sending        content/content_common.gypi
Sending        content/content_renderer.gypi
Adding         content/renderer/push_messaging_dispatcher.cc
Adding         content/renderer/push_messaging_dispatcher.h
Sending        content/renderer/render_view_impl.cc
Sending        content/renderer/render_view_impl.h
Sending        ipc/ipc_message_start.h
Transmitting file data .............
Committed revision 262419.

Warning: post-commit hook failed (exit code 2) with output:
/opt/shared_hooks/post-commit.d/10_mailer.py:137: DeprecationWarning: The popen2
module is deprecated.  Use the subprocess module.
  import popen2
Post-commit mail timed out for r262419. Your commit succeeded,
but no mail was sent.