Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2395123002:
Connecting UserClassifier to NtpSnippetsFetcher (Closed)
DescriptionConnecting UserClassifier to NtpSnippetsFetcher
This CL applies UserClassifier in NtpSnippetsFetcher. The application is
twofold:
- current user class is sent in each request to content suggestion
server,
- RequestThrottler for NtpSnippetsFetcher is notified of current class
and thus can apply different quota for different user classes.
To make this possible, this CL add to RequestThrottler support for
changing RequestType on the fly.
The CL also fixes a typo from CL 2400133002: "top_languages" -> "topLanguages".
BUG=651813, 653526
Committed: https://crrev.com/b5893c0d4748c70ae1eb140d17ba188ce6e1e842
Cr-Commit-Position: refs/heads/master@{#424132}
Patch Set 1 #Patch Set 2 : A small fix #
Total comments: 9
Patch Set 3 : Marc's comments #Patch Set 4 : A minor fix #
Total comments: 10
Patch Set 5 : Marc's comments #2 #Patch Set 6 : Further polish #Patch Set 7 : Robert's comment #Patch Set 8 : Robert's comment #2 #Patch Set 9 : Rebase #Patch Set 10 : Rebase #
Total comments: 2
Patch Set 11 : Adding a variation parameter guard & rebase #Patch Set 12 : Minor fixes #Patch Set 13 : Rebase #
Total comments: 4
Patch Set 14 : Marc's comments #3 #Patch Set 15 : Rebase #Patch Set 16 : Unit-test fix #Messages
Total messages: 63 (41 generated)
jkrcal@chromium.org changed reviewers: + rkaplow@chromium.org, treib@chromium.org
Marc, could you PTAL? Robert, could you PTAL at histograms.xml?
https://codereview.chromium.org/2395123002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2395123002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:160: service->user_classifier()), You'll also have to do this for ios. https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:21: #include "components/ntp_snippets/user_classifier.h" Not needed I think, the forward decl below should suffice. (In any case, do only one of the two) https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.h:51: void ChangeRequestType(RequestType new_type); Hm, I find this rather weird. Could we instead just create three separate throttlers, one for each user class? https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:69: std::string GetUserClassNameForProto() const; What does this have to do with protobufs? Should this live in the fetcher instead? The rest of the server protocol is defined there.
jkrcal@chromium.org changed reviewers: + noyau@chromium.org
Thanks, Marc! PTAL again. Éric, could you PTAL at the ios factory? https://codereview.chromium.org/2395123002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2395123002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:160: service->user_classifier()), On 2016/10/06 13:41:54, Marc Treib wrote: > You'll also have to do this for ios. Thanks! Done. https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:21: #include "components/ntp_snippets/user_classifier.h" On 2016/10/06 13:41:55, Marc Treib wrote: > Not needed I think, the forward decl below should suffice. (In any case, do only > one of the two) Done. https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.h:51: void ChangeRequestType(RequestType new_type); On 2016/10/06 13:41:55, Marc Treib wrote: > Hm, I find this rather weird. Could we instead just create three separate > throttlers, one for each user class? Okay (I would still like them to share the same pref values). https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/user_classifier.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/user_classifier.h:69: std::string GetUserClassNameForProto() const; On 2016/10/06 13:41:55, Marc Treib wrote: > What does this have to do with protobufs? > Should this live in the fetcher instead? The rest of the server protocol is > defined there. Makes sense. Done.
https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.h (right): https://codereview.chromium.org/2395123002/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.h:51: void ChangeRequestType(RequestType new_type); On 2016/10/06 14:45:58, jkrcal wrote: > On 2016/10/06 13:41:55, Marc Treib wrote: > > Hm, I find this rather weird. Could we instead just create three separate > > throttlers, one for each user class? > > Okay (I would still like them to share the same pref values). Yup, sharing prefs makes sense. https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:160: user_classifier_(nullptr /* pref_service_ */), Any reason for not passing in a pref service? https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.cc:126: // day_pref is enough. Any particular reason for not just checking each pref individually? https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:271: if (!pref_service_) Is this necessary? All things being equal, I prefer to avoid "null in tests".
Thanks, PTAL again. https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:160: user_classifier_(nullptr /* pref_service_ */), On 2016/10/06 14:54:37, Marc Treib wrote: > Any reason for not passing in a pref service? Done. https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.cc:126: // day_pref is enough. On 2016/10/06 14:54:37, Marc Treib wrote: > Any particular reason for not just checking each pref individually? The code is slightly simpler / more efficient. It makes absolutely no sense for types to share something and not to share everything. Again, if you insist, I can change it. https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:271: if (!pref_service_) On 2016/10/06 14:54:37, Marc Treib wrote: > Is this necessary? All things being equal, I prefer to avoid "null in tests". If the pref_service is null, we anyway get some default value (based on default values deeper in the calls), it is just very unclear what it is. If you insist, I can remove it.
The CQ bit was checked by jkrcal@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...
LGTM, thanks! https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.cc:126: // day_pref is enough. On 2016/10/06 15:18:41, jkrcal wrote: > On 2016/10/06 14:54:37, Marc Treib wrote: > > Any particular reason for not just checking each pref individually? > > The code is slightly simpler / more efficient. It makes absolutely no sense for > types to share something and not to share everything. Again, if you insist, I > can change it. I think I'd find it slightly simpler/easier to understand if the code would just check all prefs :) At least, it'd make this 3-line comment unnecessary. Anyway, this is not important enough to fight over :) https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:271: if (!pref_service_) On 2016/10/06 15:18:41, jkrcal wrote: > On 2016/10/06 14:54:37, Marc Treib wrote: > > Is this necessary? All things being equal, I prefer to avoid "null in tests". > > If the pref_service is null, we anyway get some default value (based on default > values deeper in the calls), it is just very unclear what it is. If you insist, > I can remove it. Ah, I just noticed that there's lots of other null checks for the pref_service_ in this class... no, then one more doesn't matter. For the future, it'd be nice if we could get rid of them all. It's fairly simple to pass in a fake pref service in tests.
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 jkrcal@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...
Thanks, Marc! https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/request_throttler.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/request_throttler.cc:126: // day_pref is enough. On 2016/10/06 15:27:26, Marc Treib wrote: > On 2016/10/06 15:18:41, jkrcal wrote: > > On 2016/10/06 14:54:37, Marc Treib wrote: > > > Any particular reason for not just checking each pref individually? > > > > The code is slightly simpler / more efficient. It makes absolutely no sense > for > > types to share something and not to share everything. Again, if you insist, I > > can change it. > > I think I'd find it slightly simpler/easier to understand if the code would just > check all prefs :) At least, it'd make this 3-line comment unnecessary. > Anyway, this is not important enough to fight over :) I 've rewritten the function in a way I like (and you hopefully as well) :) https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... File components/ntp_snippets/user_classifier.cc (right): https://codereview.chromium.org/2395123002/diff/60001/components/ntp_snippets... components/ntp_snippets/user_classifier.cc:271: if (!pref_service_) On 2016/10/06 15:27:26, Marc Treib wrote: > On 2016/10/06 15:18:41, jkrcal wrote: > > On 2016/10/06 14:54:37, Marc Treib wrote: > > > Is this necessary? All things being equal, I prefer to avoid "null in > tests". > > > > If the pref_service is null, we anyway get some default value (based on > default > > values deeper in the calls), it is just very unclear what it is. If you > insist, > > I can remove it. > > Ah, I just noticed that there's lots of other null checks for the pref_service_ > in this class... no, then one more doesn't matter. For the future, it'd be nice > if we could get rid of them all. It's fairly simple to pass in a fake pref > service in tests. Makes sense...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Looks like there is a typo, SuggestionFetcherRareNTPUser and SuggestionFetcherRareNTPUser*s* for all the suffixes
On 2016/10/06 20:24:29, rkaplow wrote: > Looks like there is a typo, SuggestionFetcherRareNTPUser and > SuggestionFetcherRareNTPUser*s* for all the suffixes Oh, you are right! Fixed. PTAL again!
On 2016/10/06 20:32:24, jkrcal wrote: > On 2016/10/06 20:24:29, rkaplow wrote: > > Looks like there is a typo, SuggestionFetcherRareNTPUser and > > SuggestionFetcherRareNTPUser*s* for all the suffixes > > Oh, you are right! Fixed. PTAL again! Hold on. One more typo to fix :(
The CQ bit was checked by jkrcal@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...
On 2016/10/06 20:33:26, jkrcal wrote: > On 2016/10/06 20:32:24, jkrcal wrote: > > On 2016/10/06 20:24:29, rkaplow wrote: > > > Looks like there is a typo, SuggestionFetcherRareNTPUser and > > > SuggestionFetcherRareNTPUser*s* for all the suffixes > > > > Oh, you are right! Fixed. PTAL again! > > Hold on. One more typo to fix :( PTAL, again. I hope I've made it right, finally :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
jkrcal@chromium.org changed reviewers: + sdefresne@chromium.org
Sylvain, could you PTAL at the iOS factory (as Éric does not react)?
ios/ lgtm
The CQ bit was checked by jkrcal@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.
jwd@chromium.org changed reviewers: + jwd@chromium.org
https://codereview.chromium.org/2395123002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2395123002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:108136: + suggestions consumers"/> Do these three overlap? It seems like an active NTP user could also be an active suggestions consumer. Is there any way to get a total view of these three cases? And fyi, suffixes can apply to histogram names generated by suffixes. So you could also do this with a new suffix applying to the ...SuggestionFetcher histograms. I don't think you'd be able to obsolete the base ...SuggestionFetcher histograms like you're doing here though.
Thanks, Jesse! https://codereview.chromium.org/2395123002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2395123002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:108136: + suggestions consumers"/> On 2016/10/07 14:28:24, jwd wrote: > Do these three overlap? It seems like an active NTP user could also be an active > suggestions consumer. Is there any way to get a total view of these three cases? No, these do not overlap. You need to look at the three classes of histograms to get an overview. I actually don't want to add more structure to it because I want to keep the class RequestThrottler as simple as possible (and it is and will be used for other cases than for these three classes of users, see SuggestionThumbnailFetcher below). > And fyi, suffixes can apply to histogram names generated by suffixes. So you > could also do this with a new suffix applying to the ...SuggestionFetcher > histograms. I don't think you'd be able to obsolete the base > ...SuggestionFetcher histograms like you're doing here though. Huh, interesting :) Not desired in this case: I want a 1 to 1 correspondence between these suffixes and entries in kRequestTypeInfo in request_throttler.c
lgtm
The CQ bit was checked by jkrcal@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_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Marc, I've changed a few bits in ntp_snippets_fetcher.cc. Guarding the message to the server behind a variation parameter. Unfortunately, this is all mixed with a substantial rebase. I've double checked all manually: These lines in the new version have actually changed: - 73 - 78 - 132 - 151 - 447 - 449 - 520 - 523 Could you PTAL?
The CQ bit was checked by jkrcal@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.
ntp_snippet_fetcher still LGTM, modulo the question about naming convention https://codereview.chromium.org/2395123002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2395123002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:132: bool IsBooleanParameterEnabled(std::string param_name, bool default_value) { nit: const std::string& https://codereview.chromium.org/2395123002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:448: request->SetString("user_activeness_class", user_class); What's up with the naming conventions here - "excludedSuggestionIds" vs "user_activeness_class"? Are we sure all this works as expected?
Thanks, Marc! https://codereview.chromium.org/2395123002/diff/240001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2395123002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:132: bool IsBooleanParameterEnabled(std::string param_name, bool default_value) { On 2016/10/10 08:01:01, Marc Treib wrote: > nit: const std::string& Oh, again :| By the way, I am sick of this parsing of variation params over and over again. I'd like to put some common functions in variations:: when I find a bit of time to discuss it with Alexei (and to do it). https://codereview.chromium.org/2395123002/diff/240001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:448: request->SetString("user_activeness_class", user_class); On 2016/10/10 08:01:01, Marc Treib wrote: > What's up with the naming conventions here - "excludedSuggestionIds" vs > "user_activeness_class"? Are we sure all this works as expected? Oh, thanks for spotting it! It's hard to test as the server currently still does not recognize the param in any form :) Let me sneak in a fix for another new param "top_languages" below, as well.
The CQ bit was checked by jkrcal@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...
Description was changed from ========== Connecting UserClassifier to NtpSnippetsFetcher This CL applies UserClassifier in NtpSnippetsFetcher. The application is twofold: - current user class is sent in each request to content suggestion server, - RequestThrottler for NtpSnippetsFetcher is notified of current class and thus can apply different quota for different user classes. To make this possible, this CL add to RequestThrottler support for changing RequestType on the fly. BUG=651813,653526 ========== to ========== Connecting UserClassifier to NtpSnippetsFetcher This CL applies UserClassifier in NtpSnippetsFetcher. The application is twofold: - current user class is sent in each request to content suggestion server, - RequestThrottler for NtpSnippetsFetcher is notified of current class and thus can apply different quota for different user classes. To make this possible, this CL add to RequestThrottler support for changing RequestType on the fly. The CL also fixes a typo from CL 2400133002: "top_languages" -> "topLanguages". BUG=651813,653526 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 jkrcal@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.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, jwd@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2395123002/#ps300001 (title: "Unit-test fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Connecting UserClassifier to NtpSnippetsFetcher This CL applies UserClassifier in NtpSnippetsFetcher. The application is twofold: - current user class is sent in each request to content suggestion server, - RequestThrottler for NtpSnippetsFetcher is notified of current class and thus can apply different quota for different user classes. To make this possible, this CL add to RequestThrottler support for changing RequestType on the fly. The CL also fixes a typo from CL 2400133002: "top_languages" -> "topLanguages". BUG=651813,653526 ========== to ========== Connecting UserClassifier to NtpSnippetsFetcher This CL applies UserClassifier in NtpSnippetsFetcher. The application is twofold: - current user class is sent in each request to content suggestion server, - RequestThrottler for NtpSnippetsFetcher is notified of current class and thus can apply different quota for different user classes. To make this possible, this CL add to RequestThrottler support for changing RequestType on the fly. The CL also fixes a typo from CL 2400133002: "top_languages" -> "topLanguages". BUG=651813,653526 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Connecting UserClassifier to NtpSnippetsFetcher This CL applies UserClassifier in NtpSnippetsFetcher. The application is twofold: - current user class is sent in each request to content suggestion server, - RequestThrottler for NtpSnippetsFetcher is notified of current class and thus can apply different quota for different user classes. To make this possible, this CL add to RequestThrottler support for changing RequestType on the fly. The CL also fixes a typo from CL 2400133002: "top_languages" -> "topLanguages". BUG=651813,653526 ========== to ========== Connecting UserClassifier to NtpSnippetsFetcher This CL applies UserClassifier in NtpSnippetsFetcher. The application is twofold: - current user class is sent in each request to content suggestion server, - RequestThrottler for NtpSnippetsFetcher is notified of current class and thus can apply different quota for different user classes. To make this possible, this CL add to RequestThrottler support for changing RequestType on the fly. The CL also fixes a typo from CL 2400133002: "top_languages" -> "topLanguages". BUG=651813,653526 Committed: https://crrev.com/b5893c0d4748c70ae1eb140d17ba188ce6e1e842 Cr-Commit-Position: refs/heads/master@{#424132} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/b5893c0d4748c70ae1eb140d17ba188ce6e1e842 Cr-Commit-Position: refs/heads/master@{#424132} |
