Add FakeDisplaySnapshot builder and related changes.

ui/display/fake_display_delegate.cc

Index: ui/display/fake_display_delegate.cc
diff --git a/ui/display/fake_display_delegate.cc b/ui/display/fake_display_delegate.cc
index b1fda156787f78b0db57989708a5fe7acc0b54a0..7337f94fa05d27386dad7b33984b723737603f2a 100644
--- a/ui/display/fake_display_delegate.cc
+++ b/ui/display/fake_display_delegate.cc
@@ -5,6 +5,7 @@
 #include "ui/display/fake_display_delegate.h"
 
 #include <string>
+#include <utility>
 
 #include "base/command_line.h"
 #include "base/hash.h"
@@ -34,30 +35,49 @@ FakeDisplayDelegate::FakeDisplayDelegate() {}
 FakeDisplayDelegate::~FakeDisplayDelegate() {}
 
 int64_t FakeDisplayDelegate::AddDisplay(const gfx::Size& display_size) {
+  DCHECK(!display_size.IsEmpty());
+
   if (next_display_id_ == 0xFF) {
     LOG(ERROR) << "Exceeded display id limit";
-    return display::Display::kInvalidDisplayID;
+    return Display::kInvalidDisplayID;
   }
 
   int64_t id = GenerateDisplayID(kReservedManufacturerID, kProductCodeHash,
                                  ++next_display_id_);
+
+  FakeDisplaySnapshot::Builder builder;
+  builder.SetId(id).SetNativeMode(display_size);
+  builder.SetName(base::StringPrintf("Fake Display %d", next_display_id_));
+
   // Add the first display as internal.
-  ui::DisplayConnectionType type = displays_.empty()
-                                       ? ui::DISPLAY_CONNECTION_TYPE_INTERNAL
-                                       : ui::DISPLAY_CONNECTION_TYPE_UNKNOWN;
-  std::string name = base::StringPrintf("Fake Display %d", next_display_id_);
+  if (displays_.empty())
+    builder.SetType(ui::DISPLAY_CONNECTION_TYPE_INTERNAL);
+
+  return AddDisplay(builder.Build()) ? id : Display::kInvalidDisplayID;
+}
 
-  displays_.push_back(
-      base::MakeUnique<FakeDisplaySnapshot>(id, display_size, type, name));
+bool FakeDisplayDelegate::AddDisplay(
+    std::unique_ptr<ui::DisplaySnapshot> display) {
+  DCHECK(display);
+
+  // Check there is no existing display with the same id.
+  for (auto& existing_display : displays_) {
+    if (existing_display->display_id() == display->display_id())
+      return false;

[Daniel Erat, 2016/09/20 17:12:49]

nit: worthwhile logging a warning or error here?

[kylechar, 2016/09/20 17:47:40]

Done.

+  }
+
+  DVLOG(1) << "Added display " << display->ToString();
+  displays_.push_back(std::move(display));
   OnConfigurationChanged();
 
-  return id;
+  return true;
 }
 
 bool FakeDisplayDelegate::RemoveDisplay(int64_t display_id) {
   // Find display snapshot with matching id and remove it.
   for (auto iter = displays_.begin(); iter != displays_.end(); ++iter) {
     if ((*iter)->display_id() == display_id) {
+      DVLOG(1) << "Removed display " << (*iter)->ToString();
       displays_.erase(iter);
       OnConfigurationChanged();
       return true;
@@ -69,10 +89,9 @@ bool FakeDisplayDelegate::RemoveDisplay(int64_t display_id) {
 
 void FakeDisplayDelegate::Initialize() {
   DCHECK(!initialized_);
-  InitFromCommandLine();
 
   // If no command line flags are provided then initialize a default display.
-  if (displays_.empty())
+  if (!InitFromCommandLine())
     AddDisplay(gfx::Size(1024, 768));
 
   initialized_ = true;
@@ -162,26 +181,58 @@ FakeDisplayController* FakeDisplayDelegate::GetFakeDisplayController() {
   return static_cast<FakeDisplayController*>(this);
 }
 
-void FakeDisplayDelegate::InitFromCommandLine() {
+bool FakeDisplayDelegate::InitFromCommandLine() {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (!command_line->HasSwitch(switches::kScreenConfig))
-    return;
+    return false;
 
   const std::string command_string =
       command_line->GetSwitchValueASCII(switches::kScreenConfig);
 
+  // Start without any displays.
+  if (command_string == "none")
+    return true;
+
   // Split on commas and parse each display string.
   for (std::string part : base::SplitString(
            command_string, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL)) {
-    int width = 0;
-    int height = 0;
-    if (sscanf(part.c_str(), "%dx%d", &width, &height) >= 2) {
-      gfx::Size display_size(width, height);
-      AddDisplay(display_size);
+    std::unique_ptr<ui::DisplaySnapshot> snapshot =
+        CreateSnapshotFromSpec(part);
+    if (snapshot) {
+      AddDisplay(std::move(snapshot));
     } else {
       LOG(ERROR) << "Failed to parse display \"" << part << "\"";
     }
   }
+
+  return true;
+}
+
+std::unique_ptr<ui::DisplaySnapshot>
+FakeDisplayDelegate::CreateSnapshotFromSpec(const std::string& spec) {
+  int width = 0;
+  int height = 0;
+  int dpi = 0;
+
+  int found = sscanf(spec.c_str(), "%dx%d^%d", &width, &height, &dpi);

[Daniel Erat, 2016/09/20 17:12:49]

nit: probably worthwhile adding a comment here explicitly mentioning that dpi is
optional

[kylechar, 2016/09/20 17:47:40]

Done.

+  if (found < 2)
+    return nullptr;
+
+  int64_t id = GenerateDisplayID(kReservedManufacturerID, kProductCodeHash,
+                                 ++next_display_id_);
+
+  FakeDisplaySnapshot::Builder builder;
+  builder.SetId(id).SetNativeMode(gfx::Size(width, height));
+  builder.SetName(base::StringPrintf("Fake Display %d", next_display_id_));

[Daniel Erat, 2016/09/20 17:12:49]

did you mean to use |id|, or maybe |next_display_id_ - 1|, rather than
next_display_id_?

[kylechar, 2016/09/20 17:47:40]

It was intended. Using |next_display_id_| after it's incremented means it starts
with "Fake Display 1" which seemed nicer. It would be fine to use
|next_display_id_ - 1| to though. I'd rather not use |id| for the name.

+
+  if (found >= 3)
+    builder.SetDPI(dpi);
+
+  // TODO(kylechar): Add type to the spec string.
+  if (displays_.empty())
+    builder.SetType(ui::DISPLAY_CONNECTION_TYPE_INTERNAL);
+
+  return builder.Build();
 }
 
 void FakeDisplayDelegate::OnConfigurationChanged() {