Skip to content

Commit

Permalink
Revert "Fix rounding error in coordinate conversion"
Browse files Browse the repository at this point in the history 
This reverts commit 545c7c0.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Fix rounding error in coordinate conversion
> 
> Fix the rounding error in code for coordinate conversion from
> native host coordinate to screen DIP coordinate.
> 
> In the original code, when mouse cursor is in the warp region of
> secondary display, its screen DIP coordinate which is computed by
> ConvertHostPointToRelativeToRootWindow may be out of secondary display
> due to rounding error. It is also found that this rounding error only
> happens with specific display zoom factors and display rotation
> degrees.
> 
> test: ash_unittests
> 
> Bug: 905035
> Change-Id: I36af26afe4e210d70b6812a1bddb1a95feb7235f
> Reviewed-on: https://chromium-review.googlesource.com/c/1343249
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Commit-Queue: Andrew Xu <andrewxu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612923}

TBR=oshima@chromium.org,afakhry@chromium.org,andrewxu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 905035
Change-Id: I46f3dacd9c293ad4dcc68314969ae8c1127d523d
Reviewed-on: https://chromium-review.googlesource.com/c/1364052
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614145}
  • Loading branch information
Andrew Xu authored and Commit Bot committed Dec 5, 2018
1 parent 25754cd commit beb3651
Show file tree
Hide file tree
Showing 6 changed files with 4 additions and 156 deletions.
11 changes: 1 addition & 10 deletions ash/display/extended_mouse_warp_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,19 +89,10 @@ ExtendedMouseWarpController::WarpRegion::GetIndicatorBoundsForTest(
int64_t id) const {
if (a_display_id_ == id)
return a_indicator_bounds_;
DCHECK_EQ(b_display_id_, id);
CHECK_EQ(b_display_id_, id);
return b_indicator_bounds_;
}

const gfx::Rect&
ExtendedMouseWarpController::WarpRegion::GetIndicatorNativeBoundsForTest(
int64_t id) const {
if (a_display_id_ == id)
return a_edge_bounds_in_native_;
DCHECK_EQ(b_display_id_, id);
return b_edge_bounds_in_native_;
}

