Oriol Brufau [:Oriol] Assignee
	Description • 7 years ago

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}); // "" :(

	Oriol Brufau [:Oriol] Assignee
	Comment 2 • 7 years ago

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.

	Andrew Swan [:aswan]
	Comment 3 • 7 years ago
mozreview-review

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?

	Oriol Brufau [:Oriol] Assignee
	Comment 4 • 7 years ago

(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?

	Andrew Swan [:aswan]
	Comment 5 • 7 years ago

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).

	Oriol Brufau [:Oriol] Assignee
	Comment 6 • 7 years ago

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.

	Andrew Swan [:aswan]
	Comment 7 • 7 years ago

Right, this would require modifying the schema to have something like

```
"parameters": [
  {
    "name": "details",
    "choices": [
      {"type": "null"}.
      {
        "type": "object",
        "properties": {
          ...
        }
      }
    ]
  }
]
```

	Oriol Brufau [:Oriol] Assignee
	Comment 8 • 7 years ago

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

	Kris Maglione [:kmag]
	Comment 9 • 7 years ago

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.

	Oriol Brufau [:Oriol] Assignee
	Comment 10 • 7 years ago

(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?

	Andrew Swan [:aswan]
	Comment 11 • 7 years ago

(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
```

	Oriol Brufau [:Oriol] Assignee
	Comment 13 • 7 years ago

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.

	Andrew Swan [:aswan]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8931862 [details]
Bug 1419940 - Allow browserAction set* methods to accept a null value.

https://reviewboard.mozilla.org/r/202966/#review211546

thanks!

	Pulsebot
	Comment 15 • 7 years ago

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

	Andreea Pavel [:apavel]
	Comment 16 • 7 years ago
bugherder

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

	CosminB
	Comment 17 • 7 years ago

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!

	Oriol Brufau [:Oriol] Assignee
	Comment 18 • 7 years ago

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".

	CosminB
	Comment 19 • 7 years ago

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.

	Rémy Hubscher (:natim)
	Comment 20 • 7 years ago

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.

	Rémy Hubscher (:natim)
	Comment 21 • 7 years ago

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

	Oriol Brufau [:Oriol] Assignee
	Comment 22 • 7 years ago

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

	Rémy Hubscher (:natim)
	Comment 23 • 7 years ago

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

	Rémy Hubscher (:natim)
	Comment 24 • 7 years ago

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

	Julien Wajsberg [:julienw]
	Comment 25 • 7 years ago

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?

	Oriol Brufau [:Oriol] Assignee
	Comment 26 • 7 years ago

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?

	Rémy Hubscher (:natim)
	Comment 27 • 7 years ago

> 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.

	Rémy Hubscher (:natim)
	Comment 28 • 7 years ago

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

	Oriol Brufau [:Oriol] Assignee
	Comment 29 • 7 years ago

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?

	Rémy Hubscher (:natim)
	Comment 30 • 7 years ago

Yes thanks Oriol :)

	Will Bamberg [:wbamberg]
	Comment 31 • 7 years ago

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

	Oriol Brufau [:Oriol] Assignee
	Comment 32 • 7 years ago

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