Cleanup: Remove dead code, use early returns to reduce indenting, shorten....

Cleanup: Remove dead code, use early returns to reduce indenting, shorten.

This makes one behavioral change: a mouse release with a successful HitTest and |canceled| true will set the state to HOT instead of NORMAL.  This seemed more correct to me.

BUG=none
TEST=none

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

[sky, 2010-08-19 21:44:43]

Just a couple of style suggestions. LGTM otherwise

http://codereview.chromium.org/3175027/diff/1/2
File views/controls/button/custom_button.cc (right):

http://codereview.chromium.org/3175027/diff/1/2#newcode74
views/controls/button/custom_button.cc:74: if (enabled == (state_ ==
BS_DISABLED))
Your code is harder for non-pkasting people to read. Could you leave the
original? If you disagree, ask someone else for a third opinion and I'll go with
that.

http://codereview.chromium.org/3175027/diff/1/2#newcode147
views/controls/button/custom_button.cc:147: // handler).
I think you should keep the return here. Otherwise it's all to easy for someone
to add something around line 151'ish and not realize they might crash.

[Peter Kasting, 2010-08-19 21:50:00]

How about the new snap, are those two places any better?

[sky, 2010-08-19 21:51:19]

Yes, thanks.

LGTM

[oshima, 2010-08-31 23:25:03]

Hi,

Looks like this changed the behavior of CustomButton::OnKeyPress.
Is it intentional? Hitting tab (to move focus) seems to be stopped working on
custom buttons. (tested using
bookmark bar on linux_views, but not on win)

On 2010/08/19 21:51:19, sky wrote:
> Yes, thanks.
> 
> LGTM

[Peter Kasting, 2010-08-31 23:31:09]

On 2010/08/31 23:25:03, oshima wrote:
> Looks like this changed the behavior of CustomButton::OnKeyPress.

Reading through OnKeyPress(), the function behaves identically to before.  Are
you sure this change is at fault?  Note that CustomButton doesn't handle the tab
key at all.

[sky, 2010-08-31 23:40:01]

I believe tab key handling is in the focus manager or accelerator processing.

  -Scott

On Tue, Aug 31, 2010 at 4:25 PM,  <oshima@chromium.org> wrote:
> Hi,
>
> Looks like this changed the behavior of CustomButton::OnKeyPress.
> Is it intentional? Hitting tab (to move focus) seems to be stopped working
> on
> custom buttons. (tested using
> bookmark bar on linux_views, but not on win)
>
> On 2010/08/19 21:51:19, sky wrote:
>>
>> Yes, thanks.
>
>> LGTM
>
>
>
> http://codereview.chromium.org/3175027/show
>

[oshima, 2010-08-31 23:43:11]

On 2010/08/31 23:31:09, Peter Kasting wrote:
> On 2010/08/31 23:25:03, oshima wrote:
> > Looks like this changed the behavior of CustomButton::OnKeyPress.
> 
> Reading through OnKeyPress(), the function behaves identically to before.  Are
> you sure this change is at fault?

It used to return false when the event is not handled, and it now returns true
if it's not disabled, regardless of being handled or not.
I made a fix and can check it if if it looks good to you.

http://codereview.chromium.org/3276007

>  Note that CustomButton doesn't handle the tab
> key at all.

focus manager now checks tab only if key event is not handled (see r54947), and
custom button is eating tab event before it's being handled by focus manager.

[Peter Kasting, 2010-08-31 23:48:09]

On 2010/08/31 23:43:11, oshima wrote:
> On 2010/08/31 23:31:09, Peter Kasting wrote:
> > On 2010/08/31 23:25:03, oshima wrote:
> > > Looks like this changed the behavior of CustomButton::OnKeyPress.
> > 
> > Reading through OnKeyPress(), the function behaves identically to before. 
Are
> > you sure this change is at fault?
> 
> It used to return false when the event is not handled, and it now returns true
> if it's not disabled, regardless of being handled or not.

Argh.  I missed that in multiple read-throughs.

I LGTMed your other change.  I think the resulting code is less subtle than the
original.

[oshima, 2010-08-31 23:48:39]

correction.

On Tue, Aug 31, 2010 at 4:43 PM, <oshima@chromium.org> wrote:

> On 2010/08/31 23:31:09, Peter Kasting wrote:
>
>> On 2010/08/31 23:25:03, oshima wrote:
>> > Looks like this changed the behavior of CustomButton::OnKeyPress.
>>
>
>  Reading through OnKeyPress(), the function behaves identically to before.
>>  Are
>> you sure this change is at fault?
>>
>
> It used to return false when the event is not handled, and it now returns
> true
> if it's not disabled, regardless of being handled or not.
> I made a fix and can check it if if it looks good to you.
>
> http://codereview.chromium.org/3276007
>
>
>   Note that CustomButton doesn't handle the tab
>> key at all.
>>
>
> focus manager now checks tab only if key event is not handled (see r54947),
> and
> custom button is eating tab event before it's being handled by focus
> manager.
>
>
WidgetGtk now checks tab before passing it to focus manager. James can
explain it better than me. (James?)
I'm not sure which change has to be fixed, but this was working with old
logic at least.

- oshima


>
>
> http://codereview.chromium.org/3175027/show
>