Quirks Client for downloading and providing display profiles

components/quirks_client/quirks_client.h

+// 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_H_
+#define COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_H_
+
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/timer/timer.h"
+#include "components/quirks_client/quirks_client_export.h"
+#include "net/url_request/url_fetcher_delegate.h"
+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace base {
+class SequencedWorkerPool;
+class MessageLoopForUI;
+}
+
+namespace net {
+class URLRequestContextGetter;
+}
+
+namespace quirks_client {
+
+class QuirksClientManager;
+
+// Handles providing data from icc and other display data files.  This may
+// involve downloading and storing files from the Quirks Server, or serving
+// these files from local, writable storage.
+class QUIRKS_CLIENT_EXPORT QuirksClient : public net::URLFetcherDelegate {
+ public:
+  typedef base::Callback<void(base::FilePath)> DownloadFinishedCallback;

[stevenjb, 2016/02/02 23:45:51]

using

[Greg Levin, 2016/02/09 18:56:38]

Done.

+
+  enum RequestReason {
+    FIRST,  // Device's first server request.
+    RETRY,  // Has server file been added since last check?
+    UPDATE  // Has server file been updated since last check?
+  };
+
+  QuirksClient(int64_t product_id,
+               DownloadFinishedCallback* on_download_finished);

[stevenjb, 2016/02/02 23:45:51]

callbacks should be passed as const& (and input references and pointers should
always be const).

[Greg Levin, 2016/02/09 18:56:38]

Done.

+  ~QuirksClient() override;
+
+  // Returns path to icc file, if it exists; creates and starts QuirksServer if
+  // it doesn't.
+  static base::FilePath RequestIccProfilePath(
+      int64_t product_id,
+      DownloadFinishedCallback* on_download_finished);

[stevenjb, 2016/02/02 23:45:51]

const&

[Greg Levin, 2016/02/09 18:56:38]

Done.

+
+  // Is experimental flag set to enable Quirks Client?
+  static bool IsEnabled();
+
+  // Format int as hex string for filename.
+  static std::string IdToHexString(int64_t product_id);
+
+  void StartDownload();

[stevenjb, 2016/02/02 23:45:51]

non-static methods generally proceed static methods. Also, public methods should
have a comment.

[Greg Levin, 2016/02/09 18:56:38]

Done.

+
+ private:
+  // Self deletion when done.
+  void Shutdown();
+
+  // Schedules retry.
+  void Retry();
+
+  // net::URLFetcherDelegate:
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
+
+  // Write |data| to |file_path|.
+  static bool WriteIccFile(const base::FilePath file_path,
+                           const std::string& data);
+
+  // Callback after icc write is finished.
+  void OnWriteIccFileFinished(bool success);
+
+  // Translate json with base64-encoded data (|result|) into raw |data|.
+  bool ParseResult(const std::string& result, std::string* data);
+
+  // ID of display to request from Quirks Server.
+  const int64_t product_id_;
+
+  // Callback supplied by caller.
+  const DownloadFinishedCallback* on_download_finished_;

[stevenjb, 2016/02/02 23:45:51]

Storing raw pointers in general is dangerous, particularly to callbacks.

Ether this class should own the callback (which should be bound using WeakPtr),
or we should use a Delegate or Observer model.

[Greg Levin, 2016/02/09 18:56:38]

How about this, just storing the callback by "value"?  I've seen this approach
with numerous other class members.

[stevenjb, 2016/02/11 00:05:55]

Yes, that should be fine.

[Greg Levin, 2016/02/12 04:48:04]

Done.

+
+  // Full path to icc file.
+  const base::FilePath icc_path_;
+
+  // This fetcher is used to download icc file.
+  scoped_ptr<net::URLFetcher> url_fetcher_;
+
+  // Why are we making this server call?
+  RequestReason request_reason_;
+
+  // Pending retry.
+  base::OneShotTimer request_scheduled_;
+
+  // Number of download retries (first attempt is not counted as retry).
+  size_t retries_;
+
+  // Factory for callbacks.
+  base::WeakPtrFactory<QuirksClient> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuirksClient);
+};
+
+}  // namespace chromeos
+
+#endif  // COMPONENTS_QUIRKS_CLIENT_QUIRKS_CLIENT_H_