Quirks Client for downloading and providing display profiles

ash/display/display_color_manager_chromeos.cc

Index: ash/display/display_color_manager_chromeos.cc
diff --git a/ash/display/display_color_manager_chromeos.cc b/ash/display/display_color_manager_chromeos.cc
index 1241978958018acff967d30fbfd9dcb2d0c961dd..a992ee315259f86d63cf7de186488790501fa41b 100644
--- a/ash/display/display_color_manager_chromeos.cc
+++ b/ash/display/display_color_manager_chromeos.cc
@@ -8,7 +8,6 @@
 
 #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"
@@ -20,6 +19,7 @@
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chromeos/chromeos_paths.h"
+#include "components/quirks_client/quirks_client.h"
 #include "third_party/qcms/src/qcms.h"
 #include "ui/display/types/display_snapshot.h"
 #include "ui/display/types/gamma_ramp_rgb_entry.h"
@@ -31,8 +31,24 @@ namespace ash {
 
 namespace {
 
-bool ParseFile(const base::FilePath& path,
-               DisplayColorManager::ColorCalibrationData* data) {
+bool ParseDisplayProfile(const int64_t display_id,
+                         const int64_t product_id,
+                         DisplayColorManager::ColorCalibrationData* data) {
+  quirks_client::QuirksClient::DownloadFinishedCallback* callback = nullptr;

[oshima, 2016/01/28 20:16:30]

do you need this variable?

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

No, was just a placeholder for the TODO comment.

+  // TODO(glevin): Do we want to add a callback so that we can apply the icc
+  // profile as soon as it's downloaded?

[oshima, 2016/01/28 20:16:30]

instead, explain why we're not doing now, and why we may want to do so.

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

Done.

+  base::FilePath path =
+      quirks_client::QuirksClient::RequestIccProfilePath(product_id, callback);
+  std::string product_string = base::StringPrintf("0x%08" PRIx64, product_id);
+  if (path.empty()) {
+    VLOG(1) << "No icc file found with product id: " << product_string
+            << " for display id: " << display_id;

[stevenjb, 2016/01/28 21:05:58]

Since PathExists() will fail for an empty path, just return false here and elim
the else.

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

Done.  (Also changed "if (!PathExists) return" to DCHECK, since the QC should
only return a non-empty path if the file actually exists.)

+  } else {
+    VLOG(1) << "Loading ICC file " << path.value()
+            << " for display id: " << display_id
+            << " with product id: " << product_string;
+  }
+
   if (!base::PathExists(path))  // No icc file for this display; not an error.
     return false;
   qcms_profile* display_profile = qcms_profile_from_path(path.value().c_str());
@@ -65,18 +81,10 @@ bool ParseFile(const base::FilePath& path,
     data->lut[i].b = vcgt_data[(vcgt_channel_length * 2) + i];
   }
   qcms_profile_release(display_profile);
+  VLOG(1) << "Gamma data successfully read from icc file";
   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;
-}
-
 }  // namespace
 
 DisplayColorManager::DisplayColorManager(
@@ -119,14 +127,10 @@ void DisplayColorManager::LoadCalibrationForDisplay(
     return;
   }
 
-  base::FilePath path = PathForDisplaySnapshot(display);
-  VLOG(1) << "Loading ICC file " << path.value()
-          << " for display id: " << display->display_id()
-          << " with product id: " << display->product_id();
-
   scoped_ptr<ColorCalibrationData> data(new ColorCalibrationData());

[oshima, 2016/01/28 20:16:30]

can't you do

scoped_ptr<Data> ParseProfile()

UpdateCalibrationData(..., scoped_ptr<Data>)

instead of creating the result here and pass to both?

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

Seems to work.  (Wasn't my code, but still a good simplification.)

   base::Callback<bool(void)> request(
-      base::Bind(&ParseFile, path, base::Unretained(data.get())));
+      base::Bind(&ParseDisplayProfile, display->display_id(),
+                 display->product_id(), base::Unretained(data.get())));
   base::PostTaskAndReplyWithResult(
       blocking_pool_, FROM_HERE, request,
       base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(),