Introduce a Doodle Fetcher for NTP

components/doodle/doodle_fetcher.h

+// 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.
+
+#ifndef COMPONENTS_DOODLE_DOODLE_FETCHER_H_
+#define COMPONENTS_DOODLE_DOODLE_FETCHER_H_
+
+#include <memory>
+#include <string>
+#include <utility>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "url/gurl.h"
+
+namespace base {
+class Clock;
+class DictionaryValue;
+class Value;
+}
+
+namespace net {
+class URLRequestContextGetter;
+}
+
+namespace doodle {
+
+enum class DoodleState {
+  AVAILABLE,
+  NO_DOODLE,
+  DOWNLOAD_ERROR,
+  PARSING_ERROR,
+};
+
+enum class DoodleType {
+  SIMPLE,
+  RANDOM,
+  VIDEO,
+  INTERACTIVE,
+  INLINE_INTERACTIVE,
+  SLIDESHOW,
+};
+
+// Information about a Doodle image. If the image is invalid, the |url| will be
+// empty and invalid. By default, the background color is #ffffff and dimensions
+// are 0.
+struct DoodleImage {
+  DoodleImage();
+  ~DoodleImage();

[Marc Treib, 2017/01/31 14:46:38]

nit: ctors first, then dtor

[fhorschig, 2017/02/01 11:16:13]

Done.

+  explicit DoodleImage(GURL url);
+  DoodleImage(const DoodleImage& config);
+
+  GURL url;
+  int height;
+  int width;
+  std::string background_color;

[Marc Treib, 2017/01/31 14:46:38]

If we actually need this, we should find a proper Color type (but I don't think
we need it).

[fhorschig, 2017/02/01 11:16:13]

Removed for now. Are you sure about that?
I guess the only use case where it matters is on desktop.
We use HTML/CSS on dektop which needs exactly those strings.

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

Does the desktop NTP actually use the background color value?
Let's figure this one out once we actually work on desktop.

+  bool is_animated_gif;
+  bool is_cta;

[Marc Treib, 2017/01/31 14:46:39]

Hm, what exactly do these two mean? Could they both be true?

[fhorschig, 2017/02/01 11:16:13]

They can but for our sample doodle, they never are. 

is_animated_gif states whether the file is a *.gif

is_cta states "Whether this is a call to action. This 
may mean that there is a play button baked in to the 
image." [1] ... so i guess it's for videos?

[1] There are several sources of truth on cs/is_cta

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

Okay, I'll try to figure it out with the doodle folks.
Eventually, we should add a comment here that explains what exactly these fields
mean.

+};
+
+// All information about a current doodle that can be fetched from the remote
+// end. If no information could be fetched, the reason is stored in |state|.
+// By default, all URLs are empty and therefore invalid.
+struct DoodleConfig {
+  DoodleConfig() = delete;
+  ~DoodleConfig();

[Marc Treib, 2017/01/31 14:46:38]

Also here: dtor after ctors

[fhorschig, 2017/02/01 11:16:13]

Done.

+  explicit DoodleConfig(DoodleState doodle_state);
+  DoodleConfig(const DoodleConfig& config);

[Marc Treib, 2017/01/31 14:46:39]

Is this required? Won't the default ctor actually do the right thing here? If
so, remove this and just add a comment
// Copying and assignment allowed.

[fhorschig, 2017/02/01 11:16:13]

Added that comment to DoodleImage.
It's needed for the DoodleConfig, otherwise, it fails with:
[chromium-style] Complex class/struct needs an explicit out-of-line copy
constructor.

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

Ah, I see. Okay then.

+
+  DoodleState state;

[Marc Treib, 2017/01/31 14:46:38]

Hm, so if state isn't AVAILABLE, then all the other fields are meaningless?
Should this be outside the config?

[fhorschig, 2017/02/01 11:16:13]

It's now part of the Callback.

+  DoodleType doodle_type;
+  std::string alt_text;
+
+  base::Time expiry_date;
+  GURL search_url;
+  GURL target_url;
+  GURL fullpage_interactive_url;
+
+  DoodleImage small_image;
+  DoodleImage medium_image;
+  DoodleImage hires_image;
+  DoodleImage large_image;
+  DoodleImage transparent_large_image;
+};
+
+// This class provides information about any recent doodle, based on the
+// base_url of a user's account.

[Marc Treib, 2017/01/31 14:46:39]

base_url isn't really a thing, it's google_base_url. But I don't think that
needs to be mentioned here.

[fhorschig, 2017/02/01 11:16:13]

Done. (Deprecated since UrlTracker anyway.)

+// It works asynchronously and calls a callback when finished fetching the
+// information from the remote enpoint. Every instance can only handle one fetch
+// call at a time.
+class DoodleFetcher : public net::URLFetcherDelegate {
+ public:
+  using FinishedCallback =
+      base::Callback<void(const DoodleConfig& doodle_config)>;
+  using ParseJSONCallback = base::Callback<void(
+      const std::string& unsafe_json,
+      const base::Callback<void(std::unique_ptr<base::Value> json)>& success,
+      const base::Callback<void(const std::string&)>& error)>;
+
+  DoodleFetcher(net::URLRequestContextGetter* download_context, GURL base_url);

[Marc Treib, 2017/01/31 14:46:39]

Eventually, the version without ParseJSONCallback should probably be removed?

[fhorschig, 2017/02/01 11:16:13]

Done.

+  DoodleFetcher(net::URLRequestContextGetter* download_context,
+                GURL base_url,

[Marc Treib, 2017/01/31 14:46:38]

Hm, could we get the Google base URL within this class, on demand?
GoogleURLTracker seems to be there for just that:
https://cs.chromium.org/chromium/src/components/google/core/browser/google_ur...

(Fun fact: The Google base URL can change at runtime. So if it's passed in, then
the client would have to watch for that, and re-build the DoodleFetcher if it
changes...)

[fhorschig, 2017/02/01 11:16:13]

Okay, we now depend on the tracker to fetch a URL. This means
a new DEPS entry as well, which I assume is fine?
(//components/google/core/browser:browser)

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

Nice!
Yes, the dep should be fine. (Let's hope that one isn't "special" on ios...)

+                ParseJSONCallback json_parsing_callback);
+  ~DoodleFetcher() override;
+
+  // Fetches a doodle asynchronously. The |callback| is always called and
+  // provides a valid DoodleConfig object.
+  void FetchDoodle(const FinishedCallback& callback);
+
+  // Overrides internal clock for testing purposes.
+  void SetClockForTesting(std::unique_ptr<base::Clock> clock) {

[Marc Treib, 2017/01/31 14:46:39]

Haven't we decided against this kind of thing in the last testing meeting? ;)
(I don't really mind though. If we pass the clock in the ctor, then we kinda
need a factory, which doesn't seem worth the hassle...)

[fhorschig, 2017/02/01 11:16:13]

I  would like to add a TODO for now as my reasoning was the same.
A factory might make sense if the dependencies grow further.

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

SGTM

+    clock_ = std::move(clock);
+  }
+
+ private:
+  // net::URLFetcherDelegate implementation.
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+  void OnJsonParsed(std::unique_ptr<base::Value> json);
+  void OnJsonParseFailed(const std::string& error_message);
+  void ParseDoodle(std::unique_ptr<base::DictionaryValue> config);
+
+  bool ParseImage(const base::DictionaryValue* image_dict,

[Marc Treib, 2017/01/31 14:46:38]

Could these all be static? Or, better yet, local functions in the .cc (in an
anonymous namespace)?

nit: If image_dict can't be null, prefer const ref over pointer

[fhorschig, 2017/02/01 11:16:13]

These functions depend on base_url_ and clock_.
Each function would need four parameters in an unnamed namespace,
but if you prefer that over the crowded header, I can do that.

nit: Done.

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

Ah right, makes sense. Then it's fine as is.

+                  const std::string& image_name,
+                  DoodleImage* image) const;
+  void ParseBaseInformation(const base::DictionaryValue* ddljson,
+                            DoodleConfig* config) const;
+  void ParseRelativeUrl(const base::DictionaryValue* dict_value,
+                        const std::string& key,
+                        GURL* url) const;
+
+  // Parameter set from constructor.
+  net::URLRequestContextGetter* const download_context_;
+  ParseJSONCallback json_parsing_callback_;
+  GURL base_url_;
+
+  // Allow for an injectable tick clock for testing.
+  std::unique_ptr<base::Clock> clock_;
+
+  FinishedCallback callback_;
+  std::unique_ptr<net::URLFetcher> fetcher_;
+
+  base::WeakPtrFactory<DoodleFetcher> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DoodleFetcher);
+};
+
+extern const char kDoodleConfigUrl[];

[Marc Treib, 2017/01/31 14:46:39]

Why is this here? For testing?

[fhorschig, 2017/02/01 11:16:13]

Mainly.

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

Then please add a comment saying that. It'd be even nicer if the tests didn't
need access to this, but oh well.

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

Removed.

+
+}  // namespace doodle
+
+#endif  // COMPONENTS_DOODLE_DOODLE_FETCHER_H_