Disable the stop button when the user is hovering it and the page finishes lo...

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.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56925

[Peter Kasting, 2010-08-19 23:29:15]

sky: Windows
erg: GTK; please test that the change does the right things
shess: Mac; please test that the change does the right things

[Scott Hess - ex-Googler, 2010-08-19 23:58:17]

LGTM with the change indicated (which I verified fixes the case I described).

I'm not entirely sure I like the effect.  Incrementally, disabling the STOP
button when the page is loaded seems reasonable.  But stepping back, would it
make more sense to transition to a disabled RELOAD button?  That seems to
address the problem which originally caused this deferring-the-change code to
happen, while more reasonably showing the state of the page.  [Though seeing
disabled RELOAD might also be confusing.]

There's a display glitch on Mac because a disabled button cannot be hovered, so
the hover goes away, but as you move off the button the button enables with
hover then drops the hover.  Probably not reasonable to fix as part of this CL,
though.

I'm heading out for a bit over a week, so you should forward further review
issues to someone else.

http://codereview.chromium.org/3198005/diff/20001/21004
File chrome/browser/cocoa/reload_button.mm (right):

http://codereview.chromium.org/3198005/diff/20001/21004#newcode64
chrome/browser/cocoa/reload_button.mm:64: [self setTag:IDC_STOP];
I suspect you also want [self setEnabled:YES] here to handle cases where you
went to IDC_STOP with the mouse inside, and then initiated a reload using Cmd-r
or something on the page.

[sky, 2010-08-20 00:03:18]

LGTM with the following change.

http://codereview.chromium.org/3198005/diff/20001/21009
File views/controls/button/custom_button.cc (right):

http://codereview.chromium.org/3198005/diff/20001/21009#newcode93
views/controls/button/custom_button.cc:93: gfx::Point
cursor_pos(Screen::GetCursorScreenPoint());
I think you should only do this if GetWidget() is non-null. Otherwise it means
you're not on screen yet and there is no way the mouse can be hovered and you're
testing the wrong thing.

[Peter Kasting, 2010-08-20 00:14:11]

Adding rohitrao to take over Mac review from shess.  Really the only question I
have is the one about the Mac display glitch below.

On 2010/08/19 23:58:17, shess wrote:
> I'm not entirely sure I like the effect.  Incrementally, disabling the STOP
> button when the page is loaded seems reasonable.  But stepping back, would it
> make more sense to transition to a disabled RELOAD button?

I had the same thought yesterday and explicitly asked Glen about it.  He said
no, because "the browser said you could stop, you went to go make it stop, and
then it notified you that the thing you wanted to do was no longer available."

> There's a display glitch on Mac because a disabled button cannot be hovered,
so
> the hover goes away, but as you move off the button the button enables with
> hover then drops the hover.

I tried to work around a similar issue on GTK by explicitly setting the hover
state correctly as I undisabled the button.  I wonder if something similar is
possible on Mac?

http://codereview.chromium.org/3198005/diff/20001/21004
File chrome/browser/cocoa/reload_button.mm (right):

http://codereview.chromium.org/3198005/diff/20001/21004#newcode64
chrome/browser/cocoa/reload_button.mm:64: [self setTag:IDC_STOP];
On 2010/08/19 23:58:17, shess wrote:
> I suspect you also want [self setEnabled:YES] here to handle cases where you
> went to IDC_STOP with the mouse inside, and then initiated a reload using
Cmd-r
> or something on the page.

Yes, I do.  Thanks.

http://codereview.chromium.org/3198005/diff/20001/21009
File views/controls/button/custom_button.cc (right):

http://codereview.chromium.org/3198005/diff/20001/21009#newcode93
views/controls/button/custom_button.cc:93: gfx::Point
cursor_pos(Screen::GetCursorScreenPoint());
On 2010/08/20 00:03:18, sky wrote:
> I think you should only do this if GetWidget() is non-null. Otherwise it means
> you're not on screen yet and there is no way the mouse can be hovered and
you're
> testing the wrong thing.

Good point.

[Peter Kasting, 2010-08-20 01:39:49]

I have changed this CL to only do the main change for Mac and Windows, not GTK. 
For GTK I've preserved the two other bugfixes mentioned in the change
description.

Talking with erg, it looks like GTK is going to be tricky.  I want to be able to
commit as much as is feasible now, so that even if we can't get GTK done for M6,
none of the rest will slip.

[Peter Kasting, 2010-08-20 19:17:34]

Somehow, I didn't _actually_ add rohitrao when I said I did.

[rohitrao (ping after 24h), 2010-08-20 21:00:18]

On 2010/08/20 19:17:34, Peter Kasting wrote:
> Somehow, I didn't _actually_ add rohitrao when I said I did.

Looking now.  I do see the glitch if I move the mouse slowly enough, but I don't
know why that's happening.  Peter, do you want to submit this as-is and I'll
take care of the glitch when I figure it out (Monday, most likely)?

One other thing that's a little worse now is loading a page like www.cnn.com,
where (at least for me) the page seems to finish loading, then loads two more
resources a second later.  Without this change, the button flashes between
reload and stop, but after this change, the whole hover border flashes, which
catches my eye much more than before.  However, once we fix the underlying bug,
the whole problem will go away.

[Peter Kasting, 2010-08-20 21:02:14]

On 2010/08/20 21:00:18, rohitrao wrote:
> One other thing that's a little worse now is loading a page like
http://www.cnn.com,
> where (at least for me) the page seems to finish loading, then loads two more
> resources a second later.

Yeah.  If you hover reload buttons, this change makes the stop/reload flicker
issues more annoying.  Hopefully, people won't sit there hovering the button in
the majority of cases.

[Elliot Glaysher, 2010-08-20 21:06:57]

GTK parts LGTM.

On Fri, Aug 20, 2010 at 2:02 PM,  <pkasting@chromium.org> wrote:
> On 2010/08/20 21:00:18, rohitrao wrote:
>>
>> One other thing that's a little worse now is loading a page like
>
> http://www.cnn.com,
>>
>> where (at least for me) the page seems to finish loading, then loads two
>> more
>> resources a second later.
>
> Yeah.  If you hover reload buttons, this change makes the stop/reload
> flicker
> issues more annoying.  Hopefully, people won't sit there hovering the button
> in
> the majority of cases.
>
>
> http://codereview.chromium.org/3198005/show
>