views: Fix linux omnibox colors by refactoring omnibox_result_view.cc.

views: Fix linux omnibox colors by refactoring omnibox_result_view.cc.

Previously, colors in the omnibox were handled by checking if the result
of GetNativeTheme() was NativeThemeAura::instance(). However, this meant
in practice that there was an assumption that the native theme could
only be NativeThemeWin or NativeThemeAura. This assumption meant that we
did different things regarding omnibox colors on Win, Aura and Linux
Aura/GTK.

This patch adds color constants for the various colors used in the
omnibox. It then separates the existing initialization code into
NativeThemeWin and NativeThemeAura, as appropriate. Finally, it adds a
NativeThemeGtk2, which matches the current GTK port.

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

[Elliot Glaysher, 2014-04-03 18:14:58]

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (left):

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:147: static SkColor
colors[NUM_STATES][NUM_KINDS];
It's wrong that the colors here are stored in a local static. They'll get set
once, and will never be updated if the NativeTheme changes (or returns different
colors from the same instance).

I fiddled with making this a member variable, but instances of this class are
treated as disposable. I also don't see any evidence that we need this caching.

[msw, 2014-04-03 18:53:52]

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:137: { NORMAL,
BACKGROUND, NativeTheme::kColorId_ResultsTableNormalBackground },
nit: swapping the order of state and kind would make this more readable.

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme.h:284: // Results Tables, such as the chrome
omnibox.
I think it would be beneficial to merge these colors with those used for Tables,
like the task manager's table view. I think the general table colors could use
the values you're adding for the omnibox result table. What do you and Scott
think about that?

It'd also be great to ditch [most of] the tree colors (used for the edit
bookmark dialog's tree view, etc.) and just use the corresponding table colors,
but that could be done separately.

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:609: return
color_utils::AlphaBlend(system_colors_[COLOR_HIGHLIGHT],
Can we merge any blending code/rules across native theme classes?

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:640: system_colors_[COLOR_WINDOW], 0x34);
nit: consistency in hex/decimal blending values would be good.

[Elliot Glaysher, 2014-04-03 20:25:27]

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:137: { NORMAL,
BACKGROUND, NativeTheme::kColorId_ResultsTableNormalBackground },
On 2014/04/03 18:53:53, msw wrote:
> nit: swapping the order of state and kind would make this more readable.

Done.

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme.h:284: // Results Tables, such as the chrome
omnibox.
On 2014/04/03 18:53:53, msw wrote:
> I think it would be beneficial to merge these colors with those used for
Tables,
> like the task manager's table view. I think the general table colors could use
> the values you're adding for the omnibox result table. What do you and Scott
> think about that?
> 
> It'd also be great to ditch [most of] the tree colors (used for the edit
> bookmark dialog's tree view, etc.) and just use the corresponding table
colors,
> but that could be done separately.

This feels like a different control type. The omnibox is transient; clicking on
a result dismisses it, etc.

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:609: return
color_utils::AlphaBlend(system_colors_[COLOR_HIGHLIGHT],
On 2014/04/03 18:53:53, msw wrote:
> Can we merge any blending code/rules across native theme classes?

Not easily. The common_theme.h stuff is set up to just return individual colors.
We would and up with a whole bunch of:

 NativeTheme::GetSystemColor() {
  // ...
  case kColorId_ResultsTableSelectedDimmedText:
    return GetResultsTableBlended(this, id);
  // ...
 }

and:

  CommonTheme::GetResultsTableBlended(NativeTheme* t, int id) {
    // ...
    case kColorId_SelectedDimmedText:
      return color_utils::GetReadableColor(t->GetSystemColor(XXX),
                                           t->GetSystemColor(XXX), Y);
    // ...
  }

And I'm not sure that's cleaner.

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:640: system_colors_[COLOR_WINDOW], 0x34);
On 2014/04/03 18:53:53, msw wrote:
> nit: consistency in hex/decimal blending values would be good.

Done in all the NativeTheme instances.

