Creating a "Quirks Client" to download icc files and other display info

("quirks") from a Quirks Server.  These provide display-specific tuning
(e.g. gamma ramps) on a per-monitor basis.

Each Quirks Client handles downloading and writing a single file.  The
single Quirks Manager handles external requests for file paths, creates
clients when downloads are needed, and manages their life cycle.  For
more info, see go/cros-quirks-client-dd (particularly the "Code Design"
section).

BUG=549349
TEST=Start up device that has a an icc file on the Quirks Server, check
that file is downloaded to /var/cache/display_profiles.  At next startup,
the gamma correction in the icc file should be applied to the display
(this will only be visible to the degree that the gamma correction is
large enough to be noticeable; the correct functioning of the Quirks
Client is primarily determined by the appearance of the file).

Review URL: https://codereview.chromium.org/1528963002

Cr-Commit-Position: refs/heads/master@{#382962}

6 files changed, 165 insertions, 81 deletions

diff --git a/ash/BUILD.gn b/ash/BUILD.gn
index 31aaeba..9c7cdd0 100644
--- a/ash/BUILD.gn
+++ b/ash/BUILD.gn
@@ -105,6 +105,7 @@ component("ash") {
     deps += [
       "//chromeos",
       "//chromeos:power_manager_proto",
+      "//components/quirks",
       "//device/bluetooth",
 
       # TODO(msw): Remove this; only ash_with_content should depend on webkit.
@@ -425,7 +426,9 @@ test("ash_unittests") {
       "//chromeos",
       "//chromeos:power_manager_proto",
       "//chromeos:test_support_without_gmock",
+      "//components/quirks",
       "//device/bluetooth",
+      "//net:net",
       "//ui/chromeos:ui_chromeos",
       "//ui/display",
       "//ui/display:test_support",
diff --git a/ash/DEPS b/ash/DEPS
index 0e0ff3d..bbc8690 100644
--- a/ash/DEPS
+++ b/ash/DEPS
@@ -2,6 +2,7 @@ include_rules = [
   "+device/bluetooth",
   "+cc/debug",
   "+chromeos",
+  "+components/quirks",
   "+components/signin/core/account_id",
   "+components/user_manager",
   "+components/wallpaper",
diff --git a/ash/ash.gyp b/ash/ash.gyp
index e6d0ac8..3a8f474 100644
--- a/ash/ash.gyp
+++ b/ash/ash.gyp
@@ -1028,6 +1028,7 @@
             '../chromeos/chromeos.gyp:chromeos',
             # Ash #includes power_supply_properties.pb.h directly.
             '../chromeos/chromeos.gyp:power_manager_proto',
+            '../components/components.gyp:quirks',
             '../device/bluetooth/bluetooth.gyp:device_bluetooth',
             '../third_party/qcms/qcms.gyp:qcms',
             '../ui/chromeos/ui_chromeos.gyp:ui_chromeos_resources',
@@ -1230,6 +1231,7 @@
           'dependencies': [
             '../chromeos/chromeos.gyp:chromeos_test_support_without_gmock',
             '../chromeos/chromeos.gyp:power_manager_proto',
+            '../components/components.gyp:quirks',
             '../device/bluetooth/bluetooth.gyp:device_bluetooth',
             '../ui/display/display.gyp:display',
             '../ui/display/display.gyp:display_test_support',
diff --git a/ash/display/display_color_manager_chromeos.cc b/ash/display/display_color_manager_chromeos.cc
index 1241978..e555919 100644
--- a/ash/display/display_color_manager_chromeos.cc
+++ b/ash/display/display_color_manager_chromeos.cc
@@ -7,39 +7,27 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/command_line.h"
-#include "base/files/file_path.h"
 #include "base/files/file_util.h"
-#include "base/format_macros.h"
 #include "base/logging.h"
-#include "base/message_loop/message_loop.h"
-#include "base/path_service.h"
 #include "base/stl_util.h"
-#include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
-#include "chromeos/chromeos_paths.h"
+#include "components/quirks/quirks_manager.h"
 #include "third_party/qcms/src/qcms.h"
 #include "ui/display/types/display_snapshot.h"
 #include "ui/display/types/gamma_ramp_rgb_entry.h"
-#include "ui/display/types/native_display_delegate.h"
 #include "ui/gfx/display.h"
-#include "ui/gfx/screen.h"
 
 namespace ash {
 
 namespace {
 
-bool ParseFile(const base::FilePath& path,
-               DisplayColorManager::ColorCalibrationData* data) {
-  if (!base::PathExists(path))  // No icc file for this display; not an error.
-    return false;
+scoped_ptr<DisplayColorManager::ColorCalibrationData> ParseDisplayProfile(
+    const base::FilePath& path) {
   qcms_profile* display_profile = qcms_profile_from_path(path.value().c_str());
-
   if (!display_profile) {
     LOG(WARNING) << "Unable to load ICC file: " << path.value();
-    return false;
+    return nullptr;
   }
 
   size_t vcgt_channel_length =
@@ -47,7 +35,7 @@ bool ParseFile(const base::FilePath& path,
   if (!vcgt_channel_length) {
     LOG(WARNING) << "No vcgt table in ICC file: " << path.value();
     qcms_profile_release(display_profile);
-    return false;
+    return nullptr;
   }
 
   std::vector<uint16_t> vcgt_data;
@@ -55,9 +43,11 @@ bool ParseFile(const base::FilePath& path,
   if (!qcms_profile_get_vcgt_rgb_channels(display_profile, &vcgt_data[0])) {
     LOG(WARNING) << "Unable to get vcgt data";
     qcms_profile_release(display_profile);
-    return false;
+    return nullptr;
   }
 
+  scoped_ptr<DisplayColorManager::ColorCalibrationData> data(
+      new DisplayColorManager::ColorCalibrationData());
   data->lut.resize(vcgt_channel_length);
   for (size_t i = 0; i < vcgt_channel_length; ++i) {
     data->lut[i].r = vcgt_data[i];
@@ -65,16 +55,8 @@ bool ParseFile(const base::FilePath& path,
     data->lut[i].b = vcgt_data[(vcgt_channel_length * 2) + i];
   }
   qcms_profile_release(display_profile);
-  return true;
-}
-
-base::FilePath PathForDisplaySnapshot(const ui::DisplaySnapshot* snapshot) {
-  base::FilePath path;
-  CHECK(
-      PathService::Get(chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, &path));
-  path = path.Append(
-      base::StringPrintf("%08" PRIx64 ".icc", snapshot->product_id()));
-  return path;
+  VLOG(1) << "Gamma data successfully read from icc file";
+  return data;
 }
 
 }  // namespace
@@ -82,7 +64,9 @@ base::FilePath PathForDisplaySnapshot(const ui::DisplaySnapshot* snapshot) {
 DisplayColorManager::DisplayColorManager(
     ui::DisplayConfigurator* configurator,
     base::SequencedWorkerPool* blocking_pool)
-    : configurator_(configurator), blocking_pool_(blocking_pool) {
+    : configurator_(configurator),
+      blocking_pool_(blocking_pool),
+      weak_ptr_factory_(this) {
   configurator_->AddObserver(this);
 }
 
@@ -114,33 +98,56 @@ void DisplayColorManager::ApplyDisplayColorCalibration(int64_t display_id,
 
 void DisplayColorManager::LoadCalibrationForDisplay(
     const ui::DisplaySnapshot* display) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (display->display_id() == gfx::Display::kInvalidDisplayID) {
     LOG(WARNING) << "Trying to load calibration data for invalid display id";
     return;
   }
 
-  base::FilePath path = PathForDisplaySnapshot(display);
+  quirks::QuirksManager::Get()->RequestIccProfilePath(
+      display->product_id(),
+      base::Bind(&DisplayColorManager::FinishLoadCalibrationForDisplay,
+                 weak_ptr_factory_.GetWeakPtr(), display->display_id(),
+                 display->product_id(), display->type()));
+}
+
+void DisplayColorManager::FinishLoadCalibrationForDisplay(
+    int64_t display_id,
+    int64_t product_id,
+    ui::DisplayConnectionType type,
+    const base::FilePath& path,
+    bool file_downloaded) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  std::string product_string = quirks::IdToHexString(product_id);
+  if (path.empty()) {
+    VLOG(1) << "No ICC file found with product id: " << product_string
+            << " for display id: " << display_id;
+    return;
+  }
+
+  if (file_downloaded && type == ui::DISPLAY_CONNECTION_TYPE_INTERNAL) {
+    VLOG(1) << "Downloaded ICC file with product id: " << product_string
+            << " for internal display id: " << display_id
+            << ". Profile will be applied on next startup.";
+    return;
+  }
+
   VLOG(1) << "Loading ICC file " << path.value()
-          << " for display id: " << display->display_id()
-          << " with product id: " << display->product_id();
+          << " for display id: " << display_id
+          << " with product id: " << product_string;
 
-  scoped_ptr<ColorCalibrationData> data(new ColorCalibrationData());
-  base::Callback<bool(void)> request(
-      base::Bind(&ParseFile, path, base::Unretained(data.get())));
   base::PostTaskAndReplyWithResult(
-      blocking_pool_, FROM_HERE, request,
-      base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(),
-                 display->display_id(), display->product_id(),
-                 base::Passed(std::move(data))));
+      blocking_pool_, FROM_HERE, base::Bind(&ParseDisplayProfile, path),
+      base::Bind(&DisplayColorManager::UpdateCalibrationData,
+                 weak_ptr_factory_.GetWeakPtr(), display_id, product_id));
 }
 
 void DisplayColorManager::UpdateCalibrationData(
     int64_t display_id,
     int64_t product_id,
-    scoped_ptr<ColorCalibrationData> data,
-    bool success) {
-  DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI);
-  if (success) {
+    scoped_ptr<ColorCalibrationData> data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (data) {
     // The map takes over ownership of the underlying memory.
     calibration_map_[product_id] = data.release();
     ApplyDisplayColorCalibration(display_id, product_id);
diff --git a/ash/display/display_color_manager_chromeos.h b/ash/display/display_color_manager_chromeos.h
index b434e05..58e2d9c 100644
--- a/ash/display/display_color_manager_chromeos.h
+++ b/ash/display/display_color_manager_chromeos.h
@@ -14,15 +14,16 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "ui/display/chromeos/display_configurator.h"
-#include "ui/gfx/display.h"
-#include "ui/gfx/display_observer.h"
+#include "ui/display/types/display_constants.h"
 
 namespace base {
 class SequencedWorkerPool;
 }
 
 namespace ui {
+class DisplaySnapshot;
 struct GammaRampRGBEntry;
 }  // namespace ui
 
@@ -31,8 +32,7 @@ namespace ash {
 // An object that observes changes in display configuration applies any color
 // calibration where needed.
 class ASH_EXPORT DisplayColorManager
-    : public ui::DisplayConfigurator::Observer,
-      public base::SupportsWeakPtr<DisplayColorManager> {
+    : public ui::DisplayConfigurator::Observer {
  public:
   DisplayColorManager(ui::DisplayConfigurator* configurator,
                       base::SequencedWorkerPool* blocking_pool);
@@ -52,19 +52,28 @@ class ASH_EXPORT DisplayColorManager
     std::vector<ui::GammaRampRGBEntry> lut;
   };
 
+ protected:
+  virtual void FinishLoadCalibrationForDisplay(int64_t display_id,
+                                               int64_t product_id,
+                                               ui::DisplayConnectionType type,
+                                               const base::FilePath& path,
+                                               bool file_downloaded);
+  virtual void UpdateCalibrationData(int64_t display_id,
+                                     int64_t product_id,
+                                     scoped_ptr<ColorCalibrationData> data);
+
  private:
   void ApplyDisplayColorCalibration(int64_t display_id, int64_t product_id);
   void LoadCalibrationForDisplay(const ui::DisplaySnapshot* display);
-  void UpdateCalibrationData(
-      int64_t display_id,
-      int64_t product_id,
-      scoped_ptr<DisplayColorManager::ColorCalibrationData> data,
-      bool success);
 
   ui::DisplayConfigurator* configurator_;
   std::map<int64_t, ColorCalibrationData*> calibration_map_;
+  base::ThreadChecker thread_checker_;
   base::SequencedWorkerPool* blocking_pool_;
 
+  // Factory for callbacks.
+  base::WeakPtrFactory<DisplayColorManager> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(DisplayColorManager);
 };
 
diff --git a/ash/display/display_color_manager_chromeos_unittest.cc b/ash/display/display_color_manager_chromeos_unittest.cc
index 081042e..f99db71 100644
--- a/ash/display/display_color_manager_chromeos_unittest.cc
+++ b/ash/display/display_color_manager_chromeos_unittest.cc
@@ -13,6 +13,8 @@
 #include "base/test/scoped_path_override.h"
 #include "base/test/sequenced_worker_pool_owner.h"
 #include "chromeos/chromeos_paths.h"
+#include "components/quirks/quirks_manager.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/display/chromeos/test/action_logger_util.h"
 #include "ui/display/chromeos/test/test_display_snapshot.h"
@@ -22,30 +24,79 @@ namespace ash {
 
 namespace {
 
-// Monitors if any task is processed by the message loop.
-class TaskObserver : public base::MessageLoop::TaskObserver {
+const char kSetGammaAction[] =
+    "set_gamma_ramp(id=123,rgb[0]*rgb[255]=???????????\?)";
+
+class DisplayColorManagerForTest : public DisplayColorManager {
  public:
-  TaskObserver() { base::MessageLoop::current()->AddTaskObserver(this); }
-  ~TaskObserver() override {
-    base::MessageLoop::current()->RemoveTaskObserver(this);
-  }
+  DisplayColorManagerForTest(ui::DisplayConfigurator* configurator,
+                             base::SequencedWorkerPool* blocking_pool)
+      : DisplayColorManager(configurator, blocking_pool) {}
 
-  // MessageLoop::TaskObserver overrides.
-  void WillProcessTask(const base::PendingTask& pending_task) override {}
-  void DidProcessTask(const base::PendingTask& pending_task) override {
-    base::MessageLoop::current()->QuitWhenIdle();
+  void SetOnFinishedForTest(base::Closure on_finished_for_test) {
+    on_finished_for_test_ = on_finished_for_test;
   }
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(TaskObserver);
+  void FinishLoadCalibrationForDisplay(int64_t display_id,
+                                       int64_t product_id,
+                                       ui::DisplayConnectionType type,
+                                       const base::FilePath& path,
+                                       bool file_downloaded) override {
+    DisplayColorManager::FinishLoadCalibrationForDisplay(
+        display_id, product_id, type, path, file_downloaded);
+    // If path is empty, there is no icc file, and the DCM's work is done.
+    if (path.empty() && !on_finished_for_test_.is_null()) {
+      on_finished_for_test_.Run();
+      on_finished_for_test_.Reset();
+    }
+  }
+
+  void UpdateCalibrationData(int64_t display_id,
+                             int64_t product_id,
+                             scoped_ptr<ColorCalibrationData> data) override {
+    DisplayColorManager::UpdateCalibrationData(display_id, product_id,
+                                               std::move(data));
+    if (!on_finished_for_test_.is_null()) {
+      on_finished_for_test_.Run();
+      on_finished_for_test_.Reset();
+    }
+  }
+
+  base::Closure on_finished_for_test_;
+
+  DISALLOW_COPY_AND_ASSIGN(DisplayColorManagerForTest);
 };
 
-// Run at least one task. WARNING: Will not return unless there is at least one
-// task to be processed.
-void RunBlockingPoolTask() {
-  TaskObserver task_observer;
-  base::RunLoop().Run();
-}
+// Implementation of QuirksManager::Delegate to fake chrome-restricted parts.
+class QuirksManagerDelegateTestImpl : public quirks::QuirksManager::Delegate {
+ public:
+  QuirksManagerDelegateTestImpl(base::FilePath color_path)
+      : color_path_(color_path) {}
+
+  // Unused by these tests.
+  std::string GetApiKey() const override { return std::string(); }
+
+  base::FilePath GetBuiltInDisplayProfileDirectory() const override {
+    return color_path_;
+  }
+
+  // Unused by these tests.
+  base::FilePath GetDownloadDisplayProfileDirectory() const override {
+    return base::FilePath();
+  }
+
+  // Unused by these tests.
+  void GetDaysSinceOobe(
+      quirks::QuirksManager::DaysSinceOobeCallback callback) const override {}
+
+ private:
+  ~QuirksManagerDelegateTestImpl() override = default;
+
+  base::FilePath color_path_;
+
+  DISALLOW_COPY_AND_ASSIGN(QuirksManagerDelegateTestImpl);
+};
 
 }  // namespace
 
@@ -61,8 +112,8 @@ class DisplayColorManagerTest : public testing::Test {
     configurator_.SetDelegateForTesting(
         scoped_ptr<ui::NativeDisplayDelegate>(native_display_delegate_));
 
-    color_manager_.reset(
-        new DisplayColorManager(&configurator_, pool_owner_->pool().get()));
+    color_manager_.reset(new DisplayColorManagerForTest(
+        &configurator_, pool_owner_->pool().get()));
 
     EXPECT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &color_path_));
 
@@ -71,9 +122,23 @@ class DisplayColorManagerTest : public testing::Test {
                       .Append(FILE_PATH_LITERAL("test_data"));
     path_override_.reset(new base::ScopedPathOverride(
         chromeos::DIR_DEVICE_COLOR_CALIBRATION_PROFILES, color_path_));
+
+    quirks::QuirksManager::Initialize(
+        scoped_ptr<quirks::QuirksManager::Delegate>(
+            new QuirksManagerDelegateTestImpl(color_path_)),
+        pool_owner_->pool().get(), nullptr, nullptr);
+  }
+
+  void TearDown() override {
+    quirks::QuirksManager::Shutdown();
+    pool_owner_->pool()->Shutdown();
   }
 
-  void TearDown() override { pool_owner_->pool()->Shutdown(); }
+  void WaitOnColorCalibration() {
+    base::RunLoop run_loop;
+    color_manager_->SetOnFinishedForTest(run_loop.QuitClosure());
+    run_loop.Run();
+  }
 
   DisplayColorManagerTest() : test_api_(&configurator_) {}
   ~DisplayColorManagerTest() override {}
@@ -85,7 +150,7 @@ class DisplayColorManagerTest : public testing::Test {
   ui::DisplayConfigurator configurator_;
   ui::DisplayConfigurator::TestApi test_api_;
   ui::test::TestNativeDisplayDelegate* native_display_delegate_;  // not owned
-  scoped_ptr<DisplayColorManager> color_manager_;
+  scoped_ptr<DisplayColorManagerForTest> color_manager_;
 
   base::MessageLoopForUI ui_message_loop_;
   scoped_ptr<base::SequencedWorkerPoolOwner> pool_owner_;
@@ -114,13 +179,10 @@ TEST_F(DisplayColorManagerTest, VCGTOnly) {
 
   configurator_.OnConfigurationChanged();
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
-  log_->GetActionsAndClear();
-
-  RunBlockingPoolTask();
 
-  EXPECT_TRUE(base::MatchPattern(
-      log_->GetActionsAndClear(),
-      "set_gamma_ramp(id=123,rgb[0]*rgb[255]=???????????\?)"));
+  log_->GetActionsAndClear();
+  WaitOnColorCalibration();
+  EXPECT_TRUE(base::MatchPattern(log_->GetActionsAndClear(), kSetGammaAction));
 }
 
 TEST_F(DisplayColorManagerTest, NoMatchProductID) {
@@ -145,7 +207,7 @@ TEST_F(DisplayColorManagerTest, NoMatchProductID) {
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
 
   log_->GetActionsAndClear();
-  RunBlockingPoolTask();
+  WaitOnColorCalibration();
   EXPECT_STREQ("", log_->GetActionsAndClear().c_str());
 }
 
@@ -171,7 +233,7 @@ TEST_F(DisplayColorManagerTest, NoVCGT) {
   EXPECT_TRUE(test_api_.TriggerConfigureTimeout());
 
   log_->GetActionsAndClear();
-  RunBlockingPoolTask();
+  WaitOnColorCalibration();
   EXPECT_STREQ("", log_->GetActionsAndClear().c_str());
 }