Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925053003:
[NTP::Push] Adding BreakingNewsSuggestionsProvider (Closed)
Description[NTP::Push] Adding BreakingNewsSuggestionsProvider
BUG=730537
Review-Url: https://codereview.chromium.org/2925053003
Cr-Commit-Position: refs/heads/master@{#478534}
Committed: https://chromium.googlesource.com/chromium/src/+/25a32cd52f2c74f46f0bebe05aa41e6347da7c26
Patch Set 1 #
Total comments: 16
Patch Set 2 : sfiera@ comments. #
Total comments: 18
Patch Set 3 : sfiera@ comments. #
Total comments: 25
Patch Set 4 : jkrcal@ comments. #Patch Set 5 : Fixing the build. #Messages
Total messages: 39 (17 generated)
mamir@chromium.org changed reviewers: + sfiera@chromium.org
High-level question: where will breaking news articles be persisted? We don't want them to disappear when Chrome exits, because that happens often on Android. We have the RemoteSuggestionsDatabase for this purpose, but it's tied to remote suggestions. https://codereview.chromium.org/2925053003/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2925053003/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:308: service, std::move(handler)); So far you've punted on the question of how suggestions will be sent from the handler to the provider. I think now is a good time to settle that question. To me, the easiest option looks like passing a callback to handler->StartListening(). This would mean removing the call to handler->StartListening() from this function, and adding provider->Start() which would start listening with an appropriate callback. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:1: Nit: https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:6: #ifndef COMPONENTS_NTP_SNIPPETS_REMOTE_BREAKING_NEWS_SUGGESTIONS_PROVIDER_H_ Should have another BREAKING_NEWS_, I think? https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:29: static void RegisterProfilePrefs(PrefRegistrySimple* registry); Not implemented https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:32: std::unique_ptr<ContentSuggestionsGCMAppHandler> gcm_app_handler_; Methods before member variables https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_manager.cc (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_manager.cc:68: // TODO(mamir) do we need this? I like having a param :) When I'm working, I use alpha as the backend, instead of prod/staging, so I can develop against the latest features on the server. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:55: BREAKING_NEWS, We can't use 10002 for this—that's already been used by other remote categories. I think that 10009 is the next available ID, but maybe check with Markus and the proto conf. Or we can use a local ID, maybe. That works if the server will never serve them via pull.
Thank you a lot for your constructive feedback. PTAL https://codereview.chromium.org/2925053003/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2925053003/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:308: service, std::move(handler)); On 2017/06/07 13:58:14, sfiera wrote: > So far you've punted on the question of how suggestions will be sent from the > handler to the provider. I think now is a good time to settle that question. > > To me, the easiest option looks like passing a callback to > handler->StartListening(). This would mean removing the call to > handler->StartListening() from this function, and adding provider->Start() which > would start listening with an appropriate callback. Thank you very much for the suggestion which I liked and implemented. Please, let me know your comments. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:1: On 2017/06/07 13:58:15, sfiera wrote: > Nit: Done. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:6: #ifndef COMPONENTS_NTP_SNIPPETS_REMOTE_BREAKING_NEWS_SUGGESTIONS_PROVIDER_H_ On 2017/06/07 13:58:15, sfiera wrote: > Should have another BREAKING_NEWS_, I think? Done. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:29: static void RegisterProfilePrefs(PrefRegistrySimple* registry); On 2017/06/07 13:58:14, sfiera wrote: > Not implemented Done. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:32: std::unique_ptr<ContentSuggestionsGCMAppHandler> gcm_app_handler_; On 2017/06/07 13:58:14, sfiera wrote: > Methods before member variables Done. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... File components/ntp_snippets/breaking_news/subscription_manager.cc (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/bre... components/ntp_snippets/breaking_news/subscription_manager.cc:68: // TODO(mamir) do we need this? On 2017/06/07 13:58:15, sfiera wrote: > I like having a param :) When I'm working, I use alpha as the backend, instead > of prod/staging, so I can develop against the latest features on the server. Done. https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:55: BREAKING_NEWS, On 2017/06/07 13:58:15, sfiera wrote: > We can't use 10002 for this—that's already been used by other remote categories. > I think that 10009 is the next available ID, but maybe check with Markus and the > proto conf. > > Or we can use a local ID, maybe. That works if the server will never serve them > via pull. omm, but pushed content suggestions by definition will not be pulled. So what do you think?
Description was changed from ========== [NTP::Push] Adding BreakingNewsSuggestionsProvider BUG=730537 ========== to ========== [NTP::Push] Adding BreakingNewsSuggestionsProvider BUG=730537 ==========
https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:55: BREAKING_NEWS, On 2017/06/08 09:58:28, mamir wrote: > On 2017/06/07 13:58:15, sfiera wrote: > > We can't use 10002 for this—that's already been used by other remote > categories. > > I think that 10009 is the next available ID, but maybe check with Markus and > the > > proto conf. > > > > Or we can use a local ID, maybe. That works if the server will never serve > them > > via pull. > > omm, but pushed content suggestions by definition will not be pulled. > So what do you think? I don't think it's necessarily true that we will only ever want to push them. But, making push+pull work together is a lot more work than just picking a remote ID. I think a local ID is fine, but I'd check with Markus at our meeting today. https://codereview.chromium.org/2925053003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2925053003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:301: CHECK(instance_id_profile_service->driver()); These should probably be DCHECKs https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:21: Category::FromKnownCategory(KnownCategories::BREAKING_NEWS)) {} Please initialize category_status_ here. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:28: // TDOD(mamir): Is unretained the correct method to use? Yes. I would add a comment like: // Unretained because |this| owns |gcm_app_handler_|. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:70: const base::Callback<bool(const GURL& url)>& filter) {} Should have a TODO(mamir) to clear the provider (I think that's supposed to happen on cleared history). https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:75: // Ignored. Likewise? https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:33: void OnNewContentSuggestion(const std::string& content); Could be private? Could take a base::Value (JSON, basically)? https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:58: }; Oh, should probably have a DISALLOW_COPY_AND_ASSIGN here. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:67: } Clear on_new_content_callback_? https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/category.h:55: BREAKING_NEWS, Please add BREAKING_NEWS to SuggestionsCategoryId on the server, and set this to REMOTE_CATEGORIES_OFFSET + n.
https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/1/components/ntp_snippets/cat... components/ntp_snippets/category.h:55: BREAKING_NEWS, On 2017/06/08 13:37:15, sfiera wrote: > On 2017/06/08 09:58:28, mamir wrote: > > On 2017/06/07 13:58:15, sfiera wrote: > > > We can't use 10002 for this—that's already been used by other remote > > categories. > > > I think that 10009 is the next available ID, but maybe check with Markus and > > the > > > proto conf. > > > > > > Or we can use a local ID, maybe. That works if the server will never serve > > them > > > via pull. > > > > omm, but pushed content suggestions by definition will not be pulled. > > So what do you think? > > I don't think it's necessarily true that we will only ever want to push them. > But, making push+pull work together is a lot more work than just picking a > remote ID. I think a local ID is fine, but I'd check with Markus at our meeting > today. Oh, hah, I thought I had already published this comment. IGNORE ME
https://codereview.chromium.org/2925053003/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2925053003/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:301: CHECK(instance_id_profile_service->driver()); On 2017/06/08 13:37:15, sfiera wrote: > These should probably be DCHECKs Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:21: Category::FromKnownCategory(KnownCategories::BREAKING_NEWS)) {} On 2017/06/08 13:37:15, sfiera wrote: > Please initialize category_status_ here. Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:28: // TDOD(mamir): Is unretained the correct method to use? On 2017/06/08 13:37:15, sfiera wrote: > Yes. I would add a comment like: > // Unretained because |this| owns |gcm_app_handler_|. Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:70: const base::Callback<bool(const GURL& url)>& filter) {} On 2017/06/08 13:37:15, sfiera wrote: > Should have a TODO(mamir) to clear the provider (I think that's supposed to > happen on cleared history). Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.cc:75: // Ignored. On 2017/06/08 13:37:15, sfiera wrote: > Likewise? Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:33: void OnNewContentSuggestion(const std::string& content); On 2017/06/08 13:37:15, sfiera wrote: > Could be private? > > Could take a base::Value (JSON, basically)? Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/breaking_news_suggestions_provider.h:58: }; On 2017/06/08 13:37:15, sfiera wrote: > Oh, should probably have a DISALLOW_COPY_AND_ASSIGN here. Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:67: } On 2017/06/08 13:37:15, sfiera wrote: > Clear on_new_content_callback_? Done. https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/20001/components/ntp_snippets... components/ntp_snippets/category.h:55: BREAKING_NEWS, On 2017/06/08 13:37:16, sfiera wrote: > Please add BREAKING_NEWS to SuggestionsCategoryId on the server, and set this to > REMOTE_CATEGORIES_OFFSET + n. Done.
LGTM
mamir@chromium.org changed reviewers: + jkrcal@chromium.org
jkrcal@chromium.org: Please review changes in
Mostly nits, looks good. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:112: on_new_content_callback_.Run(base::Value()); Are you sure this will never get called after StopListening deletes on_new_content_callback? Maybe DCHECK(on_new_content_callback)? https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:82: // Called after everytime a new message is received in OnMessage() to notify nit: Called everytime after ...? (has been lgtm'd by Chris so I am not sure I should trust my intuition :) https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_manager.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:12: #include "components/variations/variations_associated_data.h" please use the new api, instead: base/metrics/field_trial_params.h https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:21: const char kPushSubscriptionBackend[] = "push_subscription_backend"; consistency nit: Can you name it kPushSubscriptionBackendParam? (throughout ntp_snippets field trial param names end with Param) https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:24: const char kPushUnsubscriptionBackend[] = "push_unsubscription_backend"; ditto https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:106: std::string endpoint = variations::GetVariationParamValueByFeature( Please use the new API with the same parameters: base::GetFieldTrialParamValueByFeature() https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:127: std::string endpoint = variations::GetVariationParamValueByFeature( ditto https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_manager.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.h:19: // consideration of the channel and variation parameters. nitty nit: the current official name for variation parameters is "field trial parameters". https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/category.h:59: LAST_KNOWN_REMOTE_CATEGORY = ARTICLES, Please update this to BREAKING_NEWS. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:71: // sync with ContentSuggestionsCategory in histograms.xml. Sync with ContentSuggestionsCategory in histograms.xml. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_constants.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_constants.cc:29: "suggestions/unsubscribe"; subscribe? https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_constants.cc:33: "subscribe"; unsubscribe?
https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:82: // Called after everytime a new message is received in OnMessage() to notify On 2017/06/09 11:32:59, jkrcal wrote: > nit: Called everytime after ...? > (has been lgtm'd by Chris so I am not sure I should trust my intuition :) Well, I'd separate it as "every time" but I would still put it after "after".
https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc:112: on_new_content_callback_.Run(base::Value()); On 2017/06/09 11:32:59, jkrcal wrote: > Are you sure this will never get called after StopListening deletes > on_new_content_callback? > > Maybe DCHECK(on_new_content_callback)? Yes, this won't be called after StopListening because the handler will be removed upon calling StopListening and before deleting on_new_content_callback. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h:82: // Called after everytime a new message is received in OnMessage() to notify On 2017/06/09 11:42:57, sfiera wrote: > On 2017/06/09 11:32:59, jkrcal wrote: > > nit: Called everytime after ...? > > (has been lgtm'd by Chris so I am not sure I should trust my intuition :) > > Well, I'd separate it as "every time" but I would still put it after "after". Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_manager.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:12: #include "components/variations/variations_associated_data.h" On 2017/06/09 11:32:59, jkrcal wrote: > please use the new api, instead: > base/metrics/field_trial_params.h Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:21: const char kPushSubscriptionBackend[] = "push_subscription_backend"; On 2017/06/09 11:32:59, jkrcal wrote: > consistency nit: Can you name it kPushSubscriptionBackendParam? > (throughout ntp_snippets field trial param names end with Param) Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:24: const char kPushUnsubscriptionBackend[] = "push_unsubscription_backend"; On 2017/06/09 11:32:59, jkrcal wrote: > ditto Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:106: std::string endpoint = variations::GetVariationParamValueByFeature( On 2017/06/09 11:32:59, jkrcal wrote: > Please use the new API with the same parameters: > base::GetFieldTrialParamValueByFeature() Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.cc:127: std::string endpoint = variations::GetVariationParamValueByFeature( On 2017/06/09 11:32:59, jkrcal wrote: > ditto Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/breaking_news/subscription_manager.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/breaking_news/subscription_manager.h:19: // consideration of the channel and variation parameters. On 2017/06/09 11:32:59, jkrcal wrote: > nitty nit: the current official name for variation parameters is "field trial > parameters". Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/category.h (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/category.h:59: LAST_KNOWN_REMOTE_CATEGORY = ARTICLES, On 2017/06/09 11:32:59, jkrcal wrote: > Please update this to BREAKING_NEWS. Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:71: // sync with ContentSuggestionsCategory in histograms.xml. On 2017/06/09 11:32:59, jkrcal wrote: > Sync with ContentSuggestionsCategory in histograms.xml. Done. https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_constants.cc (right): https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_constants.cc:29: "suggestions/unsubscribe"; On 2017/06/09 11:32:59, jkrcal wrote: > subscribe? Thank you very much for spotting this!!! https://codereview.chromium.org/2925053003/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_constants.cc:33: "subscribe"; On 2017/06/09 11:32:59, jkrcal wrote: > unsubscribe? Done.
mamir@chromium.org changed reviewers: + holte@chromium.org
Hi holte@, Please review the changes in tools/metrics/histograms/histograms.xml
lgtm, thanks!
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 Link to the patchset: https://codereview.chromium.org/2925053003/#ps60001 (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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, sfiera@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2925053003/#ps80001 (title: "Fixing the build.")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by mamir@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, sfiera@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2925053003/#ps100001 (title: "Fixing the build.")
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by mamir@chromium.org
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": 100001, "attempt_start_ts": 1497201235944790,
"parent_rev": "ce66668670874d2ce4c440d48b71e4523c95d790", "commit_rev":
"25a32cd52f2c74f46f0bebe05aa41e6347da7c26"}
Message was sent while issue was closed.
Description was changed from ========== [NTP::Push] Adding BreakingNewsSuggestionsProvider BUG=730537 ========== to ========== [NTP::Push] Adding BreakingNewsSuggestionsProvider BUG=730537 Review-Url: https://codereview.chromium.org/2925053003 Cr-Commit-Position: refs/heads/master@{#478534} Committed: https://chromium.googlesource.com/chromium/src/+/25a32cd52f2c74f46f0bebe05aa4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/25a32cd52f2c74f46f0bebe05aa4... |
