Discharge7443/brave-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Rework tab dragging overrides and patches

Browse Source 
Due to the introduction of DraggingTabsSession which now handles many of the
tab-related dragging responsibilities, we need to modify our patches and
overrides to work within this new framework.

Chromium change:
https://chromium.googlesource.com/chromium/src/+/b39ab7bc4ae0db831a930d373264e5edf8205fdc

commit b39ab7bc4ae0db831a930d373264e5edf8205fdc
Author: Taylor Bergquist <tbergquist@chromium.org>
Date:   Mon Mar 24 16:22:37 2025 -0700

    Create DraggingTabsSession to handle dragging tabs within a tabstrip.

    DraggingTabsSession handles the work of the kDraggingTabs state for
    TabDragController. This functionally boils down to implementing
    MoveAttached. TabDragController creates a new DraggingTabsHelper each
    time it enters kDraggingTabs (or, in one case, when it enters
    kWaitingToDragTabs instead), and destroys it when exiting that state.

    This removes quite a few responsibilities from TabDragController - most
    of the detailed business logic bits are in this domain, even though it's
    a minority of the code size. It's also more testable, since it doesn't
    have to deal with creating windows or running move loops. A compact
    interface with minimal dependencies, hiding lots of requirements? It's
    unit test heaven. Followup CLs will set up a test harness and convert
    TabDragControllerInteractiveUITests into DraggingTabsSessionUnitTests.

    Bug: 382754501
