Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922543002:
[NTP::Push] Add Content Suggestions GCM App Handler (Closed)
|
Created:
3 years, 6 months ago by mamir Modified:
3 years, 6 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, noyau+watch_chromium.org, droger+watchlist_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[NTP::Push] Add Content Suggestions GCM App Handler
BUG=728743
Review-Url: https://codereview.chromium.org/2922543002
Cr-Commit-Position: refs/heads/master@{#478340}
Committed: https://chromium.googlesource.com/chromium/src/+/84b98495ac596af91308b9771afb98f18f73514d
Patch Set 1 #
Total comments: 22
Patch Set 2 : sfiera@ comments. #
Total comments: 45
Patch Set 3 : Addressing reviewers comments. #Patch Set 4 : Storing the subscription token in pref #Patch Set 5 : Updates to the prefs. #
Total comments: 10
Patch Set 6 : sfiera@ comments. #Patch Set 7 : Implement unsubscribe. #
Total comments: 10
Patch Set 8 : peter@ comments. #
Total comments: 12
Patch Set 9 : jkrcal@ comments. #Patch Set 10 : Add missing dependency. #Messages
Total messages: 34 (12 generated)
mamir@chromium.org changed reviewers: + sfiera@chromium.org
https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:18: // Our sender ID we send up with all of our GCM messages. This doesn't tell me a lot more than the variable name. Maybe there are docs you could link to? https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:22: // Scope passed to getToken to obtain GCM registration tokens. // OAuth2 scope… https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:32: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { I'm not sure how this is possible. We are in the constructor—if anything points to `this` before the constructor runs, it was a dangling pointer. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:49: void ContentSuggestionsGCMAppHandler::Shutdown() { Can this object be destroyed without being Shutdown() first? If so, then we should probably remove the app handler in the destructor too. If not, then let's DCHECK() that the app handler was removed in the destructor. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:60: // TODO(mamir): Implement Show notification and update the feed. Add a DVLOG(1) for now? https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:24: private: Public declarations first https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:25: gcm::GCMDriver* gcm_driver_; I'm all in favor of declaring "gcm::GCMDriver* const gcm_driver_" and same for subscription_manager_. If you declare a member variable const and you don't initialize it in the constructor, the compiler will warn you. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:34: void DoSubscribe(instance_id::InstanceIDDriver* instance_id_driver); Please add some comments to these methods. Are DoSubscribe() and Shutdown() intended to be used by callers? Is it OK to call them multiple times? What's valid after Shutdown()? Can you re-subscribe? Can DoSubscribe() fail? What happens then? https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:35: void DidSubscribe(const std::string& subscription_id, Private? https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:40: // GCMAppHandler overrides. Typically we place these sorts of overrides in the private section, because they're not part of the public interface. Many people have mixed feelings about this, and you don't have to if you find it weird, but it's something we typically do :)
Thank for the informative review. Design has been changed as per offline discussion. PTAL https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:18: // Our sender ID we send up with all of our GCM messages. On 2017/06/02 08:43:05, sfiera wrote: > This doesn't tell me a lot more than the variable name. Maybe there are docs you > could link to? Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:22: // Scope passed to getToken to obtain GCM registration tokens. On 2017/06/02 08:43:04, sfiera wrote: > // OAuth2 scope… Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:32: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { On 2017/06/02 08:43:05, sfiera wrote: > I'm not sure how this is possible. We are in the constructor—if anything points > to `this` before the constructor runs, it was a dangling pointer. Acknowledged. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:49: void ContentSuggestionsGCMAppHandler::Shutdown() { On 2017/06/02 08:43:05, sfiera wrote: > Can this object be destroyed without being Shutdown() first? > If so, then we should probably remove the app handler in the destructor too. > If not, then let's DCHECK() that the app handler was removed in the destructor. Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:60: // TODO(mamir): Implement Show notification and update the feed. On 2017/06/02 08:43:04, sfiera wrote: > Add a DVLOG(1) for now? Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/06/02 08:43:05, sfiera wrote: > 2017 Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:24: private: On 2017/06/02 08:43:05, sfiera wrote: > Public declarations first Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:25: gcm::GCMDriver* gcm_driver_; On 2017/06/02 08:43:05, sfiera wrote: > I'm all in favor of declaring "gcm::GCMDriver* const gcm_driver_" and same for > subscription_manager_. > > If you declare a member variable const and you don't initialize it in the > constructor, the compiler will warn you. Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:34: void DoSubscribe(instance_id::InstanceIDDriver* instance_id_driver); On 2017/06/02 08:43:05, sfiera wrote: > Please add some comments to these methods. > > Are DoSubscribe() and Shutdown() intended to be used by callers? > Is it OK to call them multiple times? > What's valid after Shutdown()? Can you re-subscribe? > Can DoSubscribe() fail? What happens then? PTAL https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:35: void DidSubscribe(const std::string& subscription_id, On 2017/06/02 08:43:05, sfiera wrote: > Private? Done. https://codereview.chromium.org/2922543002/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:40: // GCMAppHandler overrides. On 2017/06/02 08:43:05, sfiera wrote: > Typically we place these sorts of overrides in the private section, because > they're not part of the public interface. Many people have mixed feelings about > this, and you don't have to if you find it weird, but it's something we > typically do :) Done.
mamir@chromium.org changed reviewers: + peter@chromium.org
Hi Peter, We need you GCM superpowers to check how reasonable this CL is :-) Thank you, Mohamed
Looks OK. We'll see what Peter thinks. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:35: Subscribe(); The subscription isn't necessary until StartListening(), is it? I would defer until then. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:45: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { Redundant. By the DCHECK(), we already know that there is no handler installed. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:104: // TODO (mamir): Implement Show notification and update the feed. Nit: I think TODO() should have no space (like a function) https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:22: // Class responsible for GCM functionality. It retrieves a subscription token Here and below, I think it's normal style to put a blank line before comments, to ensure they group visually with thing after and not the thing before. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:31: ~ContentSuggestionsGCMAppHandler(); Comment: if still listening, calls StopListening()
Exciting! Thanks for sharing :) https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:12: #include "content/public/common/push_messaging_status.h" This is specific to the Push API, I wouldn't add a dependency on //content/public/ just to use the enumeration of another feature, most of which isn't applicable to you. Consider just using InstanceID::Result unless you really need your own layer of status codes -- the former provides the best insight in case of GCM Driver-related issues. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:21: const char* kContentSuggestionsGCMSenderId = "128223710667"; style nit: define string constants as: const char kFoo[] = "bar"; https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:35: Subscribe(); On 2017/06/02 13:41:24, sfiera wrote: > The subscription isn't necessary until StartListening(), is it? I would defer > until then. +1. May I also ask you to consider adding the following code in StartListening(), *before* adding the app handler and/or creating the subscription: #if !defined(OS_ANDROID) NOTREACHED() << "The ContentSuggestionsGCMAppHandler should only be used on Android."; #endif It's a bit yuk, but having something that paints the bots red if we enable this on non-Android platforms by accident is important. Under the same banner, it'd be great if the connection could be guarded behind a base::Feature so that there's an explicit kill switch. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:39: StopListening(); Something to consider: it's safe to just always call RemoveAppHandler. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:45: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { On 2017/06/02 13:41:24, sfiera wrote: > Redundant. By the DCHECK(), we already know that there is no handler installed. AddAppHandler() itself will DCHECK, so you can remove the DCHECK here too. The comment in the class' header file is great! https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:77: DVLOG(1) << "Implement subscription token caching."; nit: we usually don't add D(V)LOG()s for TODOs. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:99: void ContentSuggestionsGCMAppHandler::OnStoreReset() {} You need to clear your token caching in preferences when this happens. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:109: // Messages don't get deleted. fwiw, better to be strict and NOTREACHED() in that case. Same with the two following methods. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:6: #define COMPONENTS_NTP_SNIPPETS_REMOTE_CONTENT_SUGGESTIONS_GCM_APP_HANDLER_H_ micro nit: update https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:23: // from the GCM server, registers/deregisters itself with the GCM service to be micro nit: deregisters -> unregisters (or "subscribes/unsubscribes" if you'd like to use the latest terminology, but the rest of Chrome hasn't updated to that yet.) https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:30: std::unique_ptr<BreakingNewsSubscriptionManager> subscriptionManager); nit: subscription_manager https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:31: ~ContentSuggestionsGCMAppHandler(); nit: this should be marked as `override` since the parent class' destructor is virtual https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:41: gcm::GCMDriver* const gcm_driver_; style nit: methods before member variables. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:42: instance_id::InstanceIDDriver* const instance_id_driver_; In order to guarantee the lifetime of these two pointers, it is important that whatever class will own this (which presumably can be traced back to a KeyedService too?) declares a dependency on the GCMProfileServiceFactory and the InstanceIDProfileServiceFactory. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:62: const std::string& message_id) override; It doesn't per se hurt, but given that the parent class defines these as public they'll just be accessible, so you may as well move them to the `public` section. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:63: }; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:12: #include "content/public/common/push_messaging_status.h" On 2017/06/02 14:02:02, Peter Beverloo wrote: > This is specific to the Push API, I wouldn't add a dependency on > //content/public/ just to use the enumeration of another feature, most of which > isn't applicable to you. Consider just using InstanceID::Result unless you > really need your own layer of status codes -- the former provides the best > insight in case of GCM Driver-related issues. Ah, sorry. I guess you can guess how this code came here by mistake ;-) https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:21: const char* kContentSuggestionsGCMSenderId = "128223710667"; On 2017/06/02 14:02:02, Peter Beverloo wrote: > style nit: define string constants as: > > const char kFoo[] = "bar"; Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:35: Subscribe(); On 2017/06/02 14:02:02, Peter Beverloo wrote: > On 2017/06/02 13:41:24, sfiera wrote: > > The subscription isn't necessary until StartListening(), is it? I would defer > > until then. > > +1. May I also ask you to consider adding the following code in > StartListening(), *before* adding the app handler and/or creating the > subscription: > > #if !defined(OS_ANDROID) > NOTREACHED() > << "The ContentSuggestionsGCMAppHandler should only be used on Android."; > #endif > > It's a bit yuk, but having something that paints the bots red if we enable this > on non-Android platforms by accident is important. Under the same banner, it'd > be great if the connection could be guarded behind a base::Feature so that > there's an explicit kill switch. The whole content provider instantiation will be guarded behind a base::Feature. Is this enough? or you mean something else by guarding the connection? https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:39: StopListening(); On 2017/06/02 14:02:02, Peter Beverloo wrote: > Something to consider: it's safe to just always call RemoveAppHandler. That because AddAppHandler DCHECKs? So we are sure no other handler has been registered to handle content suggestions pushes? https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:45: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { On 2017/06/02 14:02:02, Peter Beverloo wrote: > On 2017/06/02 13:41:24, sfiera wrote: > > Redundant. By the DCHECK(), we already know that there is no handler > installed. > > AddAppHandler() itself will DCHECK, so you can remove the DCHECK here too. The > comment in the class' header file is great! I got confused. So we remove the whole if check as well. Just add the handler directly? https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:77: DVLOG(1) << "Implement subscription token caching."; On 2017/06/02 14:02:02, Peter Beverloo wrote: > nit: we usually don't add D(V)LOG()s for TODOs. Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:99: void ContentSuggestionsGCMAppHandler::OnStoreReset() {} On 2017/06/02 14:02:02, Peter Beverloo wrote: > You need to clear your token caching in preferences when this happens. Added a TODO for now. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:104: // TODO (mamir): Implement Show notification and update the feed. On 2017/06/02 13:41:24, sfiera wrote: > Nit: I think TODO() should have no space (like a function) Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:109: // Messages don't get deleted. On 2017/06/02 14:02:02, Peter Beverloo wrote: > fwiw, better to be strict and NOTREACHED() in that case. Same with the two > following methods. Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:6: #define COMPONENTS_NTP_SNIPPETS_REMOTE_CONTENT_SUGGESTIONS_GCM_APP_HANDLER_H_ On 2017/06/02 14:02:03, Peter Beverloo wrote: > micro nit: update Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:22: // Class responsible for GCM functionality. It retrieves a subscription token On 2017/06/02 13:41:24, sfiera wrote: > Here and below, I think it's normal style to put a blank line before comments, > to ensure they group visually with thing after and not the thing before. Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:23: // from the GCM server, registers/deregisters itself with the GCM service to be On 2017/06/02 14:02:02, Peter Beverloo wrote: > micro nit: deregisters -> unregisters (or "subscribes/unsubscribes" if you'd > like to use the latest terminology, but the rest of Chrome hasn't updated to > that yet.) I will stay consistent with the rest of Chrome. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:30: std::unique_ptr<BreakingNewsSubscriptionManager> subscriptionManager); On 2017/06/02 14:02:03, Peter Beverloo wrote: > nit: subscription_manager Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:31: ~ContentSuggestionsGCMAppHandler(); On 2017/06/02 14:02:02, Peter Beverloo wrote: > nit: this should be marked as `override` since the parent class' destructor is > virtual Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:31: ~ContentSuggestionsGCMAppHandler(); On 2017/06/02 13:41:24, sfiera wrote: > Comment: if still listening, calls StopListening() Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:41: gcm::GCMDriver* const gcm_driver_; On 2017/06/02 14:02:02, Peter Beverloo wrote: > style nit: methods before member variables. Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:42: instance_id::InstanceIDDriver* const instance_id_driver_; On 2017/06/02 14:02:03, Peter Beverloo wrote: > In order to guarantee the lifetime of these two pointers, it is important that > whatever class will own this (which presumably can be traced back to a > KeyedService too?) declares a dependency on the GCMProfileServiceFactory and the > InstanceIDProfileServiceFactory. I added it to the constructor. This is enough. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:62: const std::string& message_id) override; On 2017/06/02 14:02:02, Peter Beverloo wrote: > It doesn't per se hurt, but given that the parent class defines these as public > they'll just be accessible, so you may as well move them to the `public` > section. Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:63: }; On 2017/06/02 14:02:02, Peter Beverloo wrote: > DISALLOW_COPY_AND_ASSIGN Done.
https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:42: instance_id::InstanceIDDriver* const instance_id_driver_; Actually, GCMProfileServiceFactory (and InstanceIDProfileServiceFactory) is in chrome/browser and IIUC components shouldn't depends on browser.
LGTM, except for one thing and a few comment nits https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:150: "breaking_news/content_suggestions_gcm_app_handler_unittest.cc", Not in the CL yet? https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:38: #if !defined(OS_ANDROID) "!"!? https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:55: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { The docs say that StartListening() must not be called if it's already listening, so I would drop this condition and let AddAppHandler() DCHECK. https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:27: // Class responsible for GCM functionality. It retrieves a subscription token "Class responsible for GCM functionality" seems a bit broad. "Handler for pushed GCM content suggestions"? https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:29: // called upon received push content suggestions. This sentence isn't complete; should the comma be "and"?
Patchset #7 (id:110001) has been deleted
Implemented unsubscribe. PTAL https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... File components/ntp_snippets/BUILD.gn (right): https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/BUILD.gn:150: "breaking_news/content_suggestions_gcm_app_handler_unittest.cc", On 2017/06/07 11:34:38, sfiera wrote: > Not in the CL yet? Yes, not sure how to test. Removing for now. https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:38: #if !defined(OS_ANDROID) On 2017/06/07 11:34:38, sfiera wrote: > "!"!? Done. https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:55: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { On 2017/06/07 11:34:38, sfiera wrote: > The docs say that StartListening() must not be called if it's already listening, > so I would drop this condition and let AddAppHandler() DCHECK. Done. https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:27: // Class responsible for GCM functionality. It retrieves a subscription token On 2017/06/07 11:34:38, sfiera wrote: > "Class responsible for GCM functionality" seems a bit broad. "Handler for pushed > GCM content suggestions"? Done. https://codereview.chromium.org/2922543002/diff/80001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:29: // called upon received push content suggestions. On 2017/06/07 11:34:38, sfiera wrote: > This sentence isn't complete; should the comma be "and"? Yes
gcm lgtm https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:39: StopListening(); On 2017/06/04 15:10:13, mamir wrote: > On 2017/06/02 14:02:02, Peter Beverloo wrote: > > Something to consider: it's safe to just always call RemoveAppHandler. > > That because AddAppHandler DCHECKs? > So we are sure no other handler has been registered to handle content > suggestions pushes? No - calling RemoveAppHandler() with an unregistered app id will just silently fail, which is fine. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:45: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { On 2017/06/04 15:10:13, mamir wrote: > On 2017/06/02 14:02:02, Peter Beverloo wrote: > > On 2017/06/02 13:41:24, sfiera wrote: > > > Redundant. By the DCHECK(), we already know that there is no handler > > installed. > > > > AddAppHandler() itself will DCHECK, so you can remove the DCHECK here too. The > > comment in the class' header file is great! > > I got confused. So we remove the whole if check as well. Just add the handler > directly? Yes. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/DEPS (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/DEPS:21: "+content/public/common", delete? https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:42: // #endif This has to be in a KeyedServiceFactory. ContentSuggestionsServiceFactory presumably? https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_manager_unittest.cc (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_manager_unittest.cc:67: GURL("http://valid-url.test"), nit: usually we store repeated strings in a constant in the anonymous namespace. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/pref_names.h:99: // over network every time the client subscribe to push updates. fwiw, calling GetToken() repeatedly should usually not hit the network. the preference is just a much faster way for your own bookkeeping to see if a subscription exists. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/pref_names.h:100: extern const char kContentSuggestionsGCMSubscriptionTokenCache[]; nit: blank line after this.
Please, sfiera, PTAL https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:39: StopListening(); On 2017/06/08 07:02:39, Peter Beverloo (OOO - Jun 12) wrote: > On 2017/06/04 15:10:13, mamir wrote: > > On 2017/06/02 14:02:02, Peter Beverloo wrote: > > > Something to consider: it's safe to just always call RemoveAppHandler. > > > > That because AddAppHandler DCHECKs? > > So we are sure no other handler has been registered to handle content > > suggestions pushes? > > No - calling RemoveAppHandler() with an unregistered app id will just silently > fail, which is fine. Done. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:45: if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) { On 2017/06/08 07:02:39, Peter Beverloo (OOO - Jun 12) wrote: > On 2017/06/04 15:10:13, mamir wrote: > > On 2017/06/02 14:02:02, Peter Beverloo wrote: > > > On 2017/06/02 13:41:24, sfiera wrote: > > > > Redundant. By the DCHECK(), we already know that there is no handler > > > installed. > > > > > > AddAppHandler() itself will DCHECK, so you can remove the DCHECK here too. > The > > > comment in the class' header file is great! > > > > I got confused. So we remove the whole if check as well. Just add the handler > > directly? > > Yes. Done. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/DEPS (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/DEPS:21: "+content/public/common", On 2017/06/08 07:02:40, Peter Beverloo (OOO - Jun 12) wrote: > delete? Done. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:42: // #endif On 2017/06/08 07:02:40, Peter Beverloo (OOO - Jun 12) wrote: > This has to be in a KeyedServiceFactory. ContentSuggestionsServiceFactory > presumably? Done. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_manager_unittest.cc (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_manager_unittest.cc:67: GURL("http://valid-url.test"), On 2017/06/08 07:02:40, Peter Beverloo (OOO - Jun 12) wrote: > nit: usually we store repeated strings in a constant in the anonymous namespace. Done. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/pref_names.h:99: // over network every time the client subscribe to push updates. On 2017/06/08 07:02:40, Peter Beverloo (OOO - Jun 12) wrote: > fwiw, calling GetToken() repeatedly should usually not hit the network. the > preference is just a much faster way for your own bookkeeping to see if a > subscription exists. Done. https://codereview.chromium.org/2922543002/diff/130001/components/ntp_snippet... components/ntp_snippets/pref_names.h:100: extern const char kContentSuggestionsGCMSubscriptionTokenCache[]; On 2017/06/08 07:02:40, Peter Beverloo (OOO - Jun 12) wrote: > nit: blank line after this. Done.
mamir@chromium.org changed reviewers: + jkrcal@chromium.org
jkrcal@chromium.org: Please review changes in
https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:48: Subscribe(); For the sake of symmetry, maybe add something like the following? DCHECK_EQ(gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID), nullptr); https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:64: return; I am puzzled by the assymetry in the code. When you subscribe for the first time, you get a token and store it in prefs. If you then unsubscribe, subscription_manager is called but the pref is not cleared. Thus, when you re-subscribe later, subscription_manager does not get called because the token in prefs is not empty. https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_manager.cc (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_manager.cc:60: DCHECK(!unsubscription_request_); I find this (and above) a bit subtle to use: It is not allowed to run concurrent Subscribe / Unsubscribe requests but the caller will never know when a request is finished. Why DCHECK and not simply return (e.g. false)? Is it okay to call Subscribe() before Unsubscribe() finished? https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_manager.h (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_manager.h:16: nit: Maybe add a comment that explains what is this class good for (in relation to other classes here)? https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/pref_names.h:97: // The pref name for the subscription token received from the gcm server. As The comment does not clarify the difference to kContentSuggestionsSubscriptionDataToken to me. Can you please expand the comments a bit? https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/pref_names.h:99: // see if subscription exists.. nit: Remove one of the trailing dots.
https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:48: Subscribe(); On 2017/06/09 09:10:10, jkrcal wrote: > For the sake of symmetry, maybe add something like the following? > DCHECK_EQ(gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID), nullptr); This has been discussed before in this CL tl;dr; AddAppHanlder already DCHECKs. https://codereview.chromium.org/2922543002/diff/20001/components/ntp_snippets... https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:64: return; On 2017/06/09 09:10:09, jkrcal wrote: > I am puzzled by the assymetry in the code. > > When you subscribe for the first time, you get a token and store it in prefs. If > you then unsubscribe, subscription_manager is called but the pref is not > cleared. Thus, when you re-subscribe later, subscription_manager does not get > called because the token in prefs is not empty. Thank you for spotting this. You are absolutely right. This piece of code didn't live here before. When I moved it, I forgot the adjust the rest of the flow accordingly. https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_manager.cc (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_manager.cc:60: DCHECK(!unsubscription_request_); On 2017/06/09 09:10:10, jkrcal wrote: > I find this (and above) a bit subtle to use: > It is not allowed to run concurrent Subscribe / Unsubscribe requests but the > caller will never know when a request is finished. > > Why DCHECK and not simply return (e.g. false)? > Is it okay to call Subscribe() before Unsubscribe() finished? As discussed off-review, I have added 3 methods CanSubscribeNow(), CanUnsubscribeNow(), IsSubscribed() PTAL https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/breaking_news/subscription_manager.h (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/breaking_news/subscription_manager.h:16: On 2017/06/09 09:10:10, jkrcal wrote: > nit: Maybe add a comment that explains what is this class good for (in relation > to other classes here)? PTAL https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... File components/ntp_snippets/pref_names.h (right): https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/pref_names.h:97: // The pref name for the subscription token received from the gcm server. As On 2017/06/09 09:10:10, jkrcal wrote: > The comment does not clarify the difference to > kContentSuggestionsSubscriptionDataToken to me. Can you please expand the > comments a bit? PTAL. https://codereview.chromium.org/2922543002/diff/150001/components/ntp_snippet... components/ntp_snippets/pref_names.h:99: // see if subscription exists.. On 2017/06/09 09:10:10, jkrcal wrote: > nit: Remove one of the trailing dots. Done.
Thanks, lgtm!
mamir@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in chrome/browser/prefs/browser_prefs.cc
lgtm
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2922543002/#ps170001 (title: "jkrcal@ comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, bauerb@chromium.org, jkrcal@chromium.org, sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2922543002/#ps190001 (title: "Add missing dependency.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 190001, "attempt_start_ts": 1497026026870970,
"parent_rev": "257951527deb304ec71750102753c4166c265e1f", "commit_rev":
"84b98495ac596af91308b9771afb98f18f73514d"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Push] Add Content Suggestions GCM App Handler BUG=728743 ========== to ========== [NTP::Push] Add Content Suggestions GCM App Handler BUG=728743 Review-Url: https://codereview.chromium.org/2922543002 Cr-Commit-Position: refs/heads/master@{#478340} Committed: https://chromium.googlesource.com/chromium/src/+/84b98495ac596af91308b9771afb... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:190001) as https://chromium.googlesource.com/chromium/src/+/84b98495ac596af91308b9771afb... |
