views: Add OnEnabledChanged() method to View class.

views: Add OnEnabledChanged() method to View class.

Changes done here:
- Override OnEnabledChanged() in the derived classes from View instead of SetEnable().
- Make SetEnable() a member function not a virtual one.
- Make |enabled_| a private data member variable not a protected one.
- Some other misc cleanups.

Note: The third item was a TODO for beng.

BUG=72040
TEST=None

R=ben@chromium.org

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

[Ben Goodger (Google), 2011-05-27 16:01:21]

All the places you rename to OnEnabledChanged you need to have them call
View::OnEnabledChanged not View::SetEnabled otherwis e you will recurse
infinitely.

http://codereview.chromium.org/6976048/diff/1/views/controls/button/checkbox.cc
File views/controls/button/checkbox.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/button/checkbox....
views/controls/button/checkbox.cc:94: NativeButtonBase::SetEnabled(enabled);
This is going to recurse infinitely.

http://codereview.chromium.org/6976048/diff/1/views/controls/button/native_bu...
File views/controls/button/native_button.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/button/native_bu...
views/controls/button/native_button.cc:168: Button::SetEnabled(enabled);
This is also going to recurse infinitely.

http://codereview.chromium.org/6976048/diff/1/views/controls/button/text_butt...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/button/text_butt...
views/controls/button/text_button.cc:565: CustomButton::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/combobox/combobo...
File views/controls/combobox/combobox.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/combobox/combobo...
views/controls/combobox/combobox.cc:82: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/label.cc
File views/controls/label.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/label.cc#newcode212
views/controls/label.cc:212: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control.cc
File views/controls/native_control.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control.c...
views/controls/native_control.cc:317: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_g...
File views/controls/native_control_gtk.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_g...
views/controls/native_control_gtk.cc:156: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_w...
File views/controls/native_control_win.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_w...
views/controls/native_control_win.cc:63: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/progress_bar.cc
File views/controls/progress_bar.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/progress_bar.cc#...
views/controls/progress_bar.cc:310: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield.cc:381: View::SetEnabled(enabled);
Infinite recursion.

http://codereview.chromium.org/6976048/diff/1/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6976048/diff/1/views/view.h#newcode325
views/view.h:325: virtual void OnEnabledChanged(bool enabled);
I would say don't pass the parameter here. This method should only be called if
the state changes. The caller can call enabled() to read the current state.

[tfarina, 2011-05-27 16:25:03]

http://codereview.chromium.org/6976048/diff/1/views/controls/button/checkbox.cc
File views/controls/button/checkbox.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/button/checkbox....
views/controls/button/checkbox.cc:94: NativeButtonBase::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> This is going to recurse infinitely.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/button/native_bu...
File views/controls/button/native_button.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/button/native_bu...
views/controls/button/native_button.cc:168: Button::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> This is also going to recurse infinitely.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/button/text_butt...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/button/text_butt...
views/controls/button/text_button.cc:565: CustomButton::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/combobox/combobo...
File views/controls/combobox/combobox.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/combobox/combobo...
views/controls/combobox/combobox.cc:82: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/label.cc
File views/controls/label.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/label.cc#newcode212
views/controls/label.cc:212: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control.cc
File views/controls/native_control.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control.c...
views/controls/native_control.cc:317: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_g...
File views/controls/native_control_gtk.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_g...
views/controls/native_control_gtk.cc:156: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_w...
File views/controls/native_control_win.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/native_control_w...
views/controls/native_control_win.cc:63: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/progress_bar.cc
File views/controls/progress_bar.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/progress_bar.cc#...
views/controls/progress_bar.cc:310: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/controls/textfield/textfi...
File views/controls/textfield/textfield.cc (right):