This commit is contained in:
Emerick Rogul
2025-04-16 09:30:32 +01:00
committed by Claudio DeSouza
parent 9abf436484
commit dc98e277d0
13 changed files with 237 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files
browser/ui/BUILD.gn
+2
View File
@@ -613,6 +613,8 @@ source_set("ui") {
"views/tabs/brave_tab_strip.h",
"views/tabs/brave_tab_strip_layout_helper.cc",
"views/tabs/brave_tab_strip_layout_helper.h",
"views/tabs/dragging/dragging_tabs_session.cc",
"views/tabs/dragging/dragging_tabs_session.h",
"views/tabs/dragging/tab_drag_controller.cc",
"views/tabs/dragging/tab_drag_controller.h",
"views/tabs/shared_pinned_tab_dummy_view_views.cc",
browser/ui/tabs/test/shared_pinned_tab_service_browsertest.cc
+39 -39
View File
@@ -250,45 +250,6 @@ IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, NewBrowser) {
tab_strip_model_2->GetWebContentsAt(0)));
}
IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, BringAllTabs) {
// Given that there're multiple windows with shared pinned tabs
auto* browser_1 = browser();
auto* tab_strip_model_1 = browser_1->tab_strip_model();
tab_strip_model_1->SetTabPinned(0, /* pinned= */ true);
auto* shared_pinned_tab_service = GetForBrowser(browser_1);
ASSERT_TRUE(shared_pinned_tab_service);
ASSERT_TRUE(shared_pinned_tab_service->IsSharedContents(
tab_strip_model_1->GetWebContentsAt(0)));
auto* browser_2 = CreateNewBrowser();
auto* tab_strip_model_2 = browser_2->tab_strip_model();
WaitUntil(base::BindLambdaForTesting(
[&]() { return tab_strip_model_2->count() > 1; }));
ASSERT_TRUE(tab_strip_model_2->IsTabPinned(0));
browser_2->ActivateContents(tab_strip_model_2->GetWebContentsAt(0));
browser_2->window()->Show();
WaitUntil(base::BindLambdaForTesting([&]() {
return shared_pinned_tab_service->IsSharedContents(
tab_strip_model_2->GetWebContentsAt(0));
}));
ASSERT_TRUE(shared_pinned_tab_service->IsDummyContents(
tab_strip_model_1->GetWebContentsAt(0)));
// When running "Bring all tabs to this window".
brave::BringAllTabs(browser_1);
// Then only the target browser should be left with shared contents.
auto* browser_list = BrowserList::GetInstance();
WaitUntil(
base::BindLambdaForTesting([&]() { return browser_list->size() == 1u; }));
EXPECT_EQ(browser_1, *browser_list->begin());
browser_1->window()->Show();
WaitUntil(base::BindLambdaForTesting([&]() {
return shared_pinned_tab_service->IsSharedContents(
tab_strip_model_1->GetWebContentsAt(0));
}));
}
IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, SynchronizeURL) {
// Given that there're multiple windows with shared pinned tabs
auto* browser_1 = browser();
@@ -396,6 +357,45 @@ IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, PreferenceChanged) {
}
#if !BUILDFLAG(IS_MAC)
IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, BringAllTabs) {
// Given that there're multiple windows with shared pinned tabs
auto* browser_1 = browser();
auto* tab_strip_model_1 = browser_1->tab_strip_model();
tab_strip_model_1->SetTabPinned(0, /* pinned= */ true);
auto* shared_pinned_tab_service = GetForBrowser(browser_1);
ASSERT_TRUE(shared_pinned_tab_service);
ASSERT_TRUE(shared_pinned_tab_service->IsSharedContents(
tab_strip_model_1->GetWebContentsAt(0)));
auto* browser_2 = CreateNewBrowser();
auto* tab_strip_model_2 = browser_2->tab_strip_model();
WaitUntil(base::BindLambdaForTesting(
[&]() { return tab_strip_model_2->count() > 1; }));
ASSERT_TRUE(tab_strip_model_2->IsTabPinned(0));
browser_2->ActivateContents(tab_strip_model_2->GetWebContentsAt(0));
browser_2->window()->Show();
WaitUntil(base::BindLambdaForTesting([&]() {
return shared_pinned_tab_service->IsSharedContents(
tab_strip_model_2->GetWebContentsAt(0));
}));
ASSERT_TRUE(shared_pinned_tab_service->IsDummyContents(
tab_strip_model_1->GetWebContentsAt(0)));
// When running "Bring all tabs to this window".
brave::BringAllTabs(browser_1);
// Then only the target browser should be left with shared contents.
auto* browser_list = BrowserList::GetInstance();
WaitUntil(
base::BindLambdaForTesting([&]() { return browser_list->size() == 1u; }));
EXPECT_EQ(browser_1, *browser_list->begin());
browser_1->window()->Show();
WaitUntil(base::BindLambdaForTesting([&]() {
return shared_pinned_tab_service->IsSharedContents(
tab_strip_model_1->GetWebContentsAt(0));
}));
}
IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest,
CloseTabShortCutShouldBeDisabled) {
auto* browser = CreateNewBrowser();
browser/ui/views/tabs/dragging/dragging_tabs_session.cc
+51
View File
@@ -0,0 +1,51 @@
/* Copyright (c) 2025 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */
#include "brave/browser/ui/views/tabs/dragging/dragging_tabs_session.h"
#include "chrome/browser/ui/views/tabs/dragging/drag_session_data.h"
#include "chrome/browser/ui/views/tabs/dragging/tab_drag_context.h"
#include "ui/gfx/geometry/point.h"
DraggingTabsSession::DraggingTabsSession(DragSessionData drag_data,
TabDragContext* attached_context,
int mouse_offset,
bool initial_move,
gfx::Point start_point_in_screen)
: DraggingTabsSessionChromium(drag_data,
attached_context,
mouse_offset,
initial_move,
start_point_in_screen) {}
DraggingTabsSession::~DraggingTabsSession() {}
gfx::Point DraggingTabsSession::GetAttachedDragPoint(
gfx::Point point_in_screen) {
if (!is_showing_vertical_tabs_) {
return DraggingTabsSessionChromium::GetAttachedDragPoint(point_in_screen);
}
DCHECK(attached_context_); // The tab must be attached.
gfx::Point tab_loc(point_in_screen);
views::View::ConvertPointFromScreen(attached_context_, &tab_loc);
const int x = drag_data_.tab_drag_data_.front().pinned
? tab_loc.x() - mouse_offset_
: 0;
const int y = tab_loc.y() - mouse_y_offset_;
return {x, y};
}
void DraggingTabsSession::MoveAttached(gfx::Point point_in_screen) {
DraggingTabsSessionChromium::MoveAttached(point_in_screen);
if (!is_showing_vertical_tabs_) {
return;
}
// Unlike upstream, We always update coordinate, as we use y coordinate. Since
// we don't have threshold there's no any harm for this.
views::View::ConvertPointFromScreen(attached_context_, &point_in_screen);
last_move_attached_context_loc_ = point_in_screen.y();
}
browser/ui/views/tabs/dragging/dragging_tabs_session.h
+34
View File
@@ -0,0 +1,34 @@
/* Copyright (c) 2025 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */
#ifndef BRAVE_BROWSER_UI_VIEWS_TABS_DRAGGING_DRAGGING_TABS_SESSION_H_
#define BRAVE_BROWSER_UI_VIEWS_TABS_DRAGGING_DRAGGING_TABS_SESSION_H_
#include "chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h"
class DraggingTabsSession : public DraggingTabsSessionChromium {
public:
explicit DraggingTabsSession(DragSessionData drag_data,
TabDragContext* attached_context,
int mouse_offset,
bool initial_move,
gfx::Point point_in_screen);
~DraggingTabsSession() override;
void set_mouse_y_offset(int offset) { mouse_y_offset_ = offset; }
void set_is_showing_vertical_tabs(bool show) {
is_showing_vertical_tabs_ = show;
}
// DraggingTabSessionChromium:
gfx::Point GetAttachedDragPoint(gfx::Point point_in_screen) override;
void MoveAttached(gfx::Point point_in_screen) override;
private:
int mouse_y_offset_ = 0;
bool is_showing_vertical_tabs_ = false;
};
#endif // BRAVE_BROWSER_UI_VIEWS_TABS_DRAGGING_DRAGGING_TABS_SESSION_H_
browser/ui/views/tabs/dragging/tab_drag_controller.cc
+13 -31
View File
@@ -69,6 +69,8 @@ TabDragController::Liveness TabDragController::Init(
return TabDragController::Liveness::DELETED;
}
mouse_offset_ = mouse_offset;
auto* widget = source_view->GetWidget();
DCHECK(widget);
const auto* browser =
@@ -106,23 +108,6 @@ TabDragController::Liveness TabDragController::Init(
return TabDragController::Liveness::ALIVE;
}
gfx::Point TabDragController::GetAttachedDragPoint(
const gfx::Point& point_in_screen) {
if (!is_showing_vertical_tabs_) {
return TabDragControllerChromium::GetAttachedDragPoint(point_in_screen);
}
DCHECK(attached_context_); // The tab must be attached.
gfx::Point tab_loc(point_in_screen);
views::View::ConvertPointFromScreen(attached_context_, &tab_loc);
const int x = drag_data_.tab_drag_data_.front().pinned
? tab_loc.x() - mouse_offset_.x()
: 0;
const int y = tab_loc.y() - mouse_offset_.y();
return {x, y};
}
gfx::Vector2d TabDragController::CalculateWindowDragOffset() {
gfx::Vector2d offset = TabDragControllerChromium::CalculateWindowDragOffset();
if (!is_showing_vertical_tabs_) {
@@ -140,17 +125,15 @@ gfx::Vector2d TabDragController::CalculateWindowDragOffset() {
return new_offset.OffsetFromOrigin();
}
void TabDragController::MoveAttached(gfx::Point point_in_screen,
bool just_attached) {
TabDragControllerChromium::MoveAttached(point_in_screen, just_attached);
if (!is_showing_vertical_tabs_) {
return;
}
// Unlike upstream, We always update coordinate, as we use y coordinate. Since
// we don't have threshold there's no any harm for this.
views::View::ConvertPointFromScreen(attached_context_, &point_in_screen);
last_move_attached_context_loc_ = point_in_screen.y();
void TabDragController::StartDraggingTabsSession(
bool initial_move,
gfx::Point start_point_in_screen) {
TabDragControllerChromium::StartDraggingTabsSession(initial_move,
start_point_in_screen);
CHECK(dragging_tabs_session_);
dragging_tabs_session_->set_mouse_y_offset(mouse_offset_.y());
dragging_tabs_session_->set_is_showing_vertical_tabs(
is_showing_vertical_tabs_);
}
views::Widget* TabDragController::GetAttachedBrowserWidget() {
@@ -264,15 +247,14 @@ void TabDragController::DetachAndAttachToNewContext(
// Relayout tabs with expanded bounds.
attached_context_->ForceLayout();
std::vector<raw_ptr<TabSlotView, VectorExperimental>> views(
drag_data_.tab_drag_data_.size());
std::vector<TabSlotView*> views(drag_data_.tab_drag_data_.size());
for (size_t i = 0; i < drag_data_.tab_drag_data_.size(); ++i) {
views[i] = drag_data_.tab_drag_data_[i].attached_view.get();
}
attached_context_->LayoutDraggedViewsAt(
std::move(views), drag_data_.source_view_drag_data()->attached_view,
GetCursorScreenPoint(), initial_move_);
GetCursorScreenPoint(), false);
if (old_split_view_browser_data) {
auto* new_browser = BrowserView::GetBrowserViewForNativeWindow(
browser/ui/views/tabs/dragging/tab_drag_controller.h
+3 -3
View File
@@ -29,11 +29,10 @@ class TabDragController : public TabDragControllerChromium {
ui::mojom::DragEventSource event_source);
// TabDragControllerChromium:
gfx::Point GetAttachedDragPoint(const gfx::Point& point_in_screen) override;
void MoveAttached(gfx::Point point_in_screen, bool just_attached) override;
views::Widget* GetAttachedBrowserWidget() override;
gfx::Vector2d CalculateWindowDragOffset() override;
void StartDraggingTabsSession(bool initial_move,
gfx::Point start_point_in_screen) override;
Liveness GetLocalProcessWindow(const gfx::Point& screen_point,
bool exclude_dragged_view,
gfx::NativeWindow* window) override;
@@ -47,6 +46,7 @@ class TabDragController : public TabDragControllerChromium {
private:
gfx::Vector2d GetVerticalTabStripWidgetOffset();
gfx::Point mouse_offset_;
bool is_showing_vertical_tabs_ = false;
VerticalTabStripRegionView::ScopedStateResetter vertical_tab_state_resetter_;
chromium_src/chrome/browser/ui/tabs/tab_strip_model.cc
+2 -4
View File
@@ -3,10 +3,8 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/views/tabs/dragging/tab_drag_controller.h"
#define TabDragController TabDragControllerChromium
#define DraggingTabsSession DraggingTabsSessionChromium
#include "src/chrome/browser/ui/tabs/tab_strip_model.cc" // IWYU pragma: export
#undef TabDragController
#undef DraggingTabsSession
chromium_src/chrome/browser/ui/tabs/tab_strip_model.h
+3 -2
View File
@@ -10,12 +10,13 @@
virtual SelectRelativeTab(__VA_ARGS__); \
friend class BraveTabStripModel
#define DraggingTabsSession DraggingTabsSessionChromium
#define IsReadLaterSupportedForAny virtual IsReadLaterSupportedForAny
#define TabDragController TabDragControllerChromium
#include "src/chrome/browser/ui/tabs/tab_strip_model.h" // IWYU pragma: export
#undef IsReadLaterSupportedForAny
#undef DraggingTabsSession
#undef SelectRelativeTab
#undef TabDragController
#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_TABS_TAB_STRIP_MODEL_H_
chromium_src/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.cc
+29
View File
@@ -0,0 +1,29 @@
/* Copyright (c) 2025 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */
#include "chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h"
#include "brave/browser/ui/views/tabs/vertical_tab_utils.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#define DraggingTabsSession DraggingTabsSessionChromium
// Remove drag threshold when it's vertical tab strip
#define GetHorizontalDragThreshold() \
GetHorizontalDragThreshold() - \
(tabs::utils::ShouldShowVerticalTabs( \
BrowserView::GetBrowserViewForNativeWindow( \
attached_context_->GetWidget() \
->GetTopLevelWidget() \
->GetNativeWindow()) \
->browser()) \
? attached_context_->GetHorizontalDragThreshold() \
: 0)
#include "src/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.cc"
#undef GetHorizontalDragThreshold
#undef DraggingTabsSession
chromium_src/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h
+32
View File
@@ -0,0 +1,32 @@
/* Copyright (c) 2025 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */
#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_TABS_DRAGGING_DRAGGING_TABS_SESSION_H_
#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_TABS_DRAGGING_DRAGGING_TABS_SESSION_H_
// In order to replace DraggingTabsSession with ours easily, rename upstream's
// implementation. Ours is in
// brave/browser/ui/views/tabs/dragging_tabs_session.h and the file will be
// included at the end of this file.
class DraggingTabsSession;
using DraggingTabsSessionBrave = DraggingTabsSession;
#define DraggingTabsSession DraggingTabsSessionChromium
#define GetAttachedDragPoint \
Unused_GetAttachedDragPoint() { \
return {}; \
} \
friend DraggingTabsSessionBrave; \
virtual gfx::Point GetAttachedDragPoint
#include "src/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h" // IWYU pragma: export
#undef GetAttachedDragPoint
#undef DraggingTabsSession
#include "brave/browser/ui/views/tabs/dragging/dragging_tabs_session.h"
#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_UI_VIEWS_TABS_DRAGGING_DRAGGING_TABS_SESSION_H_
chromium_src/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.cc
-17
View File
@@ -5,12 +5,6 @@
#include "chrome/browser/ui/views/tabs/dragging/tab_drag_controller.h"
#include "brave/browser/ui/views/frame/brave_browser_view.h"
#include "brave/browser/ui/views/frame/vertical_tab_strip_widget_delegate_view.h"
#include "brave/browser/ui/views/tabs/vertical_tab_utils.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "ui/views/view.h"
#include "ui/views/widget/root_view.h"
#include "ui/views/widget/widget.h"
// Prevent unrelated StackAtTop re-define.
@@ -20,16 +14,6 @@
#define TabDragController TabDragControllerChromium
// Remove drag threshold when it's vertical tab strip
#define GetHorizontalDragThreshold() \
GetHorizontalDragThreshold() - \
(tabs::utils::ShouldShowVerticalTabs( \
BrowserView::GetBrowserViewForNativeWindow( \
GetAttachedBrowserWidget()->GetNativeWindow()) \
->browser()) \
? attached_context_->GetHorizontalDragThreshold() \
: 0)
// StackAtTop() is called to bring browser window to the front.
// It's called for TabDragContext()->GetWidget(). In horizontal tab,
// returned widget is browsr window's widget. But it's vertical tab widget in
@@ -43,6 +27,5 @@
#undef GetWindowBoundsInScreen
#undef StackAtTop
#undef GetHorizontalDragThreshold
#undef GetBrowserViewForNativeWindow
#undef TabDragController
chromium_src/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.h
+7 -7
View File
@@ -14,12 +14,10 @@ using TabDragControllerBrave = TabDragController;
#define TabDragController TabDragControllerChromium
#define GetAttachedDragPoint \
Unused_GetAttachedDragPoint() { \
return {}; \
} \
friend TabDragControllerBrave; \
virtual gfx::Point GetAttachedDragPoint
#define CompleteDrag \
CompleteDrag_Unused(); \
friend TabDragControllerBrave; \
void CompleteDrag
#define GetAttachedBrowserWidget \
GetAttachedBrowserWidget_Unused() { \
@@ -36,16 +34,18 @@ using TabDragControllerBrave = TabDragController;
#define GetLocalProcessWindow virtual GetLocalProcessWindow
#define DetachAndAttachToNewContext virtual DetachAndAttachToNewContext
#define ContinueDragging virtual ContinueDragging
#define StartDraggingTabsSession virtual StartDraggingTabsSession
#include "src/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.h" // IWYU pragma: export
#undef StartDraggingTabsSession
#undef ContinueDragging
#undef DetachAndAttachToNewContext
#undef GetLocalProcessWindow
#undef CalculateWindowDragOffset
#undef GetAttachedBrowserWidget
#undef TabDragController
#undef GetAttachedDragPoint
#undef CompleteDrag
#include "brave/browser/ui/views/tabs/dragging/tab_drag_controller.h"
patches/chrome-browser-ui-views-tabs-dragging-dragging_tabs_session.h.patch
+22
View File
@@ -0,0 +1,22 @@
diff --git a/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h b/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h
index 6de26745d5489ce89aa00fba9d19e0330db0ed3c..2116cd7fbe9c5898e2575e634c1fc1a834c1aab5 100644
--- a/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h
+++ b/chrome/browser/ui/views/tabs/dragging/dragging_tabs_session.h
@@ -14,7 +14,7 @@
// Handles dragging tabs within a single TabDragContext on behalf of
// TabDragController.
-class DraggingTabsSession final : public TabDragWithScrollManager {
+class DraggingTabsSession : public TabDragWithScrollManager {
public:
// `drag_data` is a copy of the drag configuration for the full session.
// `attached_context` is the context in which the tabs are being dragged.
@@ -27,7 +27,7 @@ class DraggingTabsSession final : public TabDragWithScrollManager {
int mouse_offset,
bool initial_move,
gfx::Point point_in_screen);
- ~DraggingTabsSession() final;
+ ~DraggingTabsSession() override;
// TabDragWithScrollManager:
void MoveAttached(gfx::Point point_in_screen) override;