Closed Bug 1288979 Opened 5 years ago Closed 5 years ago

browserAction.getBadgeBackgroundColor() does not return a ColorArray

Categories

(WebExtensions :: General, defect)

defect
Not set
normal

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: wbamberg, Assigned: kmag)

Details

Attachments

(1 file)

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
This might be a bit tricky to get right...
Assignee: nobody → kmaglione+bmo
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?
Attachment #8777987 - Flags: review?(aswan) → review+
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb52e6d9b1894210f8d093b62f3ab840648bdb5
Bug 1288979: Always convert badge background color to a ColorTuple. r=aswan
https://hg.mozilla.org/mozilla-central/rev/2eb52e6d9b18
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.