1347190 - Add internal Google Chrome separator color properties

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Mike de Boer [:mikedeboer]]

See (again) https://docs.google.com/spreadsheets/d/1YScpOVL5WdNDhQY2Nngh4YkK0bOpkfwJvpRjpMSxMWo/edit#gid=0

Constants as defined in https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h are:
 * COLOR_TOOLBAR_BOTTOM_SEPARATOR
 * COLOR_TOOLBAR_TOP_SEPARATOR
 * COLOR_TOOLBAR_VERTICAL_SEPARATOR

It's important here to first implement the properties in the order and with names that fit with Firefox well and try to match these up with the Google equivalents above.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1347190 - Add support for toolbar_top/bottom/vertical_separator in the theming API.

Review commit: https://reviewboard.mozilla.org/r/194764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194764/

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8923638 [details]
Bug 1347190 - Add support for toolbar_top/bottom/vertical_separator in the theming API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194764/diff/1-2/

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8923638 [details]
Bug 1347190 - Add support for toolbar_top/bottom/vertical_separator in the theming API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194764/diff/2-3/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8923638 [details]
Bug 1347190 - Add support for toolbar_top/bottom/vertical_separator in the theming API.

https://reviewboard.mozilla.org/r/194764/#review201522

::: toolkit/modules/LightweightThemeConsumer.jsm:23
(Diff revision 3)
>    ["--lwt-background-tiling", "backgroundsTiling"],
>    ["--toolbar-bgcolor", "toolbarColor"],
>    ["--toolbar-color", "toolbar_text"],
>    ["--url-and-searchbar-background-color", "toolbar_field"],
> -  ["--url-and-searchbar-color", "toolbar_field_text"]
> +  ["--url-and-searchbar-color", "toolbar_field_text"],
> +  ["--tabs-border", "toolbar_top_separator"],

As part of this patch, can you please rename `--tabs-border` to `--tabs-border-color`? Right now it is not obvious that this only expects a color value.

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8923638 [details]
Bug 1347190 - Add support for toolbar_top/bottom/vertical_separator in the theming API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194764/diff/3-4/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acede0103156
Add support for toolbar_top/bottom/vertical_separator in the theming API. r=jaws

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/acede0103156

Comment 8

[Victor Carciu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!

Comment 9

[Will Bamberg [:wbamberg]]

I've added these properties and updated the ugly screenshot: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme

I wasn't sure how to get "toolbar_vertical_separator" to show up there though, if you can give me a hint I will update it again.