http://codereview.chromium.org/6976048/diff/1/views/controls/textfield/textfi...
views/controls/textfield/textfield.cc:381: View::SetEnabled(enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> Infinite recursion.

Done.

http://codereview.chromium.org/6976048/diff/1/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6976048/diff/1/views/view.h#newcode325
views/view.h:325: virtual void OnEnabledChanged(bool enabled);
On 2011/05/27 16:01:21, Ben Goodger wrote:
> I would say don't pass the parameter here. This method should only be called
if
> the state changes. The caller can call enabled() to read the current state.

Done.

[Ben Goodger (Google), 2011-05-27 16:30:15]

What is the "protected" issue you mentioned in the other CL that you say isn't
addressed here?

http://codereview.chromium.org/6976048/diff/8001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6976048/diff/8001/views/view.h#newcode325
views/view.h:325: virtual void OnEnabledChanged();
This method should be in the protected: section.

Add this comment:

// Override to be notified when the enabled state of this View has changed. The
default implementation calls SchedulePaint() on this View.

[tfarina, 2011-05-27 16:47:54]

On 2011/05/27 16:30:15, Ben Goodger wrote:
> What is the "protected" issue you mentioned in the other CL that you say isn't
> addressed here?

Well, this is not completely fixed by this CL. There are still four files
calling |enabled_|.

After applying this patch:
$ git grep -w enabled_ | grep -v focus_manager.* | grep -v view.cc | grep -v
view.h

So, we still need a couple of changes from the other CL, calling
View::IsEnabled() instead of |enabled_|.

http://codereview.chromium.org/6976048/diff/8001/views/view.h
File views/view.h (right):

http://codereview.chromium.org/6976048/diff/8001/views/view.h#newcode325
views/view.h:325: virtual void OnEnabledChanged();
On 2011/05/27 16:30:15, Ben Goodger wrote:
> This method should be in the protected: section.
> 
> Add this comment:
> 
> // Override to be notified when the enabled state of this View has changed.
The
> default implementation calls SchedulePaint() on this View.

Done.

[Ben Goodger (Google), 2011-05-27 16:49:20]

Can you just pull those changes over to this one?

On Fri, May 27, 2011 at 9:47 AM, <tfarina@chromium.org> wrote:

> On 2011/05/27 16:30:15, Ben Goodger wrote:
>
>> What is the "protected" issue you mentioned in the other CL that you say
>> isn't
>> addressed here?
>>
>
> Well, this is not completely fixed by this CL. There are still four files
> calling |enabled_|.
>
> After applying this patch:
> $ git grep -w enabled_ | grep -v focus_manager.* | grep -v view.cc | grep
> -v
> view.h
>
> So, we still need a couple of changes from the other CL, calling
> View::IsEnabled() instead of |enabled_|.
>
>
>
> http://codereview.chromium.org/6976048/diff/8001/views/view.h
> File views/view.h (right):
>
> http://codereview.chromium.org/6976048/diff/8001/views/view.h#newcode325
> views/view.h:325: virtual void OnEnabledChanged();
> On 2011/05/27 16:30:15, Ben Goodger wrote:
>
>> This method should be in the protected: section.
>>
>
>  Add this comment:
>>
>
>  // Override to be notified when the enabled state of this View has
>>
> changed. The
>
>> default implementation calls SchedulePaint() on this View.
>>
>
> Done.
>
>
> http://codereview.chromium.org/6976048/
>

[tfarina, 2011-05-27 17:10:40]

On 2011/05/27 16:49:20, Ben Goodger wrote:
> Can you just pull those changes over to this one?
> 
Sure. Done.

[tfarina, 2011-05-27 17:10:48]

http://codereview.chromium.org/6976048/diff/4002/views/controls/native_contro...
File views/controls/native_control.cc (right):

http://codereview.chromium.org/6976048/diff/4002/views/controls/native_contro...
views/controls/native_control.cc:181: enabled_ = true;
I've removed this. View already initialize this to true.

[Ben Goodger (Google), 2011-05-27 17:43:02]

http://codereview.chromium.org/6976048/diff/2057/views/controls/button/checkb...
File views/controls/button/checkbox.cc (right):

http://codereview.chromium.org/6976048/diff/2057/views/controls/button/checkb...
views/controls/button/checkbox.cc:96: label_->SetEnabled(View::IsEnabled());
In these cases you should be able to just call IsEnabled()

http://codereview.chromium.org/6976048/diff/2057/views/controls/link.cc
File views/controls/link.cc (right):

http://codereview.chromium.org/6976048/diff/2057/views/controls/link.cc#newco...
views/controls/link.cc:93: SchedulePaint();
View::OnEnabledChanged();

http://codereview.chromium.org/6976048/diff/2057/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/6976048/diff/2057/views/view.cc#newcode357
views/view.cc:357: if (is_visible_ != visible) {
I actually like this with the parameter first.

[tfarina, 2011-05-27 18:05:07]

http://codereview.chromium.org/6976048/diff/2057/views/controls/button/checkb...
File views/controls/button/checkbox.cc (right):

http://codereview.chromium.org/6976048/diff/2057/views/controls/button/checkb...
views/controls/button/checkbox.cc:96: label_->SetEnabled(View::IsEnabled());
On 2011/05/27 17:43:02, Ben Goodger wrote:
> In these cases you should be able to just call IsEnabled()

Done.

http://codereview.chromium.org/6976048/diff/2057/views/controls/link.cc
File views/controls/link.cc (right):

http://codereview.chromium.org/6976048/diff/2057/views/controls/link.cc#newco...
views/controls/link.cc:93: SchedulePaint();
On 2011/05/27 17:43:02, Ben Goodger wrote:
> View::OnEnabledChanged();

Done.

http://codereview.chromium.org/6976048/diff/2057/views/view.cc
File views/view.cc (right):

http://codereview.chromium.org/6976048/diff/2057/views/view.cc#newcode357
views/view.cc:357: if (is_visible_ != visible) {
On 2011/05/27 17:43:02, Ben Goodger wrote:
> I actually like this with the parameter first. 

Done.

[tfarina, 2011-05-27 18:14:43]

http://codereview.chromium.org/6976048/diff/3006/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/6976048/diff/3006/views/controls/button/text_b...
views/controls/button/text_button.cc:564: CustomButton::OnEnabledChanged();
It seems this call can be moved in the place of the SchedulePaint() call below.

http://codereview.chromium.org/6976048/diff/3006/views/controls/progress_bar.cc
File views/controls/progress_bar.cc (right):

http://codereview.chromium.org/6976048/diff/3006/views/controls/progress_bar....
views/controls/progress_bar.cc:308: View::OnEnabledChanged();
It seems we don't need to override this method as it does the same thing as the
default View implementation.

[Ben Goodger (Google), 2011-05-27 18:19:06]

LGTM

http://codereview.chromium.org/6976048/diff/3006/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/6976048/diff/3006/views/controls/button/text_b...
views/controls/button/text_button.cc:564: CustomButton::OnEnabledChanged();
On 2011/05/27 18:14:44, tfarina wrote:
> It seems this call can be moved in the place of the SchedulePaint() call
below.

Yes you can just get rid of the SchedulePaint().

[tfarina, 2011-05-27 18:22:26]

http://codereview.chromium.org/6976048/diff/3006/views/controls/button/text_b...
File views/controls/button/text_button.cc (right):

http://codereview.chromium.org/6976048/diff/3006/views/controls/button/text_b...
views/controls/button/text_button.cc:564: CustomButton::OnEnabledChanged();
On 2011/05/27 18:19:06, Ben Goodger wrote:
> On 2011/05/27 18:14:44, tfarina wrote:
> > It seems this call can be moved in the place of the SchedulePaint() call
> below.
> 
> Yes you can just get rid of the SchedulePaint().

Done.