Disable the stop button when the user is hovering it and the page finishes loading, so we don't look like we stay loading forever. This change only does this for Mac and Windows, as GTK is going to be trickier.

This also fixes a pair of other bugs on Windows and Linux:
* Because we were setting the timer after telling the browser to reload, and the browser was calling us back synchronously, the timer had no effect.
* Because the timer firing changed modes with |force| set to true, if the page finished loading before the timer fired, the timer would flip the button back to reload.

BUG=46981
TEST=Hover the reload button and hit F5.  When the page finishes loading, the stop button should become disabled.  Mousing away should change it to an enabled reload button.
Review URL: http://codereview.chromium.org/3198005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@56925 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 83 insertions, 41 deletions

diff --git a/chrome/app/theme/theme_resources.grd b/chrome/app/theme/theme_resources.grd
index 9be89d4..8556768 100644
--- a/chrome/app/theme/theme_resources.grd
+++ b/chrome/app/theme/theme_resources.grd
@@ -351,6 +351,7 @@
         <include name="IDR_STATUS_TRAY_ICON_PRESSED" file="chromium/product_logo_22.png" type="BINDATA" />
       </if>
       <include name="IDR_STOP" file="stop.png" type="BINDATA" />
+      <include name="IDR_STOP_D" file="stop_d.png" type="BINDATA" />
       <include name="IDR_STOP_H" file="stop_h.png" type="BINDATA" />
       <include name="IDR_STOP_P" file="stop_p.png" type="BINDATA" />
       <include name="IDR_TAB_ACTIVE_CENTER" file="tab_active_center.png" type="BINDATA" />
