Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1997473004:
Snippets are enabled when search suggestions are enabled. (Closed)
|
Created:
4 years, 7 months ago by Michael van Ouwerkerk Modified:
4 years, 6 months ago CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, rginda+watch_chromium.org, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSnippets are enabled when search suggestions are enabled.
* Users can now opt out of New Tab Page snippets with this preference:
Settings > Privacy > Search and URL suggestions
* Observing changes and updating previously instantiated NTPs is for a later CL.
BUG=606792
TBR=skuhne
Committed: https://crrev.com/97313225831b95666b6f7a6bdb0f6cd62858e411
Cr-Commit-Position: refs/heads/master@{#395866}
Patch Set 1 #Patch Set 2 : Fix unit test. #Patch Set 3 : Fix ios. #
Total comments: 4
Patch Set 4 : Rebase. Add a TODO. #
Total comments: 16
Patch Set 5 : Add docs. #Patch Set 6 : Rebase. #Patch Set 7 : Delete Init method. #Patch Set 8 : Set shutdown_ #
Total comments: 9
Patch Set 9 : #
Total comments: 6
Patch Set 10 : #
Total comments: 1
Patch Set 11 : Rebase. #Messages
Total messages: 80 (29 generated)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/1
mvanouwerkerk@chromium.org changed reviewers: + bauerb@chromium.org
Bernhard, could you take a look please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/40001
Description was changed from ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes to this preference is for a later CL. BUG=606792 ========== to ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ a nit and a suggestion for future work: https://codereview.chromium.org/1997473004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NtpColorUtils.java (right): https://codereview.chromium.org/1997473004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NtpColorUtils.java:18: public final class NtpColorUtils { Final? I guess it's not wrong, but we don't usually bother with making things explicitly final (and it's not like someone could create a subclass anyway, because the constructor is private). https://codereview.chromium.org/1997473004/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1997473004/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:74: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); Hm... both of these things use APIs that are available in components/, just the keys (prefs::kSearchSuggestEnabled / chrome::android::kNTPSnippetsFeature) are from chrome/. We could think about moving this into the component as well and just passing the keys in. That might be particularly interesting once we start observing pref changes, as then the NTPSnippetsService could observe the pref directly and we wouldn't have to have an additional object for that.
Thanks Bernhard! https://codereview.chromium.org/1997473004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NtpColorUtils.java (right): https://codereview.chromium.org/1997473004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NtpColorUtils.java:18: public final class NtpColorUtils { On 2016/05/20 10:43:24, Bernhard Bauer wrote: > Final? I guess it's not wrong, but we don't usually bother with making things > explicitly final (and it's not like someone could create a subclass anyway, > because the constructor is private). I think it's the accepted pattern for a utility class: https://en.wikipedia.org/wiki/Utility_class http://stackoverflow.com/questions/32375149/is-it-mandatory-utility-class-sho... https://codereview.chromium.org/1997473004/diff/40001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1997473004/diff/40001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:74: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); On 2016/05/20 10:43:24, Bernhard Bauer wrote: > Hm... both of these things use APIs that are available in components/, just the > keys (prefs::kSearchSuggestEnabled / chrome::android::kNTPSnippetsFeature) are > from chrome/. We could think about moving this into the component as well and > just passing the keys in. That might be particularly interesting once we start > observing pref changes, as then the NTPSnippetsService could observe the pref > directly and we wouldn't have to have an additional object for that. Agreed. I've added a TODO along those lines.
mvanouwerkerk@chromium.org changed reviewers: + skuhne@chromium.org
skuhne: please review deletions from profile_manager.cc as OWNER
mvanouwerkerk@chromium.org changed reviewers: + noyau@chromium.org
Eric: please take a look at the ios changes as OWNER.
https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); 7 arguments now. This starts to smell dirty. |enabled| is a little unclear. There is no documentation for it, and I'm not sure why it needs to be passed at creation time, can't is be deduced from the other arguments? I'm not sure, as someone writing code to create a snippet service, how I can make that decision. If I really want a disabled service I would not create one in the first place no?
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/60001
mvanouwerkerk@chromium.org changed reviewers: + treib@chromium.org
Thanks Eric. Please take another look. Marc, could you comment on the comment in ProfileManager? https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1232: // Note: Create the service even if the feature is disabled, so that any Marc, I don't understand this comment. Could you maybe suggest a rephrasing that explains why we're instantiating the service even if it might be disabled? https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 11:45:05, noyau wrote: > 7 arguments now. This starts to smell dirty. > > |enabled| is a little unclear. There is no documentation for it, and I'm not > sure why it needs to be passed at creation time, can't is be deduced from the > other arguments? I'm not sure, as someone writing code to create a snippet > service, how I can make that decision. If I really want a disabled service I > would not create one in the first place no? I've added some documentation to the |enabled_| field, but I'm just doing archaeology here, if anyone knows more please chime in! The previous behavior had the |enabled| argument for the Init method, while the constructor defaulted it to |false|. I think moving it to the constructor is not worse, and moves logic from ProfileManager to the factory, which seems more appropriate. On iOS the service always had enabled = false. Also, the service is never instantiated on iOS, afaict.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1232: // Note: Create the service even if the feature is disabled, so that any On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > Marc, I don't understand this comment. Could you maybe suggest a rephrasing that > explains why we're instantiating the service even if it might be disabled? When the service is instantiated and enabled, it schedules a persistent and recurring task for background fetching, at the Android OS level. If the service gets disabled later, and we wouldn't instantiate it anymore, then that task would remain active and would continue to wake up the device. Does that clear things up? https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > On 2016/05/20 11:45:05, noyau wrote: > > 7 arguments now. This starts to smell dirty. > > > > |enabled| is a little unclear. There is no documentation for it, and I'm not > > sure why it needs to be passed at creation time, can't is be deduced from the > > other arguments? I'm not sure, as someone writing code to create a snippet > > service, how I can make that decision. If I really want a disabled service I > > would not create one in the first place no? > > I've added some documentation to the |enabled_| field, but I'm just doing > archaeology here, if anyone knows more please chime in! Er, did you? Where? > The previous behavior had the |enabled| argument for the Init method, while the > constructor defaulted it to |false|. I think moving it to the constructor is not > worse, and moves logic from ProfileManager to the factory, which seems more > appropriate. I don't recall any particular reason for it being passed to Init rather than the ctor, other than that the ctor didn't need it. That said, it should be possible to just figure it out within the ctor itself, so the logic doesn't need to be duplicated in the factories. If so, that would IMO be nicer. > On iOS the service always had enabled = false. Also, the service is never > instantiated on iOS, afaict. Yup, the service isn't used on iOS yet.
Thanks Marc! Please take another look. https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 13:49:36, Marc Treib wrote: > On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > > On 2016/05/20 11:45:05, noyau wrote: > > > 7 arguments now. This starts to smell dirty. > > > > > > |enabled| is a little unclear. There is no documentation for it, and I'm not > > > sure why it needs to be passed at creation time, can't is be deduced from > the > > > other arguments? I'm not sure, as someone writing code to create a snippet > > > service, how I can make that decision. If I really want a disabled service I > > > would not create one in the first place no? > > > > I've added some documentation to the |enabled_| field, but I'm just doing > > archaeology here, if anyone knows more please chime in! > > Er, did you? Where? > > > The previous behavior had the |enabled| argument for the Init method, while > the > > constructor defaulted it to |false|. I think moving it to the constructor is > not > > worse, and moves logic from ProfileManager to the factory, which seems more > > appropriate. > > I don't recall any particular reason for it being passed to Init rather than the > ctor, other than that the ctor didn't need it. > > That said, it should be possible to just figure it out within the ctor itself, > so the logic doesn't need to be duplicated in the factories. If so, that would > IMO be nicer. > > > On iOS the service always had enabled = false. Also, the service is never > > instantiated on iOS, afaict. > > Yup, the service isn't used on iOS yet. Ah sorry, I didn't upload the docs before. Done now. If I do a followup CL for observing pref changes, I'll see if enabled state be can determined in the constructor. For the moment, it seems the factory is a better place than ProfileManager.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/80001
https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1234: NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); Here the service is created and Init is called right away. Why do we need Init in the first place? Can't the service "init" itself? This Init pattern is not something I see used in KeyedServices, and it seems more error prone than needed. https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > On 2016/05/20 11:45:05, noyau wrote: > > 7 arguments now. This starts to smell dirty. > > > > |enabled| is a little unclear. There is no documentation for it, and I'm not > > sure why it needs to be passed at creation time, can't is be deduced from the > > other arguments? I'm not sure, as someone writing code to create a snippet > > service, how I can make that decision. If I really want a disabled service I > > would not create one in the first place no? > > I've added some documentation to the |enabled_| field, but I'm just doing > archaeology here, if anyone knows more please chime in! > Don't see it. Did you forget to upload? > The previous behavior had the |enabled| argument for the Init method, while the > constructor defaulted it to |false|. I think moving it to the constructor is not > worse, and moves logic from ProfileManager to the factory, which seems more > appropriate. > > On iOS the service always had enabled = false. Also, the service is never > instantiated on iOS, afaict. Yes, that's my concern. Why is the service started in disabled mode by default? That's why I'd like to understand this boolean. It should probably default to true on iOS, or be tied to a flag. As for not being used on iOS yet, you are right. But we are planning to use it at some point, and I'd rather have working then :)
On 2016/05/20 15:16:53, noyau wrote: > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:1234: > NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); > Here the service is created and Init is called right away. Why do we need Init > in the first place? Can't the service "init" itself? > > This Init pattern is not something I see used in KeyedServices, and it seems > more error prone than needed. > > https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... > File components/ntp_snippets/ntp_snippets_service.h (right): > > https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... > components/ntp_snippets/ntp_snippets_service.h:67: > std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); > On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > > On 2016/05/20 11:45:05, noyau wrote: > > > 7 arguments now. This starts to smell dirty. > > > > > > |enabled| is a little unclear. There is no documentation for it, and I'm not > > > sure why it needs to be passed at creation time, can't is be deduced from > the > > > other arguments? I'm not sure, as someone writing code to create a snippet > > > service, how I can make that decision. If I really want a disabled service I > > > would not create one in the first place no? > > > > I've added some documentation to the |enabled_| field, but I'm just doing > > archaeology here, if anyone knows more please chime in! > > > Don't see it. Did you forget to upload? > > > > The previous behavior had the |enabled| argument for the Init method, while > the > > constructor defaulted it to |false|. I think moving it to the constructor is > not > > worse, and moves logic from ProfileManager to the factory, which seems more > > appropriate. > > > > On iOS the service always had enabled = false. Also, the service is never > > instantiated on iOS, afaict. > > Yes, that's my concern. Why is the service started in disabled mode by default? > That's why I'd like to understand this boolean. It should probably default to > true on iOS, or be tied to a flag. > > As for not being used on iOS yet, you are right. But we are planning to use it > at some point, and I'd rather have working then :) Ooops, apparently I missed a patch. Yes, I see where you are coming from. My feeling is that the decision to enable/disable the service is either done externally to the service, with an accessor to do so, or internally, and the passed in pref_service should contain all the necessary information. This mix of inside/outside seems weird to me.
LGTM, provided you want to do the "listen to pref changes" part in a follow-up CL. https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 14:56:49, Michael van Ouwerkerk wrote: > On 2016/05/20 13:49:36, Marc Treib wrote: > > On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > > > On 2016/05/20 11:45:05, noyau wrote: > > > > 7 arguments now. This starts to smell dirty. > > > > > > > > |enabled| is a little unclear. There is no documentation for it, and I'm > not > > > > sure why it needs to be passed at creation time, can't is be deduced from > > the > > > > other arguments? I'm not sure, as someone writing code to create a snippet > > > > service, how I can make that decision. If I really want a disabled service > I > > > > would not create one in the first place no? > > > > > > I've added some documentation to the |enabled_| field, but I'm just doing > > > archaeology here, if anyone knows more please chime in! > > > > Er, did you? Where? > > > > > The previous behavior had the |enabled| argument for the Init method, while > > the > > > constructor defaulted it to |false|. I think moving it to the constructor is > > not > > > worse, and moves logic from ProfileManager to the factory, which seems more > > > appropriate. > > > > I don't recall any particular reason for it being passed to Init rather than > the > > ctor, other than that the ctor didn't need it. > > > > That said, it should be possible to just figure it out within the ctor itself, > > so the logic doesn't need to be duplicated in the factories. If so, that would > > IMO be nicer. > > > > > On iOS the service always had enabled = false. Also, the service is never > > > instantiated on iOS, afaict. > > > > Yup, the service isn't used on iOS yet. > > Ah sorry, I didn't upload the docs before. Done now. > > If I do a followup CL for observing pref changes, I'll see if enabled state be > can determined in the constructor. For the moment, it seems the factory is a > better place than ProfileManager. Aah, I missed the pref-dependent part of this; I was thinking only of the FeatureList part which should be easy to figure out in the service itself. So right now this will query the pref only once on startup, which is clearly suboptimal, but okay if you want to do the dynamic part in another CL. https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 15:16:53, noyau wrote: > On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > > On 2016/05/20 11:45:05, noyau wrote: > > > 7 arguments now. This starts to smell dirty. > > > > > > |enabled| is a little unclear. There is no documentation for it, and I'm not > > > sure why it needs to be passed at creation time, can't is be deduced from > the > > > other arguments? I'm not sure, as someone writing code to create a snippet > > > service, how I can make that decision. If I really want a disabled service I > > > would not create one in the first place no? > > > > I've added some documentation to the |enabled_| field, but I'm just doing > > archaeology here, if anyone knows more please chime in! > > > Don't see it. Did you forget to upload? > > > > The previous behavior had the |enabled| argument for the Init method, while > the > > constructor defaulted it to |false|. I think moving it to the constructor is > not > > worse, and moves logic from ProfileManager to the factory, which seems more > > appropriate. > > > > On iOS the service always had enabled = false. Also, the service is never > > instantiated on iOS, afaict. > > Yes, that's my concern. Why is the service started in disabled mode by default? > That's why I'd like to understand this boolean. It should probably default to > true on iOS, or be tied to a flag. We have a flag (in base::FeatureList) which should be used for that (plus the pref that is being added in this CL). > As for not being used on iOS yet, you are right. But we are planning to use it > at some point, and I'd rather have working then :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1997473004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:67: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher); On 2016/05/20 15:16:53, noyau wrote: > On 2016/05/20 12:38:36, Michael van Ouwerkerk wrote: > > On 2016/05/20 11:45:05, noyau wrote: > > > 7 arguments now. This starts to smell dirty. > > > > > > |enabled| is a little unclear. There is no documentation for it, and I'm not > > > sure why it needs to be passed at creation time, can't is be deduced from > the > > > other arguments? I'm not sure, as someone writing code to create a snippet > > > service, how I can make that decision. If I really want a disabled service I > > > would not create one in the first place no? > > > > I've added some documentation to the |enabled_| field, but I'm just doing > > archaeology here, if anyone knows more please chime in! > > > Don't see it. Did you forget to upload? > > > > The previous behavior had the |enabled| argument for the Init method, while > the > > constructor defaulted it to |false|. I think moving it to the constructor is > not > > worse, and moves logic from ProfileManager to the factory, which seems more > > appropriate. > > > > On iOS the service always had enabled = false. Also, the service is never > > instantiated on iOS, afaict. > > Yes, that's my concern. Why is the service started in disabled mode by default? > That's why I'd like to understand this boolean. It should probably default to > true on iOS, or be tied to a flag. > > As for not being used on iOS yet, you are right. But we are planning to use it > at some point, and I'd rather have working then :) Eric, there is another CL underway for changing how "enabled" works: https://codereview.chromium.org/2000233002/ I would prefer to land this simple one as is, get the other one by Nicolas landed as well, and then polish things some more after if still needed.
Ok. But I'd like for the Init method to be removed. It serves no purpose, as the two places where the service is created both call Init right away, so its content can be folded into the constructor. https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1234: NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); On 2016/05/20 15:16:53, noyau wrote: > Here the service is created and Init is called right away. Why do we need Init > in the first place? Can't the service "init" itself? > > This Init pattern is not something I see used in KeyedServices, and it seems > more error prone than needed. Ping!
https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1234: NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); On 2016/05/24 14:19:11, noyau wrote: > On 2016/05/20 15:16:53, noyau wrote: > > Here the service is created and Init is called right away. Why do we need Init > > in the first place? Can't the service "init" itself? > > > > This Init pattern is not something I see used in KeyedServices, and it seems > > more error prone than needed. > > Ping! Done. https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1234: NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); On 2016/05/20 15:16:53, noyau wrote: > Here the service is created and Init is called right away. Why do we need Init > in the first place? Can't the service "init" itself? > > This Init pattern is not something I see used in KeyedServices, and it seems > more error prone than needed. I don't know exactly why. It's possible some of the work done in Init requires a fully constructed instance, but it's not documented... so now it's gone.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/120001
https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1234: NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); On 2016/05/24 14:38:54, Michael van Ouwerkerk wrote: > On 2016/05/20 15:16:53, noyau wrote: > > Here the service is created and Init is called right away. Why do we need Init > > in the first place? Can't the service "init" itself? > > > > This Init pattern is not something I see used in KeyedServices, and it seems > > more error prone than needed. > > I don't know exactly why. It's possible some of the work done in Init requires a > fully constructed instance, but it's not documented... so now it's gone. The reason why this is here is because we need to start scheduling fetches as soon as the browser starts. We could achieve this though by overriding ServiceWillBeCreatedWithProfile in the factory and doing everything from Init() in the constructor.
https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_manager.cc:1234: NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); On 2016/05/24 14:54:28, Bernhard Bauer wrote: > On 2016/05/24 14:38:54, Michael van Ouwerkerk wrote: > > On 2016/05/20 15:16:53, noyau wrote: > > > Here the service is created and Init is called right away. Why do we need > Init > > > in the first place? Can't the service "init" itself? > > > > > > This Init pattern is not something I see used in KeyedServices, and it seems > > > more error prone than needed. > > > > I don't know exactly why. It's possible some of the work done in Init requires > a > > fully constructed instance, but it's not documented... so now it's gone. > > The reason why this is here is because we need to start scheduling fetches as > soon as the browser starts. We could achieve this though by overriding > ServiceWillBeCreatedWithProfile in the factory and doing everything from Init() > in the constructor. I just moved everything to the constructor, I expect that's fine?
On 2016/05/24 14:54:28, Bernhard Bauer wrote: > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > File chrome/browser/profiles/profile_manager.cc (right): > > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > chrome/browser/profiles/profile_manager.cc:1234: > NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); > On 2016/05/24 14:38:54, Michael van Ouwerkerk wrote: > > On 2016/05/20 15:16:53, noyau wrote: > > > Here the service is created and Init is called right away. Why do we need > Init > > > in the first place? Can't the service "init" itself? > > > > > > This Init pattern is not something I see used in KeyedServices, and it seems > > > more error prone than needed. > > > > I don't know exactly why. It's possible some of the work done in Init requires > a > > fully constructed instance, but it's not documented... so now it's gone. > > The reason why this is here is because we need to start scheduling fetches as > soon as the browser starts. We could achieve this though by overriding > ServiceWillBeCreatedWithProfile in the factory and doing everything from Init() > in the constructor. You should be careful scheduling things right when the browser starts. The people in Paris working on launching Chrome as fast as possible might not like adding yet another fetch right on start. Shouldn't this wait until the data is needed, aka a NTP comes up on screen and get focus? Anyway, this CL lgtm.
On 2016/05/24 15:00:51, noyau wrote: > On 2016/05/24 14:54:28, Bernhard Bauer wrote: > > > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > > File chrome/browser/profiles/profile_manager.cc (right): > > > > > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > > chrome/browser/profiles/profile_manager.cc:1234: > > NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); > > On 2016/05/24 14:38:54, Michael van Ouwerkerk wrote: > > > On 2016/05/20 15:16:53, noyau wrote: > > > > Here the service is created and Init is called right away. Why do we need > > Init > > > > in the first place? Can't the service "init" itself? > > > > > > > > This Init pattern is not something I see used in KeyedServices, and it > seems > > > > more error prone than needed. > > > > > > I don't know exactly why. It's possible some of the work done in Init > requires > > a > > > fully constructed instance, but it's not documented... so now it's gone. > > > > The reason why this is here is because we need to start scheduling fetches as > > soon as the browser starts. We could achieve this though by overriding > > ServiceWillBeCreatedWithProfile in the factory and doing everything from > Init() > > in the constructor. > > You should be careful scheduling things right when the browser starts. The > people in Paris working on launching Chrome as fast as possible might not like > adding yet another fetch right on start. Shouldn't this wait until the data is > needed, aka a NTP comes up on screen and get focus? > > Anyway, this CL lgtm. Yeah, I found it like this so I'm not 100% sure of the subtleties yet. It achieves the goal of prefetching the data so the snippets are ready when a NTP needs to be shown. I haven't looked deeply enough into the scheduler yet to see whether this actually does work at startup or it just schedules it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 ========== to ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 TBR=skuhne ==========
On 2016/05/24 15:00:51, noyau wrote: > On 2016/05/24 14:54:28, Bernhard Bauer wrote: > > > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > > File chrome/browser/profiles/profile_manager.cc (right): > > > > > https://codereview.chromium.org/1997473004/diff/60001/chrome/browser/profiles... > > chrome/browser/profiles/profile_manager.cc:1234: > > NTPSnippetsServiceFactory::GetForProfile(profile)->Init(); > > On 2016/05/24 14:38:54, Michael van Ouwerkerk wrote: > > > On 2016/05/20 15:16:53, noyau wrote: > > > > Here the service is created and Init is called right away. Why do we need > > Init > > > > in the first place? Can't the service "init" itself? > > > > > > > > This Init pattern is not something I see used in KeyedServices, and it > seems > > > > more error prone than needed. > > > > > > I don't know exactly why. It's possible some of the work done in Init > requires > > a > > > fully constructed instance, but it's not documented... so now it's gone. > > > > The reason why this is here is because we need to start scheduling fetches as > > soon as the browser starts. We could achieve this though by overriding > > ServiceWillBeCreatedWithProfile in the factory and doing everything from > Init() > > in the constructor. > > You should be careful scheduling things right when the browser starts. The > people in Paris working on launching Chrome as fast as possible might not like > adding yet another fetch right on start. Shouldn't this wait until the data is > needed, aka a NTP comes up on screen and get focus? This won't do the fetch right away, it will just _schedule_ it (or unschedule if it's disabled). We're not doing the fetch fully lazily to make this work with slow internet connections, so we'll fetch in the background (at reduced frequency if the device is on a mobile connection and/or battery). > Anyway, this CL lgtm.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/140001
dgn@chromium.org changed reviewers: + dgn@chromium.org
https://codereview.chromium.org/1997473004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java (right): https://codereview.chromium.org/1997473004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java:18: && PrefServiceBridge.getInstance().isSearchSuggestEnabled(); We want to use the material UI (toolbar disabled, etc.) when the NTP_SNIPPETS flag is set. Rather than disabling it here, we probably want to clear the snippets and not fetch new ones instead, similar to https://codereview.chromium.org/1980033002 https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:76: bool enabled = profile->GetPrefs()->GetBoolean(prefs::kSearchSuggestEnabled); Why not starting to listen to pref changes now? It seems like a better solution that only applying the changes to newly opened tabs. https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (left): https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:165: enum class State { treib@ is adding a LOADED state, and I'm adding a DISABLED one. I don't think this should be removed.
not lgtm anymore - I agree with Nicolas that we want to keep the State enum. https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:76: bool enabled = profile->GetPrefs()->GetBoolean(prefs::kSearchSuggestEnabled); On 2016/05/24 16:38:35, dgn wrote: > Why not starting to listen to pref changes now? It seems like a better solution > that only applying the changes to newly opened tabs. Actually, this current code only applies at Chrome startup. Listening to pref changes will absolutely need to happen, but I'm okay with doing it in a follow-up CL if that makes things easier. https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (left): https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:165: enum class State { On 2016/05/24 16:38:35, dgn wrote: > treib@ is adding a LOADED state, and I'm adding a DISABLED one. I don't think > this should be removed. +1 :)
On 2016/05/24 16:55:24, Marc Treib wrote: > not lgtm anymore - I agree with Nicolas that we want to keep the State enum. > > https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... > File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): > > https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... > chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:76: bool enabled = > profile->GetPrefs()->GetBoolean(prefs::kSearchSuggestEnabled); > On 2016/05/24 16:38:35, dgn wrote: > > Why not starting to listen to pref changes now? It seems like a better > solution > > that only applying the changes to newly opened tabs. > > Actually, this current code only applies at Chrome startup. > Listening to pref changes will absolutely need to happen, but I'm okay with > doing it in a follow-up CL if that makes things easier. > > https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... > File components/ntp_snippets/ntp_snippets_service.h (left): > > https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... > components/ntp_snippets/ntp_snippets_service.h:165: enum class State { > On 2016/05/24 16:38:35, dgn wrote: > > treib@ is adding a LOADED state, and I'm adding a DISABLED one. I don't think > > this should be removed. > > +1 :) Oh wow, that looks scary! All red and all :D It's really only a nit :)
All done. Please take another look. https://codereview.chromium.org/1997473004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java (right): https://codereview.chromium.org/1997473004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java:18: && PrefServiceBridge.getInstance().isSearchSuggestEnabled(); On 2016/05/24 16:38:35, dgn wrote: > We want to use the material UI (toolbar disabled, etc.) when the NTP_SNIPPETS > flag is set. Rather than disabling it here, we probably want to clear the > snippets and not fetch new ones instead, similar to > https://codereview.chromium.org/1980033002 I'm not sure what you meant, but the goal of this CL is as described in the bug: "When switched off, Zine is completely removed from the UI, i.e. the NTP does not have a peeking card and no below-the-fold section." https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1997473004/diff/140001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc:76: bool enabled = profile->GetPrefs()->GetBoolean(prefs::kSearchSuggestEnabled); On 2016/05/24 16:38:35, dgn wrote: > Why not starting to listen to pref changes now? It seems like a better solution > that only applying the changes to newly opened tabs. Because such a change is more involved, and I'd like to land these changes separately now as they solve the majority of the use case. The codebase is changing quickly, and I'd like to avoid additional merge conflicts, like from your current series of CLs :-) https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (left): https://codereview.chromium.org/1997473004/diff/140001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:165: enum class State { On 2016/05/24 16:38:35, dgn wrote: > treib@ is adding a LOADED state, and I'm adding a DISABLED one. I don't think > this should be removed. Done.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/160001
https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:216: state_ = State::INITED; Hm, so the NOT_INITED state doesn't really exist anymore. Remove it? (Just INITED and SHUT_DOWN isn't really useful, but as Nicolas said we'll soon add more states.) https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:239: DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN); This doesn't apply anymore, since we'll never be NOT_INITED https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:251: state_ = State::INITED; Err, this seems wrong?
All done. Please take another look. https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:216: state_ = State::INITED; On 2016/05/24 17:04:21, Marc Treib wrote: > Hm, so the NOT_INITED state doesn't really exist anymore. Remove it? (Just > INITED and SHUT_DOWN isn't really useful, but as Nicolas said we'll soon add > more states.) Yeah that's why I made it a bool, this is now a two-state enum :-) But it soon won't be. https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:239: DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN); On 2016/05/24 17:04:21, Marc Treib wrote: > This doesn't apply anymore, since we'll never be NOT_INITED Shutdown still needs to be called, also in unit tests. https://codereview.chromium.org/1997473004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:251: state_ = State::INITED; On 2016/05/24 17:04:21, Marc Treib wrote: > Err, this seems wrong? Yikes. Done. It wouldn't have passed the unit tests though.
The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/180001
LGTM again, thanks! https://codereview.chromium.org/1997473004/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1997473004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:237: DCHECK_EQ(state_, State::SHUT_DOWN); This won't compile AFAIK, because enum classes don't have operator<< which this macro requires. :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
https://codereview.chromium.org/1997473004/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java (right): https://codereview.chromium.org/1997473004/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsConfig.java:18: && PrefServiceBridge.getInstance().isSearchSuggestEnabled(); On 2016/05/24 16:58:09, Michael van Ouwerkerk wrote: > On 2016/05/24 16:38:35, dgn wrote: > > We want to use the material UI (toolbar disabled, etc.) when the NTP_SNIPPETS > > flag is set. Rather than disabling it here, we probably want to clear the > > snippets and not fetch new ones instead, similar to > > https://codereview.chromium.org/1980033002 > > I'm not sure what you meant, but the goal of this CL is as described in the bug: > "When switched off, Zine is completely removed from the UI, i.e. the NTP does > not have a peeking card and no below-the-fold section." Ah snap I missed that, sorry. So yeah the CL to update the UI without restart might be a big one.
On 24 May 2016 at 17:05, <bauerb@chromium.org> wrote: > > This won't do the fetch right away, it will just _schedule_ it (or > unschedule if > it's disabled). > > We're not doing the fetch fully lazily to make this work with slow internet > connections, so we'll fetch in the background (at reduced frequency if the > device is on a mobile connection and/or battery). > > You sure? Looking at the code I see a call to FetchSnippets() in what used to be Init, and this fetches right away… -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, May 24, 2016 at 11:10 PM Éric Noyau <noyau@chromium.org> wrote: > On 24 May 2016 at 17:05, <bauerb@chromium.org> wrote: > >> >> This won't do the fetch right away, it will just _schedule_ it (or >> unschedule if >> it's disabled). >> >> We're not doing the fetch fully lazily to make this work with slow >> internet >> connections, so we'll fetch in the background (at reduced frequency if the >> device is on a mobile connection and/or battery). >> >> You sure? Looking at the code I see a call to FetchSnippets() in what > used to be Init, and this fetches right away… > Right now, we do fetch immediately *if* there are no previous snippets stored. We expect that in most cases, there will be. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mvanouwerkerk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, noyau@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/1997473004/#ps200001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1997473004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1997473004/200001
Message was sent while issue was closed.
Description was changed from ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 TBR=skuhne ========== to ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 TBR=skuhne ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 TBR=skuhne ========== to ========== Snippets are enabled when search suggestions are enabled. * Users can now opt out of New Tab Page snippets with this preference: Settings > Privacy > Search and URL suggestions * Observing changes and updating previously instantiated NTPs is for a later CL. BUG=606792 TBR=skuhne Committed: https://crrev.com/97313225831b95666b6f7a6bdb0f6cd62858e411 Cr-Commit-Position: refs/heads/master@{#395866} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/97313225831b95666b6f7a6bdb0f6cd62858e411 Cr-Commit-Position: refs/heads/master@{#395866} |
