Use OmniboxViewViews on Win with --enable-views-textfield.

Use OmniboxViewViews on Win with --enable-views-textfield.

Add chrome/browser/ui/views/omnibox/omnibox_views.[cc|h].
Consolidate CreateOmniboxView into a non-member function.

Add LocationBarView::use_omnibox_view_views_ state flag. Move/rename AsViews to GetOmniboxViewViews like Win variant.
Remove OmniboxViewViews::GetRelativeWindowForPopup win code.
Various cleanup, forward decl / include-what-you-use, etc.

BUG=131660
TEST=Manual, use the flag to flip the Omnibox implementation.

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

[msw, 2012-06-14 01:15:42]

Hey Scott and Oshima, PTAL; thanks!

[oshima, 2012-06-14 21:07:03]

looks good, just a few questions.

http://codereview.chromium.org/10534153/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/page_info_helper.cc (right):

http://codereview.chromium.org/10534153/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/page_info_helper.cc:7: #include
"chrome/browser/ui/omnibox/omnibox_view.h"
is this necessary?

http://codereview.chromium.org/10534153/diff/1/chrome/browser/ui/views/omnibo...
File chrome/browser/ui/views/omnibox/omnibox_views.h (right):

http://codereview.chromium.org/10534153/diff/1/chrome/browser/ui/views/omnibo...
chrome/browser/ui/views/omnibox/omnibox_views.h:16: namespace views {
not sure if this should/can use views namespace, but I'll defer  to sky/ben on
this.

[msw, 2012-06-14 21:30:21]

Updated, please take a look; thanks!

https://chromiumcodereview.appspot.com/10534153/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/page_info_helper.cc (right):

https://chromiumcodereview.appspot.com/10534153/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/page_info_helper.cc:7: #include
"chrome/browser/ui/omnibox/omnibox_view.h"
On 2012/06/14 21:07:04, oshima wrote:
> is this necessary?

Yes, Line 31's GetLocationEntry()->IsEditingOrEmpty() uses OmniboxView and I'm
removing the transitive include, which seems good overall for IWYU.

https://chromiumcodereview.appspot.com/10534153/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_views.h (right):

https://chromiumcodereview.appspot.com/10534153/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_views.h:16: namespace views {
On 2012/06/14 21:07:04, oshima wrote:
> not sure if this should/can use views namespace, but I'll defer  to sky/ben on
> this.

Removed, it does make more sense in the global namespace.

[oshima, 2012-06-14 21:41:11]

lgtm

http://codereview.chromium.org/10534153/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/page_info_helper.cc (right):

http://codereview.chromium.org/10534153/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/page_info_helper.cc:7: #include
"chrome/browser/ui/omnibox/omnibox_view.h"
On 2012/06/14 21:30:21, msw wrote:
> On 2012/06/14 21:07:04, oshima wrote:
> > is this necessary?
> 
> Yes, Line 31's GetLocationEntry()->IsEditingOrEmpty() uses OmniboxView and I'm
> removing the transitive include, which seems good overall for IWYU.

ah i see. thank you for explanation.

[sky, 2012-06-14 23:04:46]

http://codereview.chromium.org/10534153/diff/10002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10534153/diff/10002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:92: CommandLine*
command_line = CommandLine::ForCurrentProcess();
This code is called alot, should we cache the result?

[msw, 2012-06-15 00:19:28]

https://chromiumcodereview.appspot.com/10534153/diff/10002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10534153/diff/10002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:92: CommandLine*
command_line = CommandLine::ForCurrentProcess();
On 2012/06/14 23:04:46, sky wrote:
> This code is called alot, should we cache the result?

I've added class members |use_omnibox_view_views_| and GetOmniboxView[Views|Win]
(with a const Views version for
LocationBarView::IsLocationEntryFocusableInRootView).

I'm also amenable to other approaches for caching (static bool in static local
function?) as well as const handling (casting in caller, calling the const
member function from non-const with casting, or evilly returning non-const ptrs
from const  GetOmniboxView[Views|Win] functions, etc.)... thoughts?

[sky, 2012-06-15 14:35:52]

I like the static bool approach.

  -Scott

On Thu, Jun 14, 2012 at 5:19 PM,  <msw@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10534153/diff/10002/chrome/browser/ui/...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
>
>
https://chromiumcodereview.appspot.com/10534153/diff/10002/chrome/browser/ui/...
>
> chrome/browser/ui/views/location_bar/location_bar_view.cc:92:
> CommandLine* command_line = CommandLine::ForCurrentProcess();
> On 2012/06/14 23:04:46, sky wrote:
>>
>> This code is called alot, should we cache the result?
>
>
> I've added class members |use_omnibox_view_views_| and
> GetOmniboxView[Views|Win] (with a const Views version for
> LocationBarView::IsLocationEntryFocusableInRootView).
>
> I'm also amenable to other approaches for caching (static bool in static
> local function?) as well as const handling (casting in caller, calling
> the const member function from non-const with casting, or evilly
> returning non-const ptrs from const  GetOmniboxView[Views|Win]
> functions, etc.)... thoughts?
>
> https://chromiumcodereview.appspot.com/10534153/

[msw, 2012-06-15 18:36:23]

Cool, here's the static bool approach (from PS2). Please take a look; thanks!

[sky, 2012-06-15 20:19:09]

LGTM

[commit-bot: I haz the power, 2012-06-15 20:49:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10534153/17001

[commit-bot: I haz the power, 2012-06-15 22:36:51]

Change committed as 142507