cros: enable keyboard shortcut to move active window between displays

changes:
Enable the feature by default. Also fix the bug that window cannot be
moved. display::CreateDisplayIdList is sorting using the first 8 bits,
pass that comparator to std::upper_bound too.

TBR=stevenjb@chromium.org

Bug: 778438
Test: tested on device.
Change-Id: Ic8b9df780f99f0466805f8857e2cb0414a163999
Reviewed-on: https://chromium-review.googlesource.com/954361
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Qiang Xu <warx@google.com>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541886}

ash/display/display_move_window_util.cc

@@ -9,7 +9,7 @@
 #include <array>
 
 #include "ash/accessibility/accessibility_controller.h"
-#include "ash/public/cpp/ash_switches.h"
+#include "ash/public/cpp/ash_features.h"
 #include "ash/shell.h"
 #include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/window_util.h"
@@ -47,7 +47,7 @@ aura::Window* GetTargetWindow() {
 }  // namespace
 
 bool CanHandleMoveActiveWindowBetweenDisplays() {
-  if (!switches::IsDisplayMoveWindowAccelsEnabled())
+  if (!features::IsDisplayMoveWindowAccelsEnabled())
     return false;
   display::DisplayManager* display_manager = Shell::Get()->display_manager();
   // Accelerators to move window between displays on unified desktop mode and
@@ -76,7 +76,7 @@ void HandleMoveActiveWindowBetweenDisplays() {
       display::CreateDisplayIdList(displays);
   // Find target display id in sorted display id list in a cycling way.
   auto itr = std::upper_bound(display_id_list.begin(), display_id_list.end(),
-                              origin_display_id);
+                              origin_display_id, display::CompareDisplayIds);
   int64_t target_display_id =
       itr == display_id_list.end() ? display_id_list[0] : *itr;
   wm::MoveWindowToDisplay(window, target_display_id);

ash/display/display_move_window_util_unittest.cc

@@ -8,7 +8,7 @@
 #include "ash/accelerators/accelerator_table.h"
 #include "ash/accessibility/accessibility_controller.h"
 #include "ash/accessibility/test_accessibility_controller_client.h"
-#include "ash/public/cpp/ash_switches.h"
+#include "ash/public/cpp/ash_features.h"
 #include "ash/root_window_controller.h"
 #include "ash/screen_util.h"
 #include "ash/shell.h"
@@ -19,6 +19,7 @@
 #include "ash/wm/wm_event.h"
 #include "base/command_line.h"
 #include "base/macros.h"
+#include "base/test/scoped_feature_list.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/display/display.h"
 #include "ui/display/display_layout.h"
@@ -74,12 +75,15 @@ class DisplayMoveWindowUtilTest : public AshTestBase {
 
   // AshTestBase:
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kAshEnableDisplayMoveWindowAccels);
+    // Explicitly enable the display move window accels feature for the tests.
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kDisplayMoveWindowAccels);
     AshTestBase::SetUp();
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(DisplayMoveWindowUtilTest);
 };
 

ash/display/persistent_window_controller_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "ash/display/display_move_window_util.h"
 #include "ash/display/window_tree_host_manager.h"
+#include "ash/public/cpp/ash_features.h"
 #include "ash/public/cpp/ash_switches.h"
 #include "ash/session/test_session_controller_client.h"
 #include "ash/shell.h"
@@ -14,6 +15,7 @@
 #include "ash/wm/window_util.h"
 #include "base/command_line.h"
 #include "base/test/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 
 using session_manager::SessionState;
 
@@ -26,14 +28,16 @@ class PersistentWindowControllerTest : public AshTestBase {
 
   // AshTestBase:
   void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kAshEnableDisplayMoveWindowAccels);
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kDisplayMoveWindowAccels);
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kAshEnablePersistentWindowBounds);
     AshTestBase::SetUp();
   }
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(PersistentWindowControllerTest);
 };
 

ash/public/cpp/ash_features.cc

