Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1384973002:
Add InterestsFetcher which retrieves a user's interests from the server. (Closed)
DescriptionAdd InterestsFetcher which retrieves a list of topics the user is interested in according to Google Now.
BUG=537145
Previous CL:https://codereview.chromium.org/1317513004
Committed: https://crrev.com/d182b99e34694a0a4beb71068b30460bfbdea4ed
Cr-Commit-Position: refs/heads/master@{#352607}
Patch Set 1 #
Total comments: 30
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 25 (6 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, jochen@chromium.org, treib@chromium.org
I am taking over https://codereview.chromium.org/1317513004.
https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.cc:42: command_line->GetSwitchValueASCII(switches::kInterestsURL)); You can inline the constructor (`return GURL(...);`) https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.cc:49: const GURL& image_url, float relevance) The items in argument lists go either all on the same line, or each on a separate line. (Tip: run `git cl format`, which should fix things like that.) https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.cc:177: std::vector<Interest> res; If you move this to the beginning, you can just return res below instead of EmptyResponse() (which you could then remove). https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:60: private: Empty line before private section. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher_unittest.cc (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:29: namespace { Nit: empty line afterwards. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:33: "\"interests\": []\n" Alignment. Also, I would break immediately after the equals sign, so all the lines are aligned. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:58: InterestsFetcher::Interest{"Google", GURL("https://fake.com/fake.png"), 0.9}); This appears to be over 80 columns. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:170: { I don' think this scope does anything, as you are not declaring any variables inside of it. I would remove it and just separate the parts visually (empty line should be fine). https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:541: 'browser/interests/interests_fetcher.h', Ordering: interests comes before internal.
https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/OW... File chrome/browser/interests/OWNERS (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/OW... chrome/browser/interests/OWNERS:1: treib@chromium.org Feel free to also add yourself here! https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:44: double relevance; nit: The ctor param is a float, but here it's a double. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:47: typedef base::Callback<void(const std::vector<Interest>&)> nit: I find "using InterestsCallback = ..." a bit clearer (up to you though). https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:59: void FetchInterests(const InterestsCallback& callback); Add a comment saying it's illegal to call this more than once? https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher_unittest.cc (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:81: protected: nit: Empty line before protected. https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:541: 'browser/interests/interests_fetcher.h', On 2015/10/06 08:56:10, Bernhard Bauer wrote: > Ordering: interests comes before internal. And also .cc comes before .h :)
https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/OW... File chrome/browser/interests/OWNERS (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/OW... chrome/browser/interests/OWNERS:1: treib@chromium.org On 2015/10/06 09:35:47, Marc Treib wrote: > Feel free to also add yourself here! Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.cc:42: command_line->GetSwitchValueASCII(switches::kInterestsURL)); On 2015/10/06 08:56:10, Bernhard Bauer wrote: > You can inline the constructor (`return GURL(...);`) Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.cc:49: const GURL& image_url, float relevance) On 2015/10/06 08:56:10, Bernhard Bauer wrote: > The items in argument lists go either all on the same line, or each on a > separate line. > > (Tip: run `git cl format`, which should fix things like that.) Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.cc:177: std::vector<Interest> res; On 2015/10/06 08:56:10, Bernhard Bauer wrote: > If you move this to the beginning, you can just return res below instead of > EmptyResponse() (which you could then remove). I think EmptyResponse makes things a bit more readable, however I've moved parts around to make things a bit more logically grouped. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:44: double relevance; On 2015/10/06 09:35:47, Marc Treib wrote: > nit: The ctor param is a float, but here it's a double. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:47: typedef base::Callback<void(const std::vector<Interest>&)> On 2015/10/06 09:35:47, Marc Treib wrote: > nit: I find "using InterestsCallback = ..." a bit clearer (up to you though). Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:59: void FetchInterests(const InterestsCallback& callback); On 2015/10/06 09:35:47, Marc Treib wrote: > Add a comment saying it's illegal to call this more than once? Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher.h:60: private: On 2015/10/06 08:56:10, Bernhard Bauer wrote: > Empty line before private section. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... File chrome/browser/interests/interests_fetcher_unittest.cc (right): https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:29: namespace { On 2015/10/06 08:56:10, Bernhard Bauer wrote: > Nit: empty line afterwards. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:33: "\"interests\": []\n" On 2015/10/06 08:56:10, Bernhard Bauer wrote: > Alignment. Also, I would break immediately after the equals sign, so all the > lines are aligned. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:58: InterestsFetcher::Interest{"Google", GURL("https://fake.com/fake.png"), 0.9}); On 2015/10/06 08:56:10, Bernhard Bauer wrote: > This appears to be over 80 columns. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:81: protected: On 2015/10/06 09:35:47, Marc Treib wrote: > nit: Empty line before protected. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/browser/interests/in... chrome/browser/interests/interests_fetcher_unittest.cc:170: { On 2015/10/06 08:56:10, Bernhard Bauer wrote: > I don' think this scope does anything, as you are not declaring any variables > inside of it. I would remove it and just separate the parts visually (empty line > should be fine). Done. https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:541: 'browser/interests/interests_fetcher.h', On 2015/10/06 08:56:10, Bernhard Bauer wrote: > Ordering: interests comes before internal. Done. https://codereview.chromium.org/1384973002/diff/1/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:541: 'browser/interests/interests_fetcher.h', On 2015/10/06 09:35:47, Marc Treib wrote: > On 2015/10/06 08:56:10, Bernhard Bauer wrote: > > Ordering: interests comes before internal. > > And also .cc comes before .h :) Done.
@treib - What did you mean when you said that FetchInterests should only be called once? I thought it could be called multiple times provided the callback has returned before calling again. https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:103: access_token_expired_ = true; Should this be reset to false on FetchInterests?
On 2015/10/06 10:56:27, PEConn1 wrote: > @treib - What did you mean when you said that FetchInterests should only be > called once? > > I thought it could be called multiple times provided the callback has returned > before calling again. You DCHECK(callback_.is_null()) in FetchInterests, and (as far as I can see) never reset the callback. > https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... > File chrome/browser/interests/interests_fetcher.cc (right): > > https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... > chrome/browser/interests/interests_fetcher.cc:103: access_token_expired_ = true; > Should this be reset to false on FetchInterests? If you want to support multiple FetchInterests calls, then probably yes.
On 2015/10/06 10:56:27, PEConn1 wrote: > @treib - What did you mean when you said that FetchInterests should only be > called once? > > I thought it could be called multiple times provided the callback has returned > before calling again. As it is right now, calling FetchInterests() a second time will DCHECK because the callback is not null. We could change that of course, but that would require the client to keep track of the state, so keeping this class single-use and deleting it afterwards seems slightly easier. https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:103: access_token_expired_ = true; On 2015/10/06 10:56:27, PEConn1 wrote: > Should this be reset to false on FetchInterests? Again, if this class is single-use only, it doesn't matter; otherwise, yes we'd want to reset it, but maybe right after we have verified that the token is in fact not expired, i.e. further down in this method.
Changed comment about class being single use. https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/20001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:103: access_token_expired_ = true; On 2015/10/06 11:07:55, Bernhard Bauer wrote: > On 2015/10/06 10:56:27, PEConn1 wrote: > > Should this be reset to false on FetchInterests? > > Again, if this class is single-use only, it doesn't matter; otherwise, yes we'd > want to reset it, but maybe right after we have verified that the token is in > fact not expired, i.e. further down in this method. True, we don't need to reset it then.
lgtm https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:105: ->InvalidateAccessToken(account_id_, GetApiScopes(), access_token_); Did "git cl format" do this? Ugh. Oh well. https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:192: double relevance; nit: Initialize?
Ah, maybe the description could use some cleanup. What are "a user's interests"? What is "the server"? Also the "only available from corp" is redundant now that the URL is no longer in there.
LGTM with my and Marc's remaining comments addressed. On 2015/10/06 13:39:57, Marc Treib wrote: > Ah, maybe the description could use some cleanup. What are "a user's interests"? > What is "the server"? Also the "only available from corp" is redundant now that > the URL is no longer in there. Yeah, I would just mention that the server is not publicly available yet. https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.h:57: // This should only be called once per instance. Actually, I would move this to the top of the file, as this is kind of important.
https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:105: ->InvalidateAccessToken(account_id_, GetApiScopes(), access_token_); On 2015/10/06 13:38:32, Marc Treib wrote: > Did "git cl format" do this? Ugh. Oh well. Done. https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:192: double relevance; On 2015/10/06 13:38:32, Marc Treib wrote: > nit: Initialize? Done. https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.h (right): https://codereview.chromium.org/1384973002/diff/40001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.h:57: // This should only be called once per instance. On 2015/10/06 13:44:12, Bernhard Bauer wrote: > Actually, I would move this to the top of the file, as this is kind of > important. Done.
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/1384973002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384973002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384973002/60001
I meant updating the CL description :) But nevermind. https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:193: double relevance{}; Wait, this is technically C++11 uniform initialization, right? We're not allowed to use that yet :(
https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:193: double relevance{}; Nit (possibly for a follow-up CL): Please use just a numeric value. An empty initializer list doesn't really make things clearer here.
The CQ bit was unchecked by peconn@chromium.org
https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... File chrome/browser/interests/interests_fetcher.cc (right): https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:193: double relevance{}; On 2015/10/06 14:28:00, Marc Treib wrote: > Wait, this is technically C++11 uniform initialization, right? We're not allowed > to use that yet :( Done. https://codereview.chromium.org/1384973002/diff/60001/chrome/browser/interest... chrome/browser/interests/interests_fetcher.cc:193: double relevance{}; On 2015/10/06 14:30:09, Bernhard Bauer wrote: > Nit (possibly for a follow-up CL): Please use just a numeric value. An empty > initializer list doesn't really make things clearer here. Done.
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/1384973002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1384973002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1384973002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d182b99e34694a0a4beb71068b30460bfbdea4ed Cr-Commit-Position: refs/heads/master@{#352607} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
