Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1974483002:
Allow getting _only_ personalized snippets for NTP. (Closed)
DescriptionAllow getting _only_ personalized snippets for NTP.
Changing the personalization settings via variation parameters so that
we can specify to download personalized snippets only.
Splitting the previously single variation parameter into two to make
it easier to parse.
Additionally, the CL removes the fallback to non-personalized snippets if the oauth request fails.
BUG=606320
Committed: https://crrev.com/9b33018fcf78d2ce335eb43c431d922b708eb05f
Cr-Commit-Position: refs/heads/master@{#393465}
Patch Set 1 #Patch Set 2 : Minor fixes #
Total comments: 11
Patch Set 3 : After code review #1 #
Total comments: 6
Patch Set 4 : After code review #2 #
Total comments: 2
Patch Set 5 : After code review #3 #
Total comments: 5
Patch Set 6 : After code review #4 #
Total comments: 1
Messages
Total messages: 26 (9 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
PTAL
https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:52: const char kVariantName[] = "fetching_variant"; Also here: s/variant/personalization, or whatever you end up calling it. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:284: std::string auth = base::StringPrintf(kGaiaIdFormat, account_id.c_str()); nit: rename to gaia_id or something? It's not authentication. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:292: base::StringPrintf(kRequestParameterFormat, auth.c_str(), Maybe make a BuildRequestParameter helper function? https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:293: variant_ == Variant::kAll ? kFalseString : kTrueString, Ah, so when we get here, the variant can't be kNonPersonalized anymore, right? Still, I'd find it easier to read if this said variant_ == kPersonalized ? kTrue : kFalse which should be equivalent, right? https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:102: enum class Variant { kPersonalized, kNonPersonalized, kAll }; Can we rename "Variant" to something more meaningful now? "Personalization" with values kYes, kNo, kBoth, something like that?
Thanks! PTAL again. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:52: const char kVariantName[] = "fetching_variant"; On 2016/05/12 08:59:16, Marc Treib wrote: > Also here: s/variant/personalization, or whatever you end up calling it. Done. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:284: std::string auth = base::StringPrintf(kGaiaIdFormat, account_id.c_str()); On 2016/05/12 08:59:16, Marc Treib wrote: > nit: rename to gaia_id or something? It's not authentication. Done. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:292: base::StringPrintf(kRequestParameterFormat, auth.c_str(), On 2016/05/12 08:59:16, Marc Treib wrote: > Maybe make a BuildRequestParameter helper function? Done. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:293: variant_ == Variant::kAll ? kFalseString : kTrueString, On 2016/05/12 08:59:16, Marc Treib wrote: > Ah, so when we get here, the variant can't be kNonPersonalized anymore, right? > Still, I'd find it easier to read if this said > variant_ == kPersonalized ? kTrue : kFalse > which should be equivalent, right? Done. https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:102: enum class Variant { kPersonalized, kNonPersonalized, kAll }; On 2016/05/12 08:59:16, Marc Treib wrote: > Can we rename "Variant" to something more meaningful now? "Personalization" with > values kYes, kNo, kBoth, something like that? Done. I did not like kYes, kNo, kBoth - Yes and No sound too exclusive to be combined into Both.
https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1974483002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:102: enum class Variant { kPersonalized, kNonPersonalized, kAll }; On 2016/05/12 09:57:29, jkrcal wrote: > On 2016/05/12 08:59:16, Marc Treib wrote: > > Can we rename "Variant" to something more meaningful now? "Personalization" > with > > values kYes, kNo, kBoth, something like that? > > Done. I did not like kYes, kNo, kBoth - Yes and No sound too exclusive to be > combined into Both. Yup, yours are better, thanks! https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (left): https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:310: FetchSnippetsNonAuthenticated(); If you remove this, you need to call FetchFinished with appropriate "error" params here. https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: << "Unknown value for fetching_variant: " << variant; "fetching_personalization" - just use kPersonalizationName? https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:175: << "Unknown value for fetching_host_restrict: " << host_restriction; Also here: kHostRestrictionName?
Thanks again. PTAL. https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (left): https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:310: FetchSnippetsNonAuthenticated(); On 2016/05/12 10:06:15, Marc Treib wrote: > If you remove this, you need to call FetchFinished with appropriate "error" > params here. Done. Huh, thanks. I was too quick to send it out for code review :| https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:164: << "Unknown value for fetching_variant: " << variant; On 2016/05/12 10:06:15, Marc Treib wrote: > "fetching_personalization" - just use kPersonalizationName? Done. https://codereview.chromium.org/1974483002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:175: << "Unknown value for fetching_host_restrict: " << host_restriction; On 2016/05/12 10:06:15, Marc Treib wrote: > Also here: kHostRestrictionName? Done.
https://codereview.chromium.org/1974483002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1974483002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:55: // histograms, so do not change existing values. ^^ Insert new value at the end, and update the histogram definition. Also consider adding this to the comment :)
PTAL. https://codereview.chromium.org/1974483002/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1974483002/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:55: // histograms, so do not change existing values. On 2016/05/12 12:07:26, Marc Treib wrote: > ^^ > Insert new value at the end, and update the histogram definition. Also consider > adding this to the comment :) Done.
Description was changed from ========== Allow getting _only_ personalized snippets for NTP. Changing the personalization settings via variation parameters so that we can specify to download personalized snippets only. Splitting the previously single variation parameter into two to make it easier to parse. BUG=606320 ========== to ========== Allow getting _only_ personalized snippets for NTP. Changing the personalization settings via variation parameters so that we can specify to download personalized snippets only. Splitting the previously single variation parameter into two to make it easier to parse. Additionally, the CL removes the fallback to non-personalized snippets if the oauth request fails. BUG=606320 ==========
jkrcal@chromium.org changed reviewers: + asvitkine@chromium.org
Alexei: PTAL at the histograms.xml change.
lgtm https://codereview.chromium.org/1974483002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:122: return "Error in obtaining an oauth2 access token."; nit: OAuth2 https://codereview.chromium.org/1974483002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1974483002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:55: // histograms, so do not change existing values. Insert new value at the end, nit: values, plural. https://codereview.chromium.org/1974483002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1974483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:80100: + <int value="6" label="Error in obtaining oauth access token"/> nit: I'd remove the "in". "OAuth2".
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/patch-status/1974483002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974483002/100001
Thanks for all the comments! https://codereview.chromium.org/1974483002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:122: return "Error in obtaining an oauth2 access token."; On 2016/05/12 13:57:59, Marc Treib wrote: > nit: OAuth2 Done. https://codereview.chromium.org/1974483002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1974483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:80100: + <int value="6" label="Error in obtaining oauth access token"/> On 2016/05/12 13:57:59, Marc Treib wrote: > nit: I'd remove the "in". "OAuth2". My non-native sense for English prefers my version. (a quick google search does favor this choice as well)
lgtm (I think both with "in" and without "in" makes grammatical sense.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/1974483002/#ps100001 (title: "After code review #4")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974483002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974483002/100001
Message was sent while issue was closed.
Description was changed from ========== Allow getting _only_ personalized snippets for NTP. Changing the personalization settings via variation parameters so that we can specify to download personalized snippets only. Splitting the previously single variation parameter into two to make it easier to parse. Additionally, the CL removes the fallback to non-personalized snippets if the oauth request fails. BUG=606320 ========== to ========== Allow getting _only_ personalized snippets for NTP. Changing the personalization settings via variation parameters so that we can specify to download personalized snippets only. Splitting the previously single variation parameter into two to make it easier to parse. Additionally, the CL removes the fallback to non-personalized snippets if the oauth request fails. BUG=606320 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Allow getting _only_ personalized snippets for NTP. Changing the personalization settings via variation parameters so that we can specify to download personalized snippets only. Splitting the previously single variation parameter into two to make it easier to parse. Additionally, the CL removes the fallback to non-personalized snippets if the oauth request fails. BUG=606320 ========== to ========== Allow getting _only_ personalized snippets for NTP. Changing the personalization settings via variation parameters so that we can specify to download personalized snippets only. Splitting the previously single variation parameter into two to make it easier to parse. Additionally, the CL removes the fallback to non-personalized snippets if the oauth request fails. BUG=606320 Committed: https://crrev.com/9b33018fcf78d2ce335eb43c431d922b708eb05f Cr-Commit-Position: refs/heads/master@{#393465} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9b33018fcf78d2ce335eb43c431d922b708eb05f Cr-Commit-Position: refs/heads/master@{#393465}
Message was sent while issue was closed.
https://codereview.chromium.org/1974483002/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1974483002/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:174: std::string host_restriction = variations::GetVariationParamValue( Post-commit comment: The non-host-restricted case will occasionally break right now, because the NTPSnippetsService (unconditionally) filters by host in OnSuggestionsChanged. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
