Skip to content

Commit

Permalink
Reland OOBE display chooser commits + add Mash guard
Browse files Browse the repository at this point in the history 
This CL will reland / revert revert 2 commits and add a non-Mash guard
for listening to DeviceDataManager. A TODO has been added about using
InputDeviceClient in Mash, but it's not super urgent as the reaction to
the event is a NOP in Mash further down the call chain.

Revert "Revert of Remove confusing keyboard test & focus on input device"
  commit 8a2f3aa

Revert "Revert "Listen to changes to touch input devices""
  commit 0cd134a

[email protected]

(cherry picked from commit d706dfc)

Bug: 738885
Change-Id: Ic805ca3966df2969d8444b2c441eac24f75dce62
Reviewed-on: https://chromium-review.googlesource.com/575138
Commit-Queue: Felix Ekblom <[email protected]>
Reviewed-by: Mitsuru Oshima <[email protected]>
Reviewed-by: Jacob Dufault <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#488622}
Reviewed-on: https://chromium-review.googlesource.com/590352
Cr-Commit-Position: refs/branch-heads/3163@{#84}
Cr-Branched-From: ff259ba-refs/heads/master@{#488528}
  • Loading branch information
@jacobdufault-google
jacobdufault-google authored and Jacob Dufault committed Jul 27, 2017
1 parent 63431ed commit 97e7eb3
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 60 deletions.
20 changes: 20 additions & 0 deletions chrome/browser/chromeos/login/ui/login_display_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
#include "ui/compositor/scoped_layer_animation_settings.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
#include "ui/events/devices/device_data_manager.h"
#include "ui/events/event_handler.h"
#include "ui/events/event_utils.h"
#include "ui/gfx/geometry/rect.h"
Expand Down Expand Up @@ -384,6 +385,12 @@ LoginDisplayHostImpl::LoginDisplayHostImpl(const gfx::Rect& wallpaper_bounds)

display::Screen::GetScreen()->AddObserver(this);

// TODO(crbug.com/747267): Add Mash case. Not strictly needed since callee in
// observer method is NOP in Mash, but good for symmetry and to avoid leaking
// implementation details about OobeUI.
if (!ash_util::IsRunningInMash())
ui::DeviceDataManager::GetInstance()->AddObserver(this);

// We need to listen to CLOSE_ALL_BROWSERS_REQUEST but not APP_TERMINATING
// because/ APP_TERMINATING will never be fired as long as this keeps
// ref-count. CLOSE_ALL_BROWSERS_REQUEST is safe here because there will be no
Expand Down Expand Up @@ -494,6 +501,12 @@ LoginDisplayHostImpl::~LoginDisplayHostImpl() {
CrasAudioHandler::Get()->RemoveAudioObserver(this);
display::Screen::GetScreen()->RemoveObserver(this);

// TODO(crbug.com/747267): Add Mash case. Not strictly needed since callee in
// observer method is NOP in Mash, but good for symmetry and to avoid leaking
// implementation details about OobeUI.
if (!ash_util::IsRunningInMash())
ui::DeviceDataManager::GetInstance()->RemoveObserver(this);

if (login_view_ && login_window_)
login_window_->RemoveRemovalsObserver(this);

Expand Down Expand Up @@ -1017,6 +1030,13 @@ void LoginDisplayHostImpl::OnDisplayMetricsChanged(
}
}

////////////////////////////////////////////////////////////////////////////////
// LoginDisplayHostImpl, ui::InputDeviceEventObserver
void LoginDisplayHostImpl::OnTouchscreenDeviceConfigurationChanged() {
if (GetOobeUI())
GetOobeUI()->OnDisplayConfigurationChanged();
}

////////////////////////////////////////////////////////////////////////////////
// LoginDisplayHostImpl, views::WidgetRemovalsObserver:
void LoginDisplayHostImpl::OnWillRemoveView(views::Widget* widget,
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/chromeos/login/ui/login_display_host_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/web_contents_observer.h"
#include "ui/display/display_observer.h"
#include "ui/events/devices/input_device_event_observer.h"
#include "ui/gfx/geometry/rect.h"
#include "ui/views/widget/widget_removals_observer.h"
#include "ui/wm/public/scoped_drag_drop_disabler.h"
Expand All @@ -51,6 +52,7 @@ class LoginDisplayHostImpl : public LoginDisplayHost,
public chromeos::SessionManagerClient::Observer,
public chromeos::CrasAudioHandler::AudioObserver,
public display::DisplayObserver,
public ui::InputDeviceEventObserver,
public views::WidgetRemovalsObserver,
public chrome::MultiUserWindowManager::Observer {
public:
Expand Down Expand Up @@ -122,6 +124,9 @@ class LoginDisplayHostImpl : public LoginDisplayHost,
void OnDisplayMetricsChanged(const display::Display& display,
uint32_t changed_metrics) override;

// Overridden from ui::InputDeviceEventObserver
void OnTouchscreenDeviceConfigurationChanged() override;

// Overriden from views::WidgetRemovalsObserver:
void OnWillRemoveView(views::Widget* widget, views::View* view) override;

Expand Down
36 changes: 23 additions & 13 deletions chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@

#include "chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h"

#include <stdint.h>

#include "ash/display/window_tree_host_manager.h"
#include "ash/shell.h"
#include "base/stl_util.h"
#include "content/public/browser/browser_thread.h"
#include "ui/display/display.h"
#include "ui/display/display_layout.h"
#include "ui/display/manager/display_manager.h"
#include "ui/display/screen.h"
#include "ui/events/devices/device_data_manager.h"
#include "ui/events/devices/touchscreen_device.h"

using content::BrowserThread;

Expand All @@ -23,6 +27,9 @@ bool TouchSupportAvailable(const display::Display& display) {
display::Display::TouchSupport::TOUCH_SUPPORT_AVAILABLE;
}

// TODO(felixe): More context at crbug.com/738885
const uint16_t kDeviceIds[] = {0x0457, 0x266e};

} // namespace

OobeDisplayChooser::OobeDisplayChooser() : weak_ptr_factory_(this) {}
Expand Down Expand Up @@ -51,18 +58,21 @@ void OobeDisplayChooser::TryToPlaceUiOnTouchDisplay() {
void OobeDisplayChooser::MoveToTouchDisplay() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);

const display::Displays& displays =
ash::Shell::Get()->display_manager()->active_only_display_list();

if (displays.size() <= 1)
return;

for (const display::Display& display : displays) {
if (TouchSupportAvailable(display)) {
ash::Shell::Get()->window_tree_host_manager()->SetPrimaryDisplayId(
display.id());
break;
}
const ui::DeviceDataManager* device_manager =
ui::DeviceDataManager::GetInstance();
for (const ui::TouchscreenDevice& device :
device_manager->GetTouchscreenDevices()) {
if (!base::ContainsValue(kDeviceIds, device.vendor_id))
continue;

int64_t display_id =
device_manager->GetTargetDisplayForTouchDevice(device.id);
if (display_id == display::kInvalidDisplayId)
continue;

ash::Shell::Get()->window_tree_host_manager()->SetPrimaryDisplayId(
display_id);
break;
}
}

Expand Down
104 changes: 69 additions & 35 deletions chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h"

#include <memory>
#include <vector>

#include "ash/display/display_configuration_controller.h"
#include "ash/shell.h"
Expand All @@ -13,9 +14,12 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/display/display.h"
#include "ui/display/display_observer.h"
#include "ui/display/manager/chromeos/touchscreen_util.h"
#include "ui/display/manager/display_manager.h"
#include "ui/display/screen.h"
#include "ui/display/test/display_manager_test_api.h"
#include "ui/events/devices/device_data_manager.h"
#include "ui/events/devices/touchscreen_device.h"

namespace chromeos {

Expand All @@ -25,63 +29,93 @@ class OobeDisplayChooserTest : public ash::AshTestBase {
public:
OobeDisplayChooserTest() : ash::AshTestBase() {}

void SetUp() override {
ash::AshTestBase::SetUp();
display_manager_test_api_.reset(
new display::test::DisplayManagerTestApi(display_manager()));
int64_t GetPrimaryDisplay() {
return display::Screen::GetScreen()->GetPrimaryDisplay().id();
}

void EnableTouch(int64_t id) {
display_manager_test_api_->SetTouchSupport(
id, display::Display::TouchSupport::TOUCH_SUPPORT_AVAILABLE);
}
void UpdateTouchscreenDevices(const ui::TouchscreenDevice& touchscreen) {
std::vector<ui::TouchscreenDevice> devices{touchscreen};

void DisableTouch(int64_t id) {
display_manager_test_api_->SetTouchSupport(
id, display::Display::TouchSupport::TOUCH_SUPPORT_UNAVAILABLE);
}

int64_t GetPrimaryDisplay() {
return display::Screen::GetScreen()->GetPrimaryDisplay().id();
ui::DeviceHotplugEventObserver* manager =
ui::DeviceDataManager::GetInstance();
manager->OnTouchscreenDevicesUpdated(devices);
}

private:
std::unique_ptr<display::test::DisplayManagerTestApi>
display_manager_test_api_;

DISALLOW_COPY_AND_ASSIGN(OobeDisplayChooserTest);
};

const uint16_t kWhitelistedId = 0x266e;

} // namespace

TEST_F(OobeDisplayChooserTest, PreferTouchAsPrimary) {
OobeDisplayChooser display_chooser;
// Setup 2 displays, second one is intended to be a touch display
std::vector<display::ManagedDisplayInfo> display_info;
display_info.push_back(
display::ManagedDisplayInfo::CreateFromSpecWithID("0+0-3000x2000", 1));
display_info.push_back(
display::ManagedDisplayInfo::CreateFromSpecWithID("3000+0-800x600", 2));
display_manager()->OnNativeDisplaysChanged(display_info);
base::RunLoop().RunUntilIdle();

UpdateDisplay("3000x2000,800x600");
display::DisplayIdList ids = display_manager()->GetCurrentDisplayIdList();
DisableTouch(ids[0]);
EnableTouch(ids[1]);
// Make sure the non-touch display is primary
ash::Shell::Get()->window_tree_host_manager()->SetPrimaryDisplayId(1);

EXPECT_EQ(ids[0], GetPrimaryDisplay());
display_chooser.TryToPlaceUiOnTouchDisplay();
// Setup corresponding TouchscreenDevice object
ui::TouchscreenDevice touchscreen =
ui::TouchscreenDevice(1, ui::InputDeviceType::INPUT_DEVICE_EXTERNAL,
"Touchscreen", gfx::Size(800, 600), 1);
touchscreen.vendor_id = kWhitelistedId;
UpdateTouchscreenDevices(touchscreen);
base::RunLoop().RunUntilIdle();

EXPECT_EQ(ids[1], GetPrimaryDisplay());
}
// Associate touchscreen device with display
display_info[1].AddInputDevice(touchscreen.id);
display_info[1].set_touch_support(display::Display::TOUCH_SUPPORT_AVAILABLE);
display_manager()->OnNativeDisplaysChanged(display_info);
base::RunLoop().RunUntilIdle();

TEST_F(OobeDisplayChooserTest, AddingSecondTouchDisplayShouldbeNOP) {
OobeDisplayChooser display_chooser;
EXPECT_EQ(1, GetPrimaryDisplay());
display_chooser.TryToPlaceUiOnTouchDisplay();
base::RunLoop().RunUntilIdle();
EXPECT_EQ(2, GetPrimaryDisplay());
}

UpdateDisplay("3000x2000,800x600");
display::DisplayIdList ids = display_manager()->GetCurrentDisplayIdList();
EnableTouch(ids[0]);
EnableTouch(ids[1]);
TEST_F(OobeDisplayChooserTest, DontSwitchFromTouch) {
// Setup 2 displays, second one is intended to be a touch display
std::vector<display::ManagedDisplayInfo> display_info;
display_info.push_back(
display::ManagedDisplayInfo::CreateFromSpecWithID("0+0-3000x2000", 1));
display_info.push_back(
display::ManagedDisplayInfo::CreateFromSpecWithID("3000+0-800x600", 2));
display_info[0].set_touch_support(display::Display::TOUCH_SUPPORT_AVAILABLE);
display_manager()->OnNativeDisplaysChanged(display_info);
base::RunLoop().RunUntilIdle();

EXPECT_EQ(ids[0], GetPrimaryDisplay());
display_chooser.TryToPlaceUiOnTouchDisplay();
// Make sure the non-touch display is primary
ash::Shell::Get()->window_tree_host_manager()->SetPrimaryDisplayId(1);

// Setup corresponding TouchscreenDevice object
ui::TouchscreenDevice touchscreen =
ui::TouchscreenDevice(1, ui::InputDeviceType::INPUT_DEVICE_EXTERNAL,
"Touchscreen", gfx::Size(800, 600), 1);
touchscreen.vendor_id = kWhitelistedId;
UpdateTouchscreenDevices(touchscreen);
base::RunLoop().RunUntilIdle();

// Associate touchscreen device with display
display_info[1].AddInputDevice(touchscreen.id);
display_info[1].set_touch_support(display::Display::TOUCH_SUPPORT_AVAILABLE);
display_manager()->OnNativeDisplaysChanged(display_info);
base::RunLoop().RunUntilIdle();

EXPECT_EQ(ids[0], GetPrimaryDisplay());
OobeDisplayChooser display_chooser;
EXPECT_EQ(1, GetPrimaryDisplay());
display_chooser.TryToPlaceUiOnTouchDisplay();
base::RunLoop().RunUntilIdle();
EXPECT_EQ(1, GetPrimaryDisplay());
}

} // namespace chromeos
19 changes: 7 additions & 12 deletions chrome/browser/ui/webui/chromeos/login/oobe_ui.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,17 +226,12 @@ std::string GetDisplayType(const GURL& url) {
return path;
}

bool IsKeyboardConnected() {
const std::vector<ui::InputDevice>& keyboards =
ui::InputDeviceManager::GetInstance()->GetKeyboardDevices();
for (const ui::InputDevice& keyboard : keyboards) {
if (keyboard.type == ui::INPUT_DEVICE_INTERNAL ||
keyboard.type == ui::INPUT_DEVICE_EXTERNAL) {
return true;
}
}

return false;
bool IsRemoraRequisitioned() {
policy::DeviceCloudPolicyManagerChromeOS* policy_manager =
g_browser_process->platform_part()
->browser_policy_connector_chromeos()
->GetDeviceCloudPolicyManager();
return policy_manager && policy_manager->IsRemoraRequisition();
}

} // namespace
Expand Down Expand Up @@ -376,7 +371,7 @@ OobeUI::OobeUI(content::WebUI* web_ui, const GURL& url)

// TODO(felixe): Display iteration and primary display selection not supported
// in Mash. See http://crbug.com/720917.
if (!ash_util::IsRunningInMash() && !IsKeyboardConnected())
if (!ash_util::IsRunningInMash() && IsRemoraRequisitioned())
oobe_display_chooser_ = base::MakeUnique<OobeDisplayChooser>();
}

Expand Down

0 comments on commit 97e7eb3

Please sign in to comment.