Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2606753002:
Post notification after fetching articles (Closed)
DescriptionPost notification after fetching articles
Notifies unconditionally for the moment; in the future we'll want to
gate it on fields provided by the server. Behavior is enabled by the
ContentSuggestionsNotifications feature.
BUG=675561
Review-Url: https://codereview.chromium.org/2606753002
Cr-Commit-Position: refs/heads/master@{#441648}
Committed: https://chromium.googlesource.com/chromium/src/+/b0f63396085a5db6d6366668f84e8c166ee0247d
Patch Set 1 #Patch Set 2 : nullptr on desktop #Patch Set 3 : unused on desktop #Patch Set 4 : override #
Total comments: 10
Patch Set 5 : Address comments #
Total comments: 1
Patch Set 6 : Move feature out to own file. #Patch Set 7 : Service is not created with context #Patch Set 8 : Rename tags #Patch Set 9 : Add TODO. #
Total comments: 5
Dependent Patchsets: Messages
Total messages: 54 (36 generated)
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: 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 sfiera@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_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 sfiera@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_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 sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sfiera@chromium.org changed reviewers: + bauerb@chromium.org, erg@chromium.org
erg: for profile_manager.cc bauerb: for all else Still plenty of features missing, but the most important feature is in, which is that nothing changes without enabling via Finch :) I'm not expecting review to start until next Tuesday, but wanted to cut off a reviewable chunk before proceeding. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
dgn@chromium.org changed reviewers: + dgn@chromium.org
Do we need to have a new class for this? Isn't it possible to reuse NativeNotificationDisplayService method for showing Zine notifications? https://codereview.chromium.org/2606753002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:29: private static final String TAG = "ContentSuggestionsNotification"; nit: name it differently (NOTIFICATION_TAG maybe?) to avoid confusion with the tag used for logs. The latter has a limit in the number of characters that this string exceeds.
On 2016/12/28 11:00:26, dgn wrote: > Do we need to have a new class for this? Isn't it possible to reuse > NativeNotificationDisplayService method for showing Zine notifications? Maybe in the future. Right now it's pretty specific to web notifications. Initially, I was hoping to go that route, but I don't think it would be practical to hit M57 FF anymore.
https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/content_suggestions_notifier_service.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/content_suggestions_notifier_service.cc:145: registry->RegisterStringPref(kNotificationIDWithinCategory, ""); Nit: use an empty std::string() constructor to save some instructions. https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_notifier_service_factory.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_notifier_service_factory.cc:64: auto suggestions = ContentSuggestionsServiceFactory::GetForProfile(profile); Nit: The type of |suggestions| isn't totally obvious, so I'd rather spell it out. https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:596: ContentSuggestionsNotifierService::RegisterProfilePrefs(registry); Move this before DownloadSuggestionsProvider, so everything is sorted alphabetically? https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1208: ContentSuggestionsNotifierServiceFactory::GetForProfile(profile); If you want your service to be created together with the profile, override ServiceIsCreatedWithBrowserContext() in the factory to return true. Can you please also add a TODO to do the same thing for ContentSuggestionsServiceFactory?
https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/content_suggestions_notifier_service.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/content_suggestions_notifier_service.cc:145: registry->RegisterStringPref(kNotificationIDWithinCategory, ""); On 2017/01/03 17:38:59, Bernhard (just came back) wrote: > Nit: use an empty std::string() constructor to save some instructions. Done. https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_notifier_service_factory.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_notifier_service_factory.cc:64: auto suggestions = ContentSuggestionsServiceFactory::GetForProfile(profile); On 2017/01/03 17:38:59, Bernhard (just came back) wrote: > Nit: The type of |suggestions| isn't totally obvious, so I'd rather spell it > out. Done. https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:596: ContentSuggestionsNotifierService::RegisterProfilePrefs(registry); On 2017/01/03 17:38:59, Bernhard (just came back) wrote: > Move this before DownloadSuggestionsProvider, so everything is sorted > alphabetically? Done. I wonder how this ordering happened, it really makes no sense. https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1208: ContentSuggestionsNotifierServiceFactory::GetForProfile(profile); On 2017/01/03 17:38:59, Bernhard (just came back) wrote: > If you want your service to be created together with the profile, override > ServiceIsCreatedWithBrowserContext() in the factory to return true. Done. > Can you please also add a TODO to do the same thing for > ContentSuggestionsServiceFactory? I'll just fix it.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2606753002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2606753002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:1208: ContentSuggestionsNotifierServiceFactory::GetForProfile(profile); Now that you added ServiceIsCreatedWithBrowserContext(), are either of these necessary anymore?
On 2017/01/03 23:21:58, Elliot Glaysher wrote: > https://codereview.chromium.org/2606753002/diff/100001/chrome/browser/profile... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/2606753002/diff/100001/chrome/browser/profile... > chrome/browser/profiles/profile_manager.cc:1208: > ContentSuggestionsNotifierServiceFactory::GetForProfile(profile); > Now that you added ServiceIsCreatedWithBrowserContext(), are either of these > necessary anymore? Is that sufficient? I'd have thought there would need to be some form of registration, or else there wouldn't yet be a factory to call the method on.
On 2017/01/03 23:36:34, sfiera wrote: > On 2017/01/03 23:21:58, Elliot Glaysher wrote: > > Now that you added ServiceIsCreatedWithBrowserContext(), are either of these > > necessary anymore? > > Is that sufficient? I'd have thought there would need to be some form of > registration, or else there wouldn't yet be a factory to call the method on. I see what you're trying to do now. Registration for things that don't need explicit per-profile initialization during FinalInit() should go in chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc. This is executed before the profile is created in the first place so that ServiceIsCreatedWithContext() can be respected.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/03 23:53:41, Elliot Glaysher wrote: > On 2017/01/03 23:36:34, sfiera wrote: > > On 2017/01/03 23:21:58, Elliot Glaysher wrote: > > > Now that you added ServiceIsCreatedWithBrowserContext(), are either of these > > > necessary anymore? > > > > Is that sufficient? I'd have thought there would need to be some form of > > registration, or else there wouldn't yet be a factory to call the method on. > > I see what you're trying to do now. > > Registration for things that don't need explicit per-profile initialization > during FinalInit() should go in > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc. This is > executed before the profile is created in the first place so that > ServiceIsCreatedWithContext() can be respected. I tried this, but get a DCHECK-fail in GetSSLConfigService(). We don't seem to be directly using that, but I guess we might initiate a fetch when the service is created, and we fetch over HTTPS. This seems somewhat related to https://crbug.com/676662; I can add a note there.
On 2017/01/04 16:33:36, sfiera wrote: > On 2017/01/03 23:53:41, Elliot Glaysher wrote: > > On 2017/01/03 23:36:34, sfiera wrote: > > > On 2017/01/03 23:21:58, Elliot Glaysher wrote: > > > > Now that you added ServiceIsCreatedWithBrowserContext(), are either of > these > > > > necessary anymore? > > > > > > Is that sufficient? I'd have thought there would need to be some form of > > > registration, or else there wouldn't yet be a factory to call the method on. > > > > I see what you're trying to do now. > > > > Registration for things that don't need explicit per-profile initialization > > during FinalInit() should go in > > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc. This is > > executed before the profile is created in the first place so that > > ServiceIsCreatedWithContext() can be respected. > > I tried this, but get a DCHECK-fail in GetSSLConfigService(). We don't seem to > be directly using that, but I guess we might initiate a fetch when the service > is created, and we fetch over HTTPS. Do you have a stack trace? If all services correctly declare their dependencies, that shouldn't happen.
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2017/01/04 16:55:08, Bernhard Bauer wrote: > On 2017/01/04 16:33:36, sfiera wrote: > > On 2017/01/03 23:53:41, Elliot Glaysher wrote: > > > On 2017/01/03 23:36:34, sfiera wrote: > > > > On 2017/01/03 23:21:58, Elliot Glaysher wrote: > > > > > Now that you added ServiceIsCreatedWithBrowserContext(), are either of > > these > > > > > necessary anymore? > > > > > > > > Is that sufficient? I'd have thought there would need to be some form of > > > > registration, or else there wouldn't yet be a factory to call the method > on. > > > > > > I see what you're trying to do now. > > > > > > Registration for things that don't need explicit per-profile initialization > > > during FinalInit() should go in > > > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc. This is > > > executed before the profile is created in the first place so that > > > ServiceIsCreatedWithContext() can be respected. > > > > I tried this, but get a DCHECK-fail in GetSSLConfigService(). We don't seem to > > be directly using that, but I guess we might initiate a fetch when the service > > is created, and we fetch over HTTPS. > > Do you have a stack trace? If all services correctly declare their dependencies, > that shouldn't happen. https://paste.googleplex.com/4819902108008448 (this is actually the same problem as observed in https://codereview.chromium.org/2614573002, not in this CL) The heavily-abbreviated version is this: ProfileImpl::GetSSLConfigService() OffTheRecordProfileImpl::GetRequestContext() ProfileSyncServiceFactory::BuildServiceInstanceFor(content::BrowserContext*) ProfileSyncServiceFactory::GetForProfile(Profile*) ContentSuggestionsServiceFactory::BuildServiceInstanceFor(content::BrowserContext*) The CHECK-fail has an explanatory comment: // If ssl_config_service_manager_ is null, this typically means that some // KeyedService is trying to create a RequestContext at startup, // but SSLConfigServiceManager is not initialized until DoFinalInit() which is // invoked after all KeyedServices have been initialized (see // http://crbug.com/171406). https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?... Yes, that's what's happening, so it seems that if we need a ProfileSyncService, then we cannot initialize before DoFinalInit().
On 2017/01/04 19:12:09, sfiera wrote: > On 2017/01/04 16:55:08, Bernhard Bauer wrote: > > On 2017/01/04 16:33:36, sfiera wrote: > > > On 2017/01/03 23:53:41, Elliot Glaysher wrote: > > > > On 2017/01/03 23:36:34, sfiera wrote: > > > > > On 2017/01/03 23:21:58, Elliot Glaysher wrote: > > > > > > Now that you added ServiceIsCreatedWithBrowserContext(), are either of > > > these > > > > > > necessary anymore? > > > > > > > > > > Is that sufficient? I'd have thought there would need to be some form of > > > > > registration, or else there wouldn't yet be a factory to call the method > > on. > > > > > > > > I see what you're trying to do now. > > > > > > > > Registration for things that don't need explicit per-profile > initialization > > > > during FinalInit() should go in > > > > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc. This > is > > > > executed before the profile is created in the first place so that > > > > ServiceIsCreatedWithContext() can be respected. > > > > > > I tried this, but get a DCHECK-fail in GetSSLConfigService(). We don't seem > to > > > be directly using that, but I guess we might initiate a fetch when the > service > > > is created, and we fetch over HTTPS. > > > > Do you have a stack trace? If all services correctly declare their > dependencies, > > that shouldn't happen. > > https://paste.googleplex.com/4819902108008448 > > (this is actually the same problem as observed in > https://codereview.chromium.org/2614573002, not in this CL) > > The heavily-abbreviated version is this: > ProfileImpl::GetSSLConfigService() > OffTheRecordProfileImpl::GetRequestContext() > ProfileSyncServiceFactory::BuildServiceInstanceFor(content::BrowserContext*) > ProfileSyncServiceFactory::GetForProfile(Profile*) > ContentSuggestionsServiceFactory::BuildServiceInstanceFor(content::BrowserContext*) > > The CHECK-fail has an explanatory comment: > // If ssl_config_service_manager_ is null, this typically means that some > // KeyedService is trying to create a RequestContext at startup, > // but SSLConfigServiceManager is not initialized until DoFinalInit() which is > // invoked after all KeyedServices have been initialized (see > // http://crbug.com/171406). > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?... > > Yes, that's what's happening, so it seems that if we need a ProfileSyncService, > then we cannot initialize before DoFinalInit(). ProfileSyncService is relying on things existing during construction, which it shouldn't do. It can't safely be constructed at profile creation time, when it should handle that case gracefully, and since you depend on it transitively, you can't be created with the context either. But I don't believe any of this is your fault. It looks like there's a few other services that are declared "incorrectly" in ProfileManager::DoFinalInitForServices, too. lgtm without adding ServiceIsCreatedWithContext() to the services, but not happy about it.
On 2017/01/05 01:01:13, Elliot Glaysher wrote: > On 2017/01/04 19:12:09, sfiera wrote: > > On 2017/01/04 16:55:08, Bernhard Bauer wrote: > > > On 2017/01/04 16:33:36, sfiera wrote: > > > > On 2017/01/03 23:53:41, Elliot Glaysher wrote: > > > > > On 2017/01/03 23:36:34, sfiera wrote: > > > > > > On 2017/01/03 23:21:58, Elliot Glaysher wrote: > > > > > > > Now that you added ServiceIsCreatedWithBrowserContext(), are either > of > > > > these > > > > > > > necessary anymore? > > > > > > > > > > > > Is that sufficient? I'd have thought there would need to be some form > of > > > > > > registration, or else there wouldn't yet be a factory to call the > method > > > on. > > > > > > > > > > I see what you're trying to do now. > > > > > > > > > > Registration for things that don't need explicit per-profile > > initialization > > > > > during FinalInit() should go in > > > > > chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc. > This > > is > > > > > executed before the profile is created in the first place so that > > > > > ServiceIsCreatedWithContext() can be respected. > > > > > > > > I tried this, but get a DCHECK-fail in GetSSLConfigService(). We don't > seem > > to > > > > be directly using that, but I guess we might initiate a fetch when the > > service > > > > is created, and we fetch over HTTPS. > > > > > > Do you have a stack trace? If all services correctly declare their > > dependencies, > > > that shouldn't happen. > > > > https://paste.googleplex.com/4819902108008448 > > > > (this is actually the same problem as observed in > > https://codereview.chromium.org/2614573002, not in this CL) > > > > The heavily-abbreviated version is this: > > ProfileImpl::GetSSLConfigService() > > OffTheRecordProfileImpl::GetRequestContext() > > ProfileSyncServiceFactory::BuildServiceInstanceFor(content::BrowserContext*) > > ProfileSyncServiceFactory::GetForProfile(Profile*) > > > ContentSuggestionsServiceFactory::BuildServiceInstanceFor(content::BrowserContext*) > > > > The CHECK-fail has an explanatory comment: > > // If ssl_config_service_manager_ is null, this typically means that some > > // KeyedService is trying to create a RequestContext at startup, > > // but SSLConfigServiceManager is not initialized until DoFinalInit() which > is > > // invoked after all KeyedServices have been initialized (see > > // http://crbug.com/171406). > > > https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?... > > > > Yes, that's what's happening, so it seems that if we need a > ProfileSyncService, > > then we cannot initialize before DoFinalInit(). > > ProfileSyncService is relying on things existing during construction, which it > shouldn't do. It can't safely be constructed at profile creation time, when it > should handle that case gracefully, and since you depend on it transitively, you > can't be created with the context either. But I don't believe any of this is > your fault. It looks like there's a few other services that are declared > "incorrectly" in ProfileManager::DoFinalInitForServices, too. > > lgtm without adding ServiceIsCreatedWithContext() to the services, but not happy > about it. Ugh, yes. LGTM, but can you add a TODO with an associated bug to DoFinalInitForServices() to clean this up?
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, erg@chromium.org Link to the patchset: https://codereview.chromium.org/2606753002/#ps180001 (title: "Add TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fhorschig@chromium.org changed reviewers: + fhorschig@chromium.org
Just some minor drive-by questions ... https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_features.cc (right): https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_features.cc:8: "ContentSuggestionsNotifications", base::FEATURE_DISABLED_BY_DEFAULT}; Wouldn't it make sense to put that into the ntp_snippets namespace? (Helper functions would then feel home here ... analog to /components/ntp_snippets/features.cc) https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_features.h (right): https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_features.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Do we need that new file because browser features don't belong in src/components/ntp_snippets/features.{h,cc}? Couldn't we drop the ntp_snippets_ prefix then for consistency?
https://codereview.chromium.org/2606753002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2606753002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:29: private static final String TAG = "ContentSuggestionsNotification"; On 2016/12/28 11:00:26, dgn wrote: > nit: name it differently (NOTIFICATION_TAG maybe?) to avoid confusion with the > tag used for logs. The latter has a limit in the number of characters that this > string exceeds. Done. https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_features.cc (right): https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_features.cc:8: "ContentSuggestionsNotifications", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/01/05 13:24:35, fhorschig wrote: > Wouldn't it make sense to put that into the ntp_snippets namespace? We don't seem to use that namespace in chrome/browser/ntp_snippets. I would be happy if it was common practice, though. > (Helper functions would then feel home here ... analog to > /components/ntp_snippets/features.cc) https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_features.h (right): https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_features.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/05 13:24:35, fhorschig wrote: > Do we need that new file because browser features don't belong in > src/components/ntp_snippets/features.{h,cc}? I don't think that notifications features belong there, since they wouldn't (can't) be used within the component itself. > Couldn't we drop the ntp_snippets_ prefix then for consistency? There's already a features.cc in the //chrome/browser target, and targets can't have multiple files with the same basename.
https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_features.cc (right): https://codereview.chromium.org/2606753002/diff/180001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_features.cc:8: "ContentSuggestionsNotifications", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/01/05 13:35:01, sfiera wrote: > On 2017/01/05 13:24:35, fhorschig wrote: > > Wouldn't it make sense to put that into the ntp_snippets namespace? > > We don't seem to use that namespace in chrome/browser/ntp_snippets. I would be > happy if it was common practice, though. The rough (i.e. not always followed) rule of thumb is that things in chrome/ go into the top-level namespace and things in components go into a namespace named after the component.
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483622546782520,
"parent_rev": "98d3363db8506c383289ed1bbcd00c4169614bbe", "commit_rev":
"b0f63396085a5db6d6366668f84e8c166ee0247d"}
Message was sent while issue was closed.
Description was changed from ========== Post notification after fetching articles Notifies unconditionally for the moment; in the future we'll want to gate it on fields provided by the server. Behavior is enabled by the ContentSuggestionsNotifications feature. BUG=675561 ========== to ========== Post notification after fetching articles Notifies unconditionally for the moment; in the future we'll want to gate it on fields provided by the server. Behavior is enabled by the ContentSuggestionsNotifications feature. BUG=675561 Review-Url: https://codereview.chromium.org/2606753002 Cr-Commit-Position: refs/heads/master@{#441648} Committed: https://chromium.googlesource.com/chromium/src/+/b0f63396085a5db6d6366668f84e... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b0f63396085a5db6d6366668f84e... |
