Quirks Client for downloading and providing display profiles

ash/display/display_color_manager_chromeos.cc

 // Copyright 2015 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 "ash/display/display_color_manager_chromeos.h"
 
 #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 =
       qcms_profile_get_vcgt_channel_length(display_profile);
   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;
   vcgt_data.resize(vcgt_channel_length * 3);
   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];
     data->lut[i].g = vcgt_data[vcgt_channel_length + i];
     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
 
 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);
 }
 
 DisplayColorManager::~DisplayColorManager() {
   configurator_->RemoveObserver(this);
   STLDeleteValues(&calibration_map_);
 }
 
 void DisplayColorManager::OnDisplayModeChanged(
     const ui::DisplayConfigurator::DisplayStateList& display_states) {
@@ skipping 11 matching lines @@
                                                        int64_t product_id) {
   if (calibration_map_.find(product_id) != calibration_map_.end()) {
     ColorCalibrationData* ramp = calibration_map_[product_id];
     if (!configurator_->SetGammaRamp(display_id, ramp->lut))
       LOG(WARNING) << "Error applying gamma ramp";
   }
 }
 
 void DisplayColorManager::LoadCalibrationForDisplay(
     const ui::DisplaySnapshot* display) {
+  DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI);

[Bernhard Bauer, 2016/03/22 09:58:05]

Nit: Put the expected value first, for nicer error messages in case of failure.

[Greg Levin, 2016/03/22 17:49:29]

Done, but...
Is this the right check to be doing here?  There's only one other place that I
could find that TYPE_UI was being DCHECKed, and that was in actual thread code:
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/base/auto...

Should I just remove these, or replace them with a more standard check?

[Bernhard Bauer, 2016/03/22 18:46:34]

Hm... I would probably require that this class is used on a single thread, so
you'd use a ThreadChecker like in QuirksManager.

Right now you are saying that you require the method to be called on a thread
that has a message loop suitable for running UI, and no platform does UI on more
than one thread (right?), but I don't think MessageLoopForUI enforces that, i.e.
you could totally use a MessageLoop that _can_ run UI on a background thread
(you just wouldn't use UI), and given that QuirksManager is single-threaded and
there is only one instance of it, this method can really only be called on a
thread where QuirksManager can be used, so it's effectively single-threaded as
well.

[stevenjb, 2016/03/22 21:29:49]

Yeah, I think that is probably not the best test. I would suggest using a
thread_checker_ instance here just like with QuirksManager.

[Greg Levin, 2016/03/23 00:16:17]

Done.

   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()));
+}
+
+void DisplayColorManager::FinishLoadCalibrationForDisplay(
+    int64_t display_id,
+    int64_t product_id,
+    const base::FilePath& path,
+    bool file_downloaded) {
+  DCHECK_EQ(base::MessageLoop::current()->type(), base::MessageLoop::TYPE_UI);
+  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;
+  }
+
   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) {
+    scoped_ptr<ColorCalibrationData> 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);
   }
 }
 
 DisplayColorManager::ColorCalibrationData::ColorCalibrationData() {}
 
 DisplayColorManager::ColorCalibrationData::~ColorCalibrationData() {}
 
 }  // namespace ash