Process DisplaySnapshots in PlatformScreen.

services/ui/display/platform_screen_impl_ozone.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "services/ui/display/platform_screen_impl_ozone.h"
 
 #include <memory>
 
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
@@ skipping 46 matching lines @@
     } else {
       display_configurator_.AddVirtualDisplay(gfx::Size(1024, 768));
     }
   }
 }
 
 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());
   }
 
-  // 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();
+  // 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;
+    }
+  }
 
-    all_displays.insert(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;
+      }
+    }
+  }
+}
 
-    if (displays_.find(id) != displays_.end())
+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 (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(
     const ui::DisplayConfigurator::DisplayStateList& displays,
     ui::MultipleDisplayState failed_new_state) {
   LOG(ERROR) << "OnDisplayModeChangeFailed from DisplayConfigurator";
 }
 
 }  // namespace display