Say I set the default badge to "foo": browser.browserAction.setBadgeText({text: "foo"}); Then I set a tab-specific badge to "bar": browser.browserAction.setBadgeText({text: "bar", tabId}); After some time, I want to clear the tab-specific badge. Sure, I could set it to the default value: browser.browserAction.setBadgeText({text: "foo", tabId}); But now if I change the default value, the tab is not updated: browser.browserAction.setBadgeText({text: "baz"}); await browser.browserAction.getBadgeText({tabId}); // "foo" :( With title this can work by using the empty string: https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/browser/components/extensions/ext-browserAction.js#611-614 browser.browserAction.setTitle({title: "foo"}); browser.browserAction.setTitle({title: "bar", tabId}); browser.browserAction.setTitle({title: "", tabId}); browser.browserAction.setTitle({title: "baz"}); await browser.browserAction.getTitle({tabId}); // "baz" :) But does not work with badges: browser.browserAction.setBadgeText({text: "foo"}); browser.browserAction.setBadgeText({text: "bar", tabId}); browser.browserAction.setBadgeText({text: "", tabId}); browser.browserAction.setBadgeText({text: "baz"}); await browser.browserAction.getBadgeText({tabId}); // "" :(

Comment on attachment 8931862 [details] Bug 1419940 - Allow browserAction set* methods to accept a null value. Another approach could be adding new methods, like clearBadgeText({tabId}), clearIcon({tabid}), clearTitle({tabid}), etc.

Comment on attachment 8931862 [details] Bug 1419940 - Allow browserAction set* methods to accept a null value. https://reviewboard.mozilla.org/r/202966/#review208580 This looks good, thanks! To land it, it should have a test. If you need help with that, please feel free to needinfo me on the bug. ::: browser/components/extensions/ext-browserAction.js:639 (Diff revision 1) > setBadgeText: function(details) { > let tab = getTab(details.tabId); > > - browserAction.setProperty(tab, "badgeText", details.text); > + let text = details.text; > + // Clear the tab-specific badge text when given a null string. > + if (tab && text == "") { Why are you checking `tab` here?

