DisplayInfoProviderChromeOS: Cleanup

Additional cleanup in preparation for moving the ash implementation
to src/ash.

Note: This CL simplifies and clarifies when and how DisplayProperties
are applied.

Bug: 682402
Change-Id: Ic89eefecbbb2c35b018d05861963044b46dad4c3
Reviewed-on: https://chromium-review.googlesource.com/983006
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547454}

chrome/browser/extensions/display_info_provider_chromeos.cc

@@ -162,121 +162,15 @@ system_display::LayoutPosition GetLayoutPosition(
   return system_display::LayoutPosition::LAYOUT_POSITION_NONE;
 }
 
-// Checks if the given point is over the radius vector described by it's end
-// point |vector|. The point is over a vector if it's on its positive (left)
-// side. The method sees a point on the same line as the vector as being over
-// the vector.
-bool PointIsOverRadiusVector(const gfx::Point& point,
-                             const gfx::Point& vector) {
-  // |point| is left of |vector| if its radius vector's scalar product with a
-  // vector orthogonal (and facing the positive side) to |vector| is positive.
-  //
-  // An orthogonal vector of (a, b) is (b, -a), as the scalar product of these
-  // two is 0.
-  // So, (x, y) is over (a, b) if x * b + y * (-a) >= 0, which is equivalent to
-  // x * b >= y * a.
-  return static_cast<int64_t>(point.x()) * static_cast<int64_t>(vector.y()) >=
-         static_cast<int64_t>(point.y()) * static_cast<int64_t>(vector.x());
-}
-
-// Created display::DisplayPlacement value for |rectangle| compared to the
-// |reference|
-// rectangle.
-// The layout consists of two values:
-//   - position: Whether the rectangle is positioned left, right, over or under
-//     the reference.
-//   - offset: The rectangle's offset from the reference origin along the axis
-//     opposite the position direction (if the rectangle is left or right along
-//     y-axis, otherwise along x-axis).
-// The rectangle's position is calculated by dividing the space in areas defined
-// by the |reference|'s diagonals and finding the area |rectangle|'s center
-// point belongs. If the |rectangle| in the calculated layout does not share a
-// part of the bounds with the |reference|, the |rectangle| position in set to
-// the more suitable neighboring position (e.g. if |rectangle| is completely
-// over the |reference| top bound, it will be set to TOP) and the layout is
-// recalculated with the new position. This is to handle case where the
-// rectangle shares an edge with the reference, but it's center is not in the
-// same area as the reference's edge, e.g.
-//
-// +---------------------+
-// |                     |
-// | REFERENCE           |
-// |                     |
-// |                     |
-// +---------------------+
-//                 +-------------------------------------------------+
-//                 | RECTANGLE               x                       |
-//                 +-------------------------------------------------+
-//
-// The rectangle shares an egde with the reference's bottom edge, but it's
-// center point is in the left area.
-display::DisplayPlacement CreatePlacementForRectangles(
-    const gfx::Rect& reference,
-    const gfx::Rect& rectangle) {
-  // Translate coordinate system so origin is in the reference's top left point
-  // (so the reference's down-diagonal vector starts in the (0, 0)) and scale it
-  // up by two (to avoid division when calculating the rectangle's center
-  // point).
-  gfx::Point center(2 * (rectangle.x() - reference.x()) + rectangle.width(),
-                    2 * (rectangle.y() - reference.y()) + rectangle.height());
-  gfx::Point down_diag(2 * reference.width(), 2 * reference.height());
-
-  bool is_top_right = PointIsOverRadiusVector(center, down_diag);
-
-  // Translate the coordinating system again, so the bottom right point of the
-  // reference is origin (so the references up-diagonal starts at (0, 0)).
-  // Note that the coordinate system is scaled by 2.
-  center.Offset(0, -2 * reference.height());
-  // Choose the vector orientation so the points on the diagonal are considered
-  // to be left.
-  gfx::Point up_diag(-2 * reference.width(), 2 * reference.height());
-
-  bool is_bottom_right = PointIsOverRadiusVector(center, up_diag);
-
-  display::DisplayPlacement::Position position;
-  if (is_top_right) {
-    position = is_bottom_right ? display::DisplayPlacement::RIGHT
-                               : display::DisplayPlacement::TOP;
-  } else {
-    position = is_bottom_right ? display::DisplayPlacement::BOTTOM
-                               : display::DisplayPlacement::LEFT;
-  }
-
-  // If the rectangle with the calculated position would not have common side
-  // with the reference, try to position it so it shares another edge with the
-  // reference.
-  if (is_top_right == is_bottom_right) {
-    if (rectangle.y() > reference.y() + reference.height()) {
-      // The rectangle is left or right, but completely under the reference.
-      position = display::DisplayPlacement::BOTTOM;
-    } else if (rectangle.y() + rectangle.height() < reference.y()) {
-      // The rectangle is left or right, but completely over the reference.
-      position = display::DisplayPlacement::TOP;
-    }
-  } else {
-    if (rectangle.x() > reference.x() + reference.width()) {
-      // The rectangle is over or under, but completely right of the reference.
-      position = display::DisplayPlacement::RIGHT;
-    } else if (rectangle.x() + rectangle.width() < reference.x()) {
-      // The rectangle is over or under, but completely left of the reference.
-      position = display::DisplayPlacement::LEFT;
-    }
-  }
-  int offset = (position == display::DisplayPlacement::LEFT ||
-                position == display::DisplayPlacement::RIGHT)
-                   ? rectangle.y()
-                   : rectangle.x();
-  return display::DisplayPlacement(position, offset);
-}
-
 // Updates the display layout for the target display in reference to the primary
 // display.
 void UpdateDisplayLayout(const gfx::Rect& primary_display_bounds,
                          int64_t primary_display_id,
                          const gfx::Rect& target_display_bounds,
                          int64_t target_display_id) {
-  display::DisplayPlacement placement(CreatePlacementForRectangles(
-      primary_display_bounds, target_display_bounds));
+  display::DisplayPlacement placement(
+      display::DisplayLayout::CreatePlacementForRectangles(
+          primary_display_bounds, target_display_bounds));
   placement.display_id = target_display_id;
   placement.parent_display_id = primary_display_id;
 
@@ -288,44 +182,25 @@ void UpdateDisplayLayout(const gfx::Rect& primary_display_bounds,
       std::move(layout));
 }
 
