Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1922083004:
Allow fetching personalized snippets from ChromeReader. (Closed)
|
Created:
4 years, 8 months ago by jkrcal Modified:
4 years, 7 months ago CC:
chromium-reviews, sdefresne+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. |
DescriptionAllow fetching personalized snippets from ChromeReader.
This amounts to:
- adding singin_manager and token_service to the fetcher;
- adding a variation parameter to determine the type of fetching (see bug 606320);
- changes to the request strings sent to ChromeReader;
- getting the snippets ordered by their score (see bug 608713);
Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals.
(Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far).
BUG=603907, 608713, 606320
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/33a244f68391a327cde16a006150f862ae641aab
Cr-Commit-Position: refs/heads/master@{#392886}
Patch Set 1 #Patch Set 2 : A minor fix #Patch Set 3 : Feature complete #Patch Set 4 : Rebase-update and minor polish #
Total comments: 89
Patch Set 5 : Fixing a broken unittest + reflecting code review comments #
Total comments: 6
Patch Set 6 : After code review #2 #
Total comments: 10
Patch Set 7 : Rebase update #
Total comments: 6
Patch Set 8 : After code review #3 #Patch Set 9 : Minor fixes #
Total comments: 37
Patch Set 10 : After code review #4 #
Total comments: 7
Patch Set 11 : After code review #5 #
Total comments: 8
Patch Set 12 : Fixing GYP build #Patch Set 13 : After code review #6 + rebase #Patch Set 14 : Another rebase #Patch Set 15 : Another rebase #Patch Set 16 : Adding ICU deps #Patch Set 17 : A unittest fix #Patch Set 18 : Another unittest fix #Patch Set 19 : iOS fix #Patch Set 20 : Yet another unittest fix #Messages
Total messages: 50 (18 generated)
Description was changed from
==========
Allow fetching personalized snippets from ChromeReader.
The user should get personalized snippets if
1) logged in and
2) his locale is among the locales supported by ChromeReader
(currently only en*).
BUG=603907
==========
to
==========
Allow fetching personalized snippets from ChromeReader.
The user should get personalized snippets if
1) logged in and
2) his locale is among the locales supported by ChromeReader
(currently only en*).
BUG=603907
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
Description was changed from
==========
Allow fetching personalized snippets from ChromeReader.
The user should get personalized snippets if
1) logged in and
2) his locale is among the locales supported by ChromeReader
(currently only en*).
BUG=603907
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
to
==========
Allow fetching personalized snippets from ChromeReader.
This amounts to:
- adding singin_manager and token_service to the fetcher;
- adding a variation parameter to determine the type of fetching;
- changes to the request strings sent to ChromeReader;
- getting the snippets ordered by their score (see bug 608713);
Additionally, this CL adds a score member to NTPSnippet and exposes it in
snippets-internals.
(Loading and storing of NTPSnippet needed to be changed a bit as the score entry
is stored in the top level suggestion node and not in the contentInfo subnode as
everything else that we loaded so far).
BUG=603907, 608713
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========
Description was changed from ========== Allow fetching personalized snippets from ChromeReader. This amounts to: - adding singin_manager and token_service to the fetcher; - adding a variation parameter to determine the type of fetching; - changes to the request strings sent to ChromeReader; - getting the snippets ordered by their score (see bug 608713); Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals. (Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far). BUG=603907, 608713 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Allow fetching personalized snippets from ChromeReader. This amounts to: - adding singin_manager and token_service to the fetcher; - adding a variation parameter to determine the type of fetching (see bug 606320); - changes to the request strings sent to ChromeReader; - getting the snippets ordered by their score (see bug 608713); Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals. (Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far). BUG=603907, 608713, 606320 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
jkrcal@chromium.org changed reviewers: + asvitkine@chromium.org, bauerb@chromium.org, treib@chromium.org
bauerb: PTAL at the snippets-internals part. asvitkine: PTAL at the added '+components/variations' dependency treib: PTAL at everything else :)
jkrcal@chromium.org changed reviewers: + noyau@chromium.org
noyau: PTAL at ios_chrome_ntp_snippets_service_factory
https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:120: is_stable_channel_(is_stable_channel){ Space before opening brace (also in other places in this file). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:124: if (variant_ == "") .empty() https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:141: // TODO(treib): What to do if there's already a pending request? This does seem like a thing we should worry about. What is going to happen right now? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:145: char locale[20]; Why 20? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: uloc_forLanguageTag(language_code.c_str(), locale, 20, NULL, &error); nullptr https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:148: DCHECK(error == U_ZERO_ERROR) << Use DCHECK_EQ (with the expected value first) for nicer error messages in case of failure (for example, it would include the error value automatically). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:150: locale_ = std::string(locale); The std::string constructor is implicit (or you could just use assign()). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:174: void NTPSnippetsFetcher::FetchSnippetsImpl(const std::string& url, I would represent URLs as GURL as much as possible. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:215: base::StringPrintf(kContentSnippetsServerNonAuthorizedParamFormat, You could extend the format to include a parameter for the URL. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:217: HttpRequestHeaders headers; You could just inline this. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:291: } else if (source->GetResponseCode() != net::HTTP_OK) { Now that we use authentication, we need to deal with the edge case again where the auth token expires just before we send the request (in which case we need to fetch a new auth token). But really, we should extract that into a common class instead of adding it to every. single. class that uses auth tokens. I filed https://crbug.com/609084 for that. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:301: << source->GetURL().spec() << ":" Super-nit: space after colon :) https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:37: SigninManagerBase* signin_manager, If you take ownership of these pointers, you should take a std::unique_ptr (or at the very least document that you take ownership, but forget that immediately again, because taking a unique_ptr is better anyway 😃). Then when you do that, you should realize that you don't actually have ownership of the SigninManager and the OAuthTokenService, because they are keyed services whose lifetime is tied to the profile. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:83: scoped_refptr<base::SequencedTaskRunner> file_task_runner_; Why do we add this again? Bad merge conflict? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:113: std::string variant_; I would store this as an enum and only use the string values for getting the parameter from variations. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:154: FakeSigninManagerBase* signin_manager = new FakeSigninManagerBase( Nit: superfluous space before `new`. https://codereview.chromium.org/1922083004/diff/60001/ios/chrome/browser/ntp_... File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1922083004/diff/60001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:102: (SigninManagerBase*)signin_manager, token_service, request_context, The cast should be static_cast<>(), but why is it necessary at all?
(probably some duplication here with Bernhard's comments...) https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:102: double score_; I think float is good enough here :) https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:45: const char kContentSnippetsServerNonAuthorizedParamFormat[] = "?key=%s"; Put in the server URL also via StringPrintf, instead of string concatenation? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:49: const char kVariantName[] = "variant"; IMO this is overly generic. At least make it "fetching_variant"? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:57: // which is exactly what we require. We listed title, snippet, thumbnail separately on purpose, so things don't break if more metadata fields are added in the future. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:79: const char kRequestParameterFormatAuth[] = This is mostly the same as the NonAuth one. Can we merge them, and put in the differences via StringPrintf etc? Or maybe the time has come to build this thing as a json dict (via base::Value) instead of string manipulation... https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:124: if (variant_ == "") variant_.empty() Also, what if it's an unknown string? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:141: // TODO(treib): What to do if there's already a pending request? Bad merge: This line had been removed (there's now a comment in the header explaining things). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:145: char locale[20]; Why 20? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: uloc_forLanguageTag(language_code.c_str(), locale, 20, NULL, &error); s/NULL/nullptr/ https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:148: DCHECK(error == U_ZERO_ERROR) << I don't think this warrants a DCHECK. (D)LOG(WARNING)? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:156: // signed-in: get OAuth for the API --> fetch snippets nit: Start with a captial letter and end with a period. Also, "get OAuth token" (or even "access token"), and I'd remove "for the API" - the token isn't really tied to the API. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:158: no empty line https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:167: } Hm, so the "else" here (not authenticated or auth in progress) can never happen, because UseAuthenticated checks for that. I find that confusing, it's not clear from reading this method. Can we just pull the signing_manager checks out of UseAuthenticated? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:193: std::string NTPSnippetsFetcher::GetHostsRestrict() const { GetHostRestricts https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:228: no empty line https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:234: FetchSnippetsImpl(std::string(kContentSnippetsServer), headers, I don't think you need the explicit conversion to std::string https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:264: DLOG(ERROR) << "Unable to get token: " << error.ToString(); Should we fall back to the non-authenticated case here? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:299: source->GetResponseAsString(&error_response); Now GetResponseAsString is called in both cases, so pull it out of the if? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:17: #include "net/http/http_request_headers.h" I think you can forward-declare this, and keep the include in the .cc https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:60: net::HttpRequestHeaders& headers, const please. If you need it to be non-const, use a pointer instead (or, better, refactor the code so you don't need mutable parameters :) ). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:63: bool UseAuthenticated(); Authenticated what? Either "UseAuthentication" or "UseAuthenticated<Something>" please. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:83: scoped_refptr<base::SequencedTaskRunner> file_task_runner_; Doesn't seem to be needed? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:88: // Hosts to restrict the snippets to nit: Periods at the end of comments please. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:113: std::string variant_; Make this an enum?
Thanks for all the comments! PTAL again. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippet.h:102: double score_; On 2016/05/04 11:50:09, Marc Treib wrote: > I think float is good enough here :) Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:45: const char kContentSnippetsServerNonAuthorizedParamFormat[] = "?key=%s"; On 2016/05/04 11:50:09, Marc Treib wrote: > Put in the server URL also via StringPrintf, instead of string concatenation? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:49: const char kVariantName[] = "variant"; On 2016/05/04 11:50:10, Marc Treib wrote: > IMO this is overly generic. At least make it "fetching_variant"? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:57: // which is exactly what we require. On 2016/05/04 11:50:09, Marc Treib wrote: > We listed title, snippet, thumbnail separately on purpose, so things don't break > if more metadata fields are added in the future. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:79: const char kRequestParameterFormatAuth[] = On 2016/05/04 11:50:09, Marc Treib wrote: > This is mostly the same as the NonAuth one. Can we merge them, and put in the > differences via StringPrintf etc? > Or maybe the time has come to build this thing as a json dict (via base::Value) > instead of string manipulation... Merged via printf. I am not sure json dict is worth the effort, here. (less readable and less efficient code, imho) https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:120: is_stable_channel_(is_stable_channel){ On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Space before opening brace (also in other places in this file). Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:124: if (variant_ == "") On 2016/05/04 11:50:10, Marc Treib wrote: > variant_.empty() > > Also, what if it's an unknown string? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:124: if (variant_ == "") On 2016/05/04 11:46:58, Bernhard Bauer wrote: > .empty() Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:141: // TODO(treib): What to do if there's already a pending request? On 2016/05/04 11:50:09, Marc Treib wrote: > Bad merge: This line had been removed (there's now a comment in the header > explaining things). Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:141: // TODO(treib): What to do if there's already a pending request? On 2016/05/04 11:46:58, Bernhard Bauer wrote: > This does seem like a thing we should worry about. What is going to happen right > now? Bad merge: This line had been removed (there's now a comment in the header explaining things). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:145: char locale[20]; On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Why 20? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:145: char locale[20]; On 2016/05/04 11:50:10, Marc Treib wrote: > Why 20? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: uloc_forLanguageTag(language_code.c_str(), locale, 20, NULL, &error); On 2016/05/04 11:46:58, Bernhard Bauer wrote: > nullptr Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:147: uloc_forLanguageTag(language_code.c_str(), locale, 20, NULL, &error); On 2016/05/04 11:50:10, Marc Treib wrote: > s/NULL/nullptr/ Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:148: DCHECK(error == U_ZERO_ERROR) << On 2016/05/04 11:50:09, Marc Treib wrote: > I don't think this warrants a DCHECK. (D)LOG(WARNING)? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:148: DCHECK(error == U_ZERO_ERROR) << On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Use DCHECK_EQ (with the expected value first) for nicer error messages in case > of failure (for example, it would include the error value automatically). Thanks for the hint. In the end, I used DLOG(WARNING) instead. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:150: locale_ = std::string(locale); On 2016/05/04 11:46:58, Bernhard Bauer wrote: > The std::string constructor is implicit (or you could just use assign()). Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:156: // signed-in: get OAuth for the API --> fetch snippets On 2016/05/04 11:50:09, Marc Treib wrote: > nit: Start with a captial letter and end with a period. > Also, "get OAuth token" (or even "access token"), and I'd remove "for the API" - > the token isn't really tied to the API. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:158: On 2016/05/04 11:50:09, Marc Treib wrote: > no empty line Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:167: } On 2016/05/04 11:50:10, Marc Treib wrote: > Hm, so the "else" here (not authenticated or auth in progress) can never happen, > because UseAuthenticated checks for that. I find that confusing, it's not clear > from reading this method. Can we just pull the signing_manager checks out of > UseAuthenticated? This is the else of the "UseAuthenticated()"-if and it definitely can happen. Do you agree? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:174: void NTPSnippetsFetcher::FetchSnippetsImpl(const std::string& url, On 2016/05/04 11:46:58, Bernhard Bauer wrote: > I would represent URLs as GURL as much as possible. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:193: std::string NTPSnippetsFetcher::GetHostsRestrict() const { On 2016/05/04 11:50:09, Marc Treib wrote: > GetHostRestricts Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:215: base::StringPrintf(kContentSnippetsServerNonAuthorizedParamFormat, On 2016/05/04 11:46:58, Bernhard Bauer wrote: > You could extend the format to include a parameter for the URL. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:217: HttpRequestHeaders headers; On 2016/05/04 11:46:58, Bernhard Bauer wrote: > You could just inline this. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:228: On 2016/05/04 11:50:10, Marc Treib wrote: > no empty line Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:234: FetchSnippetsImpl(std::string(kContentSnippetsServer), headers, On 2016/05/04 11:50:09, Marc Treib wrote: > I don't think you need the explicit conversion to std::string Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:264: DLOG(ERROR) << "Unable to get token: " << error.ToString(); On 2016/05/04 11:50:10, Marc Treib wrote: > Should we fall back to the non-authenticated case here? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:291: } else if (source->GetResponseCode() != net::HTTP_OK) { On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Now that we use authentication, we need to deal with the edge case again where > the auth token expires just before we send the request (in which case we need to > fetch a new auth token). But really, we should extract that into a common class > instead of adding it to every. single. class that uses auth tokens. I filed > https://crbug.com/609084 for that. I added a TODO to solve this later (in a common class). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:299: source->GetResponseAsString(&error_response); On 2016/05/04 11:50:09, Marc Treib wrote: > Now GetResponseAsString is called in both cases, so pull it out of the if? In the if branch I want to use a different variable and thus keep response empty. Does it make sense? https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:301: << source->GetURL().spec() << ":" On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Super-nit: space after colon :) Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:17: #include "net/http/http_request_headers.h" On 2016/05/04 11:50:10, Marc Treib wrote: > I think you can forward-declare this, and keep the include in the .cc Done. SigninManagerBase as well. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:37: SigninManagerBase* signin_manager, On 2016/05/04 11:46:58, Bernhard Bauer wrote: > If you take ownership of these pointers, you should take a std::unique_ptr (or > at the very least document that you take ownership, but forget that immediately > again, because taking a unique_ptr is better anyway 😃). Then when you do that, > you should realize that you don't actually have ownership of the SigninManager > and the OAuthTokenService, because they are keyed services whose lifetime is > tied to the profile. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:60: net::HttpRequestHeaders& headers, On 2016/05/04 11:50:10, Marc Treib wrote: > const please. If you need it to be non-const, use a pointer instead (or, better, > refactor the code so you don't need mutable parameters :) ). Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:63: bool UseAuthenticated(); On 2016/05/04 11:50:10, Marc Treib wrote: > Authenticated what? Either "UseAuthentication" or "UseAuthenticated<Something>" > please. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:83: scoped_refptr<base::SequencedTaskRunner> file_task_runner_; On 2016/05/04 11:50:10, Marc Treib wrote: > Doesn't seem to be needed? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:83: scoped_refptr<base::SequencedTaskRunner> file_task_runner_; On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Why do we add this again? Bad merge conflict? Sorry about that, a merging error of mine. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:88: // Hosts to restrict the snippets to On 2016/05/04 11:50:10, Marc Treib wrote: > nit: Periods at the end of comments please. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:113: std::string variant_; On 2016/05/04 11:46:58, Bernhard Bauer wrote: > I would store this as an enum and only use the string values for getting the > parameter from variations. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:113: std::string variant_; On 2016/05/04 11:50:10, Marc Treib wrote: > Make this an enum? Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:154: FakeSigninManagerBase* signin_manager = new FakeSigninManagerBase( On 2016/05/04 11:46:58, Bernhard Bauer wrote: > Nit: superfluous space before `new`. Done. https://codereview.chromium.org/1922083004/diff/60001/ios/chrome/browser/ntp_... File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1922083004/diff/60001/ios/chrome/browser/ntp_... ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:102: (SigninManagerBase*)signin_manager, token_service, request_context, On 2016/05/04 11:46:58, Bernhard Bauer wrote: > The cast should be static_cast<>(), but why is it necessary at all? Done.
https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:209: GURL url = GURL(base::StringPrintf(kSnippetsServerNonAuthorizedFormat, Just directly initialize |url|. https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:258: DLOG(ERROR) << "Unable to get token: " << error.ToString() << The append operator goes at the beginning of the line, so it aligns with the one right after DLOG(ERROR) (it's the only operator for which that is the case). https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:22: The empty lines aren't necessary here.
ios/ lgtm
Addressed the comments. PTAL. https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:209: GURL url = GURL(base::StringPrintf(kSnippetsServerNonAuthorizedFormat, On 2016/05/04 14:11:55, Bernhard Bauer wrote: > Just directly initialize |url|. Done. https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:258: DLOG(ERROR) << "Unable to get token: " << error.ToString() << On 2016/05/04 14:11:55, Bernhard Bauer wrote: > The append operator goes at the beginning of the line, so it aligns with the one > right after DLOG(ERROR) (it's the only operator for which that is the case). Done. https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.h:22: On 2016/05/04 14:11:55, Bernhard Bauer wrote: > The empty lines aren't necessary here. Done.
https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:167: } On 2016/05/04 14:04:07, jkrcal wrote: > On 2016/05/04 11:50:10, Marc Treib wrote: > > Hm, so the "else" here (not authenticated or auth in progress) can never > happen, > > because UseAuthenticated checks for that. I find that confusing, it's not > clear > > from reading this method. Can we just pull the signing_manager checks out of > > UseAuthenticated? > > This is the else of the "UseAuthenticated()"-if and it definitely can happen. Do > you agree? Sorry, I wasn't clear. I meant the missing "else" for the authenticated/auth-in-progress if. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:193: std::string NTPSnippetsFetcher::GetHostsRestrict() const { On 2016/05/04 14:04:08, jkrcal wrote: > On 2016/05/04 11:50:09, Marc Treib wrote: > > GetHostRestricts > > Done. GetHostRestricts - host is singular, restricts is plural :) https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:264: DLOG(ERROR) << "Unable to get token: " << error.ToString(); On 2016/05/04 14:04:08, jkrcal wrote: > On 2016/05/04 11:50:10, Marc Treib wrote: > > Should we fall back to the non-authenticated case here? > > Done. FWIW, this was an actual question - I'm not sure what the right thing to do is here. If we do the fallback, then we need to make sure to correctly record metrics (click-through-rate for personalized vs non-personalized and such). https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:299: source->GetResponseAsString(&error_response); On 2016/05/04 14:04:08, jkrcal wrote: > On 2016/05/04 11:50:09, Marc Treib wrote: > > Now GetResponseAsString is called in both cases, so pull it out of the if? > > In the if branch I want to use a different variable and thus keep response > empty. Does it make sense? Ah, I see. Okay, fair enough. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:48: "https://chromereader-pa.googleapis.com/v1/fetch?key=%s"; I meant: Make this "%s?key=%s". Then you won't have to duplicate the server URL. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:54: // Constants listing possible values of the "variant" parameter. fetching_variant https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:62: "%s" Can you add a comment like // Gaia ID will be inserted here (or whatever will actually be inserted here :) ) Also for the similar params below. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:119: variant_ = kRestrictedPersonalized; Maybe log an error if we're getting a variant string that we don't know? https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:212: base::StringPrintf(kRequestParameterFormat, "", "", std::string() instead of "" https://codereview.chromium.org/1922083004/diff/120001/ios/chrome/browser/ntp... File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1922083004/diff/120001/ios/chrome/browser/ntp... ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:21: #include "ios/chrome/browser/signin/signin_manager_factory.h" I think you're missing an include for the actual SigninManager here?
https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:307: << error_response; This seems like it should fit on the previous line. https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:19: class SigninManagerBase; And an empty line after this one :) (The rule is roughly: a block for each namespace, with empty lines separating blocks, and no empty lines inside of a block.)
PTAL. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:167: } On 2016/05/04 15:24:24, Marc Treib wrote: > On 2016/05/04 14:04:07, jkrcal wrote: > > On 2016/05/04 11:50:10, Marc Treib wrote: > > > Hm, so the "else" here (not authenticated or auth in progress) can never > > happen, > > > because UseAuthenticated checks for that. I find that confusing, it's not > > clear > > > from reading this method. Can we just pull the signing_manager checks out of > > > UseAuthenticated? > > > > This is the else of the "UseAuthenticated()"-if and it definitely can happen. > Do > > you agree? > > Sorry, I wasn't clear. I meant the missing "else" for the > authenticated/auth-in-progress if. Done. https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:193: std::string NTPSnippetsFetcher::GetHostsRestrict() const { On 2016/05/04 15:24:24, Marc Treib wrote: > On 2016/05/04 14:04:08, jkrcal wrote: > > On 2016/05/04 11:50:09, Marc Treib wrote: > > > GetHostRestricts > > > > Done. > > GetHostRestricts - host is singular, restricts is plural :) Done. (sorry for the oversight of mine!) https://codereview.chromium.org/1922083004/diff/60001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_fetcher.cc:264: DLOG(ERROR) << "Unable to get token: " << error.ToString(); On 2016/05/04 15:24:24, Marc Treib wrote: > On 2016/05/04 14:04:08, jkrcal wrote: > > On 2016/05/04 11:50:10, Marc Treib wrote: > > > Should we fall back to the non-authenticated case here? > > > > Done. > > FWIW, this was an actual question - I'm not sure what the right thing to do is > here. If we do the fallback, then we need to make sure to correctly record > metrics (click-through-rate for personalized vs non-personalized and such). I am not sure either. I think fallback to non-auth is slightly better. We do not log any auth/non-auth histograms at the moment. We need to be careful about it later. I added a comment to make it even clearer this is a fallback. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:48: "https://chromereader-pa.googleapis.com/v1/fetch?key=%s"; On 2016/05/04 15:24:24, Marc Treib wrote: > I meant: Make this "%s?key=%s". Then you won't have to duplicate the server URL. Done. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:54: // Constants listing possible values of the "variant" parameter. On 2016/05/04 15:24:24, Marc Treib wrote: > fetching_variant Done. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:62: "%s" On 2016/05/04 15:24:24, Marc Treib wrote: > Can you add a comment like > // Gaia ID will be inserted here > (or whatever will actually be inserted here :) ) > Also for the similar params below. Done. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:119: variant_ = kRestrictedPersonalized; On 2016/05/04 15:24:24, Marc Treib wrote: > Maybe log an error if we're getting a variant string that we don't know? Done. https://codereview.chromium.org/1922083004/diff/100001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:212: base::StringPrintf(kRequestParameterFormat, "", "", On 2016/05/04 15:24:24, Marc Treib wrote: > std::string() instead of "" Done. https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:307: << error_response; On 2016/05/04 15:56:36, Bernhard Bauer wrote: > This seems like it should fit on the previous line. Done. https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:19: class SigninManagerBase; On 2016/05/04 15:56:36, Bernhard Bauer wrote: > And an empty line after this one :) > > (The rule is roughly: a block for each namespace, with empty lines separating > blocks, and no empty lines inside of a block.) Done. https://codereview.chromium.org/1922083004/diff/120001/ios/chrome/browser/ntp... File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc (right): https://codereview.chromium.org/1922083004/diff/120001/ios/chrome/browser/ntp... ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:21: #include "ios/chrome/browser/signin/signin_manager_factory.h" On 2016/05/04 15:24:25, Marc Treib wrote: > I think you're missing an include for the actual SigninManager here? Done.
https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:24: #include "components/signin/core/browser/signin_tracker.h" Is this needed? https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:51: const char kSnippetsServerAuthorized[] = Just kSnippetsServer, it's used for both cases. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:54: "%s?key=%s"; Fits on the previous line https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:96: const char kAuthorizationFormat[] = " \"obfuscated_gaia_id\": \"%s\","; This isn't really authorization... kGaiaIdFormat? https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:112: : OAuth2TokenService::Consumer("NTP_snippets"), I'd make this "ntp_snippets", most of the other implementations seem to use lower-case strings here. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:130: DLOG(WARNING) << "Unknown fetching variant provided: " << variant; I think this might be worth a non-D LOG. Bernhard, WDYT? https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:161: // Translate the BCP 47 |language_code| into a posix locale string nit: Period after comment, also in a few other places in this file. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:227: (variant_ == kRestrictedPersonalized && !hosts_.empty())); Why are we checking hosts_ here? https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:241: std::string().c_str(), I think in this case, "" would be preferred. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:278: oauth_request_.reset(); Mmh, this is actually the same as the |request| parameter, right? If so, then GetAccountId below will be a use-after-free. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:297: const std::string& account_id) { Misaligned, but should fit on the previous line? Also, should we do anything with the account_id? Like, check it's the one we expect? https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:14: #include "base/callback_list.h" Not needed I think? https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:80: enum Variant { optional: Make this an enum class? Then it's members will be used like Variant::kRestricted which is IMO a bit clearer. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:112: // Authorization for signed-in users nit: Period after comment https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:128: int count_; count_to_fetch_? Just count_ seems ambiguous. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:14: #include "base/json/json_reader.h" Not required? Also stringprintf below I think.
PTAL https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:24: #include "components/signin/core/browser/signin_tracker.h" On 2016/05/09 12:33:33, Marc Treib wrote: > Is this needed? Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:51: const char kSnippetsServerAuthorized[] = On 2016/05/09 12:33:33, Marc Treib wrote: > Just kSnippetsServer, it's used for both cases. Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:54: "%s?key=%s"; On 2016/05/09 12:33:33, Marc Treib wrote: > Fits on the previous line Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:96: const char kAuthorizationFormat[] = " \"obfuscated_gaia_id\": \"%s\","; On 2016/05/09 12:33:33, Marc Treib wrote: > This isn't really authorization... kGaiaIdFormat? Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:112: : OAuth2TokenService::Consumer("NTP_snippets"), On 2016/05/09 12:33:33, Marc Treib wrote: > I'd make this "ntp_snippets", most of the other implementations seem to use > lower-case strings here. Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:130: DLOG(WARNING) << "Unknown fetching variant provided: " << variant; On 2016/05/09 12:33:33, Marc Treib wrote: > I think this might be worth a non-D LOG. Bernhard, WDYT? Done. (And happy to change it back if Bernhard is of a different opinion.) https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:161: // Translate the BCP 47 |language_code| into a posix locale string On 2016/05/09 12:33:33, Marc Treib wrote: > nit: Period after comment, also in a few other places in this file. Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:227: (variant_ == kRestrictedPersonalized && !hosts_.empty())); On 2016/05/09 12:33:33, Marc Treib wrote: > Why are we checking hosts_ here? You are right, this is weird (especially w.r.t. changes that happened to the code since I started on this). I removed it (and added another function dealing with host restriction generally). https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:241: std::string().c_str(), On 2016/05/09 12:33:33, Marc Treib wrote: > I think in this case, "" would be preferred. Okay :) https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:278: oauth_request_.reset(); On 2016/05/09 12:33:33, Marc Treib wrote: > Mmh, this is actually the same as the |request| parameter, right? If so, then > GetAccountId below will be a use-after-free. Done. (Thanks for pointing it out!) https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:297: const std::string& account_id) { On 2016/05/09 12:33:33, Marc Treib wrote: > Misaligned, but should fit on the previous line? > Also, should we do anything with the account_id? Like, check it's the one we > expect? Thanks for pointing out the misalignment. (No, it does not fit on the previous line.) I do not think we should do anything about the account_id. There is no particular account_id we should expect - the one the user signs in with is the one we pass to ChromeReader. This callback has a unary meaning to us - now it is time to send the request for the access token. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:14: #include "base/callback_list.h" On 2016/05/09 12:33:34, Marc Treib wrote: > Not needed I think? Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:80: enum Variant { On 2016/05/09 12:33:34, Marc Treib wrote: > optional: Make this an enum class? Then it's members will be used like > Variant::kRestricted > which is IMO a bit clearer. Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:112: // Authorization for signed-in users On 2016/05/09 12:33:34, Marc Treib wrote: > nit: Period after comment Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.h:128: int count_; On 2016/05/09 12:33:33, Marc Treib wrote: > count_to_fetch_? Just count_ seems ambiguous. Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:14: #include "base/json/json_reader.h" On 2016/05/09 12:33:34, Marc Treib wrote: > Not required? Also stringprintf below I think. Done.
https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:297: const std::string& account_id) { On 2016/05/09 15:30:13, jkrcal wrote: > On 2016/05/09 12:33:33, Marc Treib wrote: > > Misaligned, but should fit on the previous line? > > Also, should we do anything with the account_id? Like, check it's the one we > > expect? > > Thanks for pointing out the misalignment. (No, it does not fit on the previous > line.) > > I do not think we should do anything about the account_id. There is no > particular account_id we should expect - the one the user signs in with is the > one we pass to ChromeReader. This callback has a unary meaning to us - now it is > time to send the request for the access token. Can we only ever have one refresh token? I think with multi-signin on Android, we can actually have multiple. I'm not completely sure though, Bernhard might know more. If we *can* have multiple refresh tokens, then we should probably check that account_id matches signin_manager_->GetAuthenticatedAccountId() https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:251: base::StringPrintf(kGaiaIdFormat, account_id.c_str()); I think this fits on the previous line? https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:278: std::unique_ptr<OAuth2TokenService::Request> oauth_request( nit: Might be worth a comment ("delete after we leave this method", something like that). Also DCHECK that |request| is actually the same? https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:281: FetchSnippetsAuthenticated(oauth_request->GetAccountId(), access_token); Have you checked that this actually returns an obfuscated Gaia ID? At least the SigninManager still deals with emails apparently... https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/...
https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:130: DLOG(WARNING) << "Unknown fetching variant provided: " << variant; On 2016/05/09 15:30:13, jkrcal wrote: > On 2016/05/09 12:33:33, Marc Treib wrote: > > I think this might be worth a non-D LOG. Bernhard, WDYT? > > Done. > > (And happy to change it back if Bernhard is of a different opinion.) Nah, just make it LOG_IF if you're not doing anything else. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:297: const std::string& account_id) { On 2016/05/09 16:48:24, Marc Treib wrote: > On 2016/05/09 15:30:13, jkrcal wrote: > > On 2016/05/09 12:33:33, Marc Treib wrote: > > > Misaligned, but should fit on the previous line? > > > Also, should we do anything with the account_id? Like, check it's the one we > > > expect? > > > > Thanks for pointing out the misalignment. (No, it does not fit on the previous > > line.) > > > > I do not think we should do anything about the account_id. There is no > > particular account_id we should expect - the one the user signs in with is the > > one we pass to ChromeReader. This callback has a unary meaning to us - now it > is > > time to send the request for the access token. > > Can we only ever have one refresh token? I think with multi-signin on Android, > we can actually have multiple. I'm not completely sure though, Bernhard might > know more. Yeah, I'm pretty certain this can happen. > If we *can* have multiple refresh tokens, then we should probably check that > account_id matches signin_manager_->GetAuthenticatedAccountId() Yup. (Also, if it couldn't happen, we should DCHECK that they're the same.)
Thanks for the quick reactions! PTAL, again. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:130: DLOG(WARNING) << "Unknown fetching variant provided: " << variant; On 2016/05/09 17:45:06, Bernhard Bauer wrote: > On 2016/05/09 15:30:13, jkrcal wrote: > > On 2016/05/09 12:33:33, Marc Treib wrote: > > > I think this might be worth a non-D LOG. Bernhard, WDYT? > > > > Done. > > > > (And happy to change it back if Bernhard is of a different opinion.) > > Nah, just make it LOG_IF if you're not doing anything else. Done. https://codereview.chromium.org/1922083004/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:297: const std::string& account_id) { On 2016/05/09 17:45:07, Bernhard Bauer wrote: > On 2016/05/09 16:48:24, Marc Treib wrote: > > On 2016/05/09 15:30:13, jkrcal wrote: > > > On 2016/05/09 12:33:33, Marc Treib wrote: > > > > Misaligned, but should fit on the previous line? > > > > Also, should we do anything with the account_id? Like, check it's the one > we > > > > expect? > > > > > > Thanks for pointing out the misalignment. (No, it does not fit on the > previous > > > line.) > > > > > > I do not think we should do anything about the account_id. There is no > > > particular account_id we should expect - the one the user signs in with is > the > > > one we pass to ChromeReader. This callback has a unary meaning to us - now > it > > is > > > time to send the request for the access token. > > > > Can we only ever have one refresh token? I think with multi-signin on Android, > > we can actually have multiple. I'm not completely sure though, Bernhard might > > know more. > > Yeah, I'm pretty certain this can happen. > > > If we *can* have multiple refresh tokens, then we should probably check that > > account_id matches signin_manager_->GetAuthenticatedAccountId() > > Yup. (Also, if it couldn't happen, we should DCHECK that they're the same.) Now I get your point. I was not aware this listens to tokens for all accounts. Done. https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:251: base::StringPrintf(kGaiaIdFormat, account_id.c_str()); On 2016/05/09 16:48:24, Marc Treib wrote: > I think this fits on the previous line? Done. https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:278: std::unique_ptr<OAuth2TokenService::Request> oauth_request( On 2016/05/09 16:48:24, Marc Treib wrote: > nit: Might be worth a comment ("delete after we leave this method", something > like that). > Also DCHECK that |request| is actually the same? Done. https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:281: FetchSnippetsAuthenticated(oauth_request->GetAccountId(), access_token); On 2016/05/09 16:48:24, Marc Treib wrote: > Have you checked that this actually returns an obfuscated Gaia ID? At least the > SigninManager still deals with emails apparently... > https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/... Yes, I've checked with Roger and it also returns my obfuscated gaia id in practice.
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/1922083004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922083004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
LGTM with some more (optional) nits, plus a (non-optional) build fix ;) https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:281: FetchSnippetsAuthenticated(oauth_request->GetAccountId(), access_token); On 2016/05/09 19:04:16, jkrcal wrote: > On 2016/05/09 16:48:24, Marc Treib wrote: > > Have you checked that this actually returns an obfuscated Gaia ID? At least > the > > SigninManager still deals with emails apparently... > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/signin/... > > Yes, I've checked with Roger and it also returns my obfuscated gaia id in > practice. Okay, great! In that case, could you send a CL to Roger to remove/update the outdated comment in SigninManagerBase? (After this lands, no hurry) https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... File components/ntp_snippets.gypi (right): https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets.gypi:32: 'ntp_snippets_constants.h', These are missing "ntp_snippets/", hence the build errors. https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:127: LOG_IF(WARNING, !variant.empty() && variant != nitty nit: Break after the "&&", and align to the"!"? The current way is a bit confusing IMO. (If this is what "git cl format" produced, then it's okay to keep as is.) https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:278: // delete after we leave this method Full sentences in comments please ;P https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:302: if (account_id == signin_manager_->GetAuthenticatedAccountId()) { nitty nit: I prefer early-outing, i.e. if (account_id != ...) return;
lgtm
Thanks for all the help, guys! https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... File components/ntp_snippets.gypi (right): https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets.gypi:32: 'ntp_snippets_constants.h', On 2016/05/10 08:42:19, Marc Treib wrote: > These are missing "ntp_snippets/", hence the build errors. Thanks! (I've realized in the meantime...) https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:127: LOG_IF(WARNING, !variant.empty() && variant != On 2016/05/10 08:42:20, Marc Treib wrote: > nitty nit: Break after the "&&", and align to the"!"? The current way is a bit > confusing IMO. > (If this is what "git cl format" produced, then it's okay to keep as is.) git cl format did it differently but it is better than before, I think. https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:278: // delete after we leave this method On 2016/05/10 08:42:20, Marc Treib wrote: > Full sentences in comments please ;P Done. https://codereview.chromium.org/1922083004/diff/200001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_fetcher.cc:302: if (account_id == signin_manager_->GetAuthenticatedAccountId()) { On 2016/05/10 08:42:19, Marc Treib wrote: > nitty nit: I prefer early-outing, i.e. > if (account_id != ...) > return; Done.
LGTM, thanks! (Looks like you need another rebase now...)
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/1922083004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922083004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on 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/patch-status/1922083004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922083004/340001
lgtm on use of variations api
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...)
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/1922083004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922083004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by jkrcal@chromium.org
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org, bauerb@chromium.org, treib@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1922083004/#ps380001 (title: "Yet another unittest fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922083004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922083004/380001
Message was sent while issue was closed.
Description was changed from ========== Allow fetching personalized snippets from ChromeReader. This amounts to: - adding singin_manager and token_service to the fetcher; - adding a variation parameter to determine the type of fetching (see bug 606320); - changes to the request strings sent to ChromeReader; - getting the snippets ordered by their score (see bug 608713); Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals. (Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far). BUG=603907, 608713, 606320 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Allow fetching personalized snippets from ChromeReader. This amounts to: - adding singin_manager and token_service to the fetcher; - adding a variation parameter to determine the type of fetching (see bug 606320); - changes to the request strings sent to ChromeReader; - getting the snippets ordered by their score (see bug 608713); Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals. (Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far). BUG=603907, 608713, 606320 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Allow fetching personalized snippets from ChromeReader. This amounts to: - adding singin_manager and token_service to the fetcher; - adding a variation parameter to determine the type of fetching (see bug 606320); - changes to the request strings sent to ChromeReader; - getting the snippets ordered by their score (see bug 608713); Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals. (Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far). BUG=603907, 608713, 606320 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Allow fetching personalized snippets from ChromeReader. This amounts to: - adding singin_manager and token_service to the fetcher; - adding a variation parameter to determine the type of fetching (see bug 606320); - changes to the request strings sent to ChromeReader; - getting the snippets ordered by their score (see bug 608713); Additionally, this CL adds a score member to NTPSnippet and exposes it in snippets-internals. (Loading and storing of NTPSnippet needed to be changed a bit as the score entry is stored in the top level suggestion node and not in the contentInfo subnode as everything else that we loaded so far). BUG=603907, 608713, 606320 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/33a244f68391a327cde16a006150f862ae641aab Cr-Commit-Position: refs/heads/master@{#392886} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/33a244f68391a327cde16a006150f862ae641aab Cr-Commit-Position: refs/heads/master@{#392886} |
