chromeos: Update autocomplete popup colors.

chromeos: Update autocomplete popup colors.

This fetches them from GTK using the same logic as is used in
the GTK port.

BUG=chromium-os:3916
TEST=built and ran it for GTK and Chrome OS Views

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

[Daniel Erat, 2010-08-24 20:57:02]

I spent some time a few weeks ago trying to implement the TODO to get the colors
from the theme provider, but it ended up being fairly tricky due to the static
function used in the existing code.

But it turns out that that probably wouldn't help here; the GTK implementation
in browser/autocomplete/autocomplete_popup_view_gtk.cc actually creates a
temporary GtkEntry widget and grabs the colors from its style.  This seems
pretty disgusting to me, but let me know if you'd rather that I tried to
abstract out that code and call it from the Views implementation.

[Peter Kasting, 2010-08-24 21:00:30]

On 2010/08/24 20:57:02, Daniel Erat wrote:
> I spent some time a few weeks ago trying to implement the TODO to get the
colors
> from the theme provider, but it ended up being fairly tricky due to the static
> function used in the existing code.

It's possible to make it non-static.

> But it turns out that that probably wouldn't help here; the GTK implementation
> in browser/autocomplete/autocomplete_popup_view_gtk.cc actually creates a
> temporary GtkEntry widget and grabs the colors from its style.  This seems
> pretty disgusting to me, but let me know if you'd rather that I tried to
> abstract out that code and call it from the Views implementation.

Doing that seems at least better than adding more hardcoded values.  I'd like to
get away from hardcoding as much as possible.

[Daniel Erat, 2010-08-24 21:54:45]

[+evan]

Another look?

http://codereview.chromium.org/3198013/diff/4001/5003
File chrome/browser/gtk/gtk_util.h (right):

http://codereview.chromium.org/3198013/diff/4001/5003#newcode289
chrome/browser/gtk/gtk_util.h:289: // Convert the passed-in GdkColor into a
SkColor.
s/ a / an / already in a local change

[Evan Martin, 2010-08-24 22:00:37]

+gtk theming master

This LGTM, maybe get Elliot to sign off on it.

http://codereview.chromium.org/3198013/diff/4001/5002
File chrome/browser/gtk/gtk_util.cc (right):

http://codereview.chromium.org/3198013/diff/4001/5002#newcode925
chrome/browser/gtk/gtk_util.cc:925: return SkColorSetRGB(color.red / 256,
color.green / 256, color.blue / 256);
Should these be by 255?  I would imagine color.red to range in [0,255] so for
full-white you 255 to map to 1.0.

[Elliot Glaysher, 2010-08-24 22:08:04]

> the GTK implementation
> in browser/autocomplete/autocomplete_popup_view_gtk.cc
> actually creates a
> temporary GtkEntry widget and grabs the colors from its 
> style.

This is a common pattern since we can't reliably query for colors without having
full widget instances. Anyway, I guess this LGTM.

http://codereview.chromium.org/3198013/diff/4001/5002
File chrome/browser/gtk/gtk_util.cc (right):

http://codereview.chromium.org/3198013/diff/4001/5002#newcode925
chrome/browser/gtk/gtk_util.cc:925: return SkColorSetRGB(color.red / 256,
color.green / 256, color.blue / 256);
On 2010/08/24 22:00:37, Evan Martin wrote:
> Should these be by 255?  I would imagine color.red to range in [0,255] so for
> full-white you 255 to map to 1.0.

257 actually, for really subtle reasons.

Use the versions in skia_utils_gtk.h instead.

[Daniel Erat, 2010-08-24 22:08:37]

On Tue, Aug 24, 2010 at 3:00 PM,  <evan@chromium.org> wrote:
> +gtk theming master
>
> This LGTM, maybe get Elliot to sign off on it.
>
>
> http://codereview.chromium.org/3198013/diff/4001/5002
> File chrome/browser/gtk/gtk_util.cc (right):
>
> http://codereview.chromium.org/3198013/diff/4001/5002#newcode925
> chrome/browser/gtk/gtk_util.cc:925: return SkColorSetRGB(color.red /
> 256, color.green / 256, color.blue / 256);
> Should these be by 255?  I would imagine color.red to range in [0,255]
> so for full-white you 255 to map to 1.0.