-// Validates that parameters passed to the SetInfo function are valid for the
-// desired display and the current display manager state.
-// Returns whether the parameters are valid. On failure |error| is set to the
-// error message.
-bool ValidateParamsForDisplay(const system_display::DisplayProperties& info,
-                              const display::Display& display,
-                              display::DisplayManager* display_manager,
-                              int64_t primary_display_id,
-                              std::string* error) {
-  int64_t id = display.id();
-  bool is_primary =
-      id == primary_display_id || (info.is_primary && *info.is_primary);
-
-  if (info.is_unified) {
-    if (!is_primary) {
-      *error = "Unified desktop mode can only be set for the primary display.";
-      return false;
-    }
-    if (info.mirroring_source_id) {
-      *error = "Unified desktop mode can not be set with mirroringSourceId.";
-      return false;
-    }
-    return true;
-  }
+// Validates that DisplayProperties input is valid. Does not perform any tests
+// with DisplayManager dependencies. Returns an error string on failure or an
+// empty string on success.
+std::string ValidateDisplayPropertiesInput(
+    const std::string& display_id_str,
+    const system_display::DisplayProperties& info) {
+  int64_t id;
+  if (!base::StringToInt64(display_id_str, &id))
+    return "Invalid display id";
 
-  // If mirroring source id is set, a display with the given id should exist,
-  // and if should not be the same as the target display's id.
-  if (info.mirroring_source_id && !info.mirroring_source_id->empty()) {
-    int64_t mirroring_id = GetDisplay(*info.mirroring_source_id).id();
-    if (mirroring_id == display::kInvalidDisplayId) {
-      *error = "Display " + *info.mirroring_source_id + " not found.";
-      return false;
-    }
+  const display::Display& primary =
+      display::Screen::GetScreen()->GetPrimaryDisplay();
+  bool is_primary = id == primary.id() || (info.is_primary && *info.is_primary);
 
-    if (*info.mirroring_source_id == base::Int64ToString(id)) {
-      *error = "Not allowed to mirror self.";
-      return false;
-    }
+  if (info.is_unified) {
+    if (!is_primary)
+      return "Unified desktop mode can only be set for the primary display.";
+    if (info.mirroring_source_id)
+      return "Unified desktop mode can not be set with mirroringSourceId.";
   }
 
   // If mirroring source parameter is specified, no other parameter should be
@@ -333,74 +208,80 @@ bool ValidateParamsForDisplay(const system_display::DisplayProperties& info,
   if (info.mirroring_source_id &&
       (info.is_primary || info.bounds_origin_x || info.bounds_origin_y ||
        info.rotation || info.overscan)) {
-    *error = "No other parameter should be set alongside mirroringSourceId.";
-    return false;
+    return "No other parameter should be set alongside mirroringSourceId.";
   }
 
   // The bounds cannot be changed for the primary display and should be inside
-  // a reasonable bounds. Note that the display is considered primary if the
-  // info has 'isPrimary' parameter set, as this will be applied before bounds
-  // origin changes.
+  // a reasonable bounds.
   if (info.bounds_origin_x || info.bounds_origin_y) {
-    if (is_primary) {
-      *error = "Bounds origin not allowed for the primary display.";
-      return false;
-    }
+    if (is_primary)
+      return "Bounds origin not allowed for the primary display.";
     if (info.bounds_origin_x && (*info.bounds_origin_x > kMaxBoundsOrigin ||
                                  *info.bounds_origin_x < -kMaxBoundsOrigin)) {
-      *error = "Bounds origin x out of bounds.";
-      return false;
+      return "Bounds origin x out of bounds.";
     }
     if (info.bounds_origin_y && (*info.bounds_origin_y > kMaxBoundsOrigin ||
                                  *info.bounds_origin_y < -kMaxBoundsOrigin)) {
-      *error = "Bounds origin y out of bounds.";
-      return false;
+      return "Bounds origin y out of bounds.";
     }
   }
 
   // Verify the rotation value is valid.
-  if (info.rotation && !IsValidRotationValue(*info.rotation)) {
-    *error = "Invalid rotation.";
-    return false;
+  if (info.rotation && !IsValidRotationValue(*info.rotation))
+    return "Invalid rotation.";
+
+  return "";
+}
+
+// Validates that DisplayProperties are valid with the current DisplayManager
+// configuration. Returns an error string on failure or an empty string on
+// success.
+std::string ValidateDisplayProperties(
+    const system_display::DisplayProperties& info,
+    const display::Display& display,
+    display::DisplayManager* display_manager) {
+  const int64_t id = display.id();
+  if (id == display::kInvalidDisplayId)
+    return "Display not found.";
+
+  if (info.mirroring_source_id && !info.mirroring_source_id->empty()) {
+    // A display with the given id should exist and should not be the same as
+    // the target display's id.
+    const int64_t mirroring_id = GetDisplay(*info.mirroring_source_id).id();
+    if (mirroring_id == display::kInvalidDisplayId)
+      return "Invalid mirroring source id";
+
+    if (mirroring_id == id)
+      return "Not allowed to mirror self";
   }
 
   // Overscan cannot be changed for the internal display, and should be at most
   // half of the screen size.
   if (info.overscan) {
-    if (display.IsInternal()) {
-      *error = "Overscan changes not allowed for the internal monitor.";
-      return false;
-    }
+    if (display.IsInternal())
+      return "Overscan changes not allowed for the internal monitor.";
 
     if (info.overscan->left < 0 || info.overscan->top < 0 ||
         info.overscan->right < 0 || info.overscan->bottom < 0) {
-      *error = "Negative overscan not allowed.";
-      return false;
+      return "Negative overscan not allowed.";
     }
 
     const gfx::Insets overscan = display_manager->GetOverscanInsets(id);
     int screen_width = display.bounds().width() + overscan.width();
     int screen_height = display.bounds().height() + overscan.height();
 
-    if ((info.overscan->left + info.overscan->right) * 2 > screen_width) {
-      *error = "Horizontal overscan is more than half of the screen width.";
-      return false;
-    }
+    if ((info.overscan->left + info.overscan->right) * 2 > screen_width)
+      return "Horizontal overscan is more than half of the screen width.";
 
-    if ((info.overscan->top + info.overscan->bottom) * 2 > screen_height) {
-      *error = "Vertical overscan is more than half of the screen height.";
-      return false;
-    }
+    if ((info.overscan->top + info.overscan->bottom) * 2 > screen_height)
+      return "Vertical overscan is more than half of the screen height.";
   }
 
-  // Update the display zoom.
   if (info.display_zoom_factor) {
     display::ManagedDisplayMode current_mode;
-    if (!display_manager->GetActiveModeForDisplayId(id, &current_mode)) {
-      *error = "Unable to find the active mode for display id " +
-               base::Int64ToString(id);
-      return false;
-    }
+    if (!display_manager->GetActiveModeForDisplayId(id, &current_mode))
+      return "No active mode for display id.";
+
     // This check is added to limit the range of display zoom that can be
     // applied via the system display API. The said range is such that when a
     // display zoom is applied, the final logical width in pixels should lie
@@ -409,59 +290,57 @@ bool ValidateParamsForDisplay(const system_display::DisplayProperties& info,
         std::max(kDefaultMaxZoomWidth, current_mode.size().width());
     const int kMinAllowedWidth =
         std::min(kDefaultMinZoomWidth, current_mode.size().width());
-
     int current_width = static_cast<float>(current_mode.size().width()) /
                         current_mode.device_scale_factor();
-    if (current_width / (*info.display_zoom_factor) <= kMaxAllowedWidth &&
-        current_width / (*info.display_zoom_factor) >= kMinAllowedWidth) {
-      display_manager->UpdateZoomFactor(id, *info.display_zoom_factor);
-    } else {
-      *error = "Zoom value is out of range for display with id: " +
-               base::Int64ToString(id);
-      return false;
+    if (current_width / (*info.display_zoom_factor) > kMaxAllowedWidth ||
+        current_width / (*info.display_zoom_factor) < kMinAllowedWidth) {
+      return "Zoom value is out of range for display.";
     }
   }
 
-  // Set the display mode.
-  if (info.display_mode) {
-    display::ManagedDisplayMode current_mode;
-    if (!display_manager->GetActiveModeForDisplayId(id, &current_mode)) {
-      *error = "Unable to find the active mode for display id " +
-               base::Int64ToString(id);
-      return false;
-    }
-    // Copy properties not set in the UI from the current mode.
-    gfx::Size size(info.display_mode->width_in_native_pixels,
-                   info.display_mode->height_in_native_pixels);
-
-    // NB: info.display_mode is neither a display::ManagedDisplayMode or a
-    // display::DisplayMode.
-    display::ManagedDisplayMode new_mode(
-        size, current_mode.refresh_rate(), current_mode.is_interlaced(),
-        info.display_mode->is_native, info.display_mode->ui_scale,
-        info.display_mode->device_scale_factor);
-
-    if (new_mode.IsEquivalent(current_mode)) {
-      *error = "Display mode matches current mode.";
-      return false;
-    }
+  return "";
+}
 
-    // If it's the internal display, the display mode will be applied directly,
-    // otherwise a confirm/revert notification will be prepared first, and the
-    // display mode will be applied. If the user accepts the mode change by
-    // dismissing the notification, StoreDisplayPrefs() will be called back to
-    // persist the new preferences.
-    if (!ash::Shell::Get()
-             ->resolution_notification_controller()
-             ->PrepareNotificationAndSetDisplayMode(
-                 id, current_mode, new_mode, base::BindRepeating([]() {
-                   chromeos::DisplayPrefs::Get()->StoreDisplayPrefs();
-                 }))) {
-      *error = "Unable to set the display mode.";
-      return false;
-    }
-  }
-  return true;
+// Attempts to set the display mode for display |id|. Returns an error string on
+// failure or an empty string on success.
+std::string SetDisplayMode(display::DisplayManager* display_manager,
+                           int64_t id,
+                           const system_display::DisplayProperties& info) {
+  if (!info.display_mode)
+    return "";  // Nothing to set.
+
+  display::ManagedDisplayMode current_mode;
+  if (!display_manager->GetActiveModeForDisplayId(id, &current_mode))
+    return "No active mode for display id.";
+
+  // Copy properties not set in the UI from the current mode.
+  const gfx::Size size(info.display_mode->width_in_native_pixels,
+                       info.display_mode->height_in_native_pixels);
+
+  // NB: info.display_mode is neither a display::ManagedDisplayMode or a
+  // display::DisplayMode.
+  const display::ManagedDisplayMode new_mode(
+      size, current_mode.refresh_rate(), current_mode.is_interlaced(),
+      info.display_mode->is_native, info.display_mode->ui_scale,
+      info.display_mode->device_scale_factor);
+
+  if (new_mode.IsEquivalent(current_mode))
+    return "";  // Do nothing but do not return an error.
+
+  // If it's the internal display, the display mode will be applied directly,
+  // otherwise a confirm/revert notification will be prepared first, and the
+  // display mode will be applied. If the user accepts the mode change by
+  // dismissing the notification, StoreDisplayPrefs() will be called back to
+  // persist the new preferences.
+  if (!ash::Shell::Get()
+           ->resolution_notification_controller()
+           ->PrepareNotificationAndSetDisplayMode(
+               id, current_mode, new_mode, base::BindRepeating([]() {
+                 chromeos::DisplayPrefs::Get()->StoreDisplayPrefs();
+               }))) {
+    return "Failed to set display mode.";
+  }
+  return "";
 }
 
 system_display::DisplayMode GetDisplayMode(
@@ -584,21 +463,16 @@ void DisplayInfoProviderChromeOS::SetDisplayProperties(
   ash::DisplayConfigurationController* display_configuration_controller =
       ash::Shell::Get()->display_configuration_controller();
 
-  const display::Display target = GetDisplay(display_id_str);
-
-  if (target.id() == display::kInvalidDisplayId) {
-    RunResultCallback(std::move(callback),
-                      "Display not found:" + display_id_str);
+  std::string error =
+      ValidateDisplayPropertiesInput(display_id_str, properties);
+  if (!error.empty()) {
+    RunResultCallback(std::move(callback), std::move(error));
     return;
   }
 
-  int64_t display_id = target.id();
-  const display::Display& primary =
-      display::Screen::GetScreen()->GetPrimaryDisplay();
-
-  std::string error;
-  if (!ValidateParamsForDisplay(properties, target, display_manager,
-                                primary.id(), &error)) {
+  const display::Display target = GetDisplay(display_id_str);
+  error = ValidateDisplayProperties(properties, target, display_manager);
+  if (!error.empty()) {
     RunResultCallback(std::move(callback), std::move(error));
     return;
   }
@@ -610,10 +484,13 @@ void DisplayInfoProviderChromeOS::SetDisplayProperties(
                                : display::DisplayManager::EXTENDED);
   }
 
+  const display::Display& primary =
+      display::Screen::GetScreen()->GetPrimaryDisplay();
+
   // Process 'isPrimary' parameter.
   if (properties.is_primary && *properties.is_primary &&
       target.id() != primary.id()) {
-    display_configuration_controller->SetPrimaryDisplayId(display_id);
+    display_configuration_controller->SetPrimaryDisplayId(target.id());
   }
 
   // Process 'mirroringSourceId' parameter.
@@ -625,7 +502,7 @@ void DisplayInfoProviderChromeOS::SetDisplayProperties(
   // Process 'overscan' parameter.
   if (properties.overscan) {
     display_manager->SetOverscanInsets(
-        display_id,
+        target.id(),
         gfx::Insets(properties.overscan->top, properties.overscan->left,
                     properties.overscan->bottom, properties.overscan->right));
   }
@@ -633,12 +510,12 @@ void DisplayInfoProviderChromeOS::SetDisplayProperties(
   // Process 'rotation' parameter.
   if (properties.rotation) {
     if (IsTabletModeWindowManagerEnabled() &&
-        display_id == display::Display::InternalDisplayId()) {
+        target.id() == display::Display::InternalDisplayId()) {
       ash::Shell::Get()->screen_orientation_controller()->SetLockToRotation(
           DegreesToRotation(*properties.rotation));
     } else {
       display_configuration_controller->SetDisplayRotation(
-          display_id, DegreesToRotation(*properties.rotation),
+          target.id(), DegreesToRotation(*properties.rotation),
           display::Display::RotationSource::USER);
     }
   }
@@ -658,7 +535,19 @@ void DisplayInfoProviderChromeOS::SetDisplayProperties(
                         target.id());
   }
 
-  RunResultCallback(std::move(callback), base::nullopt);
+  // Update the display zoom.
+  if (properties.display_zoom_factor) {
+    display_manager->UpdateZoomFactor(target.id(),
+                                      *properties.display_zoom_factor);
+  }
+
+  // Set the display mode. Note: if this returns an error, other properties
+  // will have already been applied.
+  error = SetDisplayMode(display_manager, target.id(), properties);
+  RunResultCallback(std::move(callback),
+                    error.empty()
+                        ? base::nullopt
+                        : base::make_optional<std::string>(std::move(error)));
 }
 
 void DisplayInfoProviderChromeOS::SetDisplayLayout(
@@ -1216,7 +1105,7 @@ void DisplayInfoProviderChromeOS::SetMirrorMode(
     case MirrorParamsErrors::kSuccess:
       display_manager->SetMirrorMode(display::MirrorMode::kMixed, mixed_params);
   }
-  RunResultCallback(std::move(callback), error);
+  RunResultCallback(std::move(callback), std::move(error));
 }
 
 ash::OverscanCalibrator* DisplayInfoProviderChromeOS::GetOverscanCalibrator(

chrome/browser/extensions/display_info_provider_chromeos_unittest.cc

@@ -1177,7 +1177,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetNegativeOverscan) {
 
   std::string error =
       CallSetDisplayUnitInfo(base::Int64ToString(secondary.id()), info);
-  EXPECT_EQ("Negative overscan not allowed.", error);
+  EXPECT_FALSE(error.empty());
 
   EXPECT_EQ("1200,0 300x500", secondary.bounds().ToString());
 
@@ -1185,8 +1185,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetNegativeOverscan) {
   info.overscan->right = -200;
 
   error = CallSetDisplayUnitInfo(base::Int64ToString(secondary.id()), info);
-
-  EXPECT_EQ("Negative overscan not allowed.", error);
+  EXPECT_FALSE(error.empty());
 
   EXPECT_EQ("1200,0 300x500", secondary.bounds().ToString());
 
@@ -1194,8 +1193,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetNegativeOverscan) {
   info.overscan->top = -300;
 
   error = CallSetDisplayUnitInfo(base::Int64ToString(secondary.id()), info);
-
-  EXPECT_EQ("Negative overscan not allowed.", error);
+  EXPECT_FALSE(error.empty());
 
   EXPECT_EQ("1200,0 300x500", secondary.bounds().ToString());
 
@@ -1203,8 +1201,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetNegativeOverscan) {
   info.overscan->top = -1000;
 
   error = CallSetDisplayUnitInfo(base::Int64ToString(secondary.id()), info);
-
-  EXPECT_EQ("Negative overscan not allowed.", error);
+  EXPECT_FALSE(error.empty());
 
   EXPECT_EQ("1200,0 300x500", secondary.bounds().ToString());
 
@@ -1231,8 +1228,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetOverscanLargerThanHorizontalBounds) {
 
   std::string error =
       CallSetDisplayUnitInfo(base::Int64ToString(secondary.id()), info);
-  EXPECT_EQ("Horizontal overscan is more than half of the screen width.",
-            error);
+  EXPECT_FALSE(error.empty());
 }
 
 TEST_F(DisplayInfoProviderChromeosTest, SetOverscanLargerThanVerticalBounds) {
@@ -1249,7 +1245,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetOverscanLargerThanVerticalBounds) {
 
   std::string error =
       CallSetDisplayUnitInfo(base::Int64ToString(secondary.id()), info);
-  EXPECT_EQ("Vertical overscan is more than half of the screen height.", error);
+  EXPECT_FALSE(error.empty());
 }
 
 TEST_F(DisplayInfoProviderChromeosTest, SetOverscan) {
@@ -1292,7 +1288,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetOverscanForInternal) {
 
   std::string error =
       CallSetDisplayUnitInfo(base::Int64ToString(internal_display_id), info);
-  EXPECT_EQ("Overscan changes not allowed for the internal monitor.", error);
+  EXPECT_FALSE(error.empty());
 }
 
 TEST_F(DisplayInfoProviderChromeosTest, DisplayMode) {
@@ -1530,7 +1526,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetDisplayZoomFactor) {
 
   std::string error =
       CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[0]), info);
-  EXPECT_EQ(std::string(), error);
+  EXPECT_TRUE(error.empty());
 
   EXPECT_EQ(GetDisplayZoom(display_id_list[0]), final_zoom_factor_1);
   // Display 2 has not been updated yet, so it will still have the old zoom
@@ -1539,22 +1535,18 @@ TEST_F(DisplayInfoProviderChromeosTest, SetDisplayZoomFactor) {
 
   info.display_zoom_factor = std::make_unique<double>(zoom_factor_2);
   error = CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[1]), info);
-  EXPECT_EQ(std::string(), error);
+  EXPECT_TRUE(error.empty());
 
   // Both displays should now have the correct zoom factor set.
   EXPECT_EQ(GetDisplayZoom(display_id_list[0]), final_zoom_factor_1);
   EXPECT_EQ(GetDisplayZoom(display_id_list[1]), final_zoom_factor_2);
 
-  std::string expected_err =
-      "Zoom value is out of range for display with id: " +
-      base::Int64ToString(display_id_list[0]);
-
   // This zoom factor when applied to the display with width 1200, will result
   // in an effective width greater than 4096, which is out of range.
   float invalid_zoom_factor_1 = 0.285f;
   info.display_zoom_factor = std::make_unique<double>(invalid_zoom_factor_1);
   error = CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[0]), info);
-  EXPECT_EQ(expected_err, error);
+  EXPECT_FALSE(error.empty());
 
   // This zoom factor when applied to the display with width 1200, will result
   // in an effective width greater less than 640, which is out of range.
@@ -1575,7 +1567,7 @@ TEST_F(DisplayInfoProviderChromeosTest, SetDisplayZoomFactor) {
   float valid_zoom_factor_1 = 0.8f;
   info.display_zoom_factor = std::make_unique<double>(valid_zoom_factor_1);
   error = CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[0]), info);
-  EXPECT_EQ(std::string(), error);
+  EXPECT_TRUE(error.empty());
 
   // Results in a logical width of 4200px. This is above the 4096px threshold
   // but is valid because the initial width was 4500px, so logical width of up
@@ -1583,17 +1575,17 @@ TEST_F(DisplayInfoProviderChromeosTest, SetDisplayZoomFactor) {
   float valid_zoom_factor_2 = 1.07f;
   info.display_zoom_factor = std::make_unique<double>(valid_zoom_factor_2);
   error = CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[1]), info);
-  EXPECT_EQ(std::string(), error);
+  EXPECT_TRUE(error.empty());
 
   float valid_zoom_factor_3 = 0.5f;
   info.display_zoom_factor = std::make_unique<double>(valid_zoom_factor_3);
   error = CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[0]), info);
-  EXPECT_EQ(std::string(), error);
+  EXPECT_TRUE(error.empty());
 
   float valid_zoom_factor_4 = 2.f;
   info.display_zoom_factor = std::make_unique<double>(valid_zoom_factor_4);
   error = CallSetDisplayUnitInfo(base::Int64ToString(display_id_list[1]), info);
-  EXPECT_EQ(std::string(), error);
+  EXPECT_TRUE(error.empty());
 }
 
 class DisplayInfoProviderChromeosTouchviewTest

