Quirks Client for downloading and providing display profiles

components/quirks_client/quirks_client_manager.h

Index: components/quirks_client/quirks_client_manager.h
diff --git a/components/quirks_client/quirks_client_manager.h b/components/quirks_client/quirks_client_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..1fd45850645efa6e0c5c8177037f9beae79e8f7d
--- /dev/null
+++ b/components/quirks_client/quirks_client_manager.h
@@ -0,0 +1,128 @@
+// Copyright 2016 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_QUIRKS_CLIENT_QUIRKS_CLIENT_MANAGER_H_
+#define COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_MANAGER_H_
+
+#include <map>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time/time.h"
+#include "components/quirks_client/quirks_client.h"
+
+class GURL;
+class PrefRegistrySimple;
+class PrefService;
+
+namespace base {
+class SequencedWorkerPool;
+class SingleThreadTaskRunner;
+}
+
+namespace net {
+class URLFetcher;
+class URLFetcherDelegate;
+class URLRequestContextGetter;
+}
+
+namespace quirks_client {
+
+// Handles needed components from browser (local preferences, url context
+// getter, message loops).
+class QUIRKS_CLIENT_EXPORT QuirksClientManager {
+ public:
+  using FakeQuirksFetcherCreator =
+      base::Callback<scoped_ptr<net::URLFetcher>(const GURL&,
+                                                 net::URLFetcherDelegate*)>;
+
+  // Delegate class, so implementation can access browser functionality.
+  class Delegate {
+   public:
+    virtual ~Delegate() = default;
+    virtual std::string GetApiKey() const = 0;
+    virtual base::FilePath GetDisplayProfileDirectory() const = 0;
+    virtual int GetDaysSinceOobe() const = 0;

[Bernhard Bauer, 2016/02/12 14:51:22]

Maybe add comments which threads this might be called on?

[Greg Levin, 2016/02/15 21:53:53]

Done... added comment to GetDaysSinceOobe().  The others don't have thread
restrictions (that I'm aware of).

+
+   private:
+    DISALLOW_ASSIGN(Delegate);
+  };
+
+  QuirksClientManager(Delegate* delegate,

[Bernhard Bauer, 2016/02/12 14:51:22]

If this class is meant to be created and destroyed via ::Initialize() and
::Shutdown(), make constructor and destructor private?

[Greg Levin, 2016/02/15 21:53:53]

Done.

+                      base::SequencedWorkerPool* blocking_pool,

[Bernhard Bauer, 2016/02/12 14:51:22]

If you keep a reference to this, pass it as a scoped_refptr. Same for the
|url_context_getter|.

[Greg Levin, 2016/02/15 21:53:53]

Done.

+                      PrefService* local_state,
+                      net::URLRequestContextGetter* url_context_getter);
+  virtual ~QuirksClientManager();

[Bernhard Bauer, 2016/02/12 14:51:22]

This class doesn't have any other virtual methods, so the destructor doesn't
need to be virtual either.

[Greg Levin, 2016/02/15 21:53:53]

Done.

+
+  static void Initialize(scoped_ptr<Delegate> delegate,
+                         base::SequencedWorkerPool* blocking_pool,
+                         PrefService* local_state,
+                         net::URLRequestContextGetter* url_context_getter);
+  static void Shutdown();
+  static QuirksClientManager* Get();
+
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  void RunClient(int64_t product_id,
+                 const DownloadFinishedCallback& on_download_finished);
+
+  void DeleteClient(QuirksClient* client);

[Bernhard Bauer, 2016/02/12 14:51:22]

So, this is a bit of a strange interface... This class owns the clients, but
someone else can go and delete one of the clients? That kind of contradicts the
concept of ownership. Could the caller (i.e. the QuirksClient) just delete
itself? Then this class could just keep a set of raw pointers, and if it is
destroyed, it would call Shutdown(false) on all the pending clients.

Or if you want to keep this class owning the clients, I would at least rename
this method to something like ClientFinished(), so the fact that this class
chooses to delete a finished client would not be part of the interface. You
could also store the clients in a std::set<scoped_ptr<>> and use a custom
predicate to look an entry up by raw pointer; I think that would still look
nicer than a map from raw pointer to scoped pointer.

[Greg Levin, 2016/02/16 03:14:46]

Done (went with second option: renamed function, switched to std::set).

+
+  // Determines need to check based on previous check time and device age.
+  bool NeedToCheckServer(int64_t product_id);
+
+  // Records time of most recent server check.
+  void SetLastServerCheck(int64_t product_id, const base::Time& last_check);
+
+  // UMA stats for server check reason and results.
+  void RecordReasonUmaStat(QuirksClient::RequestReason reason);
+  void RecordFileFoundUmaStat(bool success);
+
+  // Switch to fake URLFetcher creator for tests.
+  scoped_ptr<net::URLFetcher> CreateURLFetcher(
+      const std::string& url,
+      net::URLFetcherDelegate* delegate);
+
+  Delegate* delegate() const { return delegate_; }
+  base::SequencedWorkerPool* blocking_pool() const { return blocking_pool_; }
+  net::URLRequestContextGetter* url_context_getter() const {
+    return url_context_getter_.get();
+  }
+  base::SingleThreadTaskRunner* task_runner() const {
+    return task_runner_.get();
+  }
+  void SetFakeQuirksFetcherCreator(const FakeQuirksFetcherCreator& creator) {

[Bernhard Bauer, 2016/02/12 14:51:21]

...ForTesting?

[Greg Levin, 2016/02/15 21:53:53]

Done.

+    fake_quirks_fetcher_creator_ = creator;
+  }
+
+ private:
+  void RunClientOnUIThread(
+      int64_t product_id,
+      const DownloadFinishedCallback& on_download_finished);
+
+  // Check that ui thread and message loop are ready.
+  bool ValidateOnUiThread();

[Bernhard Bauer, 2016/02/12 14:51:22]

I don't quite get the point of this method. If you want to check that the UI
thread is correctly set up, returning (not DCHECKing!)
ThreadTaskRunnerHandle::IsSet() would be sufficient, but if that was not the
case, you would have already DCHECKed in the constructor, because
ThreadTaskRunnerHandle::Get() has that as a precondition.

[Greg Levin, 2016/02/15 21:53:53]

I've simplified this method and removed its DCHECK.

+
+  std::map<QuirksClient*, scoped_ptr<QuirksClient>> clients_;
+
+  // These objects provide resources from the browser.
+  Delegate* delegate_;                        // Impl runs from browser.
+  base::SequencedWorkerPool* blocking_pool_;  // For url getter and file io.
+  PrefService* local_state_;                  // For local prefs.
+  scoped_refptr<net::URLRequestContextGetter> url_context_getter_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  bool is_new_device_;  // Is within 30 days of OOBE?
+  FakeQuirksFetcherCreator fake_quirks_fetcher_creator_;  // For tests.
+
+  // Factory for callbacks.
+  base::WeakPtrFactory<QuirksClientManager> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuirksClientManager);
+};
+
+}  // namespace chromeos
+
+#endif  // COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_MANAGER_H_