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..27bd3c6403f1bef0d1990dc2d914ce141d2e75b8 100644
--- a/services/ui/display/platform_screen_impl_ozone.cc
+++ b/services/ui/display/platform_screen_impl_ozone.cc
@@ -64,40 +64,116 @@ 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) {

[sky, 2016/09/01 19:25:08]

no {}

[kylechar, 2016/09/02 15:18:33]

Done.

+    current_ids.push_back(snapshot->display_id());
+  }
+
+  // Find cached displays with no matching snapshot and mark as removed.
+  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;
+    }
   }
 
-  // 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();
+  // 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;
+      }
+    }
+  }
+}
 
-    all_displays.insert(id);
+void PlatformScreenImplOzone::ProcessModifiedDisplays(
+    const ui::DisplayConfigurator::DisplayStateList& snapshots) {
+  for (ui::DisplaySnapshot* snapshot : snapshots) {
+    DisplayInfo* display_info = nullptr;
+    for (DisplayInfo& current_info : cached_displays_) {
+      if (current_info.id == snapshot->display_id()) {
+        display_info = &current_info;
+        break;
+      }
+    }
 
-    if (displays_.find(id) != displays_.end())
+    if (display_info) {

[sky, 2016/09/01 19:25:08]

Move inside look on 101 so you don't need local.

[kylechar, 2016/09/02 15:18:33]

Done.

+      const ui::DisplayMode* current_mode = snapshot->current_mode();
+      if (current_mode->size() != display_info->bounds.size()) {
+        display_info->bounds.set_size(current_mode->size());
+      }
+    }
+  }
+}
+
+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
+  // display to the delegate.
+  next_display_origin_.SetPoint(0, 0);
+  auto iter = cached_displays_.begin();
+  while (iter != cached_displays_.end()) {

[sky, 2016/09/01 19:25:08]

Use a for loop, that way iter stays local to the loop.

[kylechar, 2016/09/02 15:18:33]

Done.

+    DisplayInfo& display_info = *iter;
+    if (display_info.removed) {
+      // Update delegate and remove from cache.
+      delegate_->OnDisplayRemoved(display_info.id);

[sky, 2016/09/01 19:25:07]

Did you consider updating cached_displays_ and then notifying the delegate?
Otherwise if the delegate calls back cached_displays_ is in a wierd state.

[kylechar, 2016/09/02 15:18:34]

Done.

+      iter = cached_displays_.erase(iter);
+    } else {
+      // Check if the display origin needs to be updated.
+      if (next_display_origin_ != display_info.bounds.origin()) {

[sky, 2016/09/01 19:25:08]

no {}

[kylechar, 2016/09/02 15:18:34]

Done. (Java styleguide habits are hard to kick.)

+        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++;

[sky, 2016/09/01 19:25:08]

++iter.

[kylechar, 2016/09/02 15:18:34]

Done.

+    }
+  }
+}
+
+void PlatformScreenImplOzone::AddNewDisplays(
+    const ui::DisplayConfigurator::DisplayStateList& snapshots) {
+  for (ui::DisplaySnapshot* snapshot : snapshots) {
+    const int64_t id = snapshot->display_id();
+
+    // Check if display already exists and skip.
+    if (std::find_if(cached_displays_.begin(), cached_displays_.end(),

[sky, 2016/09/01 19:25:07]

It seems worth a function for this, given the loop on 99-100 is effectively
doing the same thing. I'm specifically suggesting something like:
FindCachedDisplayIteratorByDisplayId() that returns an iterator into
cached_displays_ based on supplied id. That name is wordy, but you get the idea.

[kylechar, 2016/09/02 15:18:34]

I had something similar in patch 3 but removed it. Added back.

+                     [id](const DisplayInfo& display_info) {
+                       return display_info.id == 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(