[Doodle] Split DoodleFetcher into interface + Impl

components/doodle/doodle_fetcher.h

Index: components/doodle/doodle_fetcher.h
diff --git a/components/doodle/doodle_fetcher.h b/components/doodle/doodle_fetcher.h
index 951bbad3913a5605fa4208b1b0c6e86c8805bd5c..5519f1bcf3b46c93f1b2a101f098fb5516083e95 100644
--- a/components/doodle/doodle_fetcher.h
+++ b/components/doodle/doodle_fetcher.h
@@ -5,103 +5,36 @@
 #ifndef COMPONENTS_DOODLE_DOODLE_FETCHER_H_
 #define COMPONENTS_DOODLE_DOODLE_FETCHER_H_
 
-#include <memory>
-#include <string>
-#include <utility>
-#include <vector>
-
-#include "base/callback.h"
+#include "base/callback_forward.h"

[fhorschig, 2017/02/22 14:57:18]

Nice, didn't know that was possible!

 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "components/doodle/doodle_types.h"
-#include "net/url_request/url_fetcher_delegate.h"
-#include "net/url_request/url_request_context_getter.h"
-#include "url/gurl.h"
-
-class GoogleURLTracker;
-
-namespace base {
-class Clock;
-class DictionaryValue;
-class Value;
-}
 
 namespace doodle {
 
-// This class provides information about any recent doodle.
-// It works asynchronously and calls a callback when finished fetching the
-// information from the remote enpoint.
-class DoodleFetcher : public net::URLFetcherDelegate {
+// Interface for fetching the current doodle from the network.
+// It asynchronously calls a callback when fetching the doodle information from
+// the remote enpoint finishes.
+// DoodleFetcherImpl is the default implementation; this interface exists to
+// make it easy to use fakes or mocks in tests.
+class DoodleFetcher {
  public:
   // Callback that is invoked when the fetching is done.
   // |doodle_config| will only contain a value if |state| is AVAILABLE.
   using FinishedCallback = base::OnceCallback<void(
       DoodleState state,
       const base::Optional<DoodleConfig>& doodle_config)>;
-  // Callback for JSON parsing to allow injecting platform-dependent code.
-  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(scoped_refptr<net::URLRequestContextGetter> download_context,
-                GoogleURLTracker* google_url_tracker,
-                const ParseJSONCallback& json_parsing_callback);
-  ~DoodleFetcher() override;
+  DoodleFetcher() = default;
+  virtual ~DoodleFetcher() = default;
 
   // Fetches a doodle asynchronously. The |callback| is called with a
   // DoodleState indicating whether the request succeded in fetching a doodle.
   // If a fetch is already running, the callback will be queued and invoked with
   // result from the next completed request.
-  void FetchDoodle(FinishedCallback callback);
-
-  // Overrides internal clock for testing purposes.
-  void SetClockForTesting(std::unique_ptr<base::Clock> clock) {
-    clock_ = std::move(clock);
-  }
+  virtual void FetchDoodle(FinishedCallback callback) = 0;
 
  private:
-  // net::URLFetcherDelegate implementation.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
-
-  // ParseJSONCallback Success callback
-  void OnJsonParsed(std::unique_ptr<base::Value> json);
-  // ParseJSONCallback Failure callback
-  void OnJsonParseFailed(const std::string& error_message);
-
-  base::Optional<DoodleConfig> ParseDoodle(
-      const base::DictionaryValue& ddljson) const;
-  bool ParseImage(const base::DictionaryValue& image_dict,
-                  const std::string& image_name,
-                  DoodleImage* image) const;
-  void ParseBaseInformation(const base::DictionaryValue& ddljson,
-                            DoodleConfig* config) const;
-  GURL ParseRelativeUrl(const base::DictionaryValue& dict_value,
-                        const std::string& key) const;
-
-  void RespondToAllCallbacks(DoodleState state,
-                             const base::Optional<DoodleConfig>& config);
-  GURL GetGoogleBaseUrl() const;
-
-  // Returns whether a fetch is still in progress. A fetch begins when a
-  // callback is added and ends when the last callback was called.
-  bool IsFetchInProgress() const { return !callbacks_.empty(); }
-
-  // Parameters set from constructor.
-  scoped_refptr<net::URLRequestContextGetter> const download_context_;
-  ParseJSONCallback json_parsing_callback_;
-  GoogleURLTracker* google_url_tracker_;
-
-  // Allow for an injectable tick clock for testing.
-  std::unique_ptr<base::Clock> clock_;
-
-  std::vector<FinishedCallback> callbacks_;
-  std::unique_ptr<net::URLFetcher> fetcher_;
-
-  base::WeakPtrFactory<DoodleFetcher> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(DoodleFetcher);

[sfiera, 2017/02/22 17:04:55]

Is this normal for abstract classes that can't be instantiated anyway? I assume
you don't need the default constructor above without it.

[Marc Treib, 2017/02/22 17:17:21]

Hm, I *think* I've seen it before, but it doesn't seem to be very common, at
least in our corner of the world.

This way, the non-copyability is part of the interface. I'm not sure if it
should be; probably doesn't make much of a difference in practice. Do you think
I should remove it?

 };