Process DisplaySnapshots in PlatformScreen.

services/ui/display/platform_screen_impl_ozone.cc

Index: services/ui/display/platform_screen_impl_ozone.cc
diff --git a/services/ui/display/platform_screen_impl_ozone.cc b/services/ui/display/platform_screen_impl_ozone.cc
index 71ba37b39b86d50e7cbced4990065ad9e3573e77..5940af93ef7029e542833552fe996a9f60705dd1 100644
--- a/services/ui/display/platform_screen_impl_ozone.cc
+++ b/services/ui/display/platform_screen_impl_ozone.cc
@@ -23,6 +23,17 @@ namespace {
 // Needed for DisplayConfigurator::ForceInitialConfigure.
 const SkColor kChromeOsBootColor = SkColorSetRGB(0xfe, 0xfe, 0xfe);
 
+class DisplayIdPredicate {

[rjkroege, 2016/08/30 18:41:59]

what is this for?

[kylechar, 2016/08/31 15:58:01]

It's for find_if. We only have a int64_t id and we want to match on a
DisplayInfo object with the same int64_t id (but the other attributes are
unknown). You would have to implement == that only looks at the id field for it
to be equivalent.

I've removed the find_ifs though.

+ public:
+  explicit DisplayIdPredicate(int64_t id) : id_(id) {}
+  bool operator()(const PlatformScreenImplOzone::DisplayInfo& display) const {

[rjkroege, 2016/08/30 18:41:59]

why not operator== for a comparison op? Or perhaps I'm not understanding.

[kylechar, 2016/08/31 15:58:01]

See above.

+    return display.id == id_;
+  }
+
+ private:
+  int64_t id_;
+};
+
 }  // namespace
 
 // static
@@ -64,40 +75,113 @@ int64_t PlatformScreenImplOzone::GetPrimaryDisplayId() const {
   return primary_display_id_;
 }
 
