Gtk3: Add themable location bar border color

Gtk3: Add themable location bar border color

This CL fixes the HighContrastInverse theme on Linux so it looks like:
https://bugs.chromium.org/p/chromium/issues/detail?id=689769#c1

BUG=689769
R=pkasting@chromium.org,estade@chromium.org,erg@chromium.org

Review-Url: https://codereview.chromium.org/2682803003
Cr-Commit-Position: refs/heads/master@{#449227}
Committed: https://chromium.googlesource.com/chromium/src/+/307f9f29d879afce43fe94d03616ef3c7a87971d

[Tom (Use chromium acct), 2017-02-08 01:50:55]

Description was changed from

==========
Gtk3: Add themable location bar border color

BUG=
R=pkasting@chromium.org,estade@chromium.org,erg@chromium.org
==========

to

==========
Gtk3: Add themable location bar border color

This CL fixes the HighContrastInverse theme on Linux so it looks like:
https://bugs.chromium.org/p/chromium/issues/detail?id=689769#c1

BUG=689769
R=pkasting@chromium.org,estade@chromium.org,erg@chromium.org
==========

[Tom (Use chromium acct), 2017-02-08 01:55:27]

reviewers PTAL

pkasting:
  chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
  chrome/browser/ui/views/location_bar/location_bar_view.cc
  chrome/browser/ui/views/location_bar/location_bar_view.h
estade:
  chrome/browser/themes/theme_properties.h
  chrome/browser/themes/theme_service.cc
erg:
  chrome/browser/ui/libgtkui/gtk_ui.cc

[Evan Stade, 2017-02-08 16:31:13]

what does this look like before/after with a more normal gtk theme?

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:93: COLOR_TOOLBAR_ENTRY_BORDER,
this name seems gtk-inspired rather than chrome inspired.

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:442: return SkColorSetA(SK_ColorBLACK,
0x4D);
should we just re-use COLOR_TOOLBAR_TOP_SEPARATOR?

[Tom (Use chromium acct), 2017-02-08 19:28:37]

On 2017/02/08 16:31:13, Evan Stade wrote:
> what does this look like before/after with a more normal gtk theme?
> 
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> File chrome/browser/themes/theme_properties.h (right):
> 
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> chrome/browser/themes/theme_properties.h:93: COLOR_TOOLBAR_ENTRY_BORDER,
> this name seems gtk-inspired rather than chrome inspired.
> 
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> File chrome/browser/themes/theme_service.cc (right):
> 
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> chrome/browser/themes/theme_service.cc:442: return SkColorSetA(SK_ColorBLACK,
> 0x4D);
> should we just re-use COLOR_TOOLBAR_TOP_SEPARATOR?

Please see
https://bugs.chromium.org/p/chromium/issues/detail?id=689769#c2

I've attached images for several themes with a before, after, and
COLOR_TOOLBAR_TOP_SEPARATOR

[Elliot Glaysher, 2017-02-08 20:05:49]

On 2017/02/08 19:28:37, Tom Anderson wrote:
> On 2017/02/08 16:31:13, Evan Stade wrote:
> > what does this look like before/after with a more normal gtk theme?
> > 
> >
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> > File chrome/browser/themes/theme_properties.h (right):
> > 
> >
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> > chrome/browser/themes/theme_properties.h:93: COLOR_TOOLBAR_ENTRY_BORDER,
> > this name seems gtk-inspired rather than chrome inspired.
> > 
> >
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> > File chrome/browser/themes/theme_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
> > chrome/browser/themes/theme_service.cc:442: return
SkColorSetA(SK_ColorBLACK,
> > 0x4D);
> > should we just re-use COLOR_TOOLBAR_TOP_SEPARATOR?
> 
> Please see
> https://bugs.chromium.org/p/chromium/issues/detail?id=689769#c2
> 
> I've attached images for several themes with a before, after, and
> COLOR_TOOLBAR_TOP_SEPARATOR

The usage of TOOLBAR_TOP_SEPARATOR looks too dark in Ambiance and Radiance. I
think it's correct to make a new color for this.

[Tom (Use chromium acct), 2017-02-08 21:20:25]

thomasanderson@google.com changed reviewers:
+ sky@chromium.org
- pkasting@chromium.org

[Tom (Use chromium acct), 2017-02-08 21:20:27]

-pkasting since he's OOO, and +sky for
  chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
  chrome/browser/ui/views/location_bar/location_bar_view.cc
  chrome/browser/ui/views/location_bar/location_bar_view.h

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_properties.h (right):

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_properties.h:93: COLOR_TOOLBAR_ENTRY_BORDER,
On 2017/02/08 16:31:12, Evan Stade wrote:
> this name seems gtk-inspired rather than chrome inspired.

you are correct :)
I renamed it to COLOR_LOCATION_BAR_BORDER

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/2682803003/diff/20001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:442: return SkColorSetA(SK_ColorBLACK,
0x4D);
On 2017/02/08 16:31:12, Evan Stade wrote:
> should we just re-use COLOR_TOOLBAR_TOP_SEPARATOR?