@@ -7,6 +7,9 @@
 namespace ash {
 namespace features {
 
+const base::Feature kDisplayMoveWindowAccels{"DisplayMoveWindowAccels",
+                                             base::FEATURE_ENABLED_BY_DEFAULT};
+
 const base::Feature kDockedMagnifier{"DockedMagnifier",
                                      base::FEATURE_ENABLED_BY_DEFAULT};
 
@@ -22,6 +25,10 @@ const base::Feature kSystemTrayUnified{"SystemTrayUnified",
 const base::Feature kLockScreenNotifications{"LockScreenNotifications",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
 
+bool IsDisplayMoveWindowAccelsEnabled() {
+  return base::FeatureList::IsEnabled(kDisplayMoveWindowAccels);
+}
+
 bool IsDockedMagnifierEnabled() {
   return base::FeatureList::IsEnabled(kDockedMagnifier);
 }

ash/public/cpp/ash_features.h

@@ -11,6 +11,11 @@
 namespace ash {
 namespace features {
 
+// Enables the keyboard shortcut to move active window between displays.
+// TODO(warx): Remove this after the feature is fully launched.
+// https://crbug.com/773749.
+ASH_PUBLIC_EXPORT extern const base::Feature kDisplayMoveWindowAccels;
+
 // Enables the docked (a.k.a. picture-in-picture) magnifier.
 // TODO(afakhry): Remove this after the feature is fully launched.
 // https://crbug.com/709824.
@@ -32,6 +37,8 @@ ASH_PUBLIC_EXPORT extern const base::Feature kSystemTrayUnified;
 // Enables notifications on the lock screen.
 ASH_PUBLIC_EXPORT extern const base::Feature kLockScreenNotifications;
 
+ASH_PUBLIC_EXPORT bool IsDisplayMoveWindowAccelsEnabled();
+
 ASH_PUBLIC_EXPORT bool IsDockedMagnifierEnabled();
 
 ASH_PUBLIC_EXPORT bool IsKeyboardShortcutViewerEnabled();

ash/public/cpp/ash_switches.cc

@@ -41,11 +41,6 @@ const char kAshDisableTouchExplorationMode[] =
 // TODO(oshima): Remove this once the feature is launched. crbug.com/749713.
 const char kAshEnableV1AppBackButton[] = "ash-enable-v1-app-back-button";
 
-// Enables move window between displays accelerators.
-// TODO(warx): Remove this once the feature is launched. crbug.com/773749.
-const char kAshEnableDisplayMoveWindowAccels[] =
-    "ash-enable-display-move-window-accels";
-
 // Enables key bindings to scroll magnified screen.
 const char kAshEnableMagnifierKeyScroller[] =
     "ash-enable-magnifier-key-scroller";
@@ -159,11 +154,6 @@ const char kSuppressMessageCenterPopups[] = "suppress-message-center-popups";
 // enables the IME service (i.e. InputMethodMus) instead.
 const char kUseIMEService[] = "use-ime-service";
 
-bool IsDisplayMoveWindowAccelsEnabled() {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      kAshEnableDisplayMoveWindowAccels);
-}
-
 bool IsNightLightEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       kAshEnableNightLight);

ash/public/cpp/ash_switches.h

@@ -25,7 +25,6 @@ ASH_PUBLIC_EXPORT extern const char kAshDisableTabletSplitView[];
 ASH_PUBLIC_EXPORT extern const char kAshDisableTrilinearFiltering[];
 ASH_PUBLIC_EXPORT extern const char kAshDisableTouchExplorationMode[];
 ASH_PUBLIC_EXPORT extern const char kAshEnableV1AppBackButton[];
-ASH_PUBLIC_EXPORT extern const char kAshEnableDisplayMoveWindowAccels[];
 ASH_PUBLIC_EXPORT extern const char kAshEnableMagnifierKeyScroller[];
 ASH_PUBLIC_EXPORT extern const char kAshEnableNewOverviewUi[];
 ASH_PUBLIC_EXPORT extern const char kAshEnableNightLight[];

chrome/browser/about_flags.cc

@@ -3582,7 +3582,7 @@ const FeatureEntry kFeatureEntries[] = {
     {"ash-enable-display-move-window-accels",
      flag_descriptions::kAshEnableDisplayMoveWindowAccelsName,
      flag_descriptions::kAshEnableDisplayMoveWindowAccelsDescription, kOsCrOS,
-     SINGLE_VALUE_TYPE(ash::switches::kAshEnableDisplayMoveWindowAccels)},
+     FEATURE_VALUE_TYPE(ash::features::kDisplayMoveWindowAccels)},
 
     {"ash-enable-keyboard-shortcut-viewer",
      flag_descriptions::kAshEnableKeyboardShortcutViewerName,

chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc

@@ -8,7 +8,7 @@
 
 #include <memory>
 
-#include "ash/public/cpp/ash_switches.h"
+#include "ash/public/cpp/ash_features.h"
 #include "ash/shell.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -337,7 +337,7 @@ content::WebUIDataSource* CreateKeyboardOverlayUIHTMLSource(Profile* profile) {
   source->AddBoolean("voiceInteractionEnabled",
                      chromeos::switches::IsVoiceInteractionEnabled());
   source->AddBoolean("displayMoveWindowAccelsEnabled",
-                     ash::switches::IsDisplayMoveWindowAccelsEnabled());
+                     ash::features::IsDisplayMoveWindowAccelsEnabled());
   source->AddBoolean("keyboardOverlayUsesLayout2",
                      ui::DeviceUsesKeyboardLayout2());
   ash::Shell* shell = ash::Shell::Get();

tools/metrics/histograms/enums.xml

@@ -26497,7 +26497,6 @@ from previous Chrome versions.
   <int value="11698808" label="enable-dom-distiller-button-animation"/>
   <int value="23556595" label="MarkHttpAs:enabled"/>
   <int value="27507364" label="apps-keep-chrome-alive"/>
-  <int value="28272521" label="ash-enable-display-move-window-accels"/>
   <int value="31848187" label="ViewsTaskManager:disabled"/>
   <int value="33778663" label="OriginTrials:enabled"/>
   <int value="37024318" label="disable-affiliation-based-matching"/>
@@ -26529,6 +26528,7 @@ from previous Chrome versions.
   <int value="84911198" label="ScanCardsInWebPayments:disabled"/>
   <int value="88437020" label="FeaturePolicy:enabled"/>
   <int value="91938915" label="enable-suggestions-service"/>
+  <int value="92327255" label="DisplayMoveWindowAccels:disabled"/>
   <int value="98134240" label="material-design-ink-drop-animation-speed"/>
   <int value="103932290" label="show-autofill-type-predictions"/>
   <int value="105046382" label="ParallelDownloading:disabled"/>
@@ -27283,6 +27283,7 @@ from previous Chrome versions.
   <int value="1966730288" label="disable-threaded-compositing"/>
   <int value="1969604362" label="enable-pinch-virtual-viewport"/>
   <int value="1971964569" label="NewEncodeCpuLoadEstimator:disabled"/>
+  <int value="1972232935" label="DisplayMoveWindowAccels:enabled"/>
   <int value="1980011075" label="debug-packed-apps"/>
   <int value="1980648371" label="PointerEventV1SpecCapturing:enabled"/>
   <int value="1988506961" label="EnableManualSaving:enabled"/>