Quirks Client for downloading and providing display profiles

components/quirks/quirks_client.cc

Index: components/quirks/quirks_client.cc
diff --git a/components/quirks/quirks_client.cc b/components/quirks/quirks_client.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0575c9680526323f417ea873e1e3318ed3537327
--- /dev/null
+++ b/components/quirks/quirks_client.cc
@@ -0,0 +1,191 @@
+// 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.
+
+#include "components/quirks/quirks_client.h"
+
+#include "base/base64.h"
+#include "base/files/file_util.h"
+#include "base/json/json_reader.h"
+#include "base/strings/stringprintf.h"
+#include "base/task_runner_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/quirks/quirks_manager.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+
+namespace quirks {
+
+namespace {
+
+const char kQuirksUrlFormat[] =
+    "https://qa-quirksserver-pa.sandbox.googleapis.com/v2/display/%s/clients"
+    "/chromeos/M%d";
+
+const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
+    1,                // Initial errors before applying backoff
+    10000,            // 10 seconds.
+    2,                // Factor by which the waiting time will be multiplied.
+    0,                // Random fuzzing percentage.
+    1000 * 3600 * 6,  // Max wait between requests = 6 hours.
+    -1,               // Don't discard entry.
+    true,             // Use initial delay after first error.
+};
+
+// Access to singleton QuirksManager for static and nonmember funcitons.
+QuirksManager* GetManager() {
+  return QuirksManager::Get();
+}

[stevenjb, 2016/02/16 20:07:51]

Not needed (see comment below)

[Greg Levin, 2016/02/17 23:01:51]

Deleted!

+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// QuirksClient
+
+QuirksClient::QuirksClient(int64_t product_id,
+                           const DownloadFinishedCallback& on_download_finished,
+                           QuirksManager* manager)
+    : product_id_(product_id),
+      on_download_finished_(on_download_finished),
+      manager_(manager),
+      icc_path_(manager->delegate()->GetDisplayProfileDirectory().Append(
+          IdToHexString(product_id) + ".icc")),

[stevenjb, 2016/02/16 20:07:51]

.icc should be defined as a const char[] in the anon namespace

[Greg Levin, 2016/02/17 23:01:51]

Done... created IdToFileName() to go with IdToHexString() in quirks_manager.cc,
using const char[] there.

+      request_reason_(FIRST),
+      backoff_entry_(&kDefaultBackoffPolicy),
+      weak_ptr_factory_(this) {}
+
+QuirksClient::~QuirksClient() {}
+
+void QuirksClient::StartDownload() {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+
+  // URL of icc file on Quirks Server.
+  // TODO(glevin): Replace |44| with actual version.  This is fine for now, as
+  // the Quirks Server is not currently using this value.

[stevenjb, 2016/02/16 20:07:51]

We should fix this now or do something else in the meanwhile, it's very
confusing.

[Greg Levin, 2016/02/17 23:01:51]

Done.

+  std::string url = base::StringPrintf(kQuirksUrlFormat,
+                                       IdToHexString(product_id_).c_str(), 44);
+
+  VLOG(2) << "Preparing to download\n  " << url << "\nto file "
+          << icc_path_.value();
+
+  url += "?key=";
+  url += manager_->delegate()->GetApiKey();
+
+  url_fetcher_ = manager_->CreateURLFetcher(url, this);
+  url_fetcher_->SetRequestContext(manager_->url_context_getter());
+  url_fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+                             net::LOAD_DO_NOT_SAVE_COOKIES |
+                             net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SEND_AUTH_DATA);
+  url_fetcher_->Start();
+}
+
+void QuirksClient::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  DCHECK_EQ(url_fetcher_.get(), source);
+
+  const int HTTP_INTERNAL_SERVER_ERROR_LAST =
+      net::HTTP_INTERNAL_SERVER_ERROR + 99;

[stevenjb, 2016/02/16 20:07:51]

Where does 99 come from?

[Greg Levin, 2016/02/17 23:01:51]

It was originally copied from here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

with HTTP_INTERNAL_SERVER_ERROR_LAST defined as per comment:
https://codereview.chromium.org/1528963002/diff2/340001:360001/components/qui...

Codes are defined here:
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stat...

The 500-599 range is for server errors:
https://en.wikipedia.org/wiki/List_of_HTTP_status_codes

[stevenjb, 2016/02/17 23:38:55]

I see. Ugh, OK.

