Give user ability to file a feedback report from the display error no…

…tification.

Make the display error notification clickable and starts the feedback app on click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually

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

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

ash/ash_chromeos_strings.grdp

@@ -434,10 +434,10 @@ Press Shift + Alt to switch.
     Show on-screen keyboard
   </message>
   <message name="IDS_ASH_DISPLAY_FAILURE_ON_MIRRORING" desc="An error message to show that the system failed to enter the mirroring mode.">
-    Could not mirror displays since no supported resolutions found. Entered extended desktop instead.
+    Could not mirror displays since no supported resolutions found. Entered extended desktop instead. Click to send a feedback report.
   </message>
   <message name="IDS_ASH_DISPLAY_FAILURE_ON_NON_MIRRORING" desc="An error message to show that the system failed to enter the extended desktop mode or unknown status. Please translate the parentized text.">
-    Dear Monitor, it's not working out between us. (That monitor is not supported)
+    Dear Monitor, it's not working out between us. (That monitor is not supported. Click to send a feedback report.)
   </message>
   <message name="IDS_ASH_DISPLAY_RESOLUTION_CHANGE_ACCEPT" desc="A button label shown in the notification for the resolution change to accept the change">
     Accept

ash/display/display_error_observer_chromeos.cc

@@ -4,9 +4,13 @@
 
 #include "ash/display/display_error_observer_chromeos.h"
 
+#include <cinttypes>
 #include <utility>
 
+#include "ash/new_window_delegate.h"
+#include "ash/shell.h"
 #include "ash/system/system_notifier.h"
+#include "base/strings/string_number_conversions.h"
 #include "grit/ash_resources.h"
 #include "grit/ash_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -23,6 +27,27 @@ namespace {
 
 const char kDisplayErrorNotificationId[] = "chrome://settings/display/error";
 
+// A notification delegate that will start the feedback app when the notication
+// is clicked.
+class DisplayErrorNotificationDelegate
+    : public message_center::NotificationDelegate {
+ public:
+  DisplayErrorNotificationDelegate() = default;
+
+  // message_center::NotificationDelegate:
+  bool HasClickedListener() override { return true; }
+
+  void Click() override {
+    ash::Shell::GetInstance()->new_window_delegate()->OpenFeedbackPage();
+  }
+
+ private:
+  // Private destructor since NotificationDelegate is ref-counted.
+  ~DisplayErrorNotificationDelegate() override = default;
+
+  DISALLOW_COPY_AND_ASSIGN(DisplayErrorNotificationDelegate);
+};
+
 }  // namespace
 
 DisplayErrorObserver::DisplayErrorObserver() {
@@ -34,6 +59,14 @@ DisplayErrorObserver::~DisplayErrorObserver() {
 void DisplayErrorObserver::OnDisplayModeChangeFailed(
     const ui::DisplayConfigurator::DisplayStateList& displays,
     ui::MultipleDisplayState new_state) {
+  LOG(ERROR) << "Failed to configure the following display(s):";
+  for (const auto& display : displays) {
+    LOG(ERROR) << "- Display with ID = " << display->display_id()
+               << ", and EDID = " << base::HexEncode(display->edid().data(),
+                                                     display->edid().size())
+               << ".";
+  }
+
   // Always remove the notification to make sure the notification appears
   // as a popup in any situation.
   message_center::MessageCenter::Get()->RemoveNotification(
@@ -53,7 +86,8 @@ void DisplayErrorObserver::OnDisplayModeChangeFailed(
       GURL(),
       message_center::NotifierId(message_center::NotifierId::SYSTEM_COMPONENT,
                                  system_notifier::kNotifierDisplayError),
-      message_center::RichNotificationData(), NULL));
+      message_center::RichNotificationData(),
+      new DisplayErrorNotificationDelegate));
   message_center::MessageCenter::Get()->AddNotification(
       std::move(notification));
 }

ui/display/chromeos/display_snapshot_virtual.cc

@@ -31,6 +31,7 @@ DisplaySnapshotVirtual::DisplaySnapshotVirtual(int64_t display_id,
                       "Virtual display",
                       base::FilePath(),
                       std::vector<const DisplayMode*>(),
+                      std::vector<uint8_t>(),  // Virtual displays have no EDID.
                       nullptr,
                       nullptr) {
   mode_.reset(new DisplayMode(display_size, false, 30));

ui/display/chromeos/test/test_display_snapshot.cc

@@ -15,6 +15,7 @@ TestDisplaySnapshot::TestDisplaySnapshot()
                       std::string(),
                       base::FilePath(),
                       std::vector<const DisplayMode*>(),
+                      std::vector<uint8_t>(),
                       NULL,
                       NULL) {}
 