I don't think so; 65535 / 255 = 257.

> http://codereview.chromium.org/3198013/show
>

[Peter Kasting, 2010-08-24 22:09:36]

http://codereview.chromium.org/3198013/diff/4001/5001
File chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc (right):

http://codereview.chromium.org/3198013/diff/4001/5001#newcode389
chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc:389: url_text_color_
=
Nit: These lines will fit with less wrapping:

    url_text_color_ = NormalURLColor(content_text_color_);
    url_selected_text_color_ = SelectedURLColor(selected_content_text_color_,
                                                selected_background_color_);

http://codereview.chromium.org/3198013/diff/4001/5002
File chrome/browser/gtk/gtk_util.cc (right):

http://codereview.chromium.org/3198013/diff/4001/5002#newcode906
chrome/browser/gtk/gtk_util.cc:906: void GetTextColors(GdkColor* normal_base,
GdkColor* selected_base,
Nit: One arg per line

http://codereview.chromium.org/3198013/diff/4001/5003
File chrome/browser/gtk/gtk_util.h (right):

http://codereview.chromium.org/3198013/diff/4001/5003#newcode286
chrome/browser/gtk/gtk_util.h:286: void GetTextColors(GdkColor* normal_base,
GdkColor* selected_base,
Nit: One arg per line

http://codereview.chromium.org/3198013/diff/4001/5003#newcode290
chrome/browser/gtk/gtk_util.h:290: SkColor GetSkColorFromGdkColor(const
GdkColor& color);
Use the functions in gfx/skia_utils_gtk.h instead of adding this.  This will
also remove the need to add a header.

http://codereview.chromium.org/3198013/diff/4001/5004
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/3198013/diff/4001/5004#newcode68
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:68: {
Nit: I don't think it gains you anything except deeper indenting to add {} here.

[Daniel Erat, 2010-08-24 22:20:24]

Thanks, I believe that I've applied all of your feedback.

http://codereview.chromium.org/3198013/diff/4001/5001
File chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc (right):

http://codereview.chromium.org/3198013/diff/4001/5001#newcode389
chrome/browser/autocomplete/autocomplete_popup_view_gtk.cc:389: url_text_color_
=
On 2010/08/24 22:09:36, Peter Kasting wrote:
> Nit: These lines will fit with less wrapping:
> 
>     url_text_color_ = NormalURLColor(content_text_color_);
>     url_selected_text_color_ = SelectedURLColor(selected_content_text_color_,
>                                                 selected_background_color_);

Done.

http://codereview.chromium.org/3198013/diff/4001/5003
File chrome/browser/gtk/gtk_util.h (right):

http://codereview.chromium.org/3198013/diff/4001/5003#newcode286
chrome/browser/gtk/gtk_util.h:286: void GetTextColors(GdkColor* normal_base,
GdkColor* selected_base,
On 2010/08/24 22:09:36, Peter Kasting wrote:
> Nit: One arg per line

Done.

http://codereview.chromium.org/3198013/diff/4001/5003#newcode290
chrome/browser/gtk/gtk_util.h:290: SkColor GetSkColorFromGdkColor(const
GdkColor& color);
On 2010/08/24 22:09:36, Peter Kasting wrote:
> Use the functions in gfx/skia_utils_gtk.h instead of adding this.  This will
> also remove the need to add a header.

Done.

http://codereview.chromium.org/3198013/diff/4001/5004
File chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc
(right):

http://codereview.chromium.org/3198013/diff/4001/5004#newcode68
chrome/browser/views/autocomplete/autocomplete_popup_contents_view.cc:68: {
On 2010/08/24 22:09:36, Peter Kasting wrote:
> Nit: I don't think it gains you anything except deeper indenting to add {}
here.

Done.

[Peter Kasting, 2010-08-24 22:42:40]

LGTM