Introduce a Doodle Fetcher for NTP

components/doodle/doodle_fetcher.cc

Index: components/doodle/doodle_fetcher.cc
diff --git a/components/doodle/doodle_fetcher.cc b/components/doodle/doodle_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..bd7e2b2cc591581cf7b17192c610d418001a08f9
--- /dev/null
+++ b/components/doodle/doodle_fetcher.cc
@@ -0,0 +1,222 @@
+// Copyright 2017 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 "components/doodle/doodle_fetcher.h"
+
+#include <utility>
+
+#include "base/strings/string_number_conversions.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
+#include "base/time/time.h"
+#include "base/values.h"
+#include "components/google/core/browser/google_url_tracker.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "url/gurl.h"
+
+using net::URLFetcher;
+
+namespace doodle {
+
+const char kDoodleConfigPath[] = "/async/ddljson";
+
+namespace {
+
+std::string StripSafetyPreamble(const std::string& json) {
+  // The response may start with )]}'. Ignore this.
+  const char kResponsePreamble[] = ")]}'";
+
+  base::StringPiece json_sp(json);
+  if (json_sp.starts_with(kResponsePreamble)) {
+    json_sp.remove_prefix(strlen(kResponsePreamble));
+  }
+
+  return json_sp.as_string();
+}
+
+DoodleType ParseDoodleType(const base::DictionaryValue& ddljson) {
+  std::string type_str;
+  ddljson.GetString("doodle_type", &type_str);
+  if (type_str == "SIMPLE") {
+    return DoodleType::SIMPLE;
+  }
+  if (type_str == "RANDOM") {
+    return DoodleType::RANDOM;
+  }
+  if (type_str == "VIDEO") {
+    return DoodleType::VIDEO;
+  }
+  if (type_str == "INTERACTIVE") {
+    return DoodleType::INTERACTIVE;
+  }
+  if (type_str == "INLINE_INTERACTIVE") {
+    return DoodleType::INLINE_INTERACTIVE;
+  }
+  if (type_str == "SLIDESHOW") {
+    return DoodleType::SLIDESHOW;
+  }
+  return DoodleType::UNKNOWN;
+}
+
+base::Optional<DoodleConfig> NoConfig() {

[Marc Treib, 2017/02/01 11:53:14]

I think you can just write base::nullopt instead of this.

[fhorschig, 2017/02/03 13:21:04]

Nice.

+  return base::Optional<DoodleConfig>();
+}
+
+}  // namespace
+
+DoodleImage::DoodleImage()
+    : height(0), width(0), is_animated_gif(false), is_cta(false) {}
+DoodleImage::~DoodleImage() = default;
+
+DoodleConfig::DoodleConfig()
+    : doodle_type(DoodleType::UNKNOWN), expiry_date(base::Time::Now()) {}

[Marc Treib, 2017/02/01 11:53:13]

nit: I'd leave expiry_date without explicit initialization. Then it'll have a
special "zero" value, which makes it easier to see that it wasn't properly
initialized.

[fhorschig, 2017/02/03 13:21:04]

Done.

+DoodleConfig::DoodleConfig(const DoodleConfig& config) = default;
+DoodleConfig::~DoodleConfig() = default;
+
+DoodleFetcher::DoodleFetcher(net::URLRequestContextGetter* download_context,
+                             GoogleURLTracker* google_url_tracker,
+                             ParseJSONCallback json_parsing_callback)
+    : download_context_(download_context),
+      json_parsing_callback_(json_parsing_callback),
+      google_url_tracker_(google_url_tracker),
+      clock_(new base::DefaultClock()),
+      weak_ptr_factory_(this) {}
+DoodleFetcher::~DoodleFetcher() {}

[Marc Treib, 2017/02/01 11:53:14]

nit: empty line before

[fhorschig, 2017/02/03 13:21:04]

Done.

+
+void DoodleFetcher::FetchDoodle(FinishedCallback callback) {
+  callbacks_.push_back(std::move(callback));
+  if (callbacks_.size() > 1) {
+    return;  // With 2 or more callbacks, a fetcher is already running.

[Marc Treib, 2017/02/01 11:53:14]

nit: I'd formulate this as "If there already was a callback, ..."
(Took me a few seconds to understand why "2 or more")

[fhorschig, 2017/02/03 13:21:04]

Done.

+  }
+  fetcher_ = URLFetcher::Create(GetGoogleBaseUrl().Resolve(kDoodleConfigPath),

[Marc Treib, 2017/02/01 11:53:13]

Should we DCHECK here that fetcher_ was null?

[fhorschig, 2017/02/03 13:21:04]

Okay.

+                                URLFetcher::GET, this);
+  fetcher_->SetRequestContext(download_context_);
+  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                         net::LOAD_DO_NOT_SAVE_COOKIES);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
+  fetcher_->Start();
+}
+
+// net::URLFetcherDelegate implementation.
+void DoodleFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_EQ(fetcher_.get(), source);
+  std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_);
+
+  std::string json_string;
+  if (!(source->GetStatus().is_success() &&
+        source->GetResponseCode() == net::HTTP_OK &&
+        source->GetResponseAsString(&json_string))) {
+    RespondToAllCallbacks(DoodleState::DOWNLOAD_ERROR, NoConfig());
+    return;
+  }
+
+  json_parsing_callback_.Run(
+      StripSafetyPreamble(std::move(json_string)),
+      base::Bind(&DoodleFetcher::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&DoodleFetcher::OnJsonParseFailed,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DoodleFetcher::OnJsonParsed(std::unique_ptr<base::Value> json) {
+  std::unique_ptr<base::DictionaryValue> dict =
+      base::DictionaryValue::From(std::move(json));
+  if (!dict.get()) {
+    LOG(ERROR) << "Doodle JSON is not valid";
+    RespondToAllCallbacks(DoodleState::PARSING_ERROR, NoConfig());
+    return;
+  }
+
+  ParseDoodle(std::move(dict));
+}
+
+void DoodleFetcher::OnJsonParseFailed(const std::string& error_message) {
+  LOG(ERROR) << "JSON parsing failed: " << error_message;
+  RespondToAllCallbacks(DoodleState::PARSING_ERROR, NoConfig());
+}
+
+void DoodleFetcher::ParseDoodle(std::unique_ptr<base::DictionaryValue> config) {
+  const base::DictionaryValue* ddljson = nullptr;
+  if (!config->GetDictionary("ddljson", &ddljson)) {
+    LOG(ERROR) << "Doodle JSON reponse did not contain 'ddljson' key.";
+    RespondToAllCallbacks(DoodleState::PARSING_ERROR, NoConfig());
+    return;
+  }
+
+  DoodleConfig result;
+  ParseBaseInformation(*ddljson, &result);
+
+  if (!ParseImage(*ddljson, "large_image", &result.large_image)) {
+    RespondToAllCallbacks(DoodleState::NO_DOODLE, NoConfig());
+    return;
+  }
+  ParseImage(*ddljson, "transparent_large_image",
+             &result.transparent_large_image);
+  ParseImage(*ddljson, "large_cta_image", &result.large_cta_image);
+
+  RespondToAllCallbacks(DoodleState::AVAILABLE, std::move(result));
+}
+
+bool DoodleFetcher::ParseImage(const base::DictionaryValue& image_parent,
+                               const std::string& image_name,
+                               DoodleImage* image) const {
+  DCHECK(image);
+  const base::DictionaryValue* image_dict = nullptr;
+  if (!(image && image_parent.GetDictionary(image_name, &image_dict))) {

[Marc Treib, 2017/02/01 11:53:13]

You have DCHECKed image before, no reason to null-check again

[fhorschig, 2017/02/03 13:21:04]

True.

+    return false;
+  }
+  image->url = ParseRelativeUrl(*image_dict, "url");

[Marc Treib, 2017/02/01 11:53:14]

Should this return false if the url is missing or invalid?

[fhorschig, 2017/02/03 13:21:04]

Done.

+  image_dict->GetInteger("height", &image->height);
+  image_dict->GetInteger("width", &image->width);
+  image_dict->GetBoolean("is_animated_gif", &image->is_animated_gif);
+  image_dict->GetBoolean("is_cta", &image->is_cta);
+  return true;
+}
+
+void DoodleFetcher::ParseBaseInformation(const base::DictionaryValue& ddljson,
+                                         DoodleConfig* config) const {
+  config->search_url = ParseRelativeUrl(ddljson, "search_url");
+  config->target_url = ParseRelativeUrl(ddljson, "target_url");
+  config->fullpage_interactive_url =
+      ParseRelativeUrl(ddljson, "fullpage_interactive_url");
+
+  config->doodle_type = ParseDoodleType(ddljson);
+  ddljson.GetString("alt_text", &config->alt_text);
+
+  // TODO(fhorschig): Inject TickClock and test that.
+  // The JSON doesn't garantuee the number to fit into an int.

[Marc Treib, 2017/02/01 11:53:14]

s/garantuee/guarantee/

[fhorschig, 2017/02/03 13:21:04]

Thank you

+  double ttl;

[Marc Treib, 2017/02/01 11:53:13]

initialize, and/or check if parsing actually worked?

[fhorschig, 2017/02/03 13:21:04]

Done.

+  ddljson.GetDouble("time_to_live_ms", &ttl);
+  config->expiry_date = clock_->Now() + base::TimeDelta::FromMillisecondsD(ttl);
+}
+
+GURL DoodleFetcher::ParseRelativeUrl(const base::DictionaryValue& dict_value,
+                                     const std::string& key) const {
+  std::string str_url;
+  dict_value.GetString(key, &str_url);
+  if (str_url.empty()) {
+    return GURL();
+  }
+  return GetGoogleBaseUrl().Resolve(str_url);
+}
+
+void DoodleFetcher::RespondToAllCallbacks(DoodleState state,
+                                          base::Optional<DoodleConfig> config) {
+  while (!callbacks_.empty()) {
+    std::move(callbacks_.front()).Run(state, std::move(config));
+    callbacks_.pop_front();
+  }
+}
+
+GURL DoodleFetcher::GetGoogleBaseUrl() const {
+  // There might be no tracker without profile. The factory returns nullptr in
+  // tests, too.

[Marc Treib, 2017/02/01 11:53:14]

I think this class should assume that the tracker always exists. It should never
be null in live code, and tests should inject a fake.

[fhorschig, 2017/02/03 13:21:04]

Yes, it's a TODO I have in tests.
Adding it here, too.

Although I am a little suspicious about every KeyedService 
that relies on profiles. 
We have some cases where they just become nullptr when not properly
unregistered. (Do you remember the crashes in NTPSnippetsBridge?)

[Marc Treib, 2017/02/03 15:45:48]

I'm not sure I follow. They don't "become" nullptr; once initialized, they stay
around until the profile is destroyed.
It's true that they can be null in some cases; incognito is a common example,
though I'm not sure if that applies here.
Either way, we should decide whether this can be null or not, and document the
decision (with DCHECK and/or comments).

(FWIW, the crashes in the bridge (or at least some of them, namely the
hard-to-track-down ones) weren't caused by a null pointer, but rather by an
*uninitialized" pointer. So null checks didn't help :D)

[fhorschig, 2017/02/06 11:48:22]

Thank you for the clarification.
(As discussed offline, we use the nullptr for integration tests using
the tracker factory, which will then return a nullptr.)

+  if (google_url_tracker_) {
+    return google_url_tracker_->google_url();

[Marc Treib, 2017/02/01 15:22:02]

Turns out the tracker doesn't respect the command line param to override the
Google base URL :-/

So we'll have to do something like this first:

  GURL url = google_util::CommandLineGoogleBaseURL();
  if (url.is_valid()) {
    return url;
  }

[fhorschig, 2017/02/03 13:21:04]

Done.

+  }
+  return GURL(GoogleURLTracker::kDefaultGoogleHomepage);
+}
+
+}  // namespace doodle