window-service: ignore requests to focus null

Here's my comment as to why this is ignored:

  // The client is asking to remove focus from a window. This is typically a
  // side effect of the window becoming, or about to become, an unfocusable
  // Window (for example, the Window is hiding). Windows becoming unfocusable is
  // handled locally. Assume the request is for such a scenario and return
  // false to ignore the change.
  //
  // To process null requests conflicts with top-level activation changes. For
  // example, the typical sequence when a window is hidden is to first remove
  // focus, and then hide the window. FocusController keys off window hiding to
  // move activation. If this code where to set focus to null, FocusController
  // would not see the window hiding (because the active window was set to null)
  // and not automatically activate the next window.
  //
  // Another possibility for this code is to handle null as a signal to move
  // focus to the active window (if there is one). I'm going with the simpler
  // approach for now.

BUG=867654
TEST=covered by tests

Change-Id: Iee8efd8895284acca4603ff22bde19adf16d8a7f
Reviewed-on: https://chromium-review.googlesource.com/1164409Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581113}

ash/main.cc

@@ -6,6 +6,7 @@
 #include "ash/ash_service.h"
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
+#include "base/logging.h"
 #include "base/path_service.h"
 #include "services/service_manager/public/cpp/service_runner.h"
 #include "ui/base/material_design/material_design_controller.h"
@@ -15,6 +16,7 @@
 // This path is only hit in testing, not production. Production launches ash by
 // way of the utility process, which does not use this.
 MojoResult ServiceMain(MojoHandle service_request_handle) {
+  logging::SetLogPrefix("ash");
   // Load ash resources and strings.
   // TODO: investigate nuking ash_service_resources and use the same resources
   // that are used when AshService is launched via the utility process.

ash/shell/content/client/shell_main_delegate.cc

@@ -13,6 +13,7 @@
 #include "ash/shell/content/client/shell_content_browser_client.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/logging.h"
 #include "base/path_service.h"
 #include "content/public/utility/content_utility_client.h"
 #include "services/service_manager/public/cpp/service.h"
@@ -26,15 +27,18 @@ namespace shell {
 namespace {
 
 std::unique_ptr<service_manager::Service> CreateQuickLaunch() {
+  logging::SetLogPrefix("quick");
   return std::make_unique<quick_launch::QuickLaunchApplication>();
 }
 
 std::unique_ptr<service_manager::Service> CreateShortcutViewer() {
+  logging::SetLogPrefix("shortcut");
   return std::make_unique<
       keyboard_shortcut_viewer::ShortcutViewerApplication>();
 }
 
 std::unique_ptr<service_manager::Service> CreateTapVisualizer() {
+  logging::SetLogPrefix("tap");
   return std::make_unique<tap_visualizer::TapVisualizerApp>();
 }
 

services/ui/ws2/focus_handler.cc

@@ -51,6 +51,25 @@ bool FocusHandler::SetFocus(aura::Window* window) {
     return true;
   }
 
+  // The client is asking to remove focus from a window. This is typically a
+  // side effect of the window becoming, or about to become, an unfocusable
+  // Window (for example, the Window is hiding). Windows becoming unfocusable is
+  // handled locally. Assume the request is for such a scenario and return
+  // false to ignore the change.
+  //
+  // To process null requests conflicts with top-level activation changes. For
+  // example, the typical sequence when a window is hidden is to first remove
+  // focus, and then hide the window. FocusController keys off window hiding to
+  // move activation. If this code were to set focus to null, FocusController
+  // would not see the window hiding (because the active window was set to null)
+  // and not automatically activate the next window.
+  //
+  // Another possibility for this code is to handle null as a signal to move
+  // focus to the active window (if there is one). I'm going with the simpler
+  // approach for now.
+  if (!window)
+    return false;
+
   ClientChange change(window_tree_->property_change_tracker_.get(), window,
                       ClientChangeType::kFocus);
   focus_client->FocusWindow(window);

services/ui/ws2/focus_handler_unittest.cc

@@ -40,6 +40,32 @@ TEST(FocusHandlerTest, FocusTopLevel) {
   EXPECT_TRUE(top_level->HasFocus());
 }
 
+// This test simulates the typical sequence of a client closing a window:
+// SetFocus(nullptr) and then Hide().
+TEST(FocusHandlerTest, ActiveWindowChangesAfterSetFocusToNullAndHide) {
+  // Create two top-levels and focus the second.
+  WindowServiceTestSetup setup;
+  aura::Window* top_level1 =
+      setup.window_tree_test_helper()->NewTopLevelWindow();
+  ASSERT_TRUE(top_level1);
+  top_level1->Show();
+  aura::Window* top_level2 =
+      setup.window_tree_test_helper()->NewTopLevelWindow();
+  ASSERT_TRUE(top_level2);
+  top_level2->Show();
+  EXPECT_TRUE(setup.window_tree_test_helper()->SetFocus(top_level2));
+  EXPECT_TRUE(top_level2->HasFocus());
+
+  // SetFocus(nullptr) should fail (focus should not change).
+  EXPECT_FALSE(setup.window_tree_test_helper()->SetFocus(nullptr));
+  EXPECT_TRUE(top_level2->HasFocus());
+
+  // Hiding |top_level2| should focus |top_level1|.
+  top_level2->Hide();
+  EXPECT_FALSE(top_level2->HasFocus());
+  EXPECT_TRUE(top_level1->HasFocus());
+}
+
 TEST(FocusHandlerTest, FocusNull) {
   WindowServiceTestSetup setup;
   aura::Window* top_level =
@@ -48,8 +74,8 @@ TEST(FocusHandlerTest, FocusNull) {
   top_level->Show();
   EXPECT_TRUE(setup.window_tree_test_helper()->SetFocus(top_level));
   EXPECT_TRUE(top_level->HasFocus());
-  EXPECT_TRUE(setup.window_tree_test_helper()->SetFocus(nullptr));
-  EXPECT_FALSE(top_level->HasFocus());
+  EXPECT_FALSE(setup.window_tree_test_helper()->SetFocus(nullptr));
+  EXPECT_TRUE(top_level->HasFocus());
 }
 
 TEST(FocusHandlerTest, FocusChild) {