ExtendedMouseWarpController::ExtendedMouseWarpController(
aura::Window* drag_source)
: drag_source_root_(drag_source), allow_non_native_event_(false) {
Expand Down
1 change: 0 additions & 1 deletion ash/display/extended_mouse_warp_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ class ASH_EXPORT ExtendedMouseWarpController : public MouseWarpController {
const gfx::Rect& b_indicator_bounds() { return b_indicator_bounds_; }

const gfx::Rect& GetIndicatorBoundsForTest(int64_t id) const;
const gfx::Rect& GetIndicatorNativeBoundsForTest(int64_t id) const;

private:
friend class ExtendedMouseWarpController;
Expand Down
133 changes: 0 additions & 133 deletions ash/display/extended_mouse_warp_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,15 @@
#include "ash/display/extended_mouse_warp_controller.h"

#include "ash/display/mouse_cursor_event_filter.h"
#include "ash/display/screen_position_controller.h"
#include "ash/host/ash_window_tree_host_platform.h"
#include "ash/shell.h"
#include "ash/test/ash_test_base.h"
#include "ui/aura/env.h"
#include "ui/aura/test/test_windows.h"
#include "ui/aura/window_tree_host.h"
#include "ui/base/hit_test.h"
#include "ui/display/display.h"
#include "ui/display/display_layout.h"
#include "ui/display/display_layout_builder.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/base_event_utils.h"
#include "ui/events/test/event_generator.h"
#include "ui/wm/core/coordinate_conversion.h"

namespace ash {

Expand Down Expand Up @@ -52,28 +44,6 @@ class ExtendedMouseWarpControllerTest : public AshTestBase {
return GetWarpRegion(0)->GetIndicatorBoundsForTest(id);
}

const gfx::Rect& GetIndicatorNativeBounds(int64_t id) {
return GetWarpRegion(0)->GetIndicatorNativeBoundsForTest(id);
}

// Send mouse event with native event through AshWindowTreeHostPlatform.
void DispatchMouseEventWithNative(AshWindowTreeHostPlatform* host,
const gfx::Point& location_in_host_native,
ui::EventType event_type,
int event_flag1,
int event_flag2) {
ui::MouseEvent native_event(event_type, location_in_host_native,
location_in_host_native, ui::EventTimeForNow(),
event_flag1, event_flag2);
ui::MouseEvent mouseev(&native_event);
host->DispatchEventFromQueue(&mouseev);

// The test relies on the last_mouse_location, which will be updated by
// a synthesized event posted asynchronusly. Wait until the synthesized
// event is handled and last mouse location is updated.
base::RunLoop().RunUntilIdle();
}

private:
DISALLOW_COPY_AND_ASSIGN(ExtendedMouseWarpControllerTest);
};
Expand Down Expand Up @@ -386,107 +356,4 @@ TEST_F(ExtendedMouseWarpControllerTest,
event_filter()->HideSharedEdgeIndicator();
}

// Check that the point in the rotated secondary display's warp region is
// converted correctly from native host coordinates to screen DIP coordinates.
// (see https://crbug.com/905035)
TEST_F(ExtendedMouseWarpControllerTest,
CheckHostPointToScreenInMouseWarpRegion) {
// Zoom factor is needed to trigger rounding error which occured in previous
// code.
UpdateDisplay("50+50-200x200@0.8,50+300-300x100/r");

aura::Window::Windows root_windows = Shell::Get()->GetAllRootWindows();

// Check the primary display's size and scale.
display::Display primary_display =
display::Screen::GetScreen()->GetDisplayNearestWindow(root_windows[0]);
ASSERT_EQ("250x250", primary_display.size().ToString());
ASSERT_EQ(0.8f, primary_display.device_scale_factor());

// Create a window to be dragged in primary display.
std::unique_ptr<aura::test::TestWindowDelegate> test_window_delegate =
std::make_unique<aura::test::TestWindowDelegate>();
test_window_delegate->set_window_component(HTCAPTION);
const gfx::Size initial_window_size(100, 100);
std::unique_ptr<aura::Window> test_window(
CreateTestWindowInShellWithDelegateAndType(
test_window_delegate.get(), aura::client::WINDOW_TYPE_NORMAL, 0,
gfx::Rect(initial_window_size)));
ASSERT_EQ(root_windows[0], test_window->GetRootWindow());
ASSERT_FALSE(test_window->HasCapture());

AshWindowTreeHostPlatform* window_host =
static_cast<AshWindowTreeHostPlatform*>(root_windows[0]->GetHost());

// Move mouse cursor and capture the window.
gfx::Point location_in_host_native(0, 0);
DispatchMouseEventWithNative(window_host, location_in_host_native,
ui::ET_MOUSE_MOVED, ui::EF_NONE, ui::EF_NONE);
DispatchMouseEventWithNative(window_host, location_in_host_native,
ui::ET_MOUSE_PRESSED, ui::EF_LEFT_MOUSE_BUTTON,
ui::EF_LEFT_MOUSE_BUTTON);

// Window should be captured.
ASSERT_TRUE(test_window->HasCapture());

int64_t display_0_id = primary_display.id();
const gfx::Rect indicator_in_primary_display =
GetIndicatorNativeBounds(display_0_id);
int64_t display_1_id = display::Screen::GetScreen()
->GetDisplayNearestWindow(root_windows[1])
.id();
const gfx::Rect indicator_in_secondary_display =
GetIndicatorNativeBounds(display_1_id);

gfx::Point location_in_screen_native, location_in_screen_dip;

// Move mouse cursor to the warp region of first display.
location_in_screen_native = indicator_in_primary_display.CenterPoint();
location_in_host_native =
location_in_screen_native -
root_windows[0]->GetHost()->GetBoundsInPixels().OffsetFromOrigin();
DispatchMouseEventWithNative(window_host, location_in_host_native,
ui::ET_MOUSE_DRAGGED, ui::EF_LEFT_MOUSE_BUTTON,
0);

// Mouse cursor should be warped into secondary display.
location_in_screen_dip = Shell::Get()->aura_env()->last_mouse_location();
EXPECT_TRUE(
root_windows[1]->GetBoundsInScreen().Contains(location_in_screen_dip));

// Move mouse cursor to the warp region of secondary display.
location_in_screen_native = indicator_in_secondary_display.CenterPoint();
location_in_host_native =
location_in_screen_native -
root_windows[0]->GetHost()->GetBoundsInPixels().OffsetFromOrigin();
DispatchMouseEventWithNative(window_host, location_in_host_native,
ui::ET_MOUSE_DRAGGED, ui::EF_LEFT_MOUSE_BUTTON,
0);

// Mouse cursor should be warped into first display.
location_in_screen_dip = Shell::Get()->aura_env()->last_mouse_location();
EXPECT_TRUE(
root_windows[0]->GetBoundsInScreen().Contains(location_in_screen_dip));

// After mouse warping, x-coordinate of mouse location in native coordinates
// should be 2 px away from end. Primary display has zoom factor of 0.8. So
// the offset in screen coordinates should be 2/0.8, which is 2.5. The end of
// primary display in screen coordinates is 250. So x-coordinate of mouse
// cursor in screen coordinates should be 247.
EXPECT_EQ(247, Shell::Get()->aura_env()->last_mouse_location().x());

// Get cursor's location in host native coordinates.
gfx::Point location_in_host_dip;
location_in_screen_dip = Shell::Get()->aura_env()->last_mouse_location();
location_in_host_dip = location_in_screen_dip;
::wm::ConvertPointFromScreen(root_windows[0], &location_in_host_dip);
location_in_host_native = location_in_host_dip;
root_windows[0]->GetHost()->ConvertDIPToPixels(&location_in_host_native);

// Release mouse button.
DispatchMouseEventWithNative(window_host, location_in_host_native,
ui::ET_MOUSE_RELEASED, ui::EF_LEFT_MOUSE_BUTTON,
ui::EF_LEFT_MOUSE_BUTTON);
}

} // namespace ash
8 changes: 2 additions & 6 deletions ash/display/screen_position_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,9 @@ void ScreenPositionController::ConvertHostPointToRelativeToRootWindow(
// display's coordinate system to anothers may cause events in the
// primary's coordinate system which fall in the extended display.

gfx::Point location_in_native(*point);
gfx::Point location_in_native(point_in_root);

// |point| is in the native host window coordinate. Convert it to the native
// screen coordinate.
const gfx::Point& host_origin =
root_window->GetHost()->GetBoundsInPixels().origin();
location_in_native.Offset(host_origin.x(), host_origin.y());
root_window->GetHost()->ConvertDIPToScreenInPixels(&location_in_native);

for (size_t i = 0; i < root_windows.size(); ++i) {
aura::WindowTreeHost* host = root_windows[i]->GetHost();
Expand Down
2 changes: 1 addition & 1 deletion ash/display/screen_position_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ TEST_F(ScreenPositionControllerTest, ConvertHostPointToScreenZoomScale) {
EXPECT_EQ("37,187", ConvertHostPointToScreen(60, 300));
// The point is on the 2nd host. Point on 2nd host (60,150) -
// - screen [+(150,0)]
EXPECT_EQ("185,50", ConvertHostPointToScreen(60, 450));
EXPECT_EQ("184,49", ConvertHostPointToScreen(60, 450));

// Move |window_| to the 2nd.
window_->SetBoundsInScreen(gfx::Rect(300, 20, 50, 50),
Expand Down
5 changes: 0 additions & 5 deletions ash/host/ash_window_tree_host_platform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ struct PlatformWindowInitProperties;
}

namespace ash {
class ExtendedMouseWarpControllerTest;

class ASH_EXPORT AshWindowTreeHostPlatform
: public AshWindowTreeHost,
Expand All @@ -41,10 +40,6 @@ class ASH_EXPORT AshWindowTreeHostPlatform
~AshWindowTreeHostPlatform() override;

protected:
friend ExtendedMouseWarpControllerTest;
FRIEND_TEST_ALL_PREFIXES(ExtendedMouseWarpControllerTest,
CheckHostPointToScreenInMouseWarpRegion);

AshWindowTreeHostPlatform();

// AshWindowTreeHost:
Expand Down

0 comments on commit beb3651

Please sign in to comment.