Commit f792e7d8 authored by Rune Lillesveen's avatar Rune Lillesveen Committed by Commit Bot

Revert "window-service: ignore requests to focus null"

This reverts commit d00fd143.

Reason for revert: Caused test flakiness https://crbug.com/871652

Original change's description:
> 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/1164409
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Commit-Queue: Scott Violet <sky@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581113}

TBR=sky@chromium.org,msw@chromium.org

Change-Id: I7710ea88b1dcb53d5ad299ff1295ecec31ce3f87
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 867654
Reviewed-on: https://chromium-review.googlesource.com/1164782Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581148}
parent b396dc9a
...@@ -6,7 +6,6 @@ ...@@ -6,7 +6,6 @@
#include "ash/ash_service.h" #include "ash/ash_service.h"
#include "base/feature_list.h" #include "base/feature_list.h"
#include "base/files/file_path.h" #include "base/files/file_path.h"
#include "base/logging.h"
#include "base/path_service.h" #include "base/path_service.h"
#include "services/service_manager/public/cpp/service_runner.h" #include "services/service_manager/public/cpp/service_runner.h"
#include "ui/base/material_design/material_design_controller.h" #include "ui/base/material_design/material_design_controller.h"
...@@ -16,7 +15,6 @@ ...@@ -16,7 +15,6 @@
// This path is only hit in testing, not production. Production launches ash by // This path is only hit in testing, not production. Production launches ash by
// way of the utility process, which does not use this. // way of the utility process, which does not use this.
MojoResult ServiceMain(MojoHandle service_request_handle) { MojoResult ServiceMain(MojoHandle service_request_handle) {
logging::SetLogPrefix("ash");
// Load ash resources and strings. // Load ash resources and strings.
// TODO: investigate nuking ash_service_resources and use the same resources // TODO: investigate nuking ash_service_resources and use the same resources
// that are used when AshService is launched via the utility process. // that are used when AshService is launched via the utility process.
......
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "ash/shell/content/client/shell_content_browser_client.h" #include "ash/shell/content/client/shell_content_browser_client.h"
#include "base/command_line.h" #include "base/command_line.h"
#include "base/files/file_path.h" #include "base/files/file_path.h"
#include "base/logging.h"
#include "base/path_service.h" #include "base/path_service.h"
#include "content/public/utility/content_utility_client.h" #include "content/public/utility/content_utility_client.h"
#include "services/service_manager/public/cpp/service.h" #include "services/service_manager/public/cpp/service.h"
...@@ -27,18 +26,15 @@ namespace shell { ...@@ -27,18 +26,15 @@ namespace shell {
namespace { namespace {
std::unique_ptr<service_manager::Service> CreateQuickLaunch() { std::unique_ptr<service_manager::Service> CreateQuickLaunch() {
logging::SetLogPrefix("quick");
return std::make_unique<quick_launch::QuickLaunchApplication>(); return std::make_unique<quick_launch::QuickLaunchApplication>();
} }
std::unique_ptr<service_manager::Service> CreateShortcutViewer() { std::unique_ptr<service_manager::Service> CreateShortcutViewer() {
logging::SetLogPrefix("shortcut");
return std::make_unique< return std::make_unique<
keyboard_shortcut_viewer::ShortcutViewerApplication>(); keyboard_shortcut_viewer::ShortcutViewerApplication>();
} }
std::unique_ptr<service_manager::Service> CreateTapVisualizer() { std::unique_ptr<service_manager::Service> CreateTapVisualizer() {
logging::SetLogPrefix("tap");
return std::make_unique<tap_visualizer::TapVisualizerApp>(); return std::make_unique<tap_visualizer::TapVisualizerApp>();
} }
......
...@@ -51,25 +51,6 @@ bool FocusHandler::SetFocus(aura::Window* window) { ...@@ -51,25 +51,6 @@ bool FocusHandler::SetFocus(aura::Window* window) {
return true; 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, ClientChange change(window_tree_->property_change_tracker_.get(), window,
ClientChangeType::kFocus); ClientChangeType::kFocus);
focus_client->FocusWindow(window); focus_client->FocusWindow(window);
......
...@@ -40,32 +40,6 @@ TEST(FocusHandlerTest, FocusTopLevel) { ...@@ -40,32 +40,6 @@ TEST(FocusHandlerTest, FocusTopLevel) {
EXPECT_TRUE(top_level->HasFocus()); 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) { TEST(FocusHandlerTest, FocusNull) {
WindowServiceTestSetup setup; WindowServiceTestSetup setup;
aura::Window* top_level = aura::Window* top_level =
...@@ -74,8 +48,8 @@ TEST(FocusHandlerTest, FocusNull) { ...@@ -74,8 +48,8 @@ TEST(FocusHandlerTest, FocusNull) {
top_level->Show(); top_level->Show();
EXPECT_TRUE(setup.window_tree_test_helper()->SetFocus(top_level)); EXPECT_TRUE(setup.window_tree_test_helper()->SetFocus(top_level));
EXPECT_TRUE(top_level->HasFocus()); EXPECT_TRUE(top_level->HasFocus());
EXPECT_FALSE(setup.window_tree_test_helper()->SetFocus(nullptr)); EXPECT_TRUE(setup.window_tree_test_helper()->SetFocus(nullptr));
EXPECT_TRUE(top_level->HasFocus()); EXPECT_FALSE(top_level->HasFocus());
} }
TEST(FocusHandlerTest, FocusChild) { TEST(FocusHandlerTest, FocusChild) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment