1347182 - Add Google Chrome toolbar color properties

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect)

Description

[Mike de Boer [:mikedeboer]]

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

Constant as defined in https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h is:
 * COLOR_TOOLBAR ('toolbar' in the 'colors' section)

I think that the name will be the same in Firefox jargon.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Backed out changeset f26ae19477f5 (bug 1347182) due to too many regressions.

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

Comment 2

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

Comment on attachment 8877969 [details]
Backed out changeset f26ae19477f5 (bug 1347182) due to too many regressions.

https://reviewboard.mozilla.org/r/149372/#review154064

r=me with the following two issues resolved.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js:32
(Diff revision 1)
> +  await extension.startup();
> +
> +  let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +  let toolbars = [...document.getElementsByTagName("toolbar")].filter(toolbar => {

Please also open the findbar since your code sets styling for the findbar and then include the findbar in the list of toolbars that get checked.

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js:37
(Diff revision 1)
> +  await extension.startup();
> +
> +  let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +  let toolbars = [...document.getElementsByTagName("toolbar")].filter(toolbar => {
> +    let bounds = dwu.getBoundsWithoutFlushing(toolbar);
> +    return bounds.width > 0 && !!bounds.height > 0;

!!bounds.height > 0 will become true > 0 or false > 0, which will become 1 > 0 or 0 > 0, respectively.

Why can't you just use bounds.height > 0?

Also, why do we need to use getBoundsWithoutFlushing since this is only being run inside of a test? Flushing layout here will let us avoid erroneous 0 values for height or width.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8877969 [details]
Backed out changeset f26ae19477f5 (bug 1347182) due to too many regressions.

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

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Please also open the findbar since your code sets styling for the findbar
> and then include the findbar in the list of toolbars that get checked.

Good point! Done.

> !!bounds.height > 0 will become true > 0 or false > 0, which will become 1 >
> 0 or 0 > 0, respectively.
> 
> Why can't you just use bounds.height > 0?

That's something that was left-over from a previous iteration... whoops!
 
> Also, why do we need to use getBoundsWithoutFlushing since this is only
> being run inside of a test? Flushing layout here will let us avoid erroneous
> 0 values for height or width.

Now using `getBoundingClientRect()` instead. Good point.

Comment 5

[Matthew Wein [:mattw]]

Comment on attachment 8877969 [details]
Backed out changeset f26ae19477f5 (bug 1347182) due to too many regressions.

https://reviewboard.mozilla.org/r/149372/#review155294

::: toolkit/components/extensions/ext-theme.js:68
(Diff revision 2)
> +      this.logger.warn("Your theme doesn't include one of the following required " +
> +        "properties: 'headerURL', 'accentcolor' or 'textcolor'");

Thanks for adding this :)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f26ae19477f5
Add support for setting the background color of all toolbars using a WebExtension theme. r=jaws,mattw

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

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

Comment on attachment 8877969 [details]
Backed out changeset f26ae19477f5 (bug 1347182) due to too many regressions.

https://reviewboard.mozilla.org/r/149372/#review155628

::: browser/base/content/browser.css:31
(Diff revision 2)
>    background-repeat: var(--lwt-background-tiling) !important;
>  }
>  
> +#navigator-toolbox > toolbar,
> +findbar {
> +  background: var(--lwt-toolbar-color) !important;

Shouldn't this be `background-color` and not `background`? Maybe that will fix some of the regression issues.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8877969 [details]
Backed out changeset f26ae19477f5 (bug 1347182) due to too many regressions.

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

Comment 10

[Mike de Boer [:mikedeboer]]

Backed out in https://hg.mozilla.org/integration/autoland/rev/c235434fbd34

Comment 12

[Dão Gottwald [:dao]]

This doesn't belong in browser/base/content/browser.css. Can you please also ask me for review on the updated patch? Thanks.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Merged backout https://hg.mozilla.org/mozilla-central/rev/c235434fbd34

Comment 14

[Tim Nguyen :ntim]

I'm going to pick this up.

Comment 15

[Tim Nguyen :ntim]

Attachment: Bug 1347182 - Add support for setting the background color of all toolbars using a WebExtension theme.

Original patch by :mikedeboer.

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

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8893500 [details]
Bug 1347182 - Add support for setting the background color of all toolbars using a WebExtension theme.

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

Comment 17

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

Comment on attachment 8893500 [details]
Bug 1347182 - Add support for setting the background color of all toolbars using a WebExtension theme.

https://reviewboard.mozilla.org/r/164594/#review169908

Looks good, thanks!

::: toolkit/components/extensions/test/browser/browser_ext_themes_toolbars.js:33
(Diff revision 1)
> +      "image1.png": BACKGROUND,
> +    },
> +  });
> +
> +  await extension.startup();
> +  

these blank spaces will fail when run through eslint.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/395b4c460551
Add support for setting the background color of all toolbars using a WebExtension theme. r=jaws

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/395b4c460551

Comment 20

[Mike de Boer [:mikedeboer]]

Sweet, thanks Tim!

Comment 21

[marius.santa]

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

Comment 22

[Will Bamberg [:wbamberg]]

Documented at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme.