Fix up reload button to not flicker, and simplify implementation.

Fix up reload button to not flicker, and simplify implementation.

BUG=NONE
TEST=Build, and test through UI.

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

[dmac, 2011-07-20 22:44:17]

PTAL

[Scott Hess - ex-Googler, 2011-07-21 00:48:40]

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
File chrome/browser/ui/cocoa/image_button_cell.h (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
chrome/browser/ui/cocoa/image_button_cell.h:27: // and the mouse enters or exits
the cell.
I don't think the comment is right WRT "enabled".  [The reload button
specifically tracks mouse-exit when disabled.]

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
File chrome/browser/ui/cocoa/image_button_cell.mm (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
chrome/browser/ui/cocoa/image_button_cell.mm:141: NSView<ImageButton>* control =
(NSView<ImageButton>*)[self controlView];
Is this supposed to be static_cast?

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/reload_button.mm (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:23: - (void)commonInit;
-sharedInit is used in two cases, -commonInit is used in one.  Which honestly
makes me wonder if it wouldn't be better to use an init function in the
anonymous namespace.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:43: // is removed from the view
hierarchy.
Is this being added for a specific reason?  I can see it in the remove case, but
if |newWindow| is non-nil, it should have other refs before and after.

Mostly I ask because I'm trying to think of what the state should be after this
is called.  Other calls to -invalidatePendingReloadTimer come directly before
code which explicitly sets the state.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:107: [self
updateTag:IDC_RELOAD];
I _think_ that given the final else clause you added, now you could add !force
&& to the following if(), and then it would all work out.

[Basically, that would take the previous (force || !isMouseInside) -> (!force &&
isMouseInside) in the next if() clause.]

WAIT!  I _think_ that the final fall-through case in the previous code may have
actually been functional.  I think you could be called with !isLoading && !force
multiple times in some cases, so letting the existing timer continue was
important.  I think.  I'll try to remember the case, but it's been awhile.

[I was just going to suggest invalidating the timer at the top of the function,
but then was wondering why the previous code didn't release the timer at the top
of the function, and I think that was why.  You're right, there should be a
comment or something.]

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:138: return [super isEnabled]
&& !([self tag] == IDC_STOP && pendingReloadTimer_);
pendingReloadTimer_ should imply [self tag] == IDC_STOP.  It might make sense to
make pendingReloadTimer_ imply disabled more explicitly.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:142: [self
invalidatePendingReloadTimer];
-setIsLoading:force: already handles the timer.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:149: return YES;
In the old code, this couldn't happen because the button was disabled.  Now it
looks like that's being handled by short-circuiting - what does -isEnabled do
for the new code, then?

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:172: }
pendingReloadTimer_ implies that the new state is !isInside.

[pendingReloadTimer_ fire] would nicely make sure that what happens on exit
matches what happens when the timer fires.

[dmac, 2011-07-29 20:01:00]

Thanks Scott... PTAL with the new code.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
File chrome/browser/ui/cocoa/image_button_cell.h (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
chrome/browser/ui/cocoa/image_button_cell.h:27: // and the mouse enters or exits
the cell.
On 2011/07/21 00:48:40, shess wrote:
> I don't think the comment is right WRT "enabled".  [The reload button
> specifically tracks mouse-exit when disabled.]

Now it works when disabled as well.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
File chrome/browser/ui/cocoa/image_button_cell.mm (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
chrome/browser/ui/cocoa/image_button_cell.mm:141: NSView<ImageButton>* control =
(NSView<ImageButton>*)[self controlView];
On 2011/07/21 00:48:40, shess wrote:
> Is this supposed to be static_cast?

Done.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/reload_button.mm (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:23: - (void)commonInit;
On 2011/07/21 00:48:40, shess wrote:
> -sharedInit is used in two cases, -commonInit is used in one.  Which honestly
> makes me wonder if it wouldn't be better to use an init function in the
> anonymous namespace.

Not sure what you meant by the anonymous namespace, but it turns out the
sharedInit is unnecessary and initWithFrame: can call awakeFromNib.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:43: // is removed from the view
hierarchy.
On 2011/07/21 00:48:40, shess wrote:
> Is this being added for a specific reason?  I can see it in the remove case,
but
> if |newWindow| is non-nil, it should have other refs before and after.
> 
> Mostly I ask because I'm trying to think of what the state should be after
this
> is called.  Other calls to -invalidatePendingReloadTimer come directly before
> code which explicitly sets the state.

Enforces a good state whereever the view is placed now.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:107: [self
updateTag:IDC_RELOAD];
On 2011/07/21 00:48:40, shess wrote:
> I _think_ that given the final else clause you added, now you could add !force
> && to the following if(), and then it would all work out.
> 
> [Basically, that would take the previous (force || !isMouseInside) -> (!force
&&
> isMouseInside) in the next if() clause.]
> 
> WAIT!  I _think_ that the final fall-through case in the previous code may
have
> actually been functional.  I think you could be called with !isLoading &&
!force
> multiple times in some cases, so letting the existing timer continue was
> important.  I think.  I'll try to remember the case, but it's been awhile.
> 
> [I was just going to suggest invalidating the timer at the top of the
function,
> but then was wondering why the previous code didn't release the timer at the
top
> of the function, and I think that was why.  You're right, there should be a
> comment or something.]

This appears to be working for me (after pretty extensive testing). Have you
come up with any case it doesn't work?

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:138: return [super isEnabled]
&& !([self tag] == IDC_STOP && pendingReloadTimer_);
On 2011/07/21 00:48:40, shess wrote:
> pendingReloadTimer_ should imply [self tag] == IDC_STOP.  It might make sense
to
> make pendingReloadTimer_ imply disabled more explicitly.

No longer needed due to mouse tracking having been cleaned up.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:142: [self
invalidatePendingReloadTimer];
On 2011/07/21 00:48:40, shess wrote:
> -setIsLoading:force: already handles the timer.

Done, and added a bit more checking.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:149: return YES;
On 2011/07/21 00:48:40, shess wrote:
> In the old code, this couldn't happen because the button was disabled.  Now it
> looks like that's being handled by short-circuiting - what does -isEnabled do
> for the new code, then?

isEnabled has been removed.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:172: }
On 2011/07/21 00:48:40, shess wrote:
> pendingReloadTimer_ implies that the new state is !isInside.
> 
> [pendingReloadTimer_ fire] would nicely make sure that what happens on exit
> matches what happens when the timer fires.

Done. Good call.

[dmac, 2011-08-01 21:37:52]

On 2011/07/29 20:01:00, dmac wrote:
> Thanks Scott... PTAL with the new code.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
> File chrome/browser/ui/cocoa/image_button_cell.h (right):
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
> chrome/browser/ui/cocoa/image_button_cell.h:27: // and the mouse enters or
exits
> the cell.
> On 2011/07/21 00:48:40, shess wrote:
> > I don't think the comment is right WRT "enabled".  [The reload button
> > specifically tracks mouse-exit when disabled.]
> 
> Now it works when disabled as well.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
> File chrome/browser/ui/cocoa/image_button_cell.mm (right):
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/imag...
> chrome/browser/ui/cocoa/image_button_cell.mm:141: NSView<ImageButton>* control
=
> (NSView<ImageButton>*)[self controlView];
> On 2011/07/21 00:48:40, shess wrote:
> > Is this supposed to be static_cast?
> 
> Done.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> File chrome/browser/ui/cocoa/toolbar/reload_button.mm (right):
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:23: - (void)commonInit;
> On 2011/07/21 00:48:40, shess wrote:
> > -sharedInit is used in two cases, -commonInit is used in one.  Which
honestly
> > makes me wonder if it wouldn't be better to use an init function in the
> > anonymous namespace.
> 
> Not sure what you meant by the anonymous namespace, but it turns out the
> sharedInit is unnecessary and initWithFrame: can call awakeFromNib.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:43: // is removed from the
view
> hierarchy.
> On 2011/07/21 00:48:40, shess wrote:
> > Is this being added for a specific reason?  I can see it in the remove case,
> but
> > if |newWindow| is non-nil, it should have other refs before and after.
> > 
> > Mostly I ask because I'm trying to think of what the state should be after
> this
> > is called.  Other calls to -invalidatePendingReloadTimer come directly
before
> > code which explicitly sets the state.
> 
> Enforces a good state whereever the view is placed now.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:107: [self
> updateTag:IDC_RELOAD];
> On 2011/07/21 00:48:40, shess wrote:
> > I _think_ that given the final else clause you added, now you could add
!force
> > && to the following if(), and then it would all work out.
> > 
> > [Basically, that would take the previous (force || !isMouseInside) ->
(!force
> &&
> > isMouseInside) in the next if() clause.]
> > 
> > WAIT!  I _think_ that the final fall-through case in the previous code may
> have
> > actually been functional.  I think you could be called with !isLoading &&
> !force
> > multiple times in some cases, so letting the existing timer continue was
> > important.  I think.  I'll try to remember the case, but it's been awhile.
> > 
> > [I was just going to suggest invalidating the timer at the top of the
> function,
> > but then was wondering why the previous code didn't release the timer at the
> top
> > of the function, and I think that was why.  You're right, there should be a
> > comment or something.]
> 
> This appears to be working for me (after pretty extensive testing). Have you
> come up with any case it doesn't work?
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:138: return [super isEnabled]
> && !([self tag] == IDC_STOP && pendingReloadTimer_);
> On 2011/07/21 00:48:40, shess wrote:
> > pendingReloadTimer_ should imply [self tag] == IDC_STOP.  It might make
sense
> to
> > make pendingReloadTimer_ imply disabled more explicitly.
> 
> No longer needed due to mouse tracking having been cleaned up.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:142: [self
> invalidatePendingReloadTimer];
> On 2011/07/21 00:48:40, shess wrote:
> > -setIsLoading:force: already handles the timer.
> 
> Done, and added a bit more checking.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:149: return YES;
> On 2011/07/21 00:48:40, shess wrote:
> > In the old code, this couldn't happen because the button was disabled.  Now
it
> > looks like that's being handled by short-circuiting - what does -isEnabled
do
> > for the new code, then?
> 
> isEnabled has been removed.
> 
>
http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
> chrome/browser/ui/cocoa/toolbar/reload_button.mm:172: }
> On 2011/07/21 00:48:40, shess wrote:
> > pendingReloadTimer_ implies that the new state is !isInside.
> > 
> > [pendingReloadTimer_ fire] would nicely make sure that what happens on exit
> > matches what happens when the timer fires.
> 
> Done. Good call.

ping

[Scott Hess - ex-Googler, 2011-08-04 23:49:44]

LGTM.

Sorry about the delay, got distracted by something shiny.  I'll go try to see
whether I can find the edge case I worried about earlier, but I'm not sure
that's worth blocking over.

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
File chrome/browser/ui/cocoa/toolbar/reload_button.mm (right):

http://codereview.chromium.org/7466016/diff/1006/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:23: - (void)commonInit;
On 2011/07/29 20:01:00, dmac wrote:
> On 2011/07/21 00:48:40, shess wrote:
> > -sharedInit is used in two cases, -commonInit is used in one.  Which
honestly
> > makes me wonder if it wouldn't be better to use an init function in the
> > anonymous namespace.
> 
> Not sure what you meant by the anonymous namespace, but it turns out the
> sharedInit is unnecessary and initWithFrame: can call awakeFromNib.

Mostly I meant that hidden objc methods like this aren't namespaced, which is
annoying.  It could have been a function in an anonymous namespace and then it
wouldn't have that problem (but would be 23% more annoying).  But no method at
all is better!

http://codereview.chromium.org/7466016/diff/6001/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:164: 
This seems contrived (pendingReloadTimer_ is nil, which matches in
-forceReloadState:, but still).  WDYT about just calling -setIsLoading:NO
force:YES directly?  Though I suppose that makes it a bit more brittle.

http://codereview.chromium.org/7466016/diff/6001/chrome/browser/ui/cocoa/tool...
chrome/browser/ui/cocoa/toolbar/reload_button.mm:178: 
No need for the if(), and this can't possibly happen enough to be noticeable.