Converts some of the omnibox related classes to support multiple

Converts some of the omnibox related classes to support multiple
NativeThemes at the same time.

BUG=none
TEST=none
R=pkasting@chromium.org


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

[sky, 2012-11-08 02:04:24]

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/browser_view.cc:1866: if (!initialized_ && is_add
&& child == this && GetWidget()) {
This in no way relates to this patch, but checking initializes_ first allows the
if to quickly terminate for the common case.

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:166: {
I put this in a block so that there is no conflict with initialized and colors.

[Peter Kasting, 2012-11-08 18:55:47]

My only question is whether we could eliminate all the GetNativeTheme() calls
everywhere by making GetColor() non-static (and then having it call
GetNativeTheme() itself).  I don't know if all the callers could handle that,
though.

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/browser_view.cc:1866: if (!initialized_ && is_add
&& child == this && GetWidget()) {
On 2012/11/08 02:04:24, sky wrote:
> This in no way relates to this patch, but checking initializes_ first allows
the
> if to quickly terminate for the common case.

GetWidget() is expensive?

I don't mind the change, I'm just surprised it would make any meaningful
difference.

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

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:408: return
color_utils::GetReadableColor(color, GetColor(theme,
Nit: Or

      return color_utils::GetReadableColor(
          color, GetColor(theme, security_level, BACKGROUND));

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:166: {
On 2012/11/08 02:04:24, sky wrote:
> I put this in a block so that there is no conflict with initialized and
colors.

Might be slightly clearer to add a qualifier to the Windows-specific variables
in the above block, e.g. "colors" -> "win_colors" or something.

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:862: const
ui::NativeTheme* theme = GetNativeTheme();
I'd remove either the blank above or the blank below this (or both?)

[Peter Kasting, 2012-11-08 18:56:00]

Oh, LGTM, forgot that

[sky, 2012-11-08 19:12:08]

On 2012/11/08 18:55:47, Peter Kasting wrote:
> My only question is whether we could eliminate all the GetNativeTheme() calls
> everywhere by making GetColor() non-static (and then having it call
> GetNativeTheme() itself).  I don't know if all the callers could handle that,
> though.

I had actually looked into that, but shied away since it requires
OmniboxPopupContentsView to do a
static_cast<OmniboxResultView*>(child_at(0))->GetColor(). That felt brittle to
me.

>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/frame...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/frame...
> chrome/browser/ui/views/frame/browser_view.cc:1866: if (!initialized_ &&
is_add
> && child == this && GetWidget()) {
> On 2012/11/08 02:04:24, sky wrote:
> > This in no way relates to this patch, but checking initializes_ first allows
> the
> > if to quickly terminate for the common case.
> 
> GetWidget() is expensive?
> 
> I don't mind the change, I'm just surprised it would make any meaningful
> difference.
> 

Not particularly, but compared to checking a boolean it is.

  -Scott
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/locat...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/locat...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:408: return
> color_utils::GetReadableColor(color, GetColor(theme,
> Nit: Or
> 
>       return color_utils::GetReadableColor(
>           color, GetColor(theme, security_level, BACKGROUND));
> 
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
> File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):
> 
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
> chrome/browser/ui/views/omnibox/omnibox_result_view.cc:166: {
> On 2012/11/08 02:04:24, sky wrote:
> > I put this in a block so that there is no conflict with initialized and
> colors.
> 
> Might be slightly clearer to add a qualifier to the Windows-specific variables
> in the above block, e.g. "colors" -> "win_colors" or something.
> 
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
https://codereview.chromium.org/11360144/diff/1/chrome/browser/ui/views/omnib...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:862: const
> ui::NativeTheme* theme = GetNativeTheme();
> I'd remove either the blank above or the blank below this (or both?)

[Peter Kasting, 2012-11-08 19:16:33]

On 2012/11/08 19:12:08, sky wrote:
> On 2012/11/08 18:55:47, Peter Kasting wrote:
> > My only question is whether we could eliminate all the GetNativeTheme()
calls
> > everywhere by making GetColor() non-static (and then having it call
> > GetNativeTheme() itself).  I don't know if all the callers could handle
that,
> > though.
> 
> I had actually looked into that, but shied away since it requires
> OmniboxPopupContentsView to do a
> static_cast<OmniboxResultView*>(child_at(0))->GetColor(). That felt brittle to
> me.

Lots of the OmniboxPopupContentsView already does that.  See e.g.
InvalidateLine() and UpdatePopupAppearance().

Perhaps we should address all of these by actually keeping a vector of the
appropriately-typed pointers.  Would be easy to set up in Init().  Or at the
very least move this out to a helper function.

Are there any other places that need to do something nasty to not pass the theme
pointer in?  If not let's make that change.

[sky, 2012-11-09 19:27:56]

Updated. I addressed all your feedback and made the two GetColor methods
instance methods.

New snapshot uploaded.

Thanks,

  -Scott

[Peter Kasting, 2012-11-09 21:21:11]

LGTM

https://codereview.chromium.org/11360144/diff/13001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/11360144/diff/13001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:521: gfx::Rect
OmniboxPopupContentsView::CalculateTargetBounds(int h) {
Nit: While here, move this function to where it is in the .h

https://codereview.chromium.org/11360144/diff/13001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:560:
OmniboxResultView* OmniboxPopupContentsView::result_view_at(int i) {
Nit: Callers pass a size_t, maybe this should take size_t and then cast
internally?

[sky, 2012-11-09 21:28:54]

https://codereview.chromium.org/11360144/diff/13001/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc (right):

https://codereview.chromium.org/11360144/diff/13001/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc:560:
OmniboxResultView* OmniboxPopupContentsView::result_view_at(int i) {
On 2012/11/09 21:21:11, Peter Kasting wrote:
> Nit: Callers pass a size_t, maybe this should take size_t and then cast
> internally?

Done.

[commit-bot: I haz the power, 2012-11-09 21:29:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/11360144/9003

[commit-bot: I haz the power, 2012-11-09 22:09:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/11360144/14003

[commit-bot: I haz the power, 2012-11-10 01:12:52]

Change committed as 167026