Skip to content

Commit

Permalink
Revert "Split Tab Hover Cards into view and controller."
Browse files Browse the repository at this point in the history 
This reverts commit 3387f04.

Reason for revert: Consistent failure on linux-trusty-rel
Recent failure: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-trusty-rel/21525/overview

Original change's description:
> Split Tab Hover Cards into view and controller.
>
> Part of an effort to move hover cards to a MVC organization so that we
> can have more control over when cards are shown and what tabs they are
> shown for. Once the controller is separated, we can exert finer control
> over when the widget is created and destroyed and when thumbnails are
> requested.
>
> There are a number of outstanding performance issues that the above
> changes would facilitate, e.g. detecting quick move-across as per
> crbug.com/1177601 or just destroying the widget when we're not using it
> to save compositor memory.
>
> Note: This change should *not* affect any existing Hover Card behavior.
> If it does, that should be amended before improvements or optimizations
> are attempted.
>
> Change-Id: Icafe193337737b7736055bd3183b42f1ec140bcb
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2714213
> Commit-Queue: Dana Fried <dfried@chromium.org>
> Reviewed-by: Caroline Rising <corising@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#857928}

(cherry picked from commit 7792ad2)

Change-Id: Id75208aa644688c2b377e25f60757c0e2e98e65f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2720025
Auto-Submit: Jiewei Qian  <qjw@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Original-Commit-Position: refs/heads/master@{#857973}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2722977
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Commit-Queue: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/4430@{#5}
Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}
  • Loading branch information
@wacky6
wacky6 authored and Chromium LUCI CQ committed Feb 26, 2021
1 parent d628a2f commit d2cc8cc
Show file tree
Hide file tree
Showing 15 changed files with 627 additions and 901 deletions.
4 changes: 0 additions & 4 deletions chrome/browser/ui/BUILD.gn
View file
Expand Up @@ -4103,10 +4103,6 @@ static_library("ui") {
"views/tabs/tab_group_views.h",
"views/tabs/tab_hover_card_bubble_view.cc",
"views/tabs/tab_hover_card_bubble_view.h",
"views/tabs/tab_hover_card_controller.cc",
"views/tabs/tab_hover_card_controller.h",
"views/tabs/tab_hover_card_thumbnail_observer.cc",
"views/tabs/tab_hover_card_thumbnail_observer.h",
"views/tabs/tab_icon.cc",
"views/tabs/tab_icon.h",
"views/tabs/tab_search_button.cc",
Expand Down
27 changes: 9 additions & 18 deletions chrome/browser/ui/views/tabs/tab.cc
View file
Expand Up @@ -158,15 +158,13 @@ class Tab::TabCloseButtonObserver : public views::ViewObserver {

private:
void OnViewFocused(views::View* observed_view) override {
controller_->UpdateHoverCard(tab_,
TabController::HoverCardUpdateType::kFocus);
controller_->UpdateHoverCard(tab_);
}

void OnViewBlurred(views::View* observed_view) override {
// Only hide hover card if not keyboard navigating.
if (!controller_->IsFocusInTabs())
controller_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kFocus);
controller_->UpdateHoverCard(nullptr);
}

base::ScopedObservation<views::View, views::ViewObserver>
Expand Down Expand Up @@ -250,8 +248,7 @@ Tab::~Tab() {
// Observer must be unregistered before child views are destroyed.
tab_close_button_observer_.reset();
if (controller_->HoverCardIsShowingForTab(this))
controller_->UpdateHoverCard(
nullptr, TabController::HoverCardUpdateType::kTabRemoved);
controller_->UpdateHoverCard(nullptr);
}

void Tab::AnimationEnded(const gfx::Animation* animation) {
Expand Down Expand Up @@ -469,8 +466,7 @@ bool IsSelectionModifierDown(const ui::MouseEvent& event) {
} // namespace

bool Tab::OnMousePressed(const ui::MouseEvent& event) {
controller_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kEvent);
controller_->UpdateHoverCard(nullptr);
controller_->OnMouseEventInTab(this, event);

// Allow a right click from touch to drag, which corresponds to a long click.
Expand Down Expand Up @@ -596,8 +592,7 @@ void Tab::MaybeUpdateHoverStatus(const ui::MouseEvent& event) {
UpdateForegroundColors();
Layout();
if (g_show_hover_card_on_mouse_hover)
controller_->UpdateHoverCard(this,
TabController::HoverCardUpdateType::kHover);
controller_->UpdateHoverCard(this);
}

void Tab::OnMouseExited(const ui::MouseEvent& event) {
Expand All @@ -610,8 +605,7 @@ void Tab::OnMouseExited(const ui::MouseEvent& event) {
}

void Tab::OnGestureEvent(ui::GestureEvent* event) {
controller_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kEvent);
controller_->UpdateHoverCard(nullptr);
switch (event->type()) {
case ui::ET_GESTURE_TAP_DOWN: {
// TAP_DOWN is only dispatched for the first touch point.
Expand Down Expand Up @@ -697,14 +691,12 @@ void Tab::AddedToWidget() {

void Tab::OnFocus() {
View::OnFocus();
controller_->UpdateHoverCard(this,
TabController::HoverCardUpdateType::kFocus);
controller_->UpdateHoverCard(this);
}

void Tab::OnBlur() {
View::OnBlur();
controller_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kFocus);
controller_->UpdateHoverCard(nullptr);
}

void Tab::OnThemeChanged() {
Expand Down Expand Up @@ -792,8 +784,7 @@ void Tab::ActiveStateChanged() {

void Tab::AlertStateChanged() {
if (controller_->HoverCardIsShowingForTab(this))
controller_->UpdateHoverCard(
this, TabController::HoverCardUpdateType::kTabDataChanged);
controller_->UpdateHoverCard(this);
Layout();
}

Expand Down
17 changes: 2 additions & 15 deletions chrome/browser/ui/views/tabs/tab_controller.h
View file
Expand Up @@ -5,8 +5,6 @@
#ifndef CHROME_BROWSER_UI_VIEWS_TABS_TAB_CONTROLLER_H_
#define CHROME_BROWSER_UI_VIEWS_TABS_TAB_CONTROLLER_H_

#include "base/optional.h"
#include "base/strings/string16.h"
#include "chrome/browser/ui/tabs/tab_types.h"
#include "chrome/browser/ui/views/tabs/tab_strip_types.h"
#include "third_party/skia/include/core/SkColor.h"
Expand Down Expand Up @@ -37,16 +35,6 @@ class View;
// Controller for tabs.
class TabController {
public:
enum HoverCardUpdateType {
kHover,
kFocus,
kTabDataChanged,
kAnimating,
kTabRemoved,
kSelectionChanged,
kEvent
};

virtual const ui::ListSelectionModel& GetSelectionModel() const = 0;

// Returns true if multiple selection is supported.
Expand Down Expand Up @@ -134,9 +122,8 @@ class TabController {

// Updates hover-card content, anchoring and visibility based on what tab is
// hovered and whether the card should be shown. Providing a nullptr for |tab|
// will cause the tab hover card to be hidden. |update_type| is used to decide
// how the show, hide, or update will be processed.
virtual void UpdateHoverCard(Tab* tab, HoverCardUpdateType update_type) = 0;
// will cause the tab hover card to be hidden.
virtual void UpdateHoverCard(Tab* tab) = 0;

// Returns whether domain/origin should be shown in tab hover cards.
virtual bool ShowDomainInHoverCards() const = 0;
Expand Down
9 changes: 3 additions & 6 deletions chrome/browser/ui/views/tabs/tab_group_header.cc
View file
Expand Up @@ -194,8 +194,7 @@ void TabGroupHeader::OnMouseReleased(const ui::MouseEvent& event) {
void TabGroupHeader::OnMouseEntered(const ui::MouseEvent& event) {
// Hide the hover card, since there currently isn't anything to display
// for a group.
tab_strip_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kHover);
tab_strip_->UpdateHoverCard(nullptr);
}

void TabGroupHeader::OnThemeChanged() {
Expand All @@ -204,8 +203,7 @@ void TabGroupHeader::OnThemeChanged() {
}

void TabGroupHeader::OnGestureEvent(ui::GestureEvent* event) {
tab_strip_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kEvent);
tab_strip_->UpdateHoverCard(nullptr);
switch (event->type()) {
case ui::ET_GESTURE_TAP: {
bool successful_toggle =
Expand Down Expand Up @@ -234,8 +232,7 @@ void TabGroupHeader::OnGestureEvent(ui::GestureEvent* event) {

void TabGroupHeader::OnFocus() {
View::OnFocus();
tab_strip_->UpdateHoverCard(nullptr,
TabController::HoverCardUpdateType::kFocus);
tab_strip_->UpdateHoverCard(nullptr);
}

void TabGroupHeader::GetAccessibleNodeData(ui::AXNodeData* node_data) {
Expand Down

0 comments on commit d2cc8cc

Please sign in to comment.