Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1317513004:
Add InterestsFetcher which retrieves a user's interests from the server. (Closed)
DescriptionAdd 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
Dependent Patchsets: Messages
Total messages: 23 (5 generated)
tache@google.com changed reviewers: + treib@google.com
tache@google.com changed reviewers: + treib@chromium.org - treib@google.com
First pass - lots of comments below, but it's all small stuff. No major complaints :) I guess the only big question is whether it makes sense to land this yet, given that a) the API doesn't exist yet, and b) it's not used anywhere. Do you have other pending CLs that depend on this one? Do we have any idea when the API might be available? Also, I'm not quite sure where this code should go. For now, we only have plans to surface it on the NTP, right? In that case, it should probably go with the NTP code rather than its own "top-level" folder. But we can discuss this later. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:12: #include <net/url_request/test_url_fetcher_factory.h> Unneeded include https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:77: net::URLRequestContextGetter* url_request_context, const std::string& token, Each param goes on its own line. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:79: URLFetcherFactory* url_fetcher_factory = NULL) remove "= NULL" https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:104: if (url_fetcher_factory_) { Hm, this bothers me.. I dislike "if (testing)" constructs. IIUC, the whole thing is only needed because you want a FakeURLFetcher in the non-testing case - otherwise you could just always do URLFetcher::Create, right? https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:119: source->GetResponseAsString(&response_body); Move these two lines after the success check? https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:127: auto interests = ExtractInterests(response_body); Inline the ExtractInterests call into the line below? https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:135: const base::DictionaryValue* dict = NULL; nullptr :) https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:163: if (!interest_dict->GetString(kIdInterestImageUrl, &image_url)) { I think it's okay if the image URL is missing? So maybe we should not return an empty response in this case? https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:174: res.push_back(Interest{name, image_url, relevance}); We can't use uniform initialization yet - it needs C++11 library support, which is still missing on Mac :-/ (see trybot failures on Mac below!) But regular parens initialization should do just as well here. (You'll have to write a constructor for Interest, but oh well.) https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. WHAT YEAR IS THIS?! :D https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:11: #include <base/callback.h> Quotes instead of angle brackets for non-stdlib includes. Also in the other files. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:20: class URLFetcher; Either include or forward declare, not both. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:24: FORWARD_DECLARE_TEST(InterestsRetrieverTest, DefaultFakeResponse); Is this required? I don't think I've ever seen this before. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:33: bool operator==(const Interest &interest) const { Do you actually need this operator? If so, please move the implementation to the .cc file, otherwise just remove it. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:39: typedef base::Callback<void(std::vector<Interest, std::allocator<Interest>>)> Remove the explicit allocator? Also, I find using InterestsCallback = ... slightly nicer, but up to you. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:42: InterestsRetriever(net::URLRequestContextGetter *url_request_context, nit: * goes with the type, not the name. Also the &s below, and in a bunch of other places. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:43: const std::string &token, What exactly is |token|? An OAuth access token? Then maybe call it |access_token| (there's also refresh tokens!) https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:53: // create URLFetcher Not a very useful comment ;P https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:59: static std::string FakeResponse(); This one, OTOH, would deserve a comment, or at least better name. I think you only need it for the test, right? Then maybe GetFakeReponseForTest()? And once we have the API, it shouldn't be necessary at all anymore. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:73: FRIEND_TEST_ALL_PREFIXES(::InterestsRetrieverTest, DefaultFakeResponse); By convention, the FRIEND_TEST goes to the top of the private section. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever_unittest.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:23: #include <testing/gtest/include/gtest/internal/gtest-internal.h> Is this one necessary? https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:28: using testing::internal::Notification; This seems unnecessary as well. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:33: const std::string kEmptyResponse = "{\n" We don't use global vars of non-trivial types (i.e. anything with a constructor), so this should be const char[]. (The reason is that the initialization order is not well-defined. I guess it doesn't matter in a unit test, but still good to be consistent.) https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:38: const std::vector<InterestsRetriever::Interest> kExpectedSuccessfullResponse { "Successful", one "l" :) Also, again: No uniform initialization yet, and no non-POD globals. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:72: nit: there's a bunch of extra empty lines in this file https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:113: std::vector<scoped_ptr<InterestsRetriever>> requests_; You can't put scoped_ptrs into a vector, because (in C++03) vector's elements must be copyable. But I don't think you actually need the vector here, there's never more than one element, right? Otherwise, use a ScopedVector.
> Do we have any idea when the API might be available? Hopefully sometime next week. > Do you have other pending CLs that depend on this one? My next CL is the JNI part that exposes the InterestsRetriever. After that the CLs will be in Java land. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:12: #include <net/url_request/test_url_fetcher_factory.h> On 2015/09/03 09:04:36, Marc Treib wrote: > Unneeded include This is needed for FakeURLFetcher https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:77: net::URLRequestContextGetter* url_request_context, const std::string& token, On 2015/09/03 09:04:36, Marc Treib wrote: > Each param goes on its own line. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:79: URLFetcherFactory* url_fetcher_factory = NULL) On 2015/09/03 09:04:36, Marc Treib wrote: > remove "= NULL" Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:104: if (url_fetcher_factory_) { On 2015/09/03 09:04:36, Marc Treib wrote: > Hm, this bothers me.. I dislike "if (testing)" constructs. > IIUC, the whole thing is only needed because you want a FakeURLFetcher in the > non-testing case - otherwise you could just always do URLFetcher::Create, right? Yes, this is only needed because of the fake response. Once the API is ready, the code in the comments will be used. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:119: source->GetResponseAsString(&response_body); On 2015/09/03 09:04:36, Marc Treib wrote: > Move these two lines after the success check? Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:127: auto interests = ExtractInterests(response_body); On 2015/09/03 09:04:36, Marc Treib wrote: > Inline the ExtractInterests call into the line below? Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:135: const base::DictionaryValue* dict = NULL; On 2015/09/03 09:04:37, Marc Treib wrote: > nullptr :) Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:163: if (!interest_dict->GetString(kIdInterestImageUrl, &image_url)) { On 2015/09/03 09:04:36, Marc Treib wrote: > I think it's okay if the image URL is missing? So maybe we should not return an > empty response in this case? Done. I kept the warning, since atm there should always be an image url in the response from the server. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:174: res.push_back(Interest{name, image_url, relevance}); On 2015/09/03 09:04:36, Marc Treib wrote: > We can't use uniform initialization yet - it needs C++11 library support, which > is still missing on Mac :-/ (see trybot failures on Mac below!) > But regular parens initialization should do just as well here. (You'll have to > write a constructor for Interest, but oh well.) Done. Leaving it as is. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/09/03 09:04:37, Marc Treib wrote: > WHAT YEAR IS THIS?! :D Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:11: #include <base/callback.h> On 2015/09/03 09:04:37, Marc Treib wrote: > Quotes instead of angle brackets for non-stdlib includes. Also in the other > files. Eclipse "organize imports" is so broken... Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:20: class URLFetcher; On 2015/09/03 09:04:37, Marc Treib wrote: > Either include or forward declare, not both. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:24: FORWARD_DECLARE_TEST(InterestsRetrieverTest, DefaultFakeResponse); On 2015/09/03 09:04:37, Marc Treib wrote: > Is this required? I don't think I've ever seen this before. https://code.google.com/p/chromium/codesearch#chromium/src/base/gtest_prod_ut... The test class needs to be forward declared for the FRIENDS macro. Looking at the comments this seems the correct way to do it. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:33: bool operator==(const Interest &interest) const { On 2015/09/03 09:04:37, Marc Treib wrote: > Do you actually need this operator? If so, please move the implementation to the > .cc file, otherwise just remove it. Only used for testing atm. Moved it to the .cc file https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:39: typedef base::Callback<void(std::vector<Interest, std::allocator<Interest>>)> On 2015/09/03 09:04:37, Marc Treib wrote: > Remove the explicit allocator? > Also, I find > using InterestsCallback = ... > slightly nicer, but up to you. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:42: InterestsRetriever(net::URLRequestContextGetter *url_request_context, On 2015/09/03 09:04:37, Marc Treib wrote: > nit: * goes with the type, not the name. Also the &s below, and in a bunch of > other places. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:43: const std::string &token, On 2015/09/03 09:04:37, Marc Treib wrote: > What exactly is |token|? An OAuth access token? Then maybe call it > |access_token| (there's also refresh tokens!) Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:53: // create URLFetcher On 2015/09/03 09:04:37, Marc Treib wrote: > Not a very useful comment ;P Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:59: static std::string FakeResponse(); On 2015/09/03 09:04:37, Marc Treib wrote: > This one, OTOH, would deserve a comment, or at least better name. I think you > only need it for the test, right? Then maybe GetFakeReponseForTest()? > And once we have the API, it shouldn't be necessary at all anymore. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:73: FRIEND_TEST_ALL_PREFIXES(::InterestsRetrieverTest, DefaultFakeResponse); On 2015/09/03 09:04:37, Marc Treib wrote: > By convention, the FRIEND_TEST goes to the top of the private section. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever_unittest.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:23: #include <testing/gtest/include/gtest/internal/gtest-internal.h> On 2015/09/03 09:04:37, Marc Treib wrote: > Is this one necessary? Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:28: using testing::internal::Notification; On 2015/09/03 09:04:37, Marc Treib wrote: > This seems unnecessary as well. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:33: const std::string kEmptyResponse = "{\n" On 2015/09/03 09:04:37, Marc Treib wrote: > We don't use global vars of non-trivial types (i.e. anything with a > constructor), so this should be const char[]. > (The reason is that the initialization order is not well-defined. I guess it > doesn't matter in a unit test, but still good to be consistent.) Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:38: const std::vector<InterestsRetriever::Interest> kExpectedSuccessfullResponse { On 2015/09/03 09:04:37, Marc Treib wrote: > "Successful", one "l" :) > Also, again: No uniform initialization yet, and no non-POD globals. Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:72: On 2015/09/03 09:04:37, Marc Treib wrote: > nit: there's a bunch of extra empty lines in this file Done. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever_unittest.cc:113: std::vector<scoped_ptr<InterestsRetriever>> requests_; On 2015/09/03 09:04:37, Marc Treib wrote: > You can't put scoped_ptrs into a vector, because (in C++03) vector's elements > must be copyable. > But I don't think you actually need the vector here, there's never more than one > element, right? Otherwise, use a ScopedVector. Switched to using a single scoped_ptr.
Almost there :) If it'll be only a week until the API exists, then I'd prefer waiting until then before commiting. Then we can get rid of the FakeURLFetcher shenanigans. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:12: #include <net/url_request/test_url_fetcher_factory.h> On 2015/09/03 13:26:28, tache wrote: > On 2015/09/03 09:04:36, Marc Treib wrote: > > Unneeded include > > This is needed for FakeURLFetcher Ah I see, sorry. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:163: if (!interest_dict->GetString(kIdInterestImageUrl, &image_url)) { On 2015/09/03 13:26:28, tache wrote: > On 2015/09/03 09:04:36, Marc Treib wrote: > > I think it's okay if the image URL is missing? So maybe we should not return > an > > empty response in this case? > > Done. I kept the warning, since atm there should always be an image url in the > response from the server. Okay. Please add a comment though, otherwise the next reader might think that the return was simply forgotten in this case, since the other two checks do have it. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:24: FORWARD_DECLARE_TEST(InterestsRetrieverTest, DefaultFakeResponse); On 2015/09/03 13:26:29, tache wrote: > On 2015/09/03 09:04:37, Marc Treib wrote: > > Is this required? I don't think I've ever seen this before. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/gtest_prod_ut... > > The test class needs to be forward declared for the FRIENDS macro. Looking at > the comments this seems the correct way to do it. Generally it doesn't need to be forward-declared, no. Since this seems related to namespaces, maybe it's because you explicitly specified global scope within the FRIEND_TEST below? Maybe if you remove the "::", the forward declare won't be needed? Anyway, doesn't really matter, I'm just curious. https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.h:33: bool operator==(const Interest &interest) const { On 2015/09/03 13:26:29, tache wrote: > On 2015/09/03 09:04:37, Marc Treib wrote: > > Do you actually need this operator? If so, please move the implementation to > the > > .cc file, otherwise just remove it. > > Only used for testing atm. > > Moved it to the .cc file Then you could also move it to the test file (as a non-member). I'm also okay with leaving it here though. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:21: class InterestsRetriever : public net::URLFetcherDelegate { Please add a comment explaining what this class does and how it's used. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:31: typedef base::Callback<void(std::vector<Interest>)> should the param be a const vector& ? https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:52: /* nit: /* */ are quite uncommon in chromium code. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_retriever_unittest.cc (right): https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever_unittest.cc:48: std::vector<InterestsRetriever::Interest> kExpectedEmptyResponse() { s/k/Get/g, also below https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever_unittest.cc:74: void GetInterests() { RequestInterests? "Get" sounds like it would return something. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever_unittest.cc:75: request_ = scoped_ptr<InterestsRetriever>(new InterestsRetriever( request_.reset(new ... is shorter :)
https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_retriever.cc (right): https://codereview.chromium.org/1317513004/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_retriever.cc:163: if (!interest_dict->GetString(kIdInterestImageUrl, &image_url)) { On 2015/09/03 13:54:55, Marc Treib wrote: > On 2015/09/03 13:26:28, tache wrote: > > On 2015/09/03 09:04:36, Marc Treib wrote: > > > I think it's okay if the image URL is missing? So maybe we should not return > > an > > > empty response in this case? > > > > Done. I kept the warning, since atm there should always be an image url in the > > response from the server. > > Okay. Please add a comment though, otherwise the next reader might think that > the return was simply forgotten in this case, since the other two checks do have > it. Done. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:21: class InterestsRetriever : public net::URLFetcherDelegate { On 2015/09/03 13:54:55, Marc Treib wrote: > Please add a comment explaining what this class does and how it's used. Done. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:31: typedef base::Callback<void(std::vector<Interest>)> On 2015/09/03 13:54:55, Marc Treib wrote: > should the param be a const vector& ? Done. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:52: /* On 2015/09/03 13:54:55, Marc Treib wrote: > nit: /* */ are quite uncommon in chromium code. Done. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_retriever_unittest.cc (right): https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever_unittest.cc:48: std::vector<InterestsRetriever::Interest> kExpectedEmptyResponse() { On 2015/09/03 13:54:55, Marc Treib wrote: > s/k/Get/g, also below Done. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever_unittest.cc:74: void GetInterests() { On 2015/09/03 13:54:55, Marc Treib wrote: > RequestInterests? "Get" sounds like it would return something. Done. https://codereview.chromium.org/1317513004/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_retriever_unittest.cc:75: request_ = scoped_ptr<InterestsRetriever>(new InterestsRetriever( On 2015/09/03 13:54:55, Marc Treib wrote: > request_.reset(new ... > is shorter :) thx :D
Pretty much looking good now. Now to wait for that API... https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:22: // Authentication is done using the user's OAUTH access_token. nit: OAuth https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:26: // auto request = scoped_ptr<InterestsRetriever>(new InterestsRetriever( make_scoped_ptr(new ... then you don't have to write "InterestsRetriever" twice. :) Mention what happens when request is deleted before callback is called? https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:30: class InterestsRetriever : public net::URLFetcherDelegate { Hm, "Fetcher" is much more common in Chromium than "Retriever". I realize it's a bunch of work, but can we rename this? https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:62: // Currently the InterestsRetreiever servers a static response since the API s/servers/serves/g
https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... File chrome/browser/interests/interests_retriever.h (right): https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:22: // Authentication is done using the user's OAUTH access_token. On 2015/09/03 15:35:06, Marc Treib wrote: > nit: OAuth Done. https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:26: // auto request = scoped_ptr<InterestsRetriever>(new InterestsRetriever( On 2015/09/03 15:35:06, Marc Treib wrote: > make_scoped_ptr(new ... > then you don't have to write "InterestsRetriever" twice. :) > > Mention what happens when request is deleted before callback is called? Done. https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:30: class InterestsRetriever : public net::URLFetcherDelegate { On 2015/09/03 15:35:06, Marc Treib wrote: > Hm, "Fetcher" is much more common in Chromium than "Retriever". I realize it's a > bunch of work, but can we rename this? Done. https://codereview.chromium.org/1317513004/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_retriever.h:62: // Currently the InterestsRetreiever servers a static response since the API On 2015/09/03 15:35:06, Marc Treib wrote: > s/servers/serves/g Done.
Change to using a Start() method
Can you add an OWNERS file for the new interests folder, please? Include me in it, and whoever else you think makes sense. Have you thought about moving the access token stuff into here, per the discussion on the other CL? I'd really prefer that. And a couple more nits below :) https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:13: #include "net/url_request/url_fetcher_factory.h" not needed https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:21: using net::URLFetcherFactory; not needed https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:31: const char kInterestsUrl[] = "https://www.googleapis.com/TBD/v1/interests"; Do we know the correct URL? (Even if it doesn't actually work yet) https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:122: // nit: remove empty comment line https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:17: #include "net/url_request/url_fetcher_factory.h" Not needed anymore https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:19: FORWARD_DECLARE_TEST(InterestsFetcherTest, DefaultFakeResponse); not needed anymore (test doesn't exist anymore) https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:25: // deleted only after the callback has been called. This sounds like bad things will happen if you delete the InterestsFetcher before it's done. But that's not actually the case, is it? It will basically just abort the request (by deleting the URLFetcher), which is fine. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:47: const std::string& access_token); nit: alignment https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:54: FRIEND_TEST_ALL_PREFIXES(::InterestsFetcherTest, DefaultFakeResponse); not needed anymore
https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... 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 10-05) wrote: > not needed Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:21: using net::URLFetcherFactory; On 2015/09/22 12:02:59, Marc Treib (OOO till 10-05) wrote: > not needed Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:31: const char kInterestsUrl[] = "https://www.googleapis.com/TBD/v1/interests"; On 2015/09/22 12:02:59, Marc Treib wrote: > Do we know the correct URL? (Even if it doesn't actually work yet) This should be https://www-googleapis-test.sandbox.google.com/chromenow/contextdemo https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:122: // On 2015/09/22 12:02:59, Marc Treib wrote: > nit: remove empty comment line Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:17: #include "net/url_request/url_fetcher_factory.h" On 2015/09/22 12:02:59, Marc Treib wrote: > Not needed anymore Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:19: FORWARD_DECLARE_TEST(InterestsFetcherTest, DefaultFakeResponse); On 2015/09/22 12:02:59, Marc Treib wrote: > not needed anymore (test doesn't exist anymore) Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:25: // deleted only after the callback has been called. On 2015/09/22 12:02:59, Marc Treib (OOO till 10-05) wrote: > This sounds like bad things will happen if you delete the InterestsFetcher > before it's done. But that's not actually the case, is it? It will basically > just abort the request (by deleting the URLFetcher), which is fine. Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:47: const std::string& access_token); On 2015/09/22 12:02:59, Marc Treib wrote: > nit: alignment Done. https://codereview.chromium.org/1317513004/diff/120001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:54: FRIEND_TEST_ALL_PREFIXES(::InterestsFetcherTest, DefaultFakeResponse); On 2015/09/22 12:02:59, Marc Treib wrote: > not needed anymore Done.
tache@google.com changed reviewers: + jochen@chromium.org
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
Your CL description mentions "InterestsRetriever", but the class is called InterestsFetcher. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:34: "https://www-googleapis-stagin.sandbox.google.com/chromenow/contextdemo/" stagin? Also, instead of referencing internal URLs in Chromium code, prefer passing the URL in via a command line flag. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:119: DLOG(WARNING) << "URL request failed!"; At least include the status in the log message. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:125: source->GetResponseAsString(&response_body); This doesn't seem to deal with HTTP errors, including ones thrown by an expired access token. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:141: std::vector<Interest> res; If you declare this further up, you could just always return this instead of calling EmptyResponse(). https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:26: // called else the request will be aborted. What does this comment mean? How can the request be aborted after the callback has already been run? https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:37: std::string image_url; Could this be a GURL? https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:63: void OnGetTokenSuccess(const OAuth2TokenService::Request* request, Also document which interface these methods implement. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:72: // parse the json response Capitalize the beginning of the sentence, and end it with a period.
https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... 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? Also, instead of referencing internal URLs in Chromium code, prefer > passing the URL in via a command line flag. Done. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:119: DLOG(WARNING) << "URL request failed!"; On 2015/09/29 14:01:12, Bernhard Bauer wrote: > At least include the status in the log message. Done. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:125: source->GetResponseAsString(&response_body); On 2015/09/29 14:01:12, Bernhard Bauer wrote: > This doesn't seem to deal with HTTP errors, including ones thrown by an expired > access token. Improved logging in case of HTTP errors. Also in the case of an authorization error, the access token is invalidated and the request is retried. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:141: std::vector<Interest> res; On 2015/09/29 14:01:12, Bernhard Bauer wrote: > If you declare this further up, you could just always return this instead of > calling EmptyResponse(). I also use EmptyResponse() in other methods for the return value in case of an error. I think it is better, for consistency, to explicitly call it. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:26: // called else the request will be aborted. On 2015/09/29 14:01:12, Bernhard Bauer wrote: > What does this comment mean? How can the request be aborted after the callback > has already been run? Sorry about that. Hope now it's clear. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:37: std::string image_url; On 2015/09/29 14:01:12, Bernhard Bauer wrote: > Could this be a GURL? Done. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:63: void OnGetTokenSuccess(const OAuth2TokenService::Request* request, On 2015/09/29 14:01:12, Bernhard Bauer wrote: > Also document which interface these methods implement. Done. https://codereview.chromium.org/1317513004/diff/140001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:72: // parse the json response On 2015/09/29 14:01:12, Bernhard Bauer wrote: > Capitalize the beginning of the sentence, and end it with a period. Done.
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:37: "interests/"; Remove the URL now? https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:51: if (interests_url.is_empty()) This is a no-op -- The empty GURL() constructor constructs an empty GURL. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:67: // OAuth2TokenService::Consumer implementation. This isn't necessary; just group all the methods from one interface together and add a comment before the start of the group.
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:22: using net::URLFetcher; don't use using, always use the full name https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:37: "interests/"; On 2015/09/30 at 08:14:08, Bernhard Bauer wrote: > Remove the URL now? why not just put the URL to use there? https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:52: interests_url = GURL(); and return it here - GURL(kInterestsUrl) https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:218: InterestsFetcher::~InterestsFetcher() {} dtor should come right after the ctor https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:23: // Authentication is done using the user's OAuth access_token. this comment adds almost no information. it should instead explain what interests are, and what capabilities need to be associated with the oauth token. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:36: std::string name; please add a ctor that takes parameters for all fields and a dtor https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:57: please use fewer empty lines
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:22: using net::URLFetcher; On 2015/09/30 09:23:09, jochen wrote: > don't use using, always use the full name (Just for the sake of completeness: the style guide does _not_ dictate always using fully-qualified names, but for a three-character namespace the savings are rather small.) https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:37: "interests/"; On 2015/09/30 09:23:09, jochen wrote: > On 2015/09/30 at 08:14:08, Bernhard Bauer wrote: > > Remove the URL now? > > why not just put the URL to use there? It's an internal URL that has no place in the upstream repository.
https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:22: using net::URLFetcher; On 2015/09/30 09:38:57, Bernhard Bauer wrote: > On 2015/09/30 09:23:09, jochen wrote: > > don't use using, always use the full name > > (Just for the sake of completeness: the style guide does _not_ dictate always > using fully-qualified names, but for a three-character namespace the savings are > rather small.) Done. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:37: "interests/"; On 2015/09/30 08:14:08, Bernhard Bauer wrote: > Remove the URL now? Removed the URL. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:51: if (interests_url.is_empty()) On 2015/09/30 08:14:08, Bernhard Bauer wrote: > This is a no-op -- The empty GURL() constructor constructs an empty GURL. Done. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:52: interests_url = GURL(); On 2015/09/30 09:23:09, jochen wrote: > and return it here - GURL(kInterestsUrl) kInterestsUrl was removed. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:218: InterestsFetcher::~InterestsFetcher() {} On 2015/09/30 09:23:09, jochen wrote: > dtor should come right after the ctor Done. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:23: // Authentication is done using the user's OAuth access_token. On 2015/09/30 09:23:09, jochen wrote: > this comment adds almost no information. > > it should instead explain what interests are, and what capabilities need to be > associated with the oauth token. Done. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:36: std::string name; On 2015/09/30 09:23:09, jochen wrote: > please add a ctor that takes parameters for all fields and a dtor Done. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:57: On 2015/09/30 09:23:09, jochen wrote: > please use fewer empty lines Done. https://codereview.chromium.org/1317513004/diff/180001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:67: // OAuth2TokenService::Consumer implementation. On 2015/09/30 08:14:08, Bernhard Bauer wrote: > This isn't necessary; just group all the methods from one interface together and > add a comment before the start of the group. Done.
some comments 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 please no comments of the form // Do foo DoFoo() https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:114: no empty line here https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:142: LOG(WARNING) << "HTTP error " << response_code; VLOG(2), here and everywhere https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.cc:211: InterestsFetcher::Interest::Interest(const std::string& name, i'd recommend to use the same order in the cc file as in the header 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: // please remove the example usage part of the comment. it's redundant with the code, and actually already now wrong... 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); ctor and dtor should come before data members: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... also, spelling: relevance https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:44: no empty line here https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:54: OAuth2TokenService* oauth2_token_service, 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()? https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:60: static scoped_ptr<InterestsFetcher> CreateFromProfile(Profile* profile); can you have multiple fetchers per profile? Should this be a profile-keyed-service? should the ctor be private? https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:62: void Start(const InterestsCallback& callback); this could use a comment what it starts (or a better name - StartFetching) https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:72: no empty line here https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:77: no empty line here https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:79: nor here https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:87: scoped_ptr<net::URLFetcher> fetcher_; sooo many empty lines here. https://codereview.chromium.org/1317513004/diff/200001/chrome/browser/interes... chrome/browser/interests/interests_fetcher.h:101: only one empty line here
peconn@chromium.org changed reviewers: + peconn@chromium.org
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