diff --git a/chrome/browser/browser_theme_pack.cc b/chrome/browser/browser_theme_pack.cc
index 8d2fe89..082f1a8 100644
--- a/chrome/browser/browser_theme_pack.cc
+++ b/chrome/browser/browser_theme_pack.cc
@@ -23,7 +23,7 @@ namespace {
 
 // Version number of the current theme pack. We just throw out and rebuild
 // theme packs that aren't int-equal to this.
-const int kThemePackVersion = 14;
+const int kThemePackVersion = 15;
 
 // IDs that are in the DataPack won't clash with the positive integer
 // int32_t. kHeaderID should always have the maximum value because we want the
@@ -126,21 +126,22 @@ PersistingImagesTable kPersistingImages[] = {
   { 27, IDR_RELOAD_H, NULL },
   { 28, IDR_RELOAD_P, NULL },
   { 29, IDR_STOP, NULL },
-  { 30, IDR_STOP_H, NULL },
-  { 31, IDR_STOP_P, NULL },
-  { 32, IDR_LOCATIONBG_C, NULL },
-  { 33, IDR_LOCATIONBG_L, NULL },
-  { 34, IDR_LOCATIONBG_R, NULL },
-  { 35, IDR_BROWSER_ACTIONS_OVERFLOW, NULL },
-  { 36, IDR_BROWSER_ACTIONS_OVERFLOW_H, NULL },
-  { 37, IDR_BROWSER_ACTIONS_OVERFLOW_P, NULL },
-  { 38, IDR_TOOLS, NULL },
-  { 39, IDR_TOOLS_H, NULL },
-  { 40, IDR_TOOLS_P, NULL },
-  { 41, IDR_MENU_DROPARROW, NULL },
-  { 42, IDR_THROBBER, NULL },
-  { 43, IDR_THROBBER_WAITING, NULL },
-  { 44, IDR_THROBBER_LIGHT, NULL },
+  { 30, IDR_STOP_D, NULL },
+  { 31, IDR_STOP_H, NULL },
+  { 32, IDR_STOP_P, NULL },
+  { 33, IDR_LOCATIONBG_C, NULL },
+  { 34, IDR_LOCATIONBG_L, NULL },
+  { 35, IDR_LOCATIONBG_R, NULL },
+  { 36, IDR_BROWSER_ACTIONS_OVERFLOW, NULL },
+  { 37, IDR_BROWSER_ACTIONS_OVERFLOW_H, NULL },
+  { 38, IDR_BROWSER_ACTIONS_OVERFLOW_P, NULL },
+  { 39, IDR_TOOLS, NULL },
+  { 40, IDR_TOOLS_H, NULL },
+  { 41, IDR_TOOLS_P, NULL },
+  { 42, IDR_MENU_DROPARROW, NULL },
+  { 43, IDR_THROBBER, NULL },
+  { 44, IDR_THROBBER_WAITING, NULL },
+  { 45, IDR_THROBBER_LIGHT, NULL },
 };
 
 int GetPersistentIDByName(const std::string& key) {
diff --git a/chrome/browser/browser_theme_provider.cc b/chrome/browser/browser_theme_provider.cc
index 9850c65..4964bdd 100644
--- a/chrome/browser/browser_theme_provider.cc
+++ b/chrome/browser/browser_theme_provider.cc
@@ -146,7 +146,7 @@ const int kToolbarButtonIDs[] = {
   IDR_FORWARD, IDR_FORWARD_D, IDR_FORWARD_H, IDR_FORWARD_P,
   IDR_HOME, IDR_HOME_H, IDR_HOME_P,
   IDR_RELOAD, IDR_RELOAD_H, IDR_RELOAD_P,
-  IDR_STOP, IDR_STOP_H, IDR_STOP_P,
+  IDR_STOP, IDR_STOP_D, IDR_STOP_H, IDR_STOP_P,
   IDR_LOCATIONBG_C, IDR_LOCATIONBG_L, IDR_LOCATIONBG_R,
   IDR_BROWSER_ACTIONS_OVERFLOW, IDR_BROWSER_ACTIONS_OVERFLOW_H,
   IDR_BROWSER_ACTIONS_OVERFLOW_P,
diff --git a/chrome/browser/cocoa/reload_button.mm b/chrome/browser/cocoa/reload_button.mm
index 4f82bf0..404b035 100644
--- a/chrome/browser/cocoa/reload_button.mm
+++ b/chrome/browser/cocoa/reload_button.mm
@@ -58,15 +58,18 @@ NSString* const kStopImageName = @"stop_Template.pdf";
 
   // Can always transition to stop mode.  Only transition to reload
   // mode if forced or if the mouse isn't hovering.  Otherwise, note
-  // that reload mode is desired and make no change.
+  // that reload mode is desired and disable the button.
   if (isLoading) {
     [self setImage:nsimage_cache::ImageNamed(kStopImageName)];
     [self setTag:IDC_STOP];
+    [self setEnabled:YES];
   } else if (force || ![self isMouseInside]) {
     [self setImage:nsimage_cache::ImageNamed(kReloadImageName)];
     [self setTag:IDC_RELOAD];
+    [self setEnabled:YES];
   } else if ([self tag] == IDC_STOP) {
     pendingReloadMode_ = YES;
+    [self setEnabled:NO];
   }
 }
 
diff --git a/chrome/browser/cocoa/reload_button_unittest.mm b/chrome/browser/cocoa/reload_button_unittest.mm
index 64deb96..c2c9e84 100644
--- a/chrome/browser/cocoa/reload_button_unittest.mm
+++ b/chrome/browser/cocoa/reload_button_unittest.mm
@@ -154,6 +154,7 @@ TEST_F(ReloadButtonTest, StopAfterReloadSet) {
   // Get to stop mode.
   [button_ setIsLoading:YES force:YES];
   EXPECT_EQ([button_ tag], IDC_STOP);
+  EXPECT_TRUE([button_ isEnabled]);
 
   // Expect the action once.
   [[mock_target expect] anAction:button_];
@@ -165,21 +166,23 @@ TEST_F(ReloadButtonTest, StopAfterReloadSet) {
   [NSApp postEvent:click.second atStart:YES];
   [button_ mouseDown:click.first];
   EXPECT_EQ([button_ tag], IDC_RELOAD);
+  EXPECT_TRUE([button_ isEnabled]);
 
   // Get back to stop mode.
   [button_ setIsLoading:YES force:YES];
   EXPECT_EQ([button_ tag], IDC_STOP);
+  EXPECT_TRUE([button_ isEnabled]);
 
-  // If hover prevented reload mode immediately taking effect, clicks
-  // should not send any action, but should still transition to reload
-  // mode.
+  // If hover prevented reload mode immediately taking effect, clicks should do
+  // nothing, because the button should be disabled.
   [button_ mouseEntered:nil];
   EXPECT_TRUE([button_ isMouseInside]);
   [button_ setIsLoading:NO force:NO];
   EXPECT_EQ([button_ tag], IDC_STOP);
+  EXPECT_FALSE([button_ isEnabled]);
   [NSApp postEvent:click.second atStart:YES];
   [button_ mouseDown:click.first];
-  EXPECT_EQ([button_ tag], IDC_RELOAD);
+  EXPECT_EQ([button_ tag], IDC_STOP);
 
   [button_ setTarget:nil];
 }
diff --git a/chrome/browser/gtk/reload_button_gtk.cc b/chrome/browser/gtk/reload_button_gtk.cc
index da819af..15f44b7 100644
--- a/chrome/browser/gtk/reload_button_gtk.cc
+++ b/chrome/browser/gtk/reload_button_gtk.cc
@@ -95,7 +95,7 @@ void ReloadButtonGtk::Observe(NotificationType type,
 }
 
 void ReloadButtonGtk::OnButtonTimer() {
-  ChangeMode(intended_mode_, true);
+  ChangeMode(intended_mode_, false);
 }
 
 void ReloadButtonGtk::OnClicked(GtkWidget* sender) {
@@ -130,9 +130,6 @@ void ReloadButtonGtk::OnClicked(GtkWidget* sender) {
       location_bar_->Revert();
     }
 
-    if (browser_)
-      browser_->ExecuteCommandWithDisposition(command, disposition);
-
     // Figure out the system double-click time.
     if (button_delay_ == 0) {
       GtkSettings* settings = gtk_settings_get_default();
@@ -142,11 +139,15 @@ void ReloadButtonGtk::OnClicked(GtkWidget* sender) {
 
     // Start a timer - while this timer is running, the reload button cannot be
     // changed to a stop button.  We do not set |intended_mode_| to MODE_STOP
-    // here as we want to wait for the browser to tell us that it has started
-    // loading (and this may occur only after some delay).
+    // here as the browser will do that when it actually starts loading (which
+    // may happen synchronously, thus the need to do this before telling the
+    // browser to execute the reload command).
     timer_.Stop();
     timer_.Start(base::TimeDelta::FromMilliseconds(button_delay_), this,
                  &ReloadButtonGtk::OnButtonTimer);
+
+    if (browser_)
+      browser_->ExecuteCommandWithDisposition(command, disposition);
   }
 }
 
diff --git a/chrome/browser/views/reload_button.cc b/chrome/browser/views/reload_button.cc
index 9bdf42e..71dc5b1 100644
--- a/chrome/browser/views/reload_button.cc
+++ b/chrome/browser/views/reload_button.cc
@@ -32,11 +32,18 @@ void ReloadButton::ChangeMode(Mode mode, bool force) {
   // If the change is forced, or the user isn't hovering the icon, or it's safe
   // to change it to the other image type, make the change immediately;
   // otherwise we'll let it happen later.
-  if (force || (state() != BS_HOT) || ((mode == MODE_STOP) ?
+  if (force || !IsMouseHovered() || ((mode == MODE_STOP) ?
       !timer_.IsRunning() : (visible_mode_ != MODE_STOP))) {
     timer_.Stop();
     SetToggled(mode == MODE_STOP);
     visible_mode_ = mode;
+    SetEnabled(true);
+
+  // We want to disable the button if we're preventing a change from stop to
+  // reload due to hovering, but not if we're preventing a change from reload to
+  // stop due to the timer running.  (There is no disabled reload state.)
+  } else if (visible_mode_ != MODE_RELOAD) {
+    SetEnabled(false);
   }
 }
 
@@ -74,17 +81,16 @@ void ReloadButton::ButtonPressed(views::Button* button,
       location_bar_->Revert();
     }
 
-    browser_->ExecuteCommandWithDisposition(command, disposition);
-
-    // Stop the timer.
-    timer_.Stop();
-
     // Start a timer - while this timer is running, the reload button cannot be
     // changed to a stop button.  We do not set |intended_mode_| to MODE_STOP
-    // here as we want to wait for the browser to tell us that it has started
-    // loading (and this may occur only after some delay).
+    // here as the browser will do that when it actually starts loading (which
+    // may happen synchronously, thus the need to do this before telling the
+    // browser to execute the reload command).
+    timer_.Stop();
     timer_.Start(base::TimeDelta::FromMilliseconds(GetDoubleClickTimeMS()),
                  this, &ReloadButton::OnButtonTimer);
+
+    browser_->ExecuteCommandWithDisposition(command, disposition);
   }
 }
 
@@ -107,5 +113,5 @@ bool ReloadButton::GetTooltipText(const gfx::Point& p, std::wstring* tooltip) {
 // ReloadButton, private:
 
 void ReloadButton::OnButtonTimer() {
-  ChangeMode(intended_mode_, true);
+  ChangeMode(intended_mode_, false);
 }
diff --git a/chrome/browser/views/toolbar_view.cc b/chrome/browser/views/toolbar_view.cc
index 929ea6e..26f0768 100644
--- a/chrome/browser/views/toolbar_view.cc
+++ b/chrome/browser/views/toolbar_view.cc
@@ -563,6 +563,8 @@ void ToolbarView::LoadImages() {
       tp->GetBitmapNamed(IDR_STOP_H));
   reload_->SetToggledImage(views::CustomButton::BS_PUSHED,
       tp->GetBitmapNamed(IDR_STOP_P));
+  reload_->SetToggledImage(views::CustomButton::BS_DISABLED,
+      tp->GetBitmapNamed(IDR_STOP_D));
 
   home_->SetImage(views::CustomButton::BS_NORMAL, tp->GetBitmapNamed(IDR_HOME));
   home_->SetImage(views::CustomButton::BS_HOT, tp->GetBitmapNamed(IDR_HOME_H));
diff --git a/chrome/test/data/profiles/complex_theme/Default/Extensions/mblmlcbknbnfebdfjnolmcapmdofhmme/1.1/Cached Theme.pak b/chrome/test/data/profiles/complex_theme/Default/Extensions/mblmlcbknbnfebdfjnolmcapmdofhmme/1.1/Cached Theme.pak
index 9d8251b..67ea5c6 100644
--- a/chrome/test/data/profiles/complex_theme/Default/Extensions/mblmlcbknbnfebdfjnolmcapmdofhmme/1.1/Cached Theme.pak
+++ b/chrome/test/data/profiles/complex_theme/Default/Extensions/mblmlcbknbnfebdfjnolmcapmdofhmme/1.1/Cached Theme.pak
diff --git a/views/controls/button/custom_button.cc b/views/controls/button/custom_button.cc
index 973a8e5..e87bae9 100644
--- a/views/controls/button/custom_button.cc
+++ b/views/controls/button/custom_button.cc
@@ -6,6 +6,7 @@
 
 #include "app/throb_animation.h"
 #include "base/keyboard_codes.h"
+#include "views/screen.h"
 
 namespace views {
 
@@ -71,8 +72,13 @@ bool CustomButton::GetAccessibleState(AccessibilityTypes::State* state) {
 }
 
 void CustomButton::SetEnabled(bool enabled) {
-  if (enabled ? (state_ == BS_DISABLED) : (state_ != BS_DISABLED))
-    SetState(enabled ? BS_NORMAL : BS_DISABLED);
+  if (enabled ? (state_ != BS_DISABLED) : (state_ == BS_DISABLED))
+    return;
+
+  if (enabled)
+    SetState(IsMouseHovered() ? BS_HOT : BS_NORMAL);
+  else
+    SetState(BS_DISABLED);
 }
 
 bool CustomButton::IsEnabled() const {
@@ -83,6 +89,17 @@ bool CustomButton::IsFocusable() const {
   return (state_ != BS_DISABLED) && View::IsFocusable();
 }
 
+bool CustomButton::IsMouseHovered() const {
+  // If we haven't yet been placed in an onscreen view hierarchy, we can't be
+  // hovered.
+  if (!GetWidget())
+    return false;
+
+  gfx::Point cursor_pos(Screen::GetCursorScreenPoint());
+  ConvertPointToView(NULL, this, &cursor_pos);
+  return HitTest(cursor_pos);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // CustomButton, protected:
 
diff --git a/views/controls/button/custom_button.h b/views/controls/button/custom_button.h
index 1f85e0b..4d7dff7 100644
--- a/views/controls/button/custom_button.h
+++ b/views/controls/button/custom_button.h
@@ -63,6 +63,11 @@ class CustomButton : public Button,
   }
   bool request_focus_on_press() const { return request_focus_on_press_; }
 
+  // Returns true if the mouse pointer is over this control.  Note that this
+  // isn't the same as IsHotTracked() because the mouse may be over the control
+  // when it's disabled.
+  bool IsMouseHovered() const;
+
  protected:
   // Construct the Button with a Listener. See comment for Button's ctor.
   explicit CustomButton(ButtonListener* listener);
diff --git a/views/widget/root_view.cc b/views/widget/root_view.cc
index 91ba511..a56ec99 100644
--- a/views/widget/root_view.cc
+++ b/views/widget/root_view.cc
@@ -455,8 +455,11 @@ void RootView::OnMouseReleased(const MouseEvent& e, bool canceled) {
 
 void RootView::OnMouseMoved(const MouseEvent& e) {
   View* v = GetViewForPoint(e.location());
-  // Find the first enabled view.
-  while (v && !v->IsEnabled())
+  // Find the first enabled view, or the existing move handler, whichever comes
+  // first.  The check for the existing handler is because if a view becomes
+  // disabled while handling moves, it's wrong to suddenly send ET_MOUSE_EXITED
+  // and ET_MOUSE_ENTERED events, because the mouse hasn't actually exited yet.
+  while (v && !v->IsEnabled() && (v != mouse_move_handler_))
     v = v->GetParent();
   if (v && v != this) {
     if (v != mouse_move_handler_) {