Push API: use (un)subscription instead of (un)registration

Push API: use of (un)subscription instead of (un)registration

BUG=446883
This is in response to this spec issue:
https://github.com/w3c/push-api/issues/98

Committed: https://crrev.com/078aae8c35295ce939b8eb6dba2e2990fe422826
Cr-Commit-Position: refs/heads/master@{#331062}

[Pranay, 2015-05-13 12:59:51]

pranay.kumar@samsung.com changed reviewers:
+ davidben@chromium.org, mvanouwerkerk@chromium.org

[Pranay, 2015-05-13 12:59:51]

mvanouwerkerk@chromium.org: Please review changes in
chrome/browser/push_messaging

davidben@chromium.org: Please review changes in content

[Peter Beverloo, 2015-05-13 13:18:49]

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

[Peter Beverloo, 2015-05-13 13:18:49]

+1 to renaming registration -> subscription, but I would like to know a little
bit more about whether this is a one-off patch or whether you'd like to take on
the full rename.

(For context, see the following patch which I won't be able to work on in the
near future: https://codereview.chromium.org/1129833003/).

The selection of renames you're doing is a bit arbitrary and rather incomplete,
and while I definitely agree with having multiple patches, can we try to be
complete on at least a per-file basis?

[Pranay, 2015-05-14 13:56:33]

Hi Peter,
Thanks for your feedback. Sure, i would like to take the responsibility ahead
for full renaming :)
Could you please explain more about naming conventions we should go along with?
 
Also, definitely multiple patches, I feel same about considering file by file,
but some files like (push_messaging_status.h)
can lead to a very big patch (multiple enums & enum values that have large
number of references), would it be okay to break in smaller ones in that case?
:)

Thanks
Pranay

[Peter Beverloo, 2015-05-14 14:01:59]

On 2015/05/14 13:56:33, Pranay wrote:
> Hi Peter,
> Thanks for your feedback. Sure, i would like to take the responsibility ahead
> for full renaming :)
> Could you please explain more about naming conventions we should go along
with?
>  
> Also, definitely multiple patches, I feel same about considering file by file,
> but some files like (push_messaging_status.h)
> can lead to a very big patch (multiple enums & enum values that have large
> number of references), would it be okay to break in smaller ones in that case?
> :)
> 
> Thanks
> Pranay

Sure, please go right ahead! Thanks :-)

Regarding naming conventions, the two things to keep in mind are:
  * It's a push messaging *subscription*.
  * It's a Service Worker *registration*.

How to apply the rename depends on its usage, because it should of course be
linguistically correct. For example:
  * unregister -> unsubscribe
  * getRegistration -> getSubscription
  * ... and so on

Would you like to update this patch before we review it?

[Pranay, 2015-05-15 04:15:03]

Sure Peter, Thanks for the info about naming convention. 
I'll update this patch soon

[Pranay, 2015-05-15 09:50:36]

pranay.kumar@samsung.com changed reviewers:
+ dcheng@chromium.org

[Pranay, 2015-05-15 09:50:36]

dcheng@chromium.org: Please review changes in 

content/common

[Pranay, 2015-05-15 10:15:19]

https://codereview.chromium.org/1134733006/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (left):

https://codereview.chromium.org/1134733006/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:121: void
RegisterEnd(
Based upon the usage, references & & semantics of RegisterEnd, i felt
"SubscribeFail" should  be the proper rename. Let me know your thoughts on that.

[Pranay, 2015-05-15 10:18:05]

PTAL, have tried to cover up maximum changes related to a single file in latest
patch set

Thanks :)

[Peter Beverloo, 2015-05-18 12:44:38]

