Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(289)

Issue 1317513004: Add InterestsFetcher which retrieves a user's interests from the server. (Closed)

Created:
5 years, 3 months ago by tache
Modified:
5 years, 2 months ago
CC:
chromium-reviews, Sergiu
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add InterestsFetcher which retrieves a user's interests from the server. Currently the server is only available from the corp network. BUG=537145

Patch Set 1 #

Total comments: 57

Patch Set 2 : #

Patch Set 3 : #

Total comments: 12

Patch Set 4 : #

Total comments: 8

Patch Set 5 : #

Patch Set 6 : Switch to using a Start() method #

Patch Set 7 : Removed the use of FakeURLFetcher #

Total comments: 18

Patch Set 8 : Get the access_token in the InterestsFetcher #

Total comments: 16

Patch Set 9 : Fixed comments #

Patch Set 10 : Improve handling of HTTP errors. #

Total comments: 21

Patch Set 11 : Fix comments #

Total comments: 30
Unified diffs Side-by-side diffs Delta from patch set Stats (+540 lines, -0 lines) Patch
A chrome/browser/interests/OWNERS View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A chrome/browser/interests/interests_fetcher.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +105 lines, -0 lines 22 comments Download
A chrome/browser/interests/interests_fetcher.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +221 lines, -0 lines 8 comments Download
A chrome/browser/interests/interests_fetcher_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +206 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 23 (5 generated)
Marc Treib
First pass - lots of comments below, but it's all small stuff. No major complaints ...
5 years, 3 months ago (2015-09-03 09:04:37 UTC) #3
tache
> Do we have any idea when the API might be available? Hopefully sometime next ...
5 years, 3 months ago (2015-09-03 13:26:29 UTC) #4
Marc Treib
Almost there :) If it'll be only a week until the API exists, then I'd ...
5 years, 3 months ago (2015-09-03 13:54:55 UTC) #5
tache
https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/interests_retriever.cc File chrome/browser/interests/interests_retriever.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/interests_retriever.cc#newcode163 chrome/browser/interests/interests_retriever.cc:163: if (!interest_dict->GetString(kIdInterestImageUrl, &image_url)) { On 2015/09/03 13:54:55, Marc Treib ...
5 years, 3 months ago (2015-09-03 15:21:53 UTC) #6
Marc Treib
Pretty much looking good now. Now to wait for that API... https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interests/interests_retriever.h File chrome/browser/interests/interests_retriever.h (right): ...
5 years, 3 months ago (2015-09-03 15:35:07 UTC) #7
tache
https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interests/interests_retriever.h File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interests/interests_retriever.h#newcode22 chrome/browser/interests/interests_retriever.h:22: // Authentication is done using the user's OAUTH access_token. ...
5 years, 3 months ago (2015-09-03 16:25:30 UTC) #8
tache
Change to using a Start() method
5 years, 3 months ago (2015-09-09 17:21:50 UTC) #9
Marc Treib
Can you add an OWNERS file for the new interests folder, please? Include me in ...
5 years, 3 months ago (2015-09-22 12:02:59 UTC) #10
tache
https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interests/interests_fetcher.cc#newcode13 chrome/browser/interests/interests_fetcher.cc:13: #include "net/url_request/url_fetcher_factory.h" On 2015/09/22 12:02:59, Marc Treib (OOO till ...
5 years, 2 months ago (2015-09-29 09:33:34 UTC) #11
tache
5 years, 2 months ago (2015-09-29 11:13:14 UTC) #13
Bernhard Bauer
Your CL description mentions "InterestsRetriever", but the class is called InterestsFetcher. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): ...
5 years, 2 months ago (2015-09-29 14:01:12 UTC) #15
tache
https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interests/interests_fetcher.cc#newcode34 chrome/browser/interests/interests_fetcher.cc:34: "https://www-googleapis-stagin.sandbox.google.com/chromenow/contextdemo/" On 2015/09/29 14:01:12, Bernhard Bauer wrote: > stagin? ...
5 years, 2 months ago (2015-09-29 19:58:24 UTC) #16
Bernhard Bauer
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc#newcode37 chrome/browser/interests/interests_fetcher.cc:37: "interests/"; Remove the URL now? https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc#newcode51 chrome/browser/interests/interests_fetcher.cc:51: if (interests_url.is_empty()) ...
5 years, 2 months ago (2015-09-30 08:14:08 UTC) #17
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc#newcode22 chrome/browser/interests/interests_fetcher.cc:22: using net::URLFetcher; don't use using, always use the full ...
5 years, 2 months ago (2015-09-30 09:23:10 UTC) #18
Bernhard Bauer
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc#newcode22 chrome/browser/interests/interests_fetcher.cc:22: using net::URLFetcher; On 2015/09/30 09:23:09, jochen wrote: > don't ...
5 years, 2 months ago (2015-09-30 09:38:57 UTC) #19
tache
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interests/interests_fetcher.cc#newcode22 chrome/browser/interests/interests_fetcher.cc:22: using net::URLFetcher; On 2015/09/30 09:38:57, Bernhard Bauer wrote: > ...
5 years, 2 months ago (2015-09-30 17:32:02 UTC) #20
jochen (gone - plz use gerrit)
some comments https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interests/interests_fetcher.cc File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interests/interests_fetcher.cc#newcode109 chrome/browser/interests/interests_fetcher.cc:109: // Add oauth access token please no ...
5 years, 2 months ago (2015-10-02 13:51:01 UTC) #21
PEConn
5 years, 2 months ago (2015-10-05 14:12:05 UTC) #23
The updated code can be found:
https://codereview.chromium.org/1384973002

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
File chrome/browser/interests/interests_fetcher.cc (right):

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.cc:109: // Add oauth access token
On 2015/10/02 13:51:00, jochen wrote:
> please no comments of the form 
> 
> // Do foo
> DoFoo()

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.cc:114: 
On 2015/10/02 13:51:00, jochen wrote:
> no empty line here

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.cc:142: LOG(WARNING) << "HTTP error "
<< response_code;
On 2015/10/02 13:51:00, jochen wrote:
> VLOG(2), here and everywhere

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.cc:211:
InterestsFetcher::Interest::Interest(const std::string& name,
On 2015/10/02 13:51:00, jochen wrote:
> i'd recommend to use the same order in the cc file as in the header

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
File chrome/browser/interests/interests_fetcher.h (right):

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:30: //
On 2015/10/02 13:51:00, jochen wrote:
> please remove the example usage part of the comment. it's redundant with the
> code, and actually already now wrong...

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:43: Interest(const std::string&
name, const GURL& image_url, float rlevance);
On 2015/10/02 13:51:00, jochen wrote:
> ctor and dtor should come before data members:
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...
> 
> also, spelling: relevance

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:44: 
On 2015/10/02 13:51:00, jochen wrote:
> no empty line here

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:54: OAuth2TokenService*
oauth2_token_service,
On 2015/10/02 13:51:00, jochen wrote:
> how do you guarantee that the token service is still alive by the time you use
> it? Why not pass it as a parameter to Start()?

The token service needs to be stored so we can retry things on an authorization
error.
The token service is tied to the Profile and since the InterestsFetcher should
be shorter lived than the profile, this shouldn't be an issue.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:60: static
scoped_ptr<InterestsFetcher> CreateFromProfile(Profile* profile);
On 2015/10/02 13:51:00, jochen wrote:
> can you have multiple fetchers per profile? Should this be a
> profile-keyed-service? should the ctor be private?

You can have multiple fetchers per profile.
It's perfectly valid for a user to create a InterestsFetcher from its
constructor, the 'CreateFromProfile' method is just for convenience.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:62: void Start(const
InterestsCallback& callback);
On 2015/10/02 13:51:00, jochen wrote:
> this could use a comment what it starts (or a better name - StartFetching)

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:72: 
On 2015/10/02 13:51:00, jochen wrote:
> no empty line here

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:77: 
On 2015/10/02 13:51:00, jochen wrote:
> no empty line here

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:79: 
On 2015/10/02 13:51:00, jochen wrote:
> nor here

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:87: scoped_ptr<net::URLFetcher>
fetcher_;
On 2015/10/02 13:51:00, jochen wrote:
> sooo many empty lines here.

Done.

https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes...
chrome/browser/interests/interests_fetcher.h:101: 
On 2015/10/02 13:51:00, jochen wrote:
> only one empty line here

Done.

Powered by Google App Engine
This is Rietveld 408576698