views: Convert IsFocusable() to just focusable() since it's just a simple accessor.

views: Convert IsFocusable() to just focusable() since it's just a simple accessor.

BUG=105879
R=ben@chromium.org

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

[tfarina, 2011-12-15 19:06:27]

Ben, please take a look specifically at these files:

chrome/browser/ui/views/omnibox/omnibox_view_views.cc
chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.h
chrome/browser/ui/views/tab_contents/native_tab_contents_container_gtk.h
chrome/browser/ui/views/tab_contents/native_tab_contents_container_win.h
ui/views/controls/button/custom_button.h
ui/views/controls/textfield/textfield.h

[Ben Goodger (Google), 2011-12-15 21:21:41]

It may be the case that some of the IsFocusable() overriders need to become
IsFocusableInRootView() overriders instead. (They will be renamed back to
IsFocusable() in your next step).

http://codereview.chromium.org/8964001/diff/1/chrome/browser/ui/views/omnibox...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

http://codereview.chromium.org/8964001/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:77: return
views::View::IsFocusable();
Is this comment no longer valid?

http://codereview.chromium.org/8964001/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc
(left):

http://codereview.chromium.org/8964001/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_aura.cc:100:
return container_->tab_contents() != NULL;
But can the TabContents become NULL at some point between the construction of
this object and someone calling IsFocusable()?

http://codereview.chromium.org/8964001/diff/1/chrome/browser/ui/views/tab_con...
File chrome/browser/ui/views/tab_contents/native_tab_contents_container_gtk.cc
(left):

http://codereview.chromium.org/8964001/diff/1/chrome/browser/ui/views/tab_con...
chrome/browser/ui/views/tab_contents/native_tab_contents_container_gtk.cc:102:
// clicking on the contents needs to focus us.
Same question as the other file.

[tfarina, 2011-12-16 13:59:39]

Ben, please take a look at patch set 2.

Thanks!

[Ben Goodger (Google), 2011-12-16 16:20:23]

http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:74: virtual bool
IsFocusableInRootView() const OVERRIDE {
Questions:

When you open a popup window (www.popuptest.com)
- can you still select text in the omnibox?
- what if you remove this function? can you still select?

http://codereview.chromium.org/8964001/diff/2003/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/8964001/diff/2003/ui/views/view.cc#newcode898
ui/views/view.cc:898: return focusable() && IsDrawn();
looks like you're skipping the check of enabled_ that was once done.

return focusable_ && enabled_ && IsDrawn();

[tfarina, 2011-12-16 17:45:15]

On 2011/12/16 16:20:23, Ben Goodger (Google) wrote:
>
http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:74: virtual bool
> IsFocusableInRootView() const OVERRIDE {
> Questions:
> 
> When you open a popup window (http://www.popuptest.com)
> - can you still select text in the omnibox?
> - what if you remove this function? can you still select?
> 
Yes, I were able to select it.

http://codereview.chromium.org/8964001/diff/2003/ui/views/view.cc
File ui/views/view.cc (right):

http://codereview.chromium.org/8964001/diff/2003/ui/views/view.cc#newcode898
ui/views/view.cc:898: return focusable() && IsDrawn();
On 2011/12/16 16:20:23, Ben Goodger (Google) wrote:
> looks like you're skipping the check of enabled_ that was once done.
> 
> return focusable_ && enabled_ && IsDrawn();

Done.

[tfarina, 2011-12-16 17:48:46]

On Fri, Dec 16, 2011 at 3:45 PM,  <tfarina@chromium.org> wrote:
> On 2011/12/16 16:20:23, Ben Goodger (Google) wrote:
>
>
http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
>>
>> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
>
>
>
>
http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
>>
>> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:74: virtual bool
>> IsFocusableInRootView() const OVERRIDE {
>> Questions:
>
>
>> When you open a popup window (http://www.popuptest.com)
>>
>> - can you still select text in the omnibox?
>> - what if you remove this function? can you still select?
>
>
> Yes, I were able to select it.
>
That was just with toolkit_views=1, I'll test with use_aura=1 now!

-- 
Thiago

[Ben Goodger (Google), 2011-12-16 18:02:07]

OK, if you can select it in the second case with USE_AURA=1, then I think
your initial reaction was correct, and you can just remove the override in
OmniboxViewViews.

-Ben

On Fri, Dec 16, 2011 at 9:48 AM, Thiago Farina <tfarina@chromium.org> wrote:

> On Fri, Dec 16, 2011 at 3:45 PM,  <tfarina@chromium.org> wrote:
> > On 2011/12/16 16:20:23, Ben Goodger (Google) wrote:
> >
> >
>
http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
> >>
> >> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> >
> >
> >
> >
>
http://codereview.chromium.org/8964001/diff/2003/chrome/browser/ui/views/omni...
> >>
> >> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:74: virtual bool
> >> IsFocusableInRootView() const OVERRIDE {
> >> Questions:
> >
> >
> >> When you open a popup window (http://www.popuptest.com)
> >>
> >> - can you still select text in the omnibox?
> >> - what if you remove this function? can you still select?
> >
> >
> > Yes, I were able to select it.
> >
> That was just with toolkit_views=1, I'll test with use_aura=1 now!
>
> --
> Thiago
>

[tfarina, 2011-12-16 18:48:23]

On 2011/12/16 18:02:07, Ben Goodger (Google) wrote:
> OK, if you can select it in the second case with USE_AURA=1, then I think
> your initial reaction was correct, and you can just remove the override in
> OmniboxViewViews.
>

Looks like I can remove the override from OmniboxViewViews, which is what I did
in patch set 4. Some screenshots of my tests here: http://imgur.com/a/V34fk#0

PTAL!

[Ben Goodger (Google), 2011-12-16 18:49:38]

lgtm