-void PlatformScreenImplOzone::OnDisplayModeChanged(
-    const ui::DisplayConfigurator::DisplayStateList& displays) {
-  if (displays.size() > 1) {
-    LOG(ERROR)
-        << "Mus doesn't really support multiple displays, expect it to crash";
+void PlatformScreenImplOzone::ProcessRemovedDisplays(
+    const ui::DisplayConfigurator::DisplayStateList& snapshots) {
+  std::vector<int64_t> current_ids;
+  for (ui::DisplaySnapshot* snapshot : snapshots) {
+    current_ids.push_back(snapshot->display_id());
+  }
+
+  // Find cached displays with no matching snapshot and mark as removed.

[rjkroege, 2016/08/30 18:41:59]

we should be replacing this code with the guts of
display_change_observer_chromeos.cc as soon as it's pushed into ui yes? i.e.: we
will refactor both appropriately?

[kylechar, 2016/08/31 15:58:01]

Yep something like that. I need some business logic here to test that display
<-> ws <-> ash interaction works (which it definitely doesn't right now).

+  for (DisplayInfo& display : cached_displays_) {
+    if (std::find(current_ids.begin(), current_ids.end(), display.id) ==
+        current_ids.end()) {
+      display.removed = true;
+      if (primary_display_id_ == display.id)
+        primary_display_id_ = display::Display::kInvalidDisplayID;
+    }
+  }
+
+  // If the primary display was removed find a new primary display id.
+  if (primary_display_id_ == display::Display::kInvalidDisplayID) {
+    for (const DisplayInfo& display : cached_displays_) {
+      if (!display.removed) {
+        primary_display_id_ = display.id;
+        break;
+      }
+    }
+  }
+}
+
+void PlatformScreenImplOzone::ProcessModifiedDisplays(
+    const ui::DisplayConfigurator::DisplayStateList& snapshots) {
+  for (ui::DisplaySnapshot* snapshot : snapshots) {
+    auto iter = std::find_if(cached_displays_.begin(), cached_displays_.end(),

[rjkroege, 2016/08/30 18:41:59]

I'm probably in the minority but I think that a for loop is easier to read than
find_if.

[kylechar, 2016/08/31 15:58:01]

I think you're right here. I already had DisplayIdPredicate so I reused it but
it's much harder to read than a loop.

+                             DisplayIdPredicate(snapshot->display_id()));
+
+    // If we have the display cached, check if the size matches the size from
+    // current mode of the snapshot and update if necessary.
+    if (iter != cached_displays_.end()) {
+      DisplayInfo& display_info = *iter;
+      const ui::DisplayMode* current_mode = snapshot->current_mode();
+
+      if (current_mode->size() != display_info.bounds.size()) {
+        display_info.bounds.set_size(current_mode->size());
+      }
+    }
   }
+}
 
-  // TODO(kylechar): Use DisplayLayout/DisplayLayoutStore here when possible.
-  std::set<uint64_t> all_displays;
-  for (ui::DisplaySnapshot* display : displays) {
-    const int64_t id = display->display_id();
+void PlatformScreenImplOzone::UpdateCachedDisplays() {
+  // Walk through cached displays after processing the snapshots to find any
+  // removed or modified displays. This ensures that we only send one update per

[rjkroege, 2016/08/30 18:41:59]

per displays? This is not clear to me.

[kylechar, 2016/08/31 15:58:01]

It should be per display, typo. Fixed.

+  // displays to the delegate.
+  next_display_origin_.SetPoint(0, 0);
+  auto iter = cached_displays_.begin();
+  while (iter != cached_displays_.end()) {
+    DisplayInfo& display_info = *iter;
+    if (display_info.removed) {
+      // Update delegate and remove from cache.
+      delegate_->OnDisplayRemoved(display_info.id);

[rjkroege, 2016/08/30 18:41:59]

we should have a discussion about the delegate_ api

[kylechar, 2016/08/31 15:58:01]

As per our offline discussion there are a bunch of things that can be changed
about the API. They're outside the scope of this CL though.

+      iter = cached_displays_.erase(iter);
+    } else {
+      // Check if the display origin needs to be updated.
+      if (next_display_origin_ != display_info.bounds.origin()) {
+        display_info.bounds.set_origin(next_display_origin_);
+      }
+      next_display_origin_.Offset(display_info.bounds.width(), 0);
+
+      // Check if the window bounds have changed and update delegate.
+      if (display_info.bounds != display_info.known_bounds) {
+        delegate_->OnDisplayModified(display_info.id, display_info.bounds);
+        display_info.known_bounds = display_info.bounds;
+      }
+      iter++;
+    }
+  }
+}
 
-    all_displays.insert(id);
+void PlatformScreenImplOzone::AddNewDisplays(
+    const ui::DisplayConfigurator::DisplayStateList& snapshots) {
+  for (ui::DisplaySnapshot* snapshot : snapshots) {
+    const int64_t id = snapshot->display_id();
 
-    if (displays_.find(id) != displays_.end())
+    // Check if display already exists and skip.
+    if (std::find_if(cached_displays_.begin(), cached_displays_.end(),
+                     DisplayIdPredicate(id)) != cached_displays_.end())
       continue;
 
-    const ui::DisplayMode* current_mode = display->current_mode();
+    const ui::DisplayMode* current_mode = snapshot->current_mode();
     gfx::Rect bounds(next_display_origin_, current_mode->size());
 
     // Move the origin so that next display is to the right of current display.
     next_display_origin_.Offset(current_mode->size().width(), 0);
 
-    // The first display added will be our primary display.
-    if (displays_.empty())
+    // If we have no primary display then this one should be it.
+    if (primary_display_id_ == display::Display::kInvalidDisplayID)
       primary_display_id_ = id;
 
-    // Keep track of what displays have already been added.
-    displays_.insert(display->display_id());
-
+    cached_displays_.push_back(DisplayInfo(id, bounds));
     delegate_->OnDisplayAdded(this, id, bounds);
   }
+}
 
-  DCHECK(displays_ == all_displays) << "Removing displays is not supported.";
+void PlatformScreenImplOzone::OnDisplayModeChanged(
+    const ui::DisplayConfigurator::DisplayStateList& displays) {
+  ProcessRemovedDisplays(displays);
+  ProcessModifiedDisplays(displays);
+  UpdateCachedDisplays();
+  AddNewDisplays(displays);
 }
 
 void PlatformScreenImplOzone::OnDisplayModeChangeFailed(