Changed CLB to select all text when active tab is clicked.

Changed CLB to select all text when active tab is clicked.

BUG=83537
TEST=In compact nav mode, ensure that clicking an active (foreground) tab shows the compact location bar with the text selected.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86670

[MAD, 2011-05-25 00:21:18]

How about this counter-proposal?

http://codereview.chromium.org/6992034/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/6992034/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:354:
browser_view()->SetFocusToLocationBar(true);
Feels a bit weird to have to go back to the browser view to set focus to our
location bar child... :-/

I think it might be cleaner to simply add a call to:
GetCompactLocationBarView()->SetFocusAndSelection()

What do you think?

[SteveT, 2011-05-25 04:16:42]

Back to you for a look. Agreed with your proposal.

http://codereview.chromium.org/6992034/diff/1/chrome/browser/ui/views/compact...
File chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
(right):

http://codereview.chromium.org/6992034/diff/1/chrome/browser/ui/views/compact...
chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:354:
browser_view()->SetFocusToLocationBar(true);
On 2011/05/25 00:21:18, MAD wrote:
> Feels a bit weird to have to go back to the browser view to set focus to our
> location bar child... :-/
> 
> I think it might be cleaner to simply add a call to:
> GetCompactLocationBarView()->SetFocusAndSelection()
> 
> What do you think?

Agreed. I forgot that the CLBView had a method to do that directly. Just noticed
that BrowserView had the method I wanted but didn't look at the implementation
til now.

[MAD, 2011-05-25 14:28:12]

Did you try it?

As I read the code, I thought it wouldn't, so I tried it, and it doesn't...
:-)

You need to keep the original line, and add the new one for the focus...

On Wed, May 25, 2011 at 12:16 AM, <stevet@chromium.org> wrote:

> Back to you for a look. Agreed with your proposal.
>
>
>
>
>
http://codereview.chromium.org/6992034/diff/1/chrome/browser/ui/views/compact...
> File
> chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc
> (right):
>
>
>
http://codereview.chromium.org/6992034/diff/1/chrome/browser/ui/views/compact...
> chrome/browser/ui/views/compact_nav/compact_location_bar_view_host.cc:354:
> browser_view()->SetFocusToLocationBar(true);
> On 2011/05/25 00:21:18, MAD wrote:
>
>> Feels a bit weird to have to go back to the browser view to set focus
>>
> to our
>
>> location bar child... :-/
>>
>
>  I think it might be cleaner to simply add a call to:
>> GetCompactLocationBarView()->SetFocusAndSelection()
>>
>
>  What do you think?
>>
>
> Agreed. I forgot that the CLBView had a method to do that directly. Just
> noticed that BrowserView had the method I wanted but didn't look at the
> implementation til now.
>
>
> http://codereview.chromium.org/6992034/
>

[SteveT, 2011-05-25 15:54:19]

Yeah I left it to compile overnight and tried it just now to see the same
problem you pointed out. Anyways, modified and over to you.

[MAD, 2011-05-25 15:55:59]

LGTM! :-)

Thanks!

BYE
MAD