Quirks Client for downloading and providing display profiles

components/quirks/quirks_manager.cc

Index: components/quirks/quirks_manager.cc
diff --git a/components/quirks/quirks_manager.cc b/components/quirks/quirks_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0d036fa481a93023e5dc51644c4e29b1db3a4504
--- /dev/null
+++ b/components/quirks/quirks_manager.cc
@@ -0,0 +1,240 @@
+// 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_manager.h"
+
+#include "base/command_line.h"
+#include "base/files/file_util.h"
+#include "base/format_macros.h"
+#include "base/path_service.h"
+#include "base/rand_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
+#include "chromeos/chromeos_paths.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/quirks/pref_names.h"
+#include "components/quirks/switches.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace quirks {
+
+namespace {
+
+QuirksManager* g_manager = nullptr;
+
+// How often we query Quirks Server.
+const int kDaysBetweenServerChecks = 30;
+
+// Check if file exists, VLOG results.
+bool CheckAndLogFile(const base::FilePath& path) {
+  const bool exists = base::PathExists(path);
+  VLOG(1) << (exists ? "File" : "No File") << " found at " << path.value();
+  // TODO(glevin): If file exists, do we want to implement a hash to verify that
+  // the file hasn't been corrupted or tampered with?
+  return exists;
+}
+
+}  // namespace
+
+std::string IdToHexString(int64_t product_id) {
+  return base::StringPrintf("%08" PRIx64, product_id);
+}
+
+// Must be called on file thread.

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

Requiring this to be called on the file thread is confusing and adds unnecessary
complexity to the caller. It also makes the assumption that the "file thread" is
the same as blocking_pool_ which is confusing.


Instead, the API should look something like:

quirks::RequestIccProfilePath(id, callback) {
  if (!g_manager)
    callback.Run(base::FilePath());
  g_manager->RequestIccFilePath(id, callback);
}

And the implementation something like:

