Add InterestsFetcher which retrieves a user's interests from the server.

chrome/browser/interests/interests_retriever.h

+// Copyright 2014 The Chromium Authors. All rights reserved.

[Marc Treib, 2015/09/03 09:04:37]

WHAT YEAR IS THIS?! :D

[tache, 2015/09/03 13:26:29]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_INTERESTS_INTERESTS_RETRIEVER_H_
+#define CHROME_BROWSER_INTERESTS_INTERESTS_RETRIEVER_H_
+
+#include <string>
+#include <vector>
+
+#include <base/callback.h>

[Marc Treib, 2015/09/03 09:04:37]

Quotes instead of angle brackets for non-stdlib includes. Also in the other
files.

[tache, 2015/09/03 13:26:29]

Eclipse "organize imports" is so broken...

Done.

+#include <base/gtest_prod_util.h>
+#include <base/macros.h>
+#include <base/memory/scoped_ptr.h>
+#include <net/url_request/url_fetcher.h>
+#include <net/url_request/url_fetcher_delegate.h>
+#include <net/url_request/url_fetcher_factory.h>
+
+namespace net {
+class URLFetcher;

[Marc Treib, 2015/09/03 09:04:37]

Either include or forward declare, not both.

[tache, 2015/09/03 13:26:29]

Done.

+class URLRequestContextGetter;
+}  // namespace net
+
+FORWARD_DECLARE_TEST(InterestsRetrieverTest, DefaultFakeResponse);

[Marc Treib, 2015/09/03 09:04:37]

Is this required? I don't think I've ever seen this before.

[tache, 2015/09/03 13:26:29]

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.

[Marc Treib, 2015/09/03 13:54:55]

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.

+
+class InterestsRetriever : public net::URLFetcherDelegate {
+ public:
+  struct Interest {
+    std::string name;
+    std::string image_url;
+    double relevance;
+
+    bool operator==(const Interest &interest) const {

[Marc Treib, 2015/09/03 09:04:37]

Do you actually need this operator? If so, please move the implementation to the
.cc file, otherwise just remove it.

[tache, 2015/09/03 13:26:29]

Only used for testing atm.

Moved it to the .cc file

[Marc Treib, 2015/09/03 13:54:55]

Then you could also move it to the test file (as a non-member). I'm also okay
with leaving it here though.

+      return name == interest.name && image_url == interest.image_url &&
+             relevance == interest.relevance;
+    }
+  };
+
+  typedef base::Callback<void(std::vector<Interest, std::allocator<Interest>>)>

[Marc Treib, 2015/09/03 09:04:37]

Remove the explicit allocator?
Also, I find
using InterestsCallback = ...
slightly nicer, but up to you.

[tache, 2015/09/03 13:26:29]

Done.

+      InterestsCallback;
+
+  InterestsRetriever(net::URLRequestContextGetter *url_request_context,

[Marc Treib, 2015/09/03 09:04:37]

nit: * goes with the type, not the name. Also the &s below, and in a bunch of
other places.

[tache, 2015/09/03 13:26:28]

Done.

+                     const std::string &token,

[Marc Treib, 2015/09/03 09:04:37]

What exactly is |token|? An OAuth access token? Then maybe call it
|access_token| (there's also refresh tokens!)

[tache, 2015/09/03 13:26:29]

Done.

+                     const InterestsCallback &callback,
+                     net::URLFetcherFactory *url_fetcher_factory);
+
+  ~InterestsRetriever() override;
+
+ private:
+  // net::URLFetcherDelegate implementation.
+  void OnURLFetchComplete(const net::URLFetcher *source) override;
+
+  // create URLFetcher

[Marc Treib, 2015/09/03 09:04:37]

Not a very useful comment ;P

[tache, 2015/09/03 13:26:29]

Done.

+  scoped_ptr<net::URLFetcher> CreateFetcher();
+
+  // parse the json response
+  std::vector<Interest> ExtractInterests(const std::string &response);
+
+  static std::string FakeResponse();

[Marc Treib, 2015/09/03 09:04:37]

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.

[tache, 2015/09/03 13:26:29]

Done.

+
+  net::URLRequestContextGetter *url_request_context_;
+
+  std::string token_;
+
+  InterestsCallback callback_;
+
+  scoped_ptr<net::URLFetcher> fetcher_;
+
+  net::URLFetcherFactory *url_fetcher_factory_;
+
+
+  DISALLOW_COPY_AND_ASSIGN(InterestsRetriever);
+  FRIEND_TEST_ALL_PREFIXES(::InterestsRetrieverTest, DefaultFakeResponse);

[Marc Treib, 2015/09/03 09:04:37]

By convention, the FRIEND_TEST goes to the top of the private section.

[tache, 2015/09/03 13:26:29]

Done.

+};
+
+#endif  // CHROME_BROWSER_INTERESTS_INTERESTS_RETRIEVER_H_