Add more options to --screen-config flag.

ui/display/fake_display_snapshot.cc

Index: ui/display/fake_display_snapshot.cc
diff --git a/ui/display/fake_display_snapshot.cc b/ui/display/fake_display_snapshot.cc
index a431a2a2f652418470ae1c24b3b7fbfb081d232a..9d0e03d00124cb4a7b72b225cd1a7205db55215b 100644
--- a/ui/display/fake_display_snapshot.cc
+++ b/ui/display/fake_display_snapshot.cc
@@ -11,10 +11,13 @@
 #include <vector>
 
 #include "base/memory/ptr_util.h"
+#include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 
 namespace display {
 
+using base::StringPiece;

[Daniel Erat, 2016/10/05 21:09:45]

nit: move this up above "namespace display"?

[kylechar, 2016/10/07 16:14:43]

Done.

+
 namespace {
 
 static const float kInchInMm = 25.4f;
@@ -62,6 +65,91 @@ std::string DisplayConnectionTypeString(ui::DisplayConnectionType type) {
   return "";
 }
 
+// Extracts text after specified delimiter. If the delimiter doesn't exist then
+// the result will be empty and the input string will be unmodified. Otherwise,
+// the input string will contain the text before the delimiter and the result
+// will be the text after the delimiter.
+StringPiece SplitFromString(StringPiece* str, StringPiece delimiter) {

[Daniel Erat, 2016/10/05 21:09:45]

maybe a name like "ExtractSuffix" would be clearer?

[kylechar, 2016/10/07 16:14:43]

Done.

+  std::vector<StringPiece> parts = base::SplitStringPiece(
+      *str, delimiter, base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+
+  if (parts.size() == 2) {
+    *str = parts[0];
+    return parts[1];
+  }
+
+  return StringPiece();
+}
+
+// Parses a display mode from a string in the format HxW[%R], returns null if

[Daniel Erat, 2016/10/05 21:09:45]

nit: s/returns/returning/

[kylechar, 2016/10/07 16:14:43]

Done.

+// string is not valid format.

[Daniel Erat, 2016/10/05 21:09:46]

nit: s/not valid format/invalid/

[kylechar, 2016/10/07 16:14:43]

Done.

+std::unique_ptr<ui::DisplayMode> ParseDisplayMode(const std::string& str) {
+  int width = 0;
+  int height = 0;
+  float refresh_rate = 60.0f;
+
+  // Refresh rate is optional, will be be 60 if not specified.

[Daniel Erat, 2016/10/05 21:09:46]

nit: s/, will/ and will/

[kylechar, 2016/10/07 16:14:43]

Done.

+  if (sscanf(str.c_str(), "%dx%d%%%f", &width, &height, &refresh_rate) < 2) {
+    LOG(ERROR) << "Unable to parse display mode \"" << str << "\"";
+    return nullptr;
+  }
+
+  return base::MakeUnique<ui::DisplayMode>(gfx::Size(width, height), false,
+                                           refresh_rate);
+}
+
+// Parses list of alternate display modes, adding each new display mode to the

[Daniel Erat, 2016/10/05 21:09:46]

nit: s/list/a list/

[kylechar, 2016/10/07 16:14:43]

Done.

+// display snapshot.
+void HandleModes(FakeDisplaySnapshot::Builder* builder,
+                 StringPiece resolutions) {
+  if (resolutions.empty())

[Daniel Erat, 2016/10/05 21:09:46]

nit: no need for this early return, right? i'd assume that SplitString just
returns an empty vector if the input is empty

[kylechar, 2016/10/07 16:14:43]

Done.

+    return;
+
+  for (const std::string& mode_str :
+       base::SplitString(resolutions, ":", base::TRIM_WHITESPACE,
+                         base::SPLIT_WANT_NONEMPTY)) {
+    std::unique_ptr<ui::DisplayMode> mode = ParseDisplayMode(mode_str);
+    if (mode)
+      builder->AddMode(std::move(mode));
+  }
+}
+
+// Parses device DPI and set on display snapshot.

[Daniel Erat, 2016/10/05 21:09:46]

nit: s/set on display snapshot/updates |builder|/ ?

[kylechar, 2016/10/07 16:14:43]

Done.

+void HandleDPI(FakeDisplaySnapshot::Builder* builder, StringPiece dpi) {
+  if (dpi.empty())

[Daniel Erat, 2016/10/05 21:09:46]

this doesn't seem necessary either

[kylechar, 2016/10/07 16:14:43]

This has changed a bit, PTAL.

+    return;
+
+  int dpi_value = 0;
+  if (sscanf(dpi.as_string().c_str(), "%d", &dpi_value) == 1)
+    builder->SetDPI(dpi_value);
+}
+
+// Parses list of display options and set each option true on display snapshot.
+// If an option appears more than once it will have no effect the second time.
+void HandleOptions(FakeDisplaySnapshot::Builder* builder, StringPiece options) {
+  if (options.empty())

[Daniel Erat, 2016/10/05 21:09:46]

nor this; the for loop should just be a no-op if the string is zero-length

[kylechar, 2016/10/07 16:14:43]

Done.

+    return;
+
+  for (size_t i = 0; i < options.size(); ++i) {
+    switch (options[i]) {
+      case 'o':
+        builder->SetHasOverscan(true);
+        break;
+      case 'c':
+        builder->SetHasColorCorrectionMatrix(true);
+        break;
+      case 'a':
+        builder->SetIsAspectPerservingScaling(true);
+        break;
+      case 'i':
+        builder->SetType(ui::DISPLAY_CONNECTION_TYPE_INTERNAL);
+        break;
+      default:
+        LOG(ERROR) << "Invalid option specifier \"" << options[i] << "\"";
+    }
+  }
+}
+
 }  // namespace
 
 using Builder = FakeDisplaySnapshot::Builder;
@@ -100,16 +188,31 @@ Builder& Builder::SetNativeMode(const gfx::Size& size) {
   return *this;
 }
 
+Builder& Builder::SetNativeMode(std::unique_ptr<ui::DisplayMode> mode) {
+  native_mode_ = AddOrFindDisplayMode(std::move(mode));
+  return *this;
+}
+
 Builder& Builder::SetCurrentMode(const gfx::Size& size) {
   current_mode_ = AddOrFindDisplayMode(size);
   return *this;
 }
 
+Builder& Builder::SetCurrentMode(std::unique_ptr<ui::DisplayMode> mode) {
+  current_mode_ = AddOrFindDisplayMode(std::move(mode));
+  return *this;
+}
+
 Builder& Builder::AddMode(const gfx::Size& size) {
   AddOrFindDisplayMode(size);
   return *this;
 }
 
+Builder& Builder::AddMode(std::unique_ptr<ui::DisplayMode> mode) {
+  AddOrFindDisplayMode(std::move(mode));
+  return *this;
+}
+
 Builder& Builder::SetOrigin(const gfx::Point& origin) {
   origin_ = origin;
   return *this;
@@ -169,19 +272,32 @@ const ui::DisplayMode* Builder::AddOrFindDisplayMode(const gfx::Size& size) {
   return modes_.back().get();
 }
 
-FakeDisplaySnapshot::FakeDisplaySnapshot(
-    int64_t display_id,
-    const gfx::Point& origin,
-    const gfx::Size& physical_size,
-    ui::DisplayConnectionType type,
-    bool is_aspect_preserving_scaling,
-    bool has_overscan,
-    bool has_color_correction_matrix,
-    std::string display_name,
-    int64_t product_id,
-    std::vector<std::unique_ptr<const ui::DisplayMode>> modes,
-    const ui::DisplayMode* current_mode,
-    const ui::DisplayMode* native_mode)
+const ui::DisplayMode* Builder::AddOrFindDisplayMode(
+    std::unique_ptr<ui::DisplayMode> mode) {
+  for (auto& existing : modes_) {
+    if (mode->size() == existing->size() &&
+        mode->is_interlaced() == existing->is_interlaced() &&
+        mode->refresh_rate() == existing->refresh_rate())
+      return existing.get();
+  }
+
+  // Not found, insert mode and return.
+  modes_.push_back(std::move(mode));
+  return modes_.back().get();
+}
+
+FakeDisplaySnapshot::FakeDisplaySnapshot(int64_t display_id,
+                                         const gfx::Point& origin,
+                                         const gfx::Size& physical_size,
+                                         ui::DisplayConnectionType type,
+                                         bool is_aspect_preserving_scaling,
+                                         bool has_overscan,
+                                         bool has_color_correction_matrix,
+                                         std::string display_name,
+                                         int64_t product_id,
+                                         DisplayModeList modes,
+                                         const ui::DisplayMode* current_mode,
+                                         const ui::DisplayMode* native_mode)
     : DisplaySnapshot(display_id,
                       origin,
                       physical_size,
@@ -200,6 +316,38 @@ FakeDisplaySnapshot::FakeDisplaySnapshot(
 
 FakeDisplaySnapshot::~FakeDisplaySnapshot() {}
 
+// static
+std::unique_ptr<ui::DisplaySnapshot> FakeDisplaySnapshot::CreateFromSpec(
+    int64_t id,
+    const std::string& spec) {
+  StringPiece leftover(spec);
+
+  // Cut off end of string at each delimiter to split.
+  StringPiece options = SplitFromString(&leftover, "/");
+  StringPiece dpi = SplitFromString(&leftover, "^");
+  StringPiece resolutions = SplitFromString(&leftover, "#");
+
+  // Leftovers should be just the native mode at this point.
+  std::unique_ptr<ui::DisplayMode> native_mode =
+      ParseDisplayMode(leftover.as_string());
+
+  // Fail without valid native mode.
+  if (!native_mode) {
+    LOG(ERROR) << "Failed to make display for \"" << spec << "\"";
+    return nullptr;
+  }
+
+  FakeDisplaySnapshot::Builder builder;
+  builder.SetId(id).SetNativeMode(std::move(native_mode));
+  builder.SetName(base::StringPrintf("Fake Display %" PRId64, id));
+
+  HandleModes(&builder, resolutions);

[Daniel Erat, 2016/10/05 21:09:46]

it's fine if you leave it like this, but i wouldn't object to just inlining
these Handle* methods inside of this method since they're all tiny and only
called from this one place.

[kylechar, 2016/10/07 16:14:43]

I've modified all of this a bit and added return values.

+  HandleDPI(&builder, dpi);
+  HandleOptions(&builder, options);
+
+  return builder.Build();
+}
+
 std::string FakeDisplaySnapshot::ToString() const {
   return base::StringPrintf(
       "id=%" PRId64