QuirksManager::RequestIccFilePath(id, callback) {
  CHECK(thread_checker_.CalledOnValidThread());

  blocking_pool_->PostTaskAndReply(
    Bind(GetIccFilePathOnBlockingPool, id, callback, any other args),
    Bind(QuirksManager::CompleteRequestIccFilePath, weak ptr, id, callback);
}

QuirksManager::CompleteRequestIccFilePath(id, callback, result) {
  callback.Run(result);
}

[Bernhard Bauer, 2016/02/17 17:27:40]

Also, CompleteRequestIccFilePath() could create the new QuirksClient if the
result is empty.

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

Ok, this is just my incomplete understanding of the blocking pool system, but...
I'm not sure where the confusion comes from. This function obviously needs to
run on the file thread, as it's looking for a file.  Similarly, I would assume
that any function calling it would also be running on the file thread, as the
next step after calling it is to open the requested file.  I see it as a more
sophisticated version of PathExists().  You call it from the file thread because
you know it needs to run on the file thread, and very likely you're about to use
the file.  The fact that the QM will go off on its own and get the file later if
it's not found isn't a detail that the caller needs to be aware of.
Is this assumption just in
  DCHECK(blocking_pool()->RunsTasksOnCurrentThread()),
or is it being made somewhere else that I'm not seeing? The DCHECK is just meant
to validate that it's running on the file thread, if there's a better way to do
that. I could remove the DCHECK if it's confusion / misused.
This seems like it adds to the complexity of the caller, forcing it to implement
a callback, which the current system does not.  The callback is currently
optional, only if the system wants to use the downloaded file ASAP.  It would
certainly complicate the code in display_color_manager_chromeos.cc.  Currently,
we just have

PostTaskAndReplyWithResult(ParseDisplayProfile, UpdateCalibrationData)

With the proposed scheme, we'd have to bounce through

ParseDisplayProfile ->
  RequestIccProfilePath ->
  GetIccFilePathOnBlockingPool ->
  CompleteRequestIccFilePath ->
ParseDisplayProfileCallback ->
UpdateCalibrationData

I'm not sure how to do this with the PostTaskAndReplyWithResult model.

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

Perhaps we need to discuss this in a meeting, however the fundamental problem
for me is that "Request...Path" is not the same as "PathExists":
* The name does not suggest to me that it needs to be run from a blocking pool.
* The operation can actually be quite complex if the file does not already
exist.
* As written following the threads is challenging:
  caller (blocking pool) -> 
  QM::RequestIccFilePath (*QM* blocking pool) ->
  QM::RunClientOnTaskRunner (client task runner) ->
  on_download_finished (client task runner?)

It is particularly confusing that RequestIccProfilePath is expected to be called
from the *same blocking pool* that is passed into QM(), and that
on_download_finished is called on *client task runner*.

Perhaps it would help all of us if you put together a doc that showed the
relationship between the various methods and the threads / task runners they are
expected to be run on?

[Bernhard Bauer, 2016/02/18 12:17:07]

You are correct that this doesn't really work with PostTaskAndReply(WithResult),
but that's just a convenience for running a _synchronous_ method on a background
thread. You can get the same behavior with continuation-passing style: just pass
a callback and a task runner to ParseDisplayProfile. And yes, this would make
the callback non-optional, but I'd consider that a feature 😃 -- it would allow
you to use the same callback regardless of where the file came from (i.e. it
would already address the TODO you have in there).

[Greg Levin, 2016/03/02 00:00:26]

Done. Based on stevenjb's input, I refactored QM so that RequestIccProfilePath()
is called from the ui thread with a mandatory callback.

+base::FilePath RequestIccProfilePath(
+    int64_t product_id,
+    const DownloadFinishedCallback& on_download_finished) {
+  if (g_manager)
+    return g_manager->RequestIccFilePath(product_id, on_download_finished);
+
+  VLOG(1) << "Quirks Client Manager not initialized; can't get icc file.";
+  return base::FilePath();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// QuirksManager
+
+QuirksManager::QuirksManager(
+    Delegate* delegate,
+    scoped_refptr<base::SequencedWorkerPool> blocking_pool,
+    PrefService* local_state,
+    scoped_refptr<net::URLRequestContextGetter> url_context_getter)
+    : delegate_(delegate),
+      blocking_pool_(blocking_pool),
+      local_state_(local_state),
+      url_context_getter_(url_context_getter),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      is_new_device_(false),
+      weak_ptr_factory_(this) {}
+
+QuirksManager::~QuirksManager() {
+  clients_.clear();
+  g_manager = nullptr;
+}
+
+// static
+void QuirksManager::Initialize(
+    scoped_ptr<Delegate> delegate,
+    scoped_refptr<base::SequencedWorkerPool> blocking_pool,
+    PrefService* local_state,
+    scoped_refptr<net::URLRequestContextGetter> url_context_getter) {
+  g_manager = new QuirksManager(delegate.release(), blocking_pool, local_state,
+                                url_context_getter);
+}
+
+// static
+void QuirksManager::Shutdown() {
+  delete g_manager;
+}
+
+// static
+QuirksManager* QuirksManager::Get() {
+  DCHECK(g_manager);
+  return g_manager;
+}
+
+// static
+void QuirksManager::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(prefs::kQuirksClientLastServerCheck);
+}
+
+base::FilePath QuirksManager::RequestIccFilePath(
+    int64_t product_id,
+    const DownloadFinishedCallback& on_download_finished) {
+  DCHECK(blocking_pool()->RunsTasksOnCurrentThread());
+  std::string file_name = IdToHexString(product_id) + ".icc";
+
+  // First, look for icc file in old read-only location.  If there, we don't use
+  // the Quirks server.
+  // TODO(glevin): Awaiting final decision on how to handle old read-only files.
+  base::FilePath path;
+  CHECK(
+      PathService::Get(chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, &path));

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

nit: Rather than adding a src/chromeos dependency for just this line, make
icc_filepath a constructor arg.

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

Moved it to delegate instead (as per suggestion in DEPS comment)

+  path = path.Append(file_name);
+  if (CheckAndLogFile(path))
+    return path;
+
+  // If experimental Quirks flag isn't set, no other icc file is available.
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableDisplayQuirksClient)) {
+    VLOG(1) << "Quirks Client disabled, no built-in icc file available.";
+    return base::FilePath();
+  }
+
+  // Check if QuirksClient has already downloaded icc file from server.
+  path = delegate_->GetDisplayProfileDirectory().Append(file_name);
+  if (CheckAndLogFile(path))
+    return path;
+
+  // TODO(glevin): Eventually we'll want to check the server for updates to
+  // files, so we'll still want to get down here even if we find the icc file.
+
+  // Record this on file thread for future use on ui thread.
+  is_new_device_ = (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks);
+
+  task_runner_->PostTask(FROM_HERE,
+                         base::Bind(&QuirksManager::RunClientOnUIThread,
+                                    weak_ptr_factory_.GetWeakPtr(), product_id,
+                                    on_download_finished));
+
+  return base::FilePath();
+}
+
+void QuirksManager::ClientFinished(QuirksClient* client) {
+  auto it = std::find_if(clients_.begin(), clients_.end(),
+                         [client](const scoped_ptr<QuirksClient>& c) {
+                           return c.get() == client;
+                         });
+  if (it != clients_.end())

[Bernhard Bauer, 2016/02/17 17:27:40]

I think I would DCHECK this instead of silently failing. This should never be
called with an unregistered client.

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

Done.

+    clients_.erase(it);
+}
+
+bool QuirksManager::NeedToCheckServer(int64_t product_id) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  double last_check = 0.0;
+  local_state_->GetDictionary(prefs::kQuirksClientLastServerCheck)
+      ->GetDouble(IdToHexString(product_id), &last_check);
+
+  if (last_check != 0.0) {
+    const base::TimeDelta time_since =
+        base::Time::Now() - base::Time::FromDoubleT(last_check);
+
+    // Don't need server check if we've checked within last 30 days.
+    if (time_since < base::TimeDelta::FromDays(kDaysBetweenServerChecks)) {
+      VLOG(2) << time_since.InDays()
+              << " days since last Quirks Server check for display "
+              << IdToHexString(product_id);
+      return false;
+    }
+
+    return true;
+  }
+
+  // On new devices, we want to check server immediately.
+  if (is_new_device_)
+    return true;
+
+  // Otherwise, for the first check on an older device, we want to stagger
+  // it over 30 days, so artificially set last check accordingly.
+  const int rand_days = base::RandInt(0, kDaysBetweenServerChecks);
+  const base::Time fake_last_check =
+      base::Time::Now() - base::TimeDelta::FromDays(rand_days);
+  SetLastServerCheck(product_id, fake_last_check);
+  VLOG(2) << "Delaying first Quirks Server check by "
+          << kDaysBetweenServerChecks - rand_days << " days.";
+  return false;
+}
+
+void QuirksManager::SetLastServerCheck(int64_t product_id,
+                                       const base::Time& last_check) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  DictionaryPrefUpdate dict(local_state_, prefs::kQuirksClientLastServerCheck);
+  dict->SetDouble(IdToHexString(product_id), last_check.ToDoubleT());
+}
+
+// TODO(glevin): Add code to record UMA stats here.  Also need to set
+// request_reason_ in QuirksClient.
+void QuirksManager::RecordReasonUmaStat(QuirksClient::RequestReason reason) {}
+
+void QuirksManager::RecordFileFoundUmaStat(bool success) {}
+
+scoped_ptr<net::URLFetcher> QuirksManager::CreateURLFetcher(
+    const std::string& url,
+    net::URLFetcherDelegate* delegate) {
+  if (!fake_quirks_fetcher_creator_.is_null())
+    return fake_quirks_fetcher_creator_.Run(GURL(url), delegate);
+
+  return net::URLFetcher::Create(GURL(url), net::URLFetcher::GET, delegate);
+}
+
+// Initializes QuirksClient object on the ui thread, where most of its work
+// needs to be done.
+void QuirksManager::RunClientOnUIThread(
+    int64_t product_id,
+    const DownloadFinishedCallback& on_download_finished) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  if (!ValidateOnUiThread()) {
+    VLOG(1) << "Quirks Manager not correctly initialized.";
+    return;
+  }
+
+  if (!NeedToCheckServer(product_id))
+    return;
+
+  QuirksClient* client =
+      new QuirksClient(product_id, on_download_finished, this);
+  clients_.insert(make_scoped_ptr(client));
+  client->StartDownload();
+}
+
+bool QuirksManager::ValidateOnUiThread() {
+  return base::ThreadTaskRunnerHandle::IsSet() &&
+         local_state_->CalledOnValidThread() &&
+         task_runner_->RunsTasksOnCurrentThread();
+}
+
+}  // namespace quirks