Thanks for the patch! I've got a series of nits and a few questions.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:257:
base::Bind(&UnregisterCallbackToClosure,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:311: std::string(),
nit: indentation +2, but this'll depend on renaming the method.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:338: std::string(),
Same as the previous nit.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:367:
SubscribeFail(register_callback, std::string(),
Same as the previous nit.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:381:
SubscribeFail(register_callback, std::string(),
Same as the previous nit.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:454: std::string(),
Same as the previous nit.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:616:
base::Bind(&UnregisterCallbackToClosure, barrier_closure));
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:121: void
SubscribeFail(
I don't like this rename. We call this from the DidSubscribe() method with a set
|subscription_id| when the subscription was successful.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:141: const
std::string& sender_id,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:145: const
content::PushMessagingService::UnregisterCallback&,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:516:
requesting_origin,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:671: request_id,
blink::WebPushError::ErrorTypeAbort,
Not your change, but these lines should be indented by four spaces rather than
two. Would you consider changing this? :)

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:48: // Subscriube
methods on IO thread ---------------------------------------------
Subscriube -> Subscribe

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:51: int
request_id,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:57: int64_t
service_worker_registration_id,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:168: PushRegistrationStatus
status) {
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
File content/child/push_messaging/push_provider.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.h:67: const GURL& endpoint,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.h:72: blink::WebPushError::ErrorType
error_type,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/common/push_mes...
File content/common/push_messaging_messages.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/common/push_mes...
content/common/push_messaging_messages.h:37: std::string /* push_registration_id
*/)
push_registration_id -> push_subscription_id (elsewhere in this file too)

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:25: using RegisterCallback =
Is your plan to rename these types later?

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:47: int64
service_worker_registration_id,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:58: int64
service_worker_registration_id,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:67: int64
service_worker_registration_id,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:73:
PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.h:54: const GURL&
endpoint,
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.h:58:
PushRegistrationStatus status);
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:54:
sender_id, user_visible, callback);
nit: indentation +1

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_push_messaging_service.h
(right):

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_push_messaging_service.h:44: int64
service_worker_registration_id,
nit: indentation +1

[Pranay, 2015-05-19 04:40:40]

Hi Peter,
Updated the Patch, sorry for all the indentation nits. Will be careful onwards
:) 

PTAL again
Thanks

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:257:
base::Bind(&UnregisterCallbackToClosure,
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:311: std::string(),
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> nit: indentation +2, but this'll depend on renaming the method.

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:338: std::string(),
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> Same as the previous nit.

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:367:
SubscribeFail(register_callback, std::string(),
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> Same as the previous nit.

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:381:
SubscribeFail(register_callback, std::string(),
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> Same as the previous nit.

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:454: std::string(),
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> Same as the previous nit.

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:616:
base::Bind(&UnregisterCallbackToClosure, barrier_closure));
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:121: void
SubscribeFail(
Sorry, overlooked GCMClient::SUCCESS case in DidSubscribe() which led me to
believe about usage of this function as failure, Thanks! Fixed

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:141: const
std::string& sender_id,
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:145: const
content::PushMessagingService::UnregisterCallback&,
On 2015/05/18 12:44:36, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:516:
requesting_origin,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:671: request_id,
blink::WebPushError::ErrorTypeAbort,
Sure..Done :)

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:48: // Subscriube
methods on IO thread ---------------------------------------------
Oops.. Sorry for that, Done

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:51: int
request_id,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:57: int64_t
service_worker_registration_id,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:168: PushRegistrationStatus
status) {
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
File content/child/push_messaging/push_provider.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.h:67: const GURL& endpoint,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/child/push_mess...
content/child/push_messaging/push_provider.h:72: blink::WebPushError::ErrorType
error_type,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/common/push_mes...
File content/common/push_messaging_messages.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/common/push_mes...
content/common/push_messaging_messages.h:37: std::string /* push_registration_id
*/)
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> push_registration_id -> push_subscription_id (elsewhere in this file too)

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
File content/public/browser/push_messaging_service.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:25: using RegisterCallback =
Yes..definitely, will cover up next :) (didnt cover in this CL as this callback
is majorly used)

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:47: int64
service_worker_registration_id,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:58: int64
service_worker_registration_id,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/public/browser/...
content/public/browser/push_messaging_service.h:67: int64
service_worker_registration_id,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.cc (right):

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.cc:73:
PUSH_REGISTRATION_STATUS_NO_SENDER_ID);
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
File content/renderer/push_messaging/push_messaging_dispatcher.h (right):

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.h:54: const GURL&
endpoint,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/renderer/push_m...
content/renderer/push_messaging/push_messaging_dispatcher.h:58:
PushRegistrationStatus status);
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_push_messaging_service.cc
(right):

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_push_messaging_service.cc:54:
sender_id, user_visible, callback);
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_push_messaging_service.h
(right):