extensions/browser/api/system_display/display_info_provider.cc

@@ -34,7 +34,9 @@ int RotationToDegrees(display::Display::Rotation rotation) {
 
 }  // namespace
 
-DisplayInfoProvider::~DisplayInfoProvider() {}
+DisplayInfoProvider::DisplayInfoProvider() = default;
+
+DisplayInfoProvider::~DisplayInfoProvider() = default;
 
 // static
 DisplayInfoProvider* DisplayInfoProvider::Get() {
@@ -91,12 +93,12 @@ void DisplayInfoProvider::SetDisplayProperties(
     const std::string& display_id,
     const api::system_display::DisplayProperties& properties,
     ErrorCallback callback) {
-  LOG(ERROR) << "SetDisplayProperties not implemented";
+  NOTREACHED() << "SetDisplayProperties not implemented";
 }
 
 void DisplayInfoProvider::SetDisplayLayout(const DisplayLayoutList& layouts,
                                            ErrorCallback callback) {
-  LOG(ERROR) << "SetDisplayLayout not implemented";
+  NOTREACHED() << "SetDisplayLayout not implemented";
 }
 
 void DisplayInfoProvider::EnableUnifiedDesktop(bool enable) {}

extensions/browser/api/system_display/display_info_provider.h

@@ -115,7 +115,7 @@ class DisplayInfoProvider {
                              ErrorCallback callback);
 
  protected:
-  DisplayInfoProvider() = default;
+  DisplayInfoProvider();
 
   // Create a DisplayUnitInfo from a display::Display for implementations of
   // GetAllDisplaysInfo()

extensions/common/api/system_display.idl

@@ -221,7 +221,7 @@ namespace system.display {
     // other properties may not apply. This is has no effect if not provided.
     boolean? isUnified;
 
-    // Deprecated. Please use setMirrorMode() instead.
+    // Deprecated. Please use $(ref:setMirrorMode) instead.
     // Chrome OS only. If set and not empty, enables mirroring for this display.
     // Otherwise disables mirroring for this display. This value should indicate
     // the id of the source display to mirror, which must not be the same as the
@@ -229,36 +229,37 @@ namespace system.display {
     DOMString? mirroringSourceId;
 
     // If set to true, makes the display primary. No-op if set to false.
+    // Note: If set, the display is considered primary for all other properties
+    // (i.e. $(ref:isUnified) may be set and bounds origin may not).
     boolean? isPrimary;
 
     // If set, sets the display's overscan insets to the provided values. Note
     // that overscan values may not be negative or larger than a half of the
     // screen's size. Overscan cannot be changed on the internal monitor.
-    // It's applied after <code>isPrimary</code> parameter.
     Insets? overscan;
 
     // If set, updates the display's rotation.
     // Legal values are [0, 90, 180, 270]. The rotation is set clockwise,
     // relative to the display's vertical position.
-    // It's applied after <code>overscan</code> paramter.
     long? rotation;
 
-    // If set, updates the display's logical bounds origin along x-axis. Applied
-    // together with <code>boundsOriginY</code>, if <code>boundsOriginY</code>
-    // is set. Note that, when updating the display origin, some constraints
-    // will be applied, so the final bounds origin may be different than the one
-    // set. The final bounds can be retrieved using $(ref:getInfo).
-    // The bounds origin is applied after <code>rotation</code>.
-    // The bounds origin cannot be changed on the primary display. Note that is
-    // also invalid to set bounds origin values if <code>isPrimary</code> is
-    // also set (as <code>isPrimary</code> parameter is applied first).
+    // If set, updates the display's logical bounds origin along the x-axis.
+    // Applied together with $(ref:boundsOriginY). Defaults to the current value
+    // if not set and $(ref:boundsOriginY) is set. Note that when updating the
+    // display origin, some constraints will be applied, so the final bounds
+    // origin may be different than the one set. The final bounds can be
+    // retrieved using $(ref:getInfo). The bounds origin cannot be changed on
+    // the primary display.
     long? boundsOriginX;
 
-    // If set, updates the display's logical bounds origin along y-axis.
-    // See documentation for <code>boundsOriginX</code> parameter.
+    // If set, updates the display's logical bounds origin along the y-axis.
+    // See documentation for $(ref:boundsOriginX) parameter.
     long? boundsOriginY;
 
     // If set, updates the display mode to the mode matching this value.
+    // If other parameters are invalid, this will not be applied. If the
+    // display mode is invalid, it will not be applied and an error will be
+    // set, but other properties will still be applied.
     DisplayMode? displayMode;
 
     // If set, updates the zoom associated with the display. This zoom performs

ui/display/display_layout.cc

@@ -16,6 +16,8 @@
 #include "base/values.h"
 #include "ui/display/display.h"
 #include "ui/gfx/geometry/insets.h"
+#include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/rect.h"
 
 namespace display {
 namespace {
@@ -329,6 +331,23 @@ void DeIntersectDisplays(int64_t primary_id,
   UpdatePlacementList(display_list, placement_list);
 }
 
+// Checks if the given point is over the radius vector described by its end
+// point |vector|. The point is over a vector if it's on its positive (left)
+// side. The method sees a point on the same line as the vector as being over
+// the vector.
+bool IsPointOverRadiusVector(const gfx::Point& point,
+                             const gfx::Point& vector) {
+  // |point| is left of |vector| if its radius vector's scalar product with a
+  // vector orthogonal (and facing the positive side) to |vector| is positive.
+  //
+  // An orthogonal vector of (a, b) is (b, -a), as the scalar product of these
+  // two is 0.
+  // So, (x, y) is over (a, b) if x * b + y * (-a) >= 0, which is equivalent to
+  // x * b >= y * a.
+  return static_cast<int64_t>(point.x()) * static_cast<int64_t>(vector.y()) >=
+         static_cast<int64_t>(point.y()) * static_cast<int64_t>(vector.x());
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -634,6 +653,97 @@ DisplayPlacement DisplayLayout::FindPlacementById(int64_t display_id) const {
                                         : DisplayPlacement(*iter);
 }
 
+// Creates a display::DisplayPlacement value for |rectangle| relative to the
+// |reference| rectangle.
+// The layout consists of two values:
+//   - position: Whether the rectangle is positioned left, right, over or under
+//     the reference.
+//   - offset: The rectangle's offset from the reference origin along the axis
+//     opposite the position direction (if the rectangle is left or right along
+//     y-axis, otherwise along x-axis).
+// The rectangle's position is calculated by dividing the space in areas defined
+// by the |reference|'s diagonals and finding the area |rectangle|'s center
+// point belongs. If the |rectangle| in the calculated layout does not share a
+// part of the bounds with the |reference|, the |rectangle| position in set to
+// the more suitable neighboring position (e.g. if |rectangle| is completely
+// over the |reference| top bound, it will be set to TOP) and the layout is
+// recalculated with the new position. This is to handle the case where the
+// rectangle shares an edge with the reference, but it's center is not in the
+// same area as the reference's edge, e.g.
+//
+// +---------------------+
+// |                     |
+// | REFERENCE           |
+// |                     |
+// |                     |
+// +---------------------+
+//                 +-------------------------------------------------+
+//                 | RECTANGLE               x                       |
+//                 +-------------------------------------------------+
+//
+// The rectangle shares an egde with the reference's bottom edge, but it's
+// center point is in the left area.
+
+// static
+DisplayPlacement DisplayLayout::CreatePlacementForRectangles(
+    const gfx::Rect& reference,
+    const gfx::Rect& rectangle) {
+  // Translate coordinate system so origin is in the reference's top left point
+  // (so the reference's down-diagonal vector starts in the (0, 0)) and scale it
+  // up by two (to avoid division when calculating the rectangle's center
+  // point).
+  gfx::Point center(2 * (rectangle.x() - reference.x()) + rectangle.width(),
+                    2 * (rectangle.y() - reference.y()) + rectangle.height());
+  gfx::Point down_diag(2 * reference.width(), 2 * reference.height());
+
+  bool is_top_right = IsPointOverRadiusVector(center, down_diag);
+
+  // Translate the coordinate system again, so the bottom right point of the
+  // reference is origin (so the reference's up-diagonal starts at (0, 0)).
+  // Note that the coordinate system is scaled by 2.
+  center.Offset(0, -2 * reference.height());
+  // Choose the vector orientation so the points on the diagonal are considered
+  // to be left.
+  gfx::Point up_diag(-2 * reference.width(), 2 * reference.height());
+
+  bool is_bottom_right = IsPointOverRadiusVector(center, up_diag);
+
+  DisplayPlacement::Position position;
+  if (is_top_right) {
+    position =
+        is_bottom_right ? DisplayPlacement::RIGHT : DisplayPlacement::TOP;
+  } else {
+    position =
+        is_bottom_right ? DisplayPlacement::BOTTOM : DisplayPlacement::LEFT;
+  }
+
+  // If the rectangle with the calculated position would not have common side
+  // with the reference, try to position it so it shares another edge with the
+  // reference.
+  if (is_top_right == is_bottom_right) {
+    if (rectangle.y() > reference.bottom()) {
+      // The rectangle is left or right, but completely under the reference.
+      position = DisplayPlacement::BOTTOM;
+    } else if (rectangle.bottom() < reference.y()) {
+      // The rectangle is left or right, but completely over the reference.
+      position = DisplayPlacement::TOP;
+    }
+  } else {
+    if (rectangle.x() > reference.right()) {
+      // The rectangle is over or under, but completely right of the reference.
+      position = DisplayPlacement::RIGHT;
+    } else if (rectangle.right() < reference.x()) {
+      // The rectangle is over or under, but completely left of the reference.
+      position = DisplayPlacement::LEFT;
+    }
+  }
+  int offset = (position == DisplayPlacement::LEFT ||
+                position == DisplayPlacement::RIGHT)
+                   ? rectangle.y()
+                   : rectangle.x();
+  return DisplayPlacement(position, offset);
+}
+
 // static
 bool DisplayLayout::ApplyDisplayPlacement(const DisplayPlacement& placement,
                                           Displays* display_list,

ui/display/display_layout.h

@@ -15,6 +15,10 @@
 #include "base/strings/string_piece.h"
 #include "ui/display/display_export.h"
 
+namespace gfx {
+class Rect;
+}
+
 namespace display {
 class Display;
 
@@ -123,6 +127,12 @@ class DISPLAY_EXPORT DisplayLayout final {
   // otherwise returns a DisplayPlacement with an invalid display id.
   DisplayPlacement FindPlacementById(int64_t display_id) const;
 
+  // Creates a display::DisplayPlacement value for |rectangle| relative to the
+  // |reference| rectangle.
+  static DisplayPlacement CreatePlacementForRectangles(
+      const gfx::Rect& reference,
+      const gfx::Rect& rectangle);
+
  private:
   // Apply the display placement to |display_list|.
   // Returns true if the display bounds were updated.