Test that OmniboxViewViews has opaque background.

Test that OmniboxViewViews has opaque background.

To make subtle regressions where subpixel rendering isn't
used on Chrome OS less likely, add an OmniboxViewViews
browsertest that checks that the omnibox text isn't rendered
onto a transparent background.

Also remove an outdated reference to GTK and fix a style
issue in LocationBarView.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269418

[dmazzoni, 2014-05-06 21:12:03]

lgtm

[Peter Kasting, 2014-05-06 21:28:33]

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1414: void
LocationBarView::OnFocus() {
Is it possible to remove this function entirely and just make the omnibox view
automatically get focus by virtue of being the only focusable child?

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:66: gfx::RenderText*
GetRenderTextForTesting();
Can we instead bracket this with #if defined(UNIT_TEST) and define it inline?:

// Expose the RenderText for tests.
#if defined(UNIT_TEST)
  gfx::RenderText* GetRenderText() {
    return views::Textfield::GetRenderText();
  }
#endif

This enforces that non-test code can't use it.

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:237: #if
defined(OS_CHROMEOS)
Why CrOS-only?  Doesn't this affect other platforms too?

[Daniel Erat, 2014-05-07 18:43:02]

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1414: void
LocationBarView::OnFocus() {
On 2014/05/06 21:28:33, Peter Kasting wrote:
> Is it possible to remove this function entirely and just make the omnibox view
> automatically get focus by virtue of being the only focusable child?

seems to work for chrome os and linux. dominic, would you mind testing this on
windows again?

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views.h (right):

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views.h:66: gfx::RenderText*
GetRenderTextForTesting();
On 2014/05/06 21:28:33, Peter Kasting wrote:
> Can we instead bracket this with #if defined(UNIT_TEST) and define it inline?:
> 
> // Expose the RenderText for tests.
> #if defined(UNIT_TEST)
>   gfx::RenderText* GetRenderText() {
>     return views::Textfield::GetRenderText();
>   }
> #endif
> 
> This enforces that non-test code can't use it.

thanks, i didn't know about this #define. done.

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/262093014/diff/1/chrome/browser/ui/views/omni...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:237: #if
defined(OS_CHROMEOS)
On 2014/05/06 21:28:33, Peter Kasting wrote:
> Why CrOS-only?  Doesn't this affect other platforms too?

chrome os is the only platform where i've heard this concern, but you're right,
RenderTextWin also checks the background's transparency. done.

[Peter Kasting, 2014-05-07 18:48:50]

LGTM

https://codereview.chromium.org/262093014/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/262093014/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:239: // Chrome
OS is unable to use subpixel rendering: http://crbug.com/233364
Nit: I'd just change this whole line to "we can't use subpixel rendering." and
drop the bug reference.

https://codereview.chromium.org/262093014/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:241:
EXPECT_FALSE(static_cast<OmniboxViewViews*>(view)->GetRenderText()->
Nit: We can avoid this cast if we use something like
BrowserWindowTesting::GetLocationBarView()->omnibox_view().  (That's assuming
the browser window here is actually a BrowserWindowTesting that you can get a
handle to somehow.)

[Daniel Erat, 2014-05-07 23:12:35]

https://codereview.chromium.org/262093014/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/262093014/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:239: // Chrome
OS is unable to use subpixel rendering: http://crbug.com/233364
On 2014/05/07 18:48:51, Peter Kasting wrote:
> Nit: I'd just change this whole line to "we can't use subpixel rendering." and
> drop the bug reference.

Done.

https://codereview.chromium.org/262093014/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:241:
EXPECT_FALSE(static_cast<OmniboxViewViews*>(view)->GetRenderText()->
On 2014/05/07 18:48:51, Peter Kasting wrote:
> Nit: We can avoid this cast if we use something like
> BrowserWindowTesting::GetLocationBarView()->omnibox_view().  (That's assuming
> the browser window here is actually a BrowserWindowTesting that you can get a
> handle to somehow.)

this seems to be the case. i've added in some ASSERT_TRUEs to crash a bit more
gracefully than a segfault if the implementation changes in the future such that
this becomes something other than a BrowserWindowTesting.

[dmazzoni, 2014-05-08 17:11:12]

lgtm

Patched and tested again on Windows, still works fine.

[Daniel Erat, 2014-05-08 20:49:48]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-05-08 20:52:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/262093014/40001

[commit-bot: I haz the power, 2014-05-09 21:40:22]

Change committed as 269418