1288979 - browserAction.getBadgeBackgroundColor() does not return a ColorArray

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Will Bamberg [:wbamberg]]

The docs specify that browserAction.getBadgeBackgroundColor() passes a ColorArray (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/BrowserAction/ColorArray) into its callback.

But Firefox passes the color using the same format that you used to set it. For example, if you do this:

***

chrome.browserAction.setBadgeBackgroundColor({color: "#FF0000"});

function onGot(color) {
  console.log(color);
}

chrome.browserAction.getBadgeBackgroundColor({}, onGot);

***

...in Chrome you will see:

    [255, 0, 0, 255]

...but in Firefox you will see:

    #FF0000

Comment 1

[Kris Maglione [:kmag]]

This might be a bit tricky to get right...

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1288979: Always convert badge background color to a ColorTuple.

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8777987 [details]
Bug 1288979: Always convert badge background color to a ColorTuple.

https://reviewboard.mozilla.org/r/69370/#review66510

Looks okay to me.  I'm curious about the require() usage but I suspect you have a good reason...

::: browser/components/extensions/ext-browserAction.js:9
(Diff revision 1)
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
>                                    "resource:///modules/CustomizableUI.jsm");
>  
> +XPCOMUtils.defineLazyGetter(this, "colorUtils", () => {
> +  return require("devtools/shared/css-color").colorUtils;

why require() instead of just Cu.import() like right below?

::: browser/components/extensions/ext-browserAction.js:343
(Diff revision 1)
>        setBadgeBackgroundColor: function(details) {
>          let tab = details.tabId !== null ? TabManager.getTab(details.tabId) : null;
> -        BrowserAction.for(extension).setProperty(tab, "badgeBackgroundColor", details.color);
> +        let color = details.color;
> +        if (!Array.isArray(color)) {
> +          let col = colorUtils.colorToRGBA(color);
> +          color = col && [col.r, col.g, col.b, Math.round(col.a * 255)];

under what circumstances is col falsy?  if it's if only when color is null, wouldn't it make more sense to not even call colorToRGBA in that case?

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/69370/#review66510

> why require() instead of just Cu.import() like right below?

Devtools modules aren't JSMs. They need to be loaded using the CommonJS module loader. `event-emitter.js` is an exception, with special hacks to make it work both ways.

> under what circumstances is col falsy?  if it's if only when color is null, wouldn't it make more sense to not even call colorToRGBA in that case?

Only when the string isn't a valid CSS color. We should probably report an error and bail, really, but this way is more compatible with our previous behavior.

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb52e6d9b1894210f8d093b62f3ab840648bdb5
Bug 1288979: Always convert badge background color to a ColorTuple. r=aswan

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2eb52e6d9b18