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

chrome/browser/interests/interests_retriever.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <chrome/browser/interests/interests_retriever.h>
+
+#include <base/json/json_reader.h>
+#include <base/logging.h>
+#include <base/values.h>
+#include <net/base/load_flags.h>
+#include <net/http/http_status_code.h>
+#include <net/url_request/test_url_fetcher_factory.h>

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

Unneeded include

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

This is needed for FakeURLFetcher

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

Ah I see, sorry.

+#include <net/url_request/url_fetcher.h>
+#include <net/url_request/url_fetcher_factory.h>
+#include <net/url_request/url_request_context_getter.h>
+#include <net/url_request/url_request_status.h>
+
+using net::FakeURLFetcher;
+using net::URLFetcher;
+using net::URLFetcherDelegate;
+using net::URLRequestContextGetter;
+using net::URLRequestStatus;
+using net::URLFetcherFactory;
+
+namespace {
+
+const int kNumRetries = 1;
+const char kIdInterests[] = "interest";
+const char kIdInterestName[] = "name";
+const char kIdInterestImageUrl[] = "imageUrl";
+const char kIdInterestRelevance[] = "relevance";
+
+const char kInterestsUrl[] = "https://www.googleapis.com/TBD/v1/interests";
+
+const char kFakeResponse[] =
+    "{"
+    "  \"interest\": ["
+    "    {\n"
+    "      \"name\": \"Munich\",\n"
+    "      \"imageUrl\": "
+    "\"https://usercontent.googleapis.com/freebase/v1/image/m/"
+    "02h6_6p?maxwidth=200&maxheight=200&mode=fill\","
+    "      \"relevance\": 0.85"
+    "    },"
+    "    {"
+    "      \"name\": \"Defense of the Ancients\","
+    "      \"imageUrl\": "
+    "\"https://usercontent.googleapis.com/freebase/v1/image/m/"
+    "073pnt?maxwidth=200&maxheight=200&mode=fill\","
+    "      \"relevance\": 1"
+    "    },"
+    "    {"
+    "      \"name\": \"Google\","
+    "      \"imageUrl\": "
+    "\"https://usercontent.googleapis.com/freebase/v1/image/m/"
+    "045c7b?maxwidth=200&maxheight=200&mode=fill&A\","
+    "      \"relevance\": 0.9"
+    "    },"
+    "    {"
+    "      \"name\": \"Google Chrome\","
+    "      \"imageUrl\": "
+    "\"https://usercontent.googleapis.com/freebase/v1/image/m/"
+    "04j7cyf?maxwidth=200&maxheight=200&mode=fill\","
+    "      \"relevance\": 0.98"
+    "    }"
+    "  ]"
+    "}"
+    "";
+
+std::vector<InterestsRetriever::Interest> EmptyResponse() {
+  return std::vector<InterestsRetriever::Interest>();
+}
+
+}  // namespace
+
+InterestsRetriever::InterestsRetriever(
+    net::URLRequestContextGetter* url_request_context, const std::string& token,

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

Each param goes on its own line.

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

Done.

+    const InterestsRetriever::InterestsCallback& callback,
+    URLFetcherFactory* url_fetcher_factory = NULL)

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

remove "= NULL"

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

Done.

+    : url_request_context_(url_request_context),
+      token_(token),
+      callback_(callback),
+      url_fetcher_factory_(url_fetcher_factory) {
+  fetcher_ = CreateFetcher();
+
+  // Setup fetcher
+  fetcher_->SetRequestContext(url_request_context_);
+  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                         net::LOAD_DO_NOT_SAVE_COOKIES);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(kNumRetries);
+
+  // Add oauth token
+  fetcher_->AddExtraRequestHeader(std::string("Authorization: Bearer ") +
+                                  token_);
+
+  fetcher_->Start();
+}
+
+scoped_ptr<URLFetcher> InterestsRetriever::CreateFetcher() {
+  // The API is not accessible yet. So a static response is provided.
+  // return URLFetcher::Create(GURL(kInterestsUrl), URLFetcher::GET, this);
+
+  // If a Factory is provided use it. This is needed for testing.
+  if (url_fetcher_factory_) {

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

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?

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

Yes, this is only needed because of the fake response. Once the API is ready,
the code in the comments will be used.

+    return url_fetcher_factory_->CreateURLFetcher(0, GURL(kInterestsUrl),
+                                                  URLFetcher::GET, this);
+
+  } else {
+    return scoped_ptr<URLFetcher>(new FakeURLFetcher(
+        GURL(kInterestsUrl), this, std::string(kFakeResponse), net::HTTP_OK,
+        net::URLRequestStatus::SUCCESS));
+  }
+}
+
+void InterestsRetriever::OnURLFetchComplete(const net::URLFetcher* source) {
+  const URLRequestStatus& status = source->GetStatus();
+
+  std::string response_body;
+  source->GetResponseAsString(&response_body);

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

Move these two lines after the success check?

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

Done.

+
+  if (!status.is_success()) {
+    DLOG(WARNING) << "URL request failed!";
+    callback_.Run(EmptyResponse());
+    return;
+  }
+
+  auto interests = ExtractInterests(response_body);

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

Inline the ExtractInterests call into the line below?

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

Done.

+  callback_.Run(interests);
+}
+
+std::vector<InterestsRetriever::Interest> InterestsRetriever::ExtractInterests(
+    const std::string& response) {
+  scoped_ptr<base::Value> value = base::JSONReader::Read(response);
+
+  const base::DictionaryValue* dict = NULL;

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

nullptr :)

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

Done.

+  if (!value || !value->GetAsDictionary(&dict)) {
+    DLOG(WARNING) << "ExtractInterests failed to parse global dictionary";
+    return EmptyResponse();
+  }
+
+  const base::ListValue* interests_list = NULL;
+  std::vector<Interest> res;
+
+  if (!dict->GetList(kIdInterests, &interests_list)) {
+    DLOG(WARNING) << "ExtractInterests failed to parse interests list";
+    return EmptyResponse();
+  }
+
+  for (const base::Value* entry : *interests_list) {
+    const base::DictionaryValue* interest_dict = NULL;
+    if (!entry->GetAsDictionary(&interest_dict)) {
+      DLOG(WARNING) << "ExtractInterests failed to parse interest dictionary";
+      return EmptyResponse();
+    }
+
+    std::string name;
+    if (!interest_dict->GetString(kIdInterestName, &name)) {
+      DLOG(WARNING) << "ExtractInterests failed to parse interest name";
+      return EmptyResponse();
+    }
+
+    std::string image_url;
+    if (!interest_dict->GetString(kIdInterestImageUrl, &image_url)) {

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

I think it's okay if the image URL is missing? So maybe we should not return an
empty response in this case?

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

Done. I kept the warning, since atm there should always be an image url in the
response from the server.

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

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.

[tache, 2015/09/03 15:21:52]

Done.

+      DLOG(WARNING) << "ExtractInterests failed to parse interest image URL";
+      return EmptyResponse();
+    }
+
+    double relevance;
+    if (!interest_dict->GetDouble(kIdInterestRelevance, &relevance)) {
+      DLOG(WARNING) << "ExtractInterests failed to parse interest relevance";
+      return EmptyResponse();
+    }
+
+    res.push_back(Interest{name, image_url, relevance});

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

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.)

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

Done. Leaving it as is.

+  }
+
+  return res;
+}
+
+InterestsRetriever::~InterestsRetriever() {}
+
+std::string InterestsRetriever::FakeResponse() { return kFakeResponse; }