@@ -37,6 +38,7 @@ TestDisplaySnapshot::TestDisplaySnapshot(
                       std::string(),
                       base::FilePath(),
                       modes,
+                      std::vector<uint8_t>(),
                       current_mode,
                       native_mode) {
   product_id_ = product_id;

ui/display/chromeos/x11/display_snapshot_x11.cc

@@ -18,6 +18,7 @@ DisplaySnapshotX11::DisplaySnapshotX11(
     bool has_overscan,
     std::string display_name,
     const std::vector<const DisplayMode*>& modes,
+    const std::vector<uint8_t>& edid,
     const DisplayMode* current_mode,
     const DisplayMode* native_mode,
     RROutput output,
@@ -34,6 +35,7 @@ DisplaySnapshotX11::DisplaySnapshotX11(
                       //                 descriptor that maps to the device.
                       base::FilePath(),
                       modes,
+                      edid,
                       current_mode,
                       native_mode),
       output_(output),

ui/display/chromeos/x11/display_snapshot_x11.h

@@ -28,6 +28,7 @@ class DISPLAY_EXPORT DisplaySnapshotX11 : public DisplaySnapshot {
                      bool has_overscan,
                      std::string display_name,
                      const std::vector<const DisplayMode*>& modes,
+                     const std::vector<uint8_t>& edid,
                      const DisplayMode* current_mode,
                      const DisplayMode* native_mode,
                      RROutput output,

ui/display/chromeos/x11/native_display_delegate_x11.cc

@@ -311,11 +311,12 @@ DisplaySnapshotX11* NativeDisplayDelegateX11::InitDisplaySnapshot(
     std::set<RRCrtc>* last_used_crtcs,
     int index) {
   int64_t display_id = 0;
-  if (!GetDisplayId(output, static_cast<uint8_t>(index), &display_id))
+  ui::EDIDParserX11 edid_parser(output);
+  if (!edid_parser.GetDisplayId(static_cast<uint8_t>(index), &display_id))
     display_id = index;
 
   bool has_overscan = false;
-  GetOutputOverscanFlag(output, &has_overscan);
+  edid_parser.GetOutputOverscanFlag(&has_overscan);
 
   DisplayConnectionType type = GetDisplayConnectionTypeFromName(info->name);
   if (type == DISPLAY_CONNECTION_TYPE_UNKNOWN)
@@ -367,8 +368,9 @@ DisplaySnapshotX11* NativeDisplayDelegateX11::InitDisplaySnapshot(
                              type,
                              IsOutputAspectPreservingScaling(output),
                              has_overscan,
-                             GetDisplayName(output),
+                             edid_parser.GetDisplayName(),
                              display_modes,
+                             edid_parser.edid(),
                              current_mode,
                              native_mode,
                              output,

ui/display/chromeos/x11/native_display_event_dispatcher_x11_unittest.cc

@@ -36,6 +36,7 @@ DisplaySnapshotX11* CreateOutput(int64_t id,
       false,
       std::string(),
       std::vector<const DisplayMode*>(1, kDefaultDisplayMode),
+      std::vector<uint8_t>(),
       kDefaultDisplayMode,
       NULL,
       output,

ui/display/types/display_snapshot.cc

@@ -6,6 +6,16 @@
 
 namespace ui {
 
+namespace {
+
+// The display serial number beginning byte position and its length in the
+// EDID number as defined in the spec.
+// https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
+const size_t kSerialNumberBeginingByte = 12U;
+const size_t kSerialNumberLengthInBytes = 4U;
+
+}  // namespace
+
 DisplaySnapshot::DisplaySnapshot(int64_t display_id,
                                  const gfx::Point& origin,
                                  const gfx::Size& physical_size,
@@ -15,6 +25,7 @@ DisplaySnapshot::DisplaySnapshot(int64_t display_id,
                                  std::string display_name,
                                  const base::FilePath& sys_path,
                                  const std::vector<const DisplayMode*>& modes,
+                                 const std::vector<uint8_t>& edid,
                                  const DisplayMode* current_mode,
                                  const DisplayMode* native_mode)
     : display_id_(display_id),
@@ -26,9 +37,17 @@ DisplaySnapshot::DisplaySnapshot(int64_t display_id,
       display_name_(display_name),
       sys_path_(sys_path),
       modes_(modes),
+      edid_(edid),
       current_mode_(current_mode),
       native_mode_(native_mode),
-      product_id_(kInvalidProductID) {}
+      product_id_(kInvalidProductID) {
+  // We must explicitly clear out the bytes that represent the serial number.
+  const size_t end =
+      std::min(kSerialNumberBeginingByte + kSerialNumberLengthInBytes,
+               edid_.size());
+  for (size_t i = kSerialNumberBeginingByte; i < end; ++i)
+    edid_[i] = 0;
+}
 
 DisplaySnapshot::~DisplaySnapshot() {}
 

ui/display/types/display_snapshot.h

@@ -32,6 +32,7 @@ class DISPLAY_TYPES_EXPORT DisplaySnapshot {
                   std::string display_name,
                   const base::FilePath& sys_path,
                   const std::vector<const DisplayMode*>& modes,
+                  const std::vector<uint8_t>& edid,
                   const DisplayMode* current_mode,
                   const DisplayMode* native_mode);
   virtual ~DisplaySnapshot();
@@ -53,6 +54,7 @@ class DISPLAY_TYPES_EXPORT DisplaySnapshot {
   int64_t product_id() const { return product_id_; }
 
   const std::vector<const DisplayMode*>& modes() const { return modes_; }
+  const std::vector<uint8_t>& edid() const { return edid_; }
 
   void set_current_mode(const DisplayMode* mode) { current_mode_ = mode; }
   void set_origin(const gfx::Point& origin) { origin_ = origin; }
@@ -85,6 +87,10 @@ class DISPLAY_TYPES_EXPORT DisplaySnapshot {
 
   std::vector<const DisplayMode*> modes_;  // Not owned.
 
+  // The display's EDID. It can be empty if nothing extracted such as in the
+  // case of a virtual display.
+  std::vector<uint8_t> edid_;
+
   // Mode currently being used by the output.
   const DisplayMode* current_mode_;
 

ui/display/util/x11/edid_parser_x11.cc

@@ -75,48 +75,35 @@ bool GetEDIDProperty(XID output, std::vector<uint8_t>* edid) {
   return true;
 }
 
-// Gets some useful data from the specified output device, such like
-// manufacturer's ID, product code, and human readable name. Returns false if it
-// fails to get those data and doesn't touch manufacturer ID/product code/name.
-// nullptr can be passed for unwanted output parameters.
-bool GetOutputDeviceData(XID output,
-                         uint16_t* manufacturer_id,
-                         std::string* human_readable_name) {
-  std::vector<uint8_t> edid;
-  if (!GetEDIDProperty(output, &edid))
-    return false;
+}  // namespace
 
-  return ParseOutputDeviceData(edid, manufacturer_id, nullptr,
-                               human_readable_name, nullptr, nullptr);
+EDIDParserX11::EDIDParserX11(XID output_id)
+    : output_id_(output_id) {
+  GetEDIDProperty(output_id_, &edid_);
 }
 
-}  // namespace
+EDIDParserX11::~EDIDParserX11() {
+}
 
-bool GetDisplayId(XID output_id,
-                  uint8_t output_index,
-                  int64_t* display_id_out) {
-  std::vector<uint8_t> edid;
-  if (!GetEDIDProperty(output_id, &edid))
+bool EDIDParserX11::GetDisplayId(uint8_t index, int64_t* out_display_id) const {
+  if (edid_.empty())
     return false;
 
-  bool result =
-      GetDisplayIdFromEDID(edid, output_index, display_id_out, nullptr);
-  return result;
+  return GetDisplayIdFromEDID(edid_, index, out_display_id, nullptr);
 }
 
-std::string GetDisplayName(RROutput output) {
+std::string EDIDParserX11::GetDisplayName() const {
   std::string display_name;
-  GetOutputDeviceData(output, nullptr, &display_name);
+  ParseOutputDeviceData(edid_, nullptr, nullptr, &display_name, nullptr,
+                        nullptr);
   return display_name;
 }
 
-bool GetOutputOverscanFlag(RROutput output, bool* flag) {
-  std::vector<uint8_t> edid;
-  if (!GetEDIDProperty(output, &edid))
+bool EDIDParserX11::GetOutputOverscanFlag(bool* out_flag) const {
+  if (edid_.empty())
     return false;
 
-  bool found = ParseOutputOverscanFlag(edid, flag);
-  return found;
+  return ParseOutputOverscanFlag(edid_, out_flag);
 }
 
 }  // namespace ui

ui/display/util/x11/edid_parser_x11.h

@@ -8,7 +8,9 @@
 #include <stdint.h>
 
 #include <string>
+#include <vector>
 
+#include "base/macros.h"
 #include "ui/display/types/display_constants.h"
 #include "ui/display/util/display_util_export.h"
 
@@ -19,21 +21,39 @@ typedef XID RROutput;
 
 namespace ui {
 
-// Gets the EDID data from |output| and generates the display id through
-// |GetDisplayIdFromEDID|.
-DISPLAY_UTIL_EXPORT bool GetDisplayId(XID output,
-                                      uint8_t index,
-                                      int64_t* display_id_out);
-
-// Generate the human readable string from EDID obtained from |output|.
-DISPLAY_UTIL_EXPORT std::string GetDisplayName(RROutput output);
-
-// Gets the overscan flag from |output| and stores to |flag|. Returns true if
-// the flag is found. Otherwise returns false and doesn't touch |flag|. The
-// output will produce overscan if |flag| is set to true, but the output may
-// still produce overscan even though it returns true and |flag| is set to
-// false.
-DISPLAY_UTIL_EXPORT bool GetOutputOverscanFlag(RROutput output, bool* flag);
+// Xrandr utility class to help get EDID information.
+class DISPLAY_UTIL_EXPORT EDIDParserX11 {
+ public:
+  EDIDParserX11(XID output_id);
+  ~EDIDParserX11();
+
+  // Sets |out_display_id| to the display ID from the EDID of this output.
+  // Returns true if successful, false otherwise.
+  bool GetDisplayId(uint8_t index, int64_t* out_display_id) const;
+
+  // Generate the human readable string from EDID obtained from |output|.
+  // Returns an empty string upon error.
+  std::string GetDisplayName() const;
+
+  // Gets the overscan flag and stores to |out_flag|. Returns true if the flag
+  // is found. Otherwise returns false and doesn't touch |out_flag|. The output
+  // will produce overscan if |out_flag| is set to true, but the output may
+  // still produce overscan even though it returns true and |out_flag| is set to
+  // false.
+  bool GetOutputOverscanFlag(bool* out_flag) const;
+
+  XID output_id() const { return output_id_; }
+  const std::vector<uint8_t>& edid() const { return edid_; }
+
+ private:
+  XID output_id_;
+
+  // This will be an empty vector upon failure to get the EDID from the
+  // |output_id_|.
+  std::vector<uint8_t> edid_;
+
+  DISALLOW_COPY_AND_ASSIGN(EDIDParserX11);
+};
 
 }  // namespace ui
 

ui/ozone/common/display_snapshot_proxy.cc

@@ -30,6 +30,7 @@ DisplaySnapshotProxy::DisplaySnapshotProxy(const DisplaySnapshot_Params& params)
                       params.display_name,
                       params.sys_path,
                       std::vector<const DisplayMode*>(),
+                      params.edid,
                       NULL,
                       NULL),
       string_representation_(params.string_representation) {

ui/ozone/common/display_util.cc

@@ -54,6 +54,8 @@ DisplaySnapshot_Params GetDisplaySnapshotParams(
   for (size_t i = 0; i < display.modes().size(); ++i)
     params.modes.push_back(GetDisplayModeParams(*display.modes()[i]));
 
+  params.edid = display.edid();
+
   params.has_current_mode = display.current_mode() != NULL;
   if (params.has_current_mode)
     params.current_mode = GetDisplayModeParams(*display.current_mode());

ui/ozone/common/gpu/ozone_gpu_message_params.h

@@ -43,6 +43,7 @@ struct OZONE_BASE_EXPORT DisplaySnapshot_Params {
   std::string display_name;
   base::FilePath sys_path;
   std::vector<DisplayMode_Params> modes;
+  std::vector<uint8_t> edid;
   bool has_current_mode = false;
   DisplayMode_Params current_mode;
   bool has_native_mode = false;

ui/ozone/common/gpu/ozone_gpu_messages.h

@@ -51,6 +51,7 @@ IPC_STRUCT_TRAITS_BEGIN(ui::DisplaySnapshot_Params)
   IPC_STRUCT_TRAITS_MEMBER(display_name)
   IPC_STRUCT_TRAITS_MEMBER(sys_path)
   IPC_STRUCT_TRAITS_MEMBER(modes)
+  IPC_STRUCT_TRAITS_MEMBER(edid)
   IPC_STRUCT_TRAITS_MEMBER(has_current_mode)
   IPC_STRUCT_TRAITS_MEMBER(current_mode)
   IPC_STRUCT_TRAITS_MEMBER(has_native_mode)