Skip to content

Commit

Permalink
Remove unnecessary spin in ToolTipController
Browse files Browse the repository at this point in the history 
The motivation for the change is to reduce idle wakeups and therefore
reduce idle power.

ToolTipController has a repeating 500ms timer that tries to update
itself periodically, while most of the time there is no change at all.

This CL removes this spin but keeps the same functionality by
1) upgrading OnMouseEvent() in TooltipController;
2) overriding OnWindowPropertyChanged() in TooltipController;
3) overriding OnCursorVisibilityChanged() in TooltipController.

After the change, tooltip is updated on demand via observer pattern,
rather than unnecessary periodic checking.

BUG=668198

Review-Url: https://codereview.chromium.org/2615993002
Cr-Commit-Position: refs/heads/master@{#443584}
  • Loading branch information
chengx authored and Commit bot committed Jan 13, 2017
1 parent 1e4883b commit 6b8312a
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 123 deletions.
17 changes: 8 additions & 9 deletions ash/tooltips/tooltip_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,20 +109,21 @@ TEST_F(TooltipControllerTest, HideTooltipWhenCursorHidden) {
view->bounds().CenterPoint());
base::string16 expected_tooltip = base::ASCIIToUTF16("Tooltip Text");

// Fire tooltip timer so tooltip becomes visible.
helper_->FireTooltipTimer();
// Mouse event triggers tooltip update so it becomes visible.
EXPECT_TRUE(helper_->IsTooltipVisible());

// Hide the cursor and check again.
// Disable mouse event which hides the cursor and check again.
ash::Shell::GetInstance()->cursor_manager()->DisableMouseEvents();
helper_->FireTooltipTimer();
RunAllPendingInMessageLoop();
EXPECT_FALSE(ash::Shell::GetInstance()->cursor_manager()->IsCursorVisible());
helper_->UpdateIfRequired();
EXPECT_FALSE(helper_->IsTooltipVisible());

// Show the cursor and re-check.
RunAllPendingInMessageLoop();
// Enable mouse event which shows the cursor and re-check.
ash::Shell::GetInstance()->cursor_manager()->EnableMouseEvents();
RunAllPendingInMessageLoop();
helper_->FireTooltipTimer();
EXPECT_TRUE(ash::Shell::GetInstance()->cursor_manager()->IsCursorVisible());
helper_->UpdateIfRequired();
EXPECT_TRUE(helper_->IsTooltipVisible());
}