https://codereview.chromium.org/1134733006/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_push_messaging_service.h:44: int64
service_worker_registration_id,
On 2015/05/18 12:44:37, Peter Beverloo wrote:
> nit: indentation +1

Done.

[Peter Beverloo, 2015-05-19 10:40:52]

lgtm, thanks!

At some point we should do a similar clean-up for int64 -> int64_t, but I don't
think that has to be done in this patch :).

https://codereview.chromium.org/1134733006/diff/60001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1134733006/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:670: Send(
nit: This indentation is a bit odd. Could we keep the |new
PushMessagingMsg_UnsubscribeError(| on the same line as Send(?

[Pranay, 2015-05-19 10:49:33]

Sure, we would do int64 changes later on, Thanks for the LGTM :)

https://codereview.chromium.org/1134733006/diff/60001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1134733006/diff/60001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.cc:670: Send(
On 2015/05/19 10:40:52, Peter Beverloo wrote:
> nit: This indentation is a bit odd. Could we keep the |new
> PushMessagingMsg_UnsubscribeError(| on the same line as Send(?

Done.

[Pranay, 2015-05-19 10:52:05]

Hi David,
Please review changes in content folder

Thanks
-Pranay

[dcheng, 2015-05-19 19:57:08]

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:258:
message_handled_closure));
This formatting is wrong.

Usually I'd recommend just running CLs through git cl-format. You don't have to
think about these sort of things with clang format =)

(Though you have to be careful in this case: the clang formatter misformats
IPC_MESSAGE_HANDLER, etc macros)

[dcheng, 2015-05-19 19:57:22]

(Sorry, I forgot to add: IPC changes LGTM)

[davidben, 2015-05-19 20:10:28]

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:294: // Subscribe
and GetPermissionStatus methods ------------------------------------
80 character column limit

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:469: // Unsubscribe
methods ----------------------------------------------------------
80 character column limit

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:119: // Subscribe
methods ----------------------------------------------------------
80 character column limit

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:138: // Unsubscribe
methods --------------------------------------------------------
80 character column limit

https://codereview.chromium.org/1134733006/diff/80001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/1134733006/diff/80001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:48: // Subscribe
methods on IO thread ---------------------------------------------
80 character column limit

https://codereview.chromium.org/1134733006/diff/80001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:97: //
Unsubscribe methods on IO thread -------------------------------------------
80 character column limit

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:152: const std::string&
registration_id) {
You missed this one.

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
File content/child/push_messaging/push_provider.h (right):

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
content/child/push_messaging/push_provider.h:69: void
OnSubscribeFromWorkerError(int request_id, PushRegistrationStatus status);
80 character column limit

[Pranay, 2015-05-20 03:51:57]

Hi David & Dcheng,
Thanks for the review. Have updated the patch, PTAL

-Pranay

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:258:
message_handled_closure));
Yes..same thing happened, didnt know about that IPC_MESSAGE_HANDLER
misformatting
Thanks for clarifying :)

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:294: // Subscribe
and GetPermissionStatus methods ------------------------------------
On 2015/05/19 20:10:27, David Benjamin wrote:
> 80 character column limit

Done.

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:469: // Unsubscribe
methods ----------------------------------------------------------
On 2015/05/19 20:10:28, David Benjamin wrote:
> 80 character column limit

Done.

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:119: // Subscribe
methods ----------------------------------------------------------
On 2015/05/19 20:10:28, David Benjamin wrote:
> 80 character column limit

Done.

https://codereview.chromium.org/1134733006/diff/80001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:138: // Unsubscribe
methods --------------------------------------------------------
On 2015/05/19 20:10:28, David Benjamin wrote:
> 80 character column limit

Done.

https://codereview.chromium.org/1134733006/diff/80001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_message_filter.h (right):

https://codereview.chromium.org/1134733006/diff/80001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:48: // Subscribe
methods on IO thread ---------------------------------------------
On 2015/05/19 20:10:28, David Benjamin wrote:
> 80 character column limit

