|
|
Created:
5 years, 7 months ago by Pranay Modified:
5 years, 7 months ago Reviewers:
Peter Beverloo, dcheng, davidben, Michael van Ouwerkerk CC:
chromium-reviews, darin-cc_chromium.org, jam, johnme+watch_chromium.org, mvanouwerkerk+watch_chromium.org, peter+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPush 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}
Patch Set 1 #Patch Set 2 : Updated Patch Set #
Total comments: 1
Patch Set 3 : Updated patch set 2 #
Total comments: 56
Patch Set 4 : Fixed Review Comments #
Total comments: 2
Patch Set 5 : Fixed Review Comments 2 #
Total comments: 18
Patch Set 6 : Fixed Review Comments 3 #Patch Set 7 : Fixed pre-submit warning #
Created: 5 years, 7 months ago
Messages
Total messages: 40 (10 generated)
pranay.kumar@samsung.com changed reviewers: + davidben@chromium.org, mvanouwerkerk@chromium.org
mvanouwerkerk@chromium.org: Please review changes in chrome/browser/push_messaging davidben@chromium.org: Please review changes in content
peter@chromium.org changed reviewers: + peter@chromium.org
+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?
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
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?
Sure Peter, Thanks for the info about naming convention. I'll update this patch soon
pranay.kumar@samsung.com changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes in content/common
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.
PTAL, have tried to cover up maximum changes related to a single file in latest patch set Thanks :)
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
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.
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(?
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.
Hi David, Please review changes in content folder Thanks -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)); 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)
(Sorry, I forgot to add: IPC changes LGTM)
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
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.
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
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 :)
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
lgtm
The CQ bit was checked by pranay.kumar@samsung.com to run a CQ dry run
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")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134733006/100001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by pranay.kumar@samsung.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134733006/120001
Hi Peter, Sure, though have been following "gclient sync". Thanks :) Will mail you if I continue to face this problem. -Pranay
Fixed one presubmit nit, checking CQ bit
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by pranay.kumar@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1134733006/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/078aae8c35295ce939b8eb6dba2e2990fe422826 Cr-Commit-Position: refs/heads/master@{#331062} |