Expand Down Expand Up @@ -150,7 +151,6 @@ TEST_F(TooltipControllerTest, TooltipsOnMultiDisplayShouldNotCrash) {
ui::test::EventGenerator generator(root_windows[1]);
generator.MoveMouseRelativeTo(widget2->GetNativeView(),
view2->bounds().CenterPoint());
helper_->FireTooltipTimer();
EXPECT_TRUE(helper_->IsTooltipVisible());

// Get rid of secondary display. This destroy's the tooltip's aura window. If
Expand All @@ -164,7 +164,6 @@ TEST_F(TooltipControllerTest, TooltipsOnMultiDisplayShouldNotCrash) {
ui::test::EventGenerator generator1(root_windows[0]);
generator1.MoveMouseRelativeTo(widget1->GetNativeView(),
view1->bounds().CenterPoint());
helper_->FireTooltipTimer();
EXPECT_TRUE(helper_->IsTooltipVisible());
}

Expand Down
60 changes: 21 additions & 39 deletions ui/views/corewm/tooltip_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@
#include "ui/gfx/text_elider.h"
#include "ui/views/corewm/tooltip.h"
#include "ui/views/widget/tooltip_manager.h"
#include "ui/wm/public/tooltip_client.h"

namespace views {
namespace corewm {
namespace {

const int kTooltipTimeoutMs = 500;
const int kDefaultTooltipShownTimeoutMs = 10000;
#if defined(OS_WIN)
// Drawing a long word in tooltip is very slow on Windows. crbug.com/513693
Expand Down Expand Up @@ -129,11 +129,7 @@ TooltipController::TooltipController(std::unique_ptr<Tooltip> tooltip)
tooltip_id_(NULL),
tooltip_window_at_mouse_press_(NULL),
tooltip_(std::move(tooltip)),
tooltips_enabled_(true) {
tooltip_timer_.Start(FROM_HERE,
base::TimeDelta::FromMilliseconds(kTooltipTimeoutMs),
this, &TooltipController::TooltipTimerFired);
}
tooltips_enabled_(true) {}

TooltipController::~TooltipController() {
if (tooltip_window_)
Expand All @@ -150,27 +146,14 @@ void TooltipController::UpdateTooltip(aura::Window* target) {
UpdateIfRequired();

// Reset |tooltip_window_at_mouse_press_| if the moving within the same window
// but over a region that has different tooltip text. By resetting
// |tooltip_window_at_mouse_press_| we ensure the next time the timer fires
// we'll requery for the tooltip text.
// but over a region that has different tooltip text.
// This handles the case of clicking on a view, moving within the same window
// but over a different view, than back to the original.
if (tooltip_window_at_mouse_press_ &&
target == tooltip_window_at_mouse_press_ &&
aura::client::GetTooltipText(target) != tooltip_text_at_mouse_press_) {
tooltip_window_at_mouse_press_ = NULL;
}

// If we had stopped the tooltip timer for some reason, we must restart it if
// there is a change in the tooltip.
if (!tooltip_timer_.IsRunning()) {
if (tooltip_window_ != target || (tooltip_window_ &&
tooltip_text_ != aura::client::GetTooltipText(tooltip_window_))) {
tooltip_timer_.Start(FROM_HERE,
base::TimeDelta::FromMilliseconds(kTooltipTimeoutMs),
this, &TooltipController::TooltipTimerFired);
}
}
}

void TooltipController::SetTooltipShownTimeout(aura::Window* target,
Expand Down Expand Up @@ -211,10 +194,10 @@ void TooltipController::OnMouseEvent(ui::MouseEvent* event) {
target = GetTooltipTarget(*event, &curr_mouse_loc_);
}
SetTooltipWindow(target);
if (tooltip_timer_.IsRunning())
tooltip_timer_.Reset();

if (tooltip_->IsVisible())
if (tooltip_->IsVisible() ||
(tooltip_window_ &&
tooltip_text_ != aura::client::GetTooltipText(tooltip_window_)))
UpdateIfRequired();
break;
}
Expand All @@ -232,10 +215,6 @@ void TooltipController::OnMouseEvent(ui::MouseEvent* event) {
// Hide the tooltip for click, release, drag, wheel events.
if (tooltip_->IsVisible())
tooltip_->Hide();

// Don't reshow the tooltip during scroll.
if (tooltip_timer_.IsRunning())
tooltip_timer_.Reset();
break;
default:
break;
Expand All @@ -255,6 +234,10 @@ void TooltipController::OnCancelMode(ui::CancelModeEvent* event) {
SetTooltipWindow(NULL);
}

void TooltipController::OnCursorVisibilityChanged(bool is_visible) {
UpdateIfRequired();
}

void TooltipController::OnWindowDestroyed(aura::Window* window) {
if (tooltip_window_ == window) {
tooltip_->Hide();
Expand All @@ -263,20 +246,22 @@ void TooltipController::OnWindowDestroyed(aura::Window* window) {
}
}

void TooltipController::OnWindowPropertyChanged(aura::Window* window,
const void* key,
intptr_t old) {
if ((key == aura::client::kTooltipIdKey ||
key == aura::client::kTooltipTextKey) &&
aura::client::GetTooltipText(window) != base::string16() &&
(tooltip_text_ != aura::client::GetTooltipText(window) ||
tooltip_id_ != aura::client::GetTooltipId(window)))
UpdateIfRequired();
}

////////////////////////////////////////////////////////////////////////////////
// TooltipController private:

void TooltipController::TooltipTimerFired() {
UpdateIfRequired();
}

void TooltipController::TooltipShownTimerFired() {
tooltip_->Hide();

// Since the user presumably no longer needs the tooltip, we also stop the
// tooltip timer so that tooltip does not pop back up. We will restart this
// timer if the tooltip changes (see UpdateTooltip()).
tooltip_timer_.Stop();
}

void TooltipController::UpdateIfRequired() {
Expand Down Expand Up @@ -309,9 +294,6 @@ void TooltipController::UpdateIfRequired() {
ids_differ = tooltip_id_ != tooltip_id;
tooltip_id_ = tooltip_id;

// We add the !tooltip_->IsVisible() below because when we come here from
// TooltipTimerFired(), the tooltip_text may not have changed but we still
// want to update the tooltip because the timer has fired.
// If we come here from UpdateTooltip(), we have already checked for tooltip
// visibility and this check below will have no effect.
if (tooltip_text_ != tooltip_text || !tooltip_->IsVisible() || ids_differ) {
Expand Down
18 changes: 12 additions & 6 deletions ui/views/corewm/tooltip_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/macros.h"
#include "base/strings/string16.h"
#include "base/timer/timer.h"
#include "ui/aura/client/cursor_client_observer.h"
#include "ui/aura/window_observer.h"
#include "ui/events/event_handler.h"
#include "ui/gfx/geometry/point.h"
Expand All @@ -31,9 +32,11 @@ class TooltipControllerTestHelper;
} // namespace test

// TooltipController provides tooltip functionality for aura.
class VIEWS_EXPORT TooltipController : public aura::client::TooltipClient,
public ui::EventHandler,
public aura::WindowObserver {
class VIEWS_EXPORT TooltipController
: public aura::client::TooltipClient,
public ui::EventHandler,
public aura::client::CursorClientObserver,
public aura::WindowObserver {
public:
explicit TooltipController(std::unique_ptr<Tooltip> tooltip);
~TooltipController() override;
Expand All @@ -50,15 +53,20 @@ class VIEWS_EXPORT TooltipController : public aura::client::TooltipClient,
void OnTouchEvent(ui::TouchEvent* event) override;
void OnCancelMode(ui::CancelModeEvent* event) override;

// Overridden from aura::client::CursorClientObserver.
void OnCursorVisibilityChanged(bool is_visible) override;

// Overridden from aura::WindowObserver.
void OnWindowDestroyed(aura::Window* window) override;
void OnWindowPropertyChanged(aura::Window* window,
const void* key,
intptr_t old) override;

const gfx::Point& mouse_location() const { return curr_mouse_loc_; }

private:
friend class test::TooltipControllerTestHelper;

void TooltipTimerFired();
void TooltipShownTimerFired();

// Updates the tooltip if required (if there is any change in the tooltip
Expand Down Expand Up @@ -90,8 +98,6 @@ class VIEWS_EXPORT TooltipController : public aura::client::TooltipClient,

std::unique_ptr<Tooltip> tooltip_;

base::RepeatingTimer tooltip_timer_;

// Timer to timeout the life of an on-screen tooltip. We hide the tooltip when
// this timer fires.
base::OneShotTimer tooltip_shown_timer_;
Expand Down
8 changes: 2 additions & 6 deletions ui/views/corewm/tooltip_controller_test_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,8 @@ aura::Window* TooltipControllerTestHelper::GetTooltipWindow() {
return controller_->tooltip_window_;
}

void TooltipControllerTestHelper::FireTooltipTimer() {
controller_->TooltipTimerFired();
}

bool TooltipControllerTestHelper::IsTooltipTimerRunning() {
return controller_->tooltip_timer_.IsRunning();
void TooltipControllerTestHelper::UpdateIfRequired() {
controller_->UpdateIfRequired();
}

void TooltipControllerTestHelper::FireTooltipShownTimer() {
Expand Down
3 changes: 1 addition & 2 deletions ui/views/corewm/tooltip_controller_test_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ class TooltipControllerTestHelper {
// These are mostly cover methods for TooltipController private methods.
base::string16 GetTooltipText();
aura::Window* GetTooltipWindow();
void FireTooltipTimer();
bool IsTooltipTimerRunning();
void UpdateIfRequired();
void FireTooltipShownTimer();
bool IsTooltipShownTimerRunning();
bool IsTooltipVisible();
Expand Down
Loading

0 comments on commit 6b8312a

Please sign in to comment.