+  const net::URLRequestStatus status = source->GetStatus();
+  const int response_code = source->GetResponseCode();
+  const bool server_error = !status.is_success() ||
+                            (response_code >= net::HTTP_INTERNAL_SERVER_ERROR &&
+                             response_code <= HTTP_INTERNAL_SERVER_ERROR_LAST);
+
+  VLOG(2) << "QuirksClient::OnURLFetchComplete():"
+          << "  status=" << status.status()
+          << ",  response_code=" << response_code
+          << ",  server_error=" << server_error;
+
+  manager_->RecordReasonUmaStat(request_reason_);
+
+  if (response_code == net::HTTP_NOT_FOUND) {
+    VLOG(1) << IdToHexString(product_id_) << ".icc not found on Quirks server.";
+    manager_->RecordFileFoundUmaStat(false);
+    Shutdown(false);
+    return;
+  }
+
+  if (server_error) {
+    url_fetcher_.reset();
+    Retry();
+    return;
+  }
+
+  manager_->RecordFileFoundUmaStat(true);
+  std::string response;
+  url_fetcher_->GetResponseAsString(&response);
+  VLOG(2) << "Quirks server response:\n" << response;
+
+  // Parse response data and write to file on file thread.
+  std::string data;
+  if (!ParseResult(response, &data)) {
+    Shutdown(false);
+    return;
+  }
+
+  base::PostTaskAndReplyWithResult(
+      manager_->blocking_pool(), FROM_HERE,
+      base::Bind(&WriteIccFile, icc_path_, data),
+      base::Bind(&QuirksClient::Shutdown, weak_ptr_factory_.GetWeakPtr()));
+}
+
+void QuirksClient::Shutdown(bool success) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  if (!on_download_finished_.is_null())
+    on_download_finished_.Run(success ? icc_path_ : base::FilePath());
+
+  manager_->SetLastServerCheck(product_id_, base::Time::Now());

[stevenjb, 2016/02/16 20:07:51]

Maybe make this part of ClientFinished?

[Greg Levin, 2016/02/17 23:01:51]

Done.

+  manager_->ClientFinished(this);
+}
+
+void QuirksClient::Retry() {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  backoff_entry_.InformOfRequest(false);
+  const base::TimeDelta delay = backoff_entry_.GetTimeUntilRelease();
+
+  VLOG(1) << "Schedule next Quirks download attempt in " << delay.InSecondsF()
+          << " seconds (retry = " << backoff_entry_.failure_count() << ").";
+  request_scheduled_.Start(FROM_HERE, delay, this,
+                           &QuirksClient::StartDownload);
+}
+
+// static
+// TODO(glevin): would like to make this non-static and get rid of GetManager(),
+// but was having trouble working out the syntax for Bind'ing it above, since
+// "weak_ptrs can only bind to methods without return values".

[stevenjb, 2016/02/16 20:07:51]

This shouldn't need to be a static member at all, it can just be a file local
method since all it is doing is writing data to a file.

[Greg Levin, 2016/02/17 23:01:51]

Done.

+bool QuirksClient::WriteIccFile(const base::FilePath file_path,
+                                const std::string& data) {
+  DCHECK(GetManager() &&
+         GetManager()->blocking_pool()->RunsTasksOnCurrentThread());

[stevenjb, 2016/02/16 20:07:51]

Why do we need this DCHECK? If it is important that the manager still exists,
then writing the file should be handled by the manager. Otherwise this function
should not know or care about the manager.

[Greg Levin, 2016/02/17 23:01:51]

Done.

+  int bytes_written = base::WriteFile(file_path, data.data(), data.length());
+  if (bytes_written == -1)
+    VLOG(1) << "Failed to write: " << file_path.value() << ", err = " << errno;
+  else
+    VLOG(1) << bytes_written << "bytes written to: " << file_path.value();
+
+  return (bytes_written != -1);
+}
+
+bool QuirksClient::ParseResult(const std::string& result, std::string* data) {
+  std::string data64;
+  const base::DictionaryValue* dict;
+  scoped_ptr<base::Value> json = base::JSONReader::Read(result);
+  if (!json || !json->GetAsDictionary(&dict) ||
+      !dict->GetString("icc", &data64)) {
+    VLOG(1) << "Failed to parse JSON icc data";
+    return false;
+  }
+
+  if (!base::Base64Decode(data64, data)) {
+    VLOG(1) << "Failed to decode Base64 icc data";
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace chromeos