(In reply to Andrew Swan [:aswan] (PTO through 11/27) from comment #3) > Why are you checking `tab` here? I just copied it from https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/browser/components/extensions/ext-browserAction.js#611-614 However, I'm starting to think this change may be problematic, e.g. if someone wants to set a default badge for all tabs, and then remove it in a specific tab. And the patch fixes the inconsistency between titles and badges, but icons and other things are still inconsistent. So I'm becoming more convinced that an API like what I said in comment 2 should be provided: - browserAction.setWhatever with a tab-id should always create a tab-specific value, even if it's empty. This will stop that tab from inheriting the global value. - browserAction.clearWhatever with a tab-id would remove the tab-specific value (if any), so that the tab inherits the global value. - browserAction.setWhatever with no tab-id sets the global value, inherited by tabs without a tab-specific value. - browserAction.clearWhatever with no tab-id would revert the global value to its default initial value. the "Whatever" can be "BadgeText", "BadgeBackgroundColor", "Icon", "Popup" or "Title". What do you think?

I haven't looked closely at these functions but your description of global versus per-tab behavior seems reasonable. Whether or not we need separate clearWhatever methods is a separate question though. All these things should have a way to clear the value, it seems like the intention is that setting to a particular value (eg the empty string) means clear. Perhaps the simplest thing would be to go through all the set* methods and allow them to take null as an argument (since they don't all take strings we can't just use the empty string everywhere).

OK, null seems reasonable. Currently it fails with something like Type error for parameter details (Error processing text: Expected string instead of null) for browserAction.setBadgeText.

Right, this would require modifying the schema to have something like ``` "parameters": [ { "name": "details", "choices": [ {"type": "null"}. { "type": "object", "properties": { ... } } ] } ] ```

Oh, the problem is removing an icon. Should it be setIcon({path: null, tabId}), setIcon({imageData: null, tabId}), or any of them?

Comment on attachment 8931862 [details] Bug 1419940 - Allow browserAction set* methods to accept a null value. The general rule is that a `null` value clears the tab-specific value and falls back to the default value, but an empty string clears the badge for a specific tab, even if by default tabs do have a badge.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #9) > The general rule is that a `null` value clears the tab-specific value and > falls back to the default value, but an empty string clears the badge for a > specific tab, even if by default tabs do have a badge. So basically you agree with my concerns in comment 4 but using null instead of adding clear* methods, as proposed in comment 5. Is that so?

(In reply to Oriol Brufau [:Oriol] from comment #10) > So basically you agree with my concerns in comment 4 but using null instead > of adding clear* methods, as proposed in comment 5. Is that so? I believe that is what Kris was saying. You raised the point in comment 8 though that there are two different ways to set the icon. This is unfortunate but I think it would be fine to just require extensions to unset the one that has been set. That is: ``` let tabId = ...; setIcon({tabId, imageData: ...}); ... setIcon({tabId, path: null}); // this is a no-op since imageData is still set for this tab setIcon({tabId, imageData: null}); // this does remove the per-tab setting ```

Comment on attachment 8931862 [details] Bug 1419940 - Allow browserAction set* methods to accept a null value. With this patch the behavior is as follows: - The global values are no longer the default values. Instead, the global values inherit from the default values, but you can set null to remove a global value and revert to the default value defined in the manifest. - Tab-specific values can now be removed by setting them to null. Then the tab will inherit the global value. - Setting a tab-specific empty string title no longer removes it. It just creates an empty tab-specific title so that no title will be shown for that tab. - For badge background colors, invalid values (including empty string) will continue removing the current value. Maybe it would be better to ignore the invalid color and keep the current one, but that's off-topic here. - For icons, setting each one of imageData and path to one of undefined, null or empty object will remove the current value. I couldn't do what you suggested in comment 11 because when the icon is normalized, the icons specified in path and imageData are merged and nothing keeps track of where hey were defined. An empty string path behaves just like any other string, it will be considered a relative url pointing to the current background script. This will be an invalid image and no icon will be shown, just like if a non-existing path was provided. Maybe it would be better to keep the current icon, but that's off-topic here.

Comment on attachment 8931862 [details] Bug 1419940 - Allow browserAction set* methods to accept a null value. https://reviewboard.mozilla.org/r/202966/#review211546 thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d9fad1593b81 Allow browserAction set* methods to accept a null value. r=aswan

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!

There are automatic tests, but if you want to verify manually, 1. Install the attached add-on 2. The browserAction in the tab where you installed the add-on should show a "good" badge, not "bad".

I can reproduce this issue on Firefox 57.0.2 (20171206182557) under Win 7 64-bit. This issue is verified as fixed on Firefox 59.0a1 (20171221220104) under Win 7 64-bit and Mac OS X 10.13.1. Please see the attached video.

Since that bug landed we can't go back to the default theme icon: https://github.com/mozilla/notes/issues/554 Basically when we change the icon we replace the default theme icon from the manifest (either dark, light, or default) If we want to come back to that icon we are currently clearing the icon. With that patch we now go back to the default "no-icon" add-on icon rather than the default one from the manifest.

And we don't have a getIcon() method to save the previous one.

In bug 1424538 there's already a patch that treats an empty string icon as chrome://browser/content/extension.svg

Yes but then we need a way to handle the "back to default icon theme".

Apparently I should now use `setIcon({path: null})`

For the Gecko Profiler extension, we use `browser.browserAction.setIcon({ path: null })` to revert to the icons specified in manifest.json. I just realized that this patch made it work, but that it didn't before. I guess :natim's way of using `setIcon({})` was the way to go before that?

Ugh, so do `setIcon({})` and `setIcon({path: null})` behave differently now? I think they didn't when I tested. And I'm not sure I understand, https://github.com/mozilla/notes/issues/554 seems to say this bug solved that issue, but comment 20 says this introduced a regression?

> Ugh, so do `setIcon({})` and `setIcon({path: null})` behave differently now? Yes they do before setIcon({}) was doing what setIcon({path: null}) does now and vice versa. > https://github.com/mozilla/notes/issues/554 seems to say this bug solved that issue No, this bug created a regression.

You are right my bad, this bug fixes the issue in 57 and 58.

I can't find any difference in behavior between `setIcon({})` and `setIcon({path: null})`. So, this patch helps Mozilla Notes and Gecko Profiler and everybody is happy, right?

Yes thanks Oriol :)

Oriol updated the docs for this, so marked as dev-doc-complete :).

Should this appear in https://developer.mozilla.org/en-US/Firefox/Releases/59#WebExtensions ?