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..8829ae386294a4f2ad7282074af82ad8eaaff12e 100644
--- a/ash/display/display_color_manager_chromeos.cc
+++ b/ash/display/display_color_manager_chromeos.cc
@@ -6,40 +6,44 @@
 
 #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_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"
-#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;
+typedef scoped_ptr<DisplayColorManager::ColorCalibrationData> ColorDataPtr;

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

typedefing scoped_ptr<> like this is uncommon in Chrome, I would avoid it.
(Also, we prefer 'using' to 'typedef' now).

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

Done.  (Do you have any alternate suggestions for mitigating the slight
formatting mess caused by using
"scoped_ptr<DisplayColorManager::ColorCalibrationData>" in multiple places?)

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

DisplayColorManager:: isn't needed in several places so you can remove that.
otherwise I think the extra verosity is fine. clang-format is your friend.

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

Done.

+
+ColorDataPtr ParseDisplayProfile(const int64_t display_id,
+                                 const int64_t product_id) {
+  // TODO(glevin): At some point, we may want to add a callback, and apply the
+  // icc file as soon as it's downloaded.  For now, we'll just apply it on the

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

nit: only one space between sentences

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

Done.

+  // next restart.

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

The wording is a little confusing, it sounds like this code is involved with the
restart process, but I assume you just mean something like 'For now we only
apply the icc file if it exists at startup.'

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

Done.

+  base::FilePath path =
+      quirks_client::QuirksClient::RequestIccProfilePath(product_id, nullptr);

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

See other comments, but rather than passing null as a callback, create a local
DoNothing method (or use base::DoNothing if the callback has no args).

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

Done.

+  std::string product_string =
+      quirks_client::QuirksClient::IdToHexString(product_id);
+  if (path.empty()) {
+    VLOG(1) << "No icc file found with product id: " << product_string
+            << " for display id: " << display_id;
+    return nullptr;
+  }
+
+  VLOG(1) << "Loading ICC file " << path.value()
+          << " for display id: " << display_id
+          << " with product id: " << product_string;
+
+  DCHECK(base::PathExists(path));  // Quirks Client should ensure this.
   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 +51,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 +59,10 @@ 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;
   }
 
+  ColorDataPtr data;

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

I would expect this to crash since data is not being allocated here...?

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

Done.  (Ugh, thanks for saving me from debugging that >_<)

   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 +70,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;

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

You may need to used data.Pass() here, although I know we've been adding more
C++11 functionality to scoped_ptr so I'm not longer certain where we need that.

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

It doesn't seem to be necessary anymore.  The code seems to work without it
(and, in fact, data.Pass() doesn't even compile: "error: no member named 'Pass'
in 'scoped_ptr<...>").
Also, the current version of
https://www.chromium.org/developers/smart-pointer-guidelines seems to indicate
that it's not needed.

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

Cool. Cheers.

 }
 
 }  // namespace
@@ -119,28 +116,19 @@ 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());
-  base::Callback<bool(void)> request(
-      base::Bind(&ParseFile, path, base::Unretained(data.get())));
+  base::Callback<ColorDataPtr(void)> request(base::Bind(

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

I would typedef the callback instead of the scoped_ptr, e.g.:

using CalibrationDataCallback =
base::Callback(scoped_ptr<ColorCalibrationData>(void)>;

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

Happy to make the change, but for my benefit, what do you see as the advantage?
I typdef'd the scoped_ptr because it was used in 4 places, and made the code
cleaner in each of them.  I think this typedef'd Callback would only get used in
this one place, and so would actually increase overall code length.  If this is
a desirable change, I'd like to understand why for future reference.  Thanks!

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

Using scoped_ptr explicitly makes it more clear that the callback takes a
familiar scoped_ptr. It's worth the extra verbosity.

Also, looking at the code again, I would personally just inline the callback in
PostTaskAndReplyWithResult() the same as you did for the 'reply' parameter
instead of using a local variable at all.

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

Done.

+      &ParseDisplayProfile, display->display_id(), display->product_id()));
   base::PostTaskAndReplyWithResult(
       blocking_pool_, FROM_HERE, request,
       base::Bind(&DisplayColorManager::UpdateCalibrationData, AsWeakPtr(),
-                 display->display_id(), display->product_id(),
-                 base::Passed(std::move(data))));
+                 display->display_id(), display->product_id()));
 }
 
-void DisplayColorManager::UpdateCalibrationData(
-    int64_t display_id,
-    int64_t product_id,
-    scoped_ptr<ColorCalibrationData> data,
-    bool success) {
+void DisplayColorManager::UpdateCalibrationData(int64_t display_id,
+                                                int64_t product_id,
+                                                ColorDataPtr data) {
   DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI);
-  if (success) {
+  if (data) {
     // The map takes over ownership of the underlying memory.
     calibration_map_[product_id] = data.release();
     ApplyDisplayColorCalibration(display_id, product_id);