[msw, 2014-04-03 20:30:24]

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/222613005/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:137: { NORMAL,
BACKGROUND, NativeTheme::kColorId_ResultsTableNormalBackground },
On 2014/04/03 20:25:27, Elliot Glaysher wrote:
> On 2014/04/03 18:53:53, msw wrote:
> > nit: swapping the order of state and kind would make this more readable.
> 
> Done.

You didn't actually do what I asked, but I guess the latest patch set is still
an improvement.

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme_win.cc (right):

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme_win.cc:609: return
color_utils::AlphaBlend(system_colors_[COLOR_HIGHLIGHT],
On 2014/04/03 20:25:27, Elliot Glaysher wrote:
> On 2014/04/03 18:53:53, msw wrote:
> > Can we merge any blending code/rules across native theme classes?
> 
> Not easily. The common_theme.h stuff is set up to just return individual
colors.
> We would and up with a whole bunch of:
> 
>  NativeTheme::GetSystemColor() {
>   // ...
>   case kColorId_ResultsTableSelectedDimmedText:
>     return GetResultsTableBlended(this, id);
>   // ...
>  }
> 
> and:
> 
>   CommonTheme::GetResultsTableBlended(NativeTheme* t, int id) {
>     // ...
>     case kColorId_SelectedDimmedText:
>       return color_utils::GetReadableColor(t->GetSystemColor(XXX),
>                                            t->GetSystemColor(XXX), Y);
>     // ...
>   }
> 
> And I'm not sure that's cleaner.

I guess I was wondering if the NativeTheme could just be used to provide the
base colors that the OmniboxResultView itself would blend. No worries if that
also doesn't make sense, I'm just spit-balling.

[sky, 2014-04-03 21:37:39]

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
ui/native_theme/native_theme.h:284: // Results Tables, such as the chrome
omnibox.
On 2014/04/03 20:25:27, Elliot Glaysher wrote:
> On 2014/04/03 18:53:53, msw wrote:
> > I think it would be beneficial to merge these colors with those used for
> Tables,
> > like the task manager's table view. I think the general table colors could
use
> > the values you're adding for the omnibox result table. What do you and Scott
> > think about that?
> > 
> > It'd also be great to ditch [most of] the tree colors (used for the edit
> > bookmark dialog's tree view, etc.) and just use the corresponding table
> colors,
> > but that could be done separately.
> 
> This feels like a different control type. The omnibox is transient; clicking
on
> a result dismisses it, etc.

I tend to favor explicit values, like Elliot has, and let the implementation
return the same values if it wants.

[sky, 2014-04-03 21:38:41]

On 2014/04/03 21:37:39, sky wrote:
>
https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
> File ui/native_theme/native_theme.h (right):
> 
>
https://codereview.chromium.org/222613005/diff/40001/ui/native_theme/native_t...
> ui/native_theme/native_theme.h:284: // Results Tables, such as the chrome
> omnibox.
> On 2014/04/03 20:25:27, Elliot Glaysher wrote:
> > On 2014/04/03 18:53:53, msw wrote:
> > > I think it would be beneficial to merge these colors with those used for
> > Tables,
> > > like the task manager's table view. I think the general table colors could
> use
> > > the values you're adding for the omnibox result table. What do you and
Scott
> > > think about that?
> > > 
> > > It'd also be great to ditch [most of] the tree colors (used for the edit
> > > bookmark dialog's tree view, etc.) and just use the corresponding table
> > colors,
> > > but that could be done separately.
> > 
> > This feels like a different control type. The omnibox is transient; clicking
> on
> > a result dismisses it, etc.
> 
> I tend to favor explicit values, like Elliot has, and let the implementation
> return the same values if it wants.

The reason I like this is because it provides the flexibility of having the
implementation return control specific values, and makes for more readable color
lookup. Otherwise when you think you're changing a color only used by one
control you might not realize you're effecting a bunch of other control types.

[Elliot Glaysher, 2014-04-03 22:39:41]

sky: could I get a review/lgtm?

[sky, 2014-04-04 14:14:28]

As long as this results in the same colors, LGTM

[Elliot Glaysher, 2014-04-04 18:04:26]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-04-04 18:04:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/222613005/70001

[commit-bot: I haz the power, 2014-04-04 21:11:39]

Change committed as 261875