Done.

https://codereview.chromium.org/1134733006/diff/80001/content/browser/push_me...
content/browser/push_messaging/push_messaging_message_filter.h:97: //
Unsubscribe methods on IO thread -------------------------------------------
On 2015/05/19 20:10:28, David Benjamin wrote:
> 80 character column limit

Done.

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
File content/child/push_messaging/push_provider.cc (right):

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
content/child/push_messaging/push_provider.cc:152: const std::string&
registration_id) {
Thanks, :) Done.

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
File content/child/push_messaging/push_provider.h (right):

https://codereview.chromium.org/1134733006/diff/80001/content/child/push_mess...
content/child/push_messaging/push_provider.h:69: void
OnSubscribeFromWorkerError(int request_id, PushRegistrationStatus status);
On 2015/05/19 20:10:28, David Benjamin wrote:
> 80 character column limit

Done.

[Peter Beverloo, 2015-05-20 10:45:47]

How did you upload the patch with lines that are longer than 80 characters? You
should have received a pre-submit warning -- please don't ignore those!

Thanks,
Peter

[Pranay, 2015-05-20 11:05:09]

Hi Peter,
Apologies for the same. Actually i was facing a problem with pre-submit. While
uploading the patch - "content/child/push_messaging/OWNERS" line 1 : file kept
on giving synatx issue- line is not a "set" directive , "*" or an email address:
"file://content/browser/push_messaging/OWNERS", regardless of my any change. In
the same reference, I must have ignored these warnings. 
Will keep in mind about pre-submit for sure :)

[Peter Beverloo, 2015-05-20 11:15:27]

On 2015/05/20 11:05:09, Pranay wrote:
> Hi Peter,
> Apologies for the same. Actually i was facing a problem with pre-submit. While
> uploading the patch - "content/child/push_messaging/OWNERS" line 1 : file kept
> on giving synatx issue- line is not a "set" directive , "*" or an email
address:
> "file://content/browser/push_messaging/OWNERS", regardless of my any change.
In
> the same reference, I must have ignored these warnings. 
> Will keep in mind about pre-submit for sure :)

Right, that's my canary for a depot_tools change that landed a month or so ago.
Please be sure to run "gclient sync" from time to time, which should fix it.

Please reach out to me by e-mail if you continue having problems, it's important
that we get them fixed! :) If they're happening to you, they may be happening to
others as well.

Thanks,
Peter

[davidben, 2015-05-20 20:40:54]

lgtm

[Pranay, 2015-05-21 02:46:23]

The CQ bit was checked by pranay.kumar@samsung.com to run a CQ dry run

[Pranay, 2015-05-21 02:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1134733006/#ps100001
(title: "Fixed Review Comments 3")

[commit-bot: I haz the power, 2015-05-21 02:48:34]

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

[commit-bot: I haz the power, 2015-05-21 02:58:53]

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

[commit-bot: I haz the power, 2015-05-21 02:58:54]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Pranay, 2015-05-21 13:50:06]

The CQ bit was checked by pranay.kumar@samsung.com

[Pranay, 2015-05-21 13:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
dcheng@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1134733006/#ps120001
(title: "Fixed pre-submit warning")

[commit-bot: I haz the power, 2015-05-21 13:50:15]

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

[Pranay, 2015-05-21 13:54:21]

Hi Peter,
Sure, though have been following "gclient sync". Thanks :) Will mail you if I
continue to face this problem.

-Pranay

[Pranay, 2015-05-21 13:54:51]

Fixed one presubmit nit, checking CQ bit

[commit-bot: I haz the power, 2015-05-21 17:18:09]

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

[commit-bot: I haz the power, 2015-05-21 17:18:11]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Pranay, 2015-05-22 03:56:11]

The CQ bit was checked by pranay.kumar@samsung.com

[commit-bot: I haz the power, 2015-05-22 03:56:38]

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

[commit-bot: I haz the power, 2015-05-22 04:00:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-22 04:01:23]

Patchset 7 (id:??) landed as
https://crrev.com/078aae8c35295ce939b8eb6dba2e2990fe422826
Cr-Commit-Position: refs/heads/master@{#331062}