I agree with erg that it's better to add the new color.  It would be hard to
find one color that works well with Ambiance, which has a dark frame but a light
toolbar.

[sky, 2017-02-08 23:25:55]

LGTM

https://codereview.chromium.org/2682803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2682803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:650:
browser_view()->toolbar()->location_bar()->GetOpaqueBorderColor(
optional: this is a bit unwieldy to read, consider creating a local for the
color.

[Elliot Glaysher, 2017-02-08 23:27:38]

gtk_ui lgtm

[Evan Stade, 2017-02-08 23:57:31]

lgtm

[Tom (Use chromium acct), 2017-02-09 05:22:15]

https://codereview.chromium.org/2682803003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/2682803003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:650:
browser_view()->toolbar()->location_bar()->GetOpaqueBorderColor(
On 2017/02/08 23:25:55, sky wrote:
> optional: this is a bit unwieldy to read, consider creating a local for the
> color.

Done.

[Tom (Use chromium acct), 2017-02-09 05:22:19]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-02-09 05:22:20]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
sky@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2682803003/#ps60001
(title: "add location_bar_border_color local")

[commit-bot: I haz the power, 2017-02-09 05:22:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 06:33:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486617739994540,
"parent_rev": "cd3f72e7cc1ad766a8357728a20f2ffdc8359f05", "commit_rev":
"307f9f29d879afce43fe94d03616ef3c7a87971d"}

[commit-bot: I haz the power, 2017-02-09 06:33:52]

Description was changed from

==========
Gtk3: Add themable location bar border color

This CL fixes the HighContrastInverse theme on Linux so it looks like:
https://bugs.chromium.org/p/chromium/issues/detail?id=689769#c1

BUG=689769
R=pkasting@chromium.org,estade@chromium.org,erg@chromium.org
==========

to

==========
Gtk3: Add themable location bar border color

This CL fixes the HighContrastInverse theme on Linux so it looks like:
https://bugs.chromium.org/p/chromium/issues/detail?id=689769#c1

BUG=689769
R=pkasting@chromium.org,estade@chromium.org,erg@chromium.org

Review-Url: https://codereview.chromium.org/2682803003
Cr-Commit-Position: refs/heads/master@{#449227}
Committed:
https://chromium.googlesource.com/chromium/src/+/307f9f29d879afce43fe94d03616...
==========

[commit-bot: I haz the power, 2017-02-09 06:33:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/307f9f29d879afce43fe94d03616...

[Peter Kasting, 2017-02-11 10:18:45]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2017-02-11 10:18:46]

Belated LGTM but make member functions const

https://codereview.chromium.org/2682803003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2682803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:294: SkColor
LocationBarView::GetOpaqueBorderColor(bool incognito) {
Should be const

https://codereview.chromium.org/2682803003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:665: SkColor
LocationBarView::GetBorderColor() {
Should be const