Skip to content

Commit

Permalink
Change the aura::WindowObserver hook used in exo::ExtendedDragSource
Browse files Browse the repository at this point in the history 
This CL changes the aura::WindowObserver hook used in
exo::ExtendedDragSource:

Previously, ExtendedDragSource::OnWindowVisibilityChanging() was
used to call ash::ToplevelWindowEventHandler::AttemptToStartDrag(),
and kick off the drag 'n drop.
Problem is that for the scenario of TabletMode ON and WebUITabStrip
OFF, using OnWindowVisibilityChanging() hook is too early.
Reason:

In today's ToT, when a window detaches, during a tab drag 'n drop,
the following check takes place and fails, causing the whole
operation to abort (see lines 309-310 below):

 295 std::unique_ptr<WindowResizer> CreateWindowResizerForTabletMode(
 296     aura::Window* window, ...) {
 (...)
 306   WindowState* window_state = WindowState::Get(window);
 307   // Only maximized/fullscreen/snapped window can be dragged from
 308   // the top of the screen.
 309   if (!window_state->IsMaximized() && !window_state->IsFullscreen()
 310       && !window_state->IsSnapped()) {
 311     return nullptr;

This happens because the |window_state| in case is only set to
MAXIMIZED from a code that uses OnWindowVisibilityChanged() hook:

(...)
#12 0x7fe849164dda TabletModeWindowManager::OnWindowVisibilityChanged()
(...)
#15 0x7fe8520d2be3 aura::Window::NotifyWindowVisibilityChanged()
#16 0x7fe8520cdf2c aura::Window::SetVisibleInternal()
#17 0x7fe8520cdc54 aura::Window::Show()
#18 0x7fe84cfcf013 views::NativeWidgetAura::Show()
#19 0x7fe84cf89398 views::Widget::Show()
#20 0x561eee1cb154 exo::ShellSurfaceBase::CommitWidget()
#21 0x561eee1cacf6 exo::ShellSurfaceBase::OnSurfaceCommit()

Hence, using OnDraggedWindowVisibilityChanging() in
exo::ExtendedDragSource class is too early.

This CL changes the hook used to OnWindowVisibilityChanged(),
and also sets the following aura::Window properties, used by the
drag 'n drop logic: ash::kTabDraggingSourceWindowKey and
ash::kIsDraggingTabsKey.

With this change, the functionality starts to take shape, and it is
possible to drag the window around. Snapping and merge-back are still
not fully functional (see follow up CLs).

R=oshima@chromium.org
BUG=1252941

Change-Id: I8600652a974210e3081e97d6d65639582ad222ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3254858
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/main@{#939226}
  • Loading branch information
@tonikitoo
tonikitoo authored and Chromium LUCI CQ committed Nov 8, 2021
1 parent 171683b commit e8eb0ab
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 10 deletions.
37 changes: 28 additions & 9 deletions components/exo/extended_drag_source.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <memory>
#include <string>

#include "ash/public/cpp/window_properties.h"
#include "ash/shell.h"
#include "ash/wm/toplevel_window_event_handler.h"
#include "base/check.h"
Expand Down Expand Up @@ -41,8 +42,8 @@ ExtendedDragSource* ExtendedDragSource::instance_ = nullptr;
// Internal representation of a toplevel window, backed by an Exo shell surface,
// which is being dragged. It supports both already mapped/visible windows as
// well as newly created ones (i.e: not added to a root window yet), in which
// case OnDraggedWindowVisibilityChanging callback is called to notify when it
// is about to get visible.
// case OnDraggedWindowVisibilityChanged callback is called to notify when it
// has just got visible.
class ExtendedDragSource::DraggedWindowHolder : public aura::WindowObserver {
public:
DraggedWindowHolder(Surface* surface,
Expand Down Expand Up @@ -81,10 +82,10 @@ class ExtendedDragSource::DraggedWindowHolder : public aura::WindowObserver {
surface_->window()->RemoveObserver(this);
}

void OnWindowVisibilityChanging(aura::Window* window, bool visible) override {
void OnWindowVisibilityChanged(aura::Window* window, bool visible) override {
DCHECK(window);
if (window == toplevel_window_)
source_->OnDraggedWindowVisibilityChanging(visible);
source_->OnDraggedWindowVisibilityChanged(visible);
}

bool FindToplevelWindow() {
Expand Down Expand Up @@ -250,15 +251,26 @@ void ExtendedDragSource::StartDrag(aura::Window* toplevel,
auto move_source = drag_event_source_ == ui::mojom::DragEventSource::kTouch
? ::wm::WINDOW_MOVE_SOURCE_TOUCH
: ::wm::WINDOW_MOVE_SOURCE_MOUSE;

auto end_closure = base::BindOnce(
[](aura::Window* toplevel,
ash::ToplevelWindowEventHandler::DragResult result) {
if (toplevel) {
toplevel->ClearProperty(ash::kIsDraggingTabsKey);
toplevel->ClearProperty(ash::kTabDraggingSourceWindowKey);
}
},
base::Unretained(toplevel));

// TODO(crbug.com/1167581): Experiment setting |update_gesture_target| back
// to true when capture is removed from drag and drop.
toplevel_handler->AttemptToStartDrag(
toplevel, pointer_location, HTCAPTION, move_source,
ash::ToplevelWindowEventHandler::EndClosure(),
/*update_gesture_target=*/false, /*grab_capture=*/false);
toplevel_handler->AttemptToStartDrag(toplevel, pointer_location, HTCAPTION,
move_source, std::move(end_closure),
/*update_gesture_target=*/false,
/*grab_capture=*/false);
}

void ExtendedDragSource::OnDraggedWindowVisibilityChanging(bool visible) {
void ExtendedDragSource::OnDraggedWindowVisibilityChanged(bool visible) {
DCHECK(dragged_window_holder_);
DVLOG(1) << "Dragged window visibility changed. visible=" << visible;

Expand All @@ -270,6 +282,13 @@ void ExtendedDragSource::OnDraggedWindowVisibilityChanging(bool visible) {
aura::Window* toplevel = dragged_window_holder_->toplevel_window();
DCHECK(toplevel);

DCHECK(drag_source_window_);
toplevel->SetProperty(ash::kIsDraggingTabsKey, true);
if (drag_source_window_ != toplevel) {
toplevel->SetProperty(ash::kTabDraggingSourceWindowKey,
drag_source_window_);
}

// The |toplevel| window for the dragged surface has just been created and
// it's about to be mapped. Calculate and set its position based on
// |drag_offset_| and |pointer_location_| before starting the actual drag.
Expand Down
2 changes: 1 addition & 1 deletion components/exo/extended_drag_source.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class ExtendedDragSource : public DataSourceObserver,
void UnlockCursor();
void StartDrag(aura::Window* toplevel,
const gfx::PointF& pointer_location_in_screen);
void OnDraggedWindowVisibilityChanging(bool visible);
void OnDraggedWindowVisibilityChanged(bool visible);
gfx::Point CalculateOrigin(aura::Window* target) const;
void Cleanup();

Expand Down
59 changes: 59 additions & 0 deletions components/exo/extended_drag_source_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
#include "ash/drag_drop/drag_drop_controller.h"
#include "ash/drag_drop/toplevel_window_drag_delegate.h"
#include "ash/shell.h"
#include "ash/wm/toplevel_window_event_handler.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/gmock_callback_support.h"
#include "components/exo/buffer.h"
#include "components/exo/data_source.h"
#include "components/exo/data_source_delegate.h"
Expand All @@ -20,6 +22,7 @@
#include "components/exo/test/exo_test_base.h"
#include "components/exo/test/exo_test_data_exchange_delegate.h"
#include "components/exo/test/exo_test_helper.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/aura/client/drag_drop_client.h"
#include "ui/aura/client/drag_drop_delegate.h"
Expand All @@ -34,6 +37,9 @@
#include "ui/gfx/geometry/size.h"
#include "ui/gfx/geometry/vector2d.h"

using ::testing::_;
using ::testing::InvokeWithoutArgs;

namespace exo {
namespace {

Expand Down Expand Up @@ -223,6 +229,41 @@ TEST_F(ExtendedDragSourceTest, DragSurfaceAlreadyMapped) {
EXPECT_EQ(gfx::Point(200, 200), window->GetBoundsInScreen().origin());
}

// This class installs an observer to the window being dragged.
// The goal is to ensure the drag 'n drop only effectively starts
// off of the aura::WindowObserver::OnWindowVisibilityChanged() hook,
// when it is guarantee the its state is properly set.
class WindowObserverHookChecker : public aura::WindowObserver {
public:
WindowObserverHookChecker(aura::Window* surface_window)
: surface_window_(surface_window) {
DCHECK(!surface_window_->GetRootWindow());
surface_window_->AddObserver(this);
}
~WindowObserverHookChecker() {
DCHECK(dragged_window_);
dragged_window_->RemoveObserver(this);
}

void OnWindowAddedToRootWindow(aura::Window* window) override {
dragged_window_ = surface_window_->GetToplevelWindow();
dragged_window_->AddObserver(this);
surface_window_->RemoveObserver(this);
}
MOCK_METHOD(void,
OnWindowVisibilityChanging,
(aura::Window*, bool),
(override));
MOCK_METHOD(void,
OnWindowVisibilityChanged,
(aura::Window*, bool),
(override));

private:
aura::Window* surface_window_ = nullptr;
aura::Window* dragged_window_ = nullptr;
};

TEST_F(ExtendedDragSourceTest, DragSurfaceNotMappedYet) {
// Create and Map the drag origin surface
auto surface = std::make_unique<Surface>();
Expand Down Expand Up @@ -252,6 +293,24 @@ TEST_F(ExtendedDragSourceTest, DragSurfaceNotMappedYet) {
EXPECT_EQ(gfx::Vector2d(10, 10),
*extended_drag_source_->GetDragOffsetForTesting());

// Ensure drag 'n drop starts after
// ExtendedDragSource::OnDraggedWindowVisibilityChanged()
WindowObserverHookChecker checker(detached_surface->window());
EXPECT_CALL(checker, OnWindowVisibilityChanging(_, _))
.Times(1)
.WillOnce(InvokeWithoutArgs([]() {
auto* toplevel_handler =
ash::Shell::Get()->toplevel_window_event_handler();
EXPECT_FALSE(toplevel_handler->is_drag_in_progress());
}));
EXPECT_CALL(checker, OnWindowVisibilityChanged(_, _))
.Times(1)
.WillOnce(InvokeWithoutArgs([]() {
auto* toplevel_handler =
ash::Shell::Get()->toplevel_window_event_handler();
EXPECT_TRUE(toplevel_handler->is_drag_in_progress());
}));

// Map the |detached_surface|.
auto detached_buffer = CreateBuffer({50, 50});
detached_surface->Attach(detached_buffer.get());
Expand Down

0 comments on commit e8eb0ab

Please sign in to comment.