From 053ef0ef0cb9765594615de430fad25b24d100c6 Mon Sep 17 00:00:00 2001 From: msw Date: Fri, 25 Mar 2016 21:18:57 -0700 Subject: Revert of Refine ash shelf tooltip closing on non-mash ChromeOS. (patchset #4 id:60001 of https://codereview.chromium.org/1828133004/ ) Reason for revert: Memory failures, eg. https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20MSan%20Tests/builds/8340 Original issue's description: > Refine ash shelf tooltip closing on non-mash ChromeOS. > > Fixes a regression from: https://codereview.chromium.org/1816753002 > Also reverse expected behavior for touching tooltips. > (closes on stable, tests expected them to stay open...) > > Inline bubble function definitions. > Call WillDeleteShelf earlier to fix a test crash... > TODO: Encapsulate more ShelfLayoutManager code. > > BUG=595853 > TEST=ChromeOS tooltips stay open on hover; close for external touches. > R=sky@chromium.org > > Committed: https://crrev.com/1a0d283fa81e19b47fb85523f28c4a03f7dc1d24 > Cr-Commit-Position: refs/heads/master@{#383404} TBR=sky@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=595853 Review URL: https://codereview.chromium.org/1836753002 Cr-Commit-Position: refs/heads/master@{#383444} --- ash/shelf/shelf_layout_manager.cc | 1 - ash/shelf/shelf_tooltip_manager.cc | 116 ++++++++++++++-------------- ash/shelf/shelf_tooltip_manager_unittest.cc | 32 ++++---- 3 files changed, 73 insertions(+), 76 deletions(-) diff --git a/ash/shelf/shelf_layout_manager.cc b/ash/shelf/shelf_layout_manager.cc index 5d70e8d..f27899c 100644 --- a/ash/shelf/shelf_layout_manager.cc +++ b/ash/shelf/shelf_layout_manager.cc @@ -236,7 +236,6 @@ void ShelfLayoutManager::PrepareForShutdown() { // Stop observing window change, otherwise we can attempt to update a // partially destructed shelf. aura::client::GetActivationClient(root_window_)->RemoveObserver(this); - FOR_EACH_OBSERVER(ShelfLayoutManagerObserver, observers_, WillDeleteShelf()); } bool ShelfLayoutManager::IsVisible() const { diff --git a/ash/shelf/shelf_tooltip_manager.cc b/ash/shelf/shelf_tooltip_manager.cc index 0f1d1fd..5e25fb2 100644 --- a/ash/shelf/shelf_tooltip_manager.cc +++ b/ash/shelf/shelf_tooltip_manager.cc @@ -24,7 +24,6 @@ namespace ash { namespace { - const int kTooltipTopBottomMargin = 3; const int kTooltipLeftRightMargin = 10; const int kTooltipAppearanceDelay = 1000; // msec @@ -49,47 +48,54 @@ class ShelfTooltipManager::ShelfTooltipBubble public: ShelfTooltipBubble(views::View* anchor, views::BubbleBorder::Arrow arrow, - const base::string16& text) - : views::BubbleDelegateView(anchor, arrow) { - gfx::Insets insets = - gfx::Insets(kArrowOffsetTopBottom, kArrowOffsetLeftRight); - // Adjust the anchor location for asymmetrical borders of shelf item. - if (anchor->border()) - insets += anchor->border()->GetInsets(); - - set_anchor_view_insets(insets); - set_close_on_esc(false); - set_close_on_deactivate(false); - set_can_activate(false); - set_accept_events(false); - set_margins(gfx::Insets(kTooltipTopBottomMargin, kTooltipLeftRightMargin)); - set_shadow(views::BubbleBorder::SMALL_SHADOW); - SetLayoutManager(new views::FillLayout()); - // The anchor may not have the widget in tests. - if (anchor->GetWidget() && anchor->GetWidget()->GetNativeWindow()) { - set_parent_window(ash::Shell::GetContainer( - anchor->GetWidget()->GetNativeWindow()->GetRootWindow(), - ash::kShellWindowId_SettingBubbleContainer)); - } - views::Label* label = new views::Label(text); - label->SetHorizontalAlignment(gfx::ALIGN_LEFT); - label->SetEnabledColor(kTooltipTextColor); - AddChildView(label); - views::BubbleDelegateView::CreateBubble(this); - SizeToContents(); - } + const base::string16& text); private: // views::View overrides: - gfx::Size GetPreferredSize() const override { - const gfx::Size size = views::BubbleDelegateView::GetPreferredSize(); - return gfx::Size(std::min(size.width(), kTooltipMaxWidth), - std::max(size.height(), kTooltipMinHeight)); - } + gfx::Size GetPreferredSize() const override; DISALLOW_COPY_AND_ASSIGN(ShelfTooltipBubble); }; +ShelfTooltipManager::ShelfTooltipBubble::ShelfTooltipBubble( + views::View* anchor, + views::BubbleBorder::Arrow arrow, + const base::string16& text) + : views::BubbleDelegateView(anchor, arrow) { + gfx::Insets insets = gfx::Insets(kArrowOffsetTopBottom, + kArrowOffsetLeftRight); + // Adjust the anchor location for asymmetrical borders of shelf item. + if (anchor->border()) + insets += anchor->border()->GetInsets(); + + set_anchor_view_insets(insets); + set_close_on_esc(false); + set_close_on_deactivate(false); + set_can_activate(false); + set_accept_events(false); + set_margins(gfx::Insets(kTooltipTopBottomMargin, kTooltipLeftRightMargin)); + set_shadow(views::BubbleBorder::SMALL_SHADOW); + SetLayoutManager(new views::FillLayout()); + // The anchor may not have the widget in tests. + if (anchor->GetWidget() && anchor->GetWidget()->GetNativeWindow()) { + set_parent_window(ash::Shell::GetContainer( + anchor->GetWidget()->GetNativeWindow()->GetRootWindow(), + ash::kShellWindowId_SettingBubbleContainer)); + } + views::Label* label = new views::Label(text); + label->SetHorizontalAlignment(gfx::ALIGN_LEFT); + label->SetEnabledColor(kTooltipTextColor); + AddChildView(label); + views::BubbleDelegateView::CreateBubble(this); + SizeToContents(); +} + +gfx::Size ShelfTooltipManager::ShelfTooltipBubble::GetPreferredSize() const { + const gfx::Size size = views::BubbleDelegateView::GetPreferredSize(); + return gfx::Size(std::min(size.width(), kTooltipMaxWidth), + std::max(size.height(), kTooltipMinHeight)); +} + ShelfTooltipManager::ShelfTooltipManager(ShelfView* shelf_view) : timer_delay_(kTooltipAppearanceDelay), shelf_view_(shelf_view), @@ -104,10 +110,8 @@ ShelfTooltipManager::~ShelfTooltipManager() { void ShelfTooltipManager::Init() { shelf_layout_manager_ = shelf_view_->shelf()->shelf_layout_manager(); shelf_layout_manager_->AddObserver(this); - - // TODO(msw): Observe events outside the shelf in mash, to close tooltips. - aura::Window* shelf_window = shelf_view_->GetWidget()->GetNativeWindow(); - shelf_window->GetRootWindow()->AddPreTargetHandler(this); + // TODO(msw): Capture events outside the shelf to close tooltips? + shelf_view_->GetWidget()->GetNativeWindow()->AddPreTargetHandler(this); } void ShelfTooltipManager::Close() { @@ -160,29 +164,31 @@ void ShelfTooltipManager::ShowTooltipWithDelay(views::View* view) { } void ShelfTooltipManager::OnEvent(ui::Event* event) { + // Close the tooltip on mouse press or exit, and on most non-mouse events. if (event->type() == ui::ET_MOUSE_PRESSED || - event->type() == ui::ET_MOUSE_EXITED || !event->IsMouseEvent() || - event->target() != shelf_view_->GetWidget()->GetNativeWindow()) { + event->type() == ui::ET_MOUSE_EXITED || !event->IsMouseEvent()) { if (!event->IsKeyEvent()) Close(); return; } gfx::Point point = static_cast(event)->location(); - aura::Window::ConvertPointToTarget( - static_cast(event->target()), - shelf_view_->GetWidget()->GetNativeWindow(), &point); views::View::ConvertPointFromWidget(shelf_view_, &point); + if (IsVisible() && shelf_view_->ShouldHideTooltip(point)) { + Close(); + return; + } + views::View* view = shelf_view_->GetTooltipHandlerForPoint(point); const bool should_show = shelf_view_->ShouldShowTooltipForView(view); - - timer_.Stop(); - if (IsVisible() && should_show && bubble_->GetAnchorView() != view) + if (IsVisible() && bubble_->GetAnchorView() != view && should_show) ShowTooltip(view); - else if (!IsVisible() && should_show && event->type() == ui::ET_MOUSE_MOVED) - ShowTooltipWithDelay(view); - else if (IsVisible() && shelf_view_->ShouldHideTooltip(point)) - Close(); + + if (!IsVisible() && event->type() == ui::ET_MOUSE_MOVED) { + timer_.Stop(); + if (should_show) + ShowTooltipWithDelay(view); + } } void ShelfTooltipManager::WillDeleteShelf() { @@ -190,11 +196,9 @@ void ShelfTooltipManager::WillDeleteShelf() { shelf_layout_manager_->RemoveObserver(this); shelf_layout_manager_ = nullptr; - if (shelf_view_ && shelf_view_->GetWidget()) { - aura::Window* shelf_window = shelf_view_->GetWidget()->GetNativeWindow(); - if (shelf_window && shelf_window->GetRootWindow()) - shelf_window->GetRootWindow()->RemovePreTargetHandler(this); - } + views::Widget* widget = shelf_view_ ? shelf_view_->GetWidget() : nullptr; + if (widget && widget->GetNativeWindow()) + widget->GetNativeWindow()->RemovePreTargetHandler(this); shelf_view_ = nullptr; } diff --git a/ash/shelf/shelf_tooltip_manager_unittest.cc b/ash/shelf/shelf_tooltip_manager_unittest.cc index 7f6650e..3f2b2e8 100644 --- a/ash/shelf/shelf_tooltip_manager_unittest.cc +++ b/ash/shelf/shelf_tooltip_manager_unittest.cc @@ -173,41 +173,35 @@ TEST_F(ShelfTooltipManagerTest, HideForEvents) { EXPECT_FALSE(TooltipIsVisible()); } -TEST_F(ShelfTooltipManagerTest, HideForExternalEvents) { +// TODO(msw): Hiding for touch and gesture events outside the shelf is broken. +TEST_F(ShelfTooltipManagerTest, HideForEventsBroken) { ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow()); - // TODO(msw): Observe events outside the shelf in mash, to close tooltips. - aura::Window* shelf_window = shelf_->shelf_widget()->GetNativeWindow(); - bool closes = shelf_window->GetRootWindow() == Shell::GetPrimaryRootWindow(); - // Should hide for touches outside the shelf. ShowImmediately(); ASSERT_TRUE(TooltipIsVisible()); - generator.set_current_location(gfx::Point()); - generator.PressTouch(); - EXPECT_EQ(TooltipIsVisible(), !closes); - // Should hide for touch events on the tooltip. - ShowImmediately(); - ASSERT_TRUE(TooltipIsVisible()); - generator.set_current_location(GetTooltipWindow()->bounds().CenterPoint()); + generator.set_current_location(gfx::Point()); generator.PressTouch(); - EXPECT_EQ(TooltipIsVisible(), !closes); + EXPECT_TRUE(TooltipIsVisible()); - // Should hide for gestures outside the shelf. - ShowImmediately(); - ASSERT_TRUE(TooltipIsVisible()); generator.GestureTapDownAndUp(gfx::Point()); - EXPECT_EQ(TooltipIsVisible(), !closes); + EXPECT_TRUE(TooltipIsVisible()); } -TEST_F(ShelfTooltipManagerTest, DoNotHideForKeyEvents) { +TEST_F(ShelfTooltipManagerTest, DoNotHideForEvents) { ui::test::EventGenerator generator(ash::Shell::GetPrimaryRootWindow()); - // Should not hide for key events. ShowImmediately(); ASSERT_TRUE(TooltipIsVisible()); + + // Should not hide for key events. generator.PressKey(ui::VKEY_A, ui::EF_NONE); EXPECT_TRUE(TooltipIsVisible()); + + // Should not hide for touch events on the tooltip. + generator.set_current_location(GetTooltipWindow()->bounds().CenterPoint()); + generator.PressTouch(); + EXPECT_TRUE(TooltipIsVisible()); } } // namespace test -- cgit v1.1