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

I want each window to have its own title, badge text, icon, background color or popup.

The methods to set a value currently only allow to set it as a default for all tabs in all windows, or override the default value in a specific tab.

I propose adding a windowId parameter, e.g.

  browser.browserAction.setBadgeText({
    text: myString,
    windowId: myInteger
  })

Then,

 - If there is no windowId nor tabId, the value sets the global default.
 - If there is a windowId but not a tabId, the value sets the window default, which has more precedence than the global default for the tabs in that window.
 - If there is a tabId but not a windowId, the value sets the custom tab value, which has more precedence than the global default and the window default.
 - If there are both a tabId and a windowId, not sure. Maybe check that the tab with that tabId really belongs to the window with windowId, and then behave like in the previous case. If the check fails, throw or do nothing.

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

The reasoning behind this proposal is that if I only set a global default and I update it when the focused window changes, then I won't see the different values if I have two windows side by side.

Then one could think that the tabId is a more general feature, so windowId is not needed. But in fact it's not that nice.
If I open a new tab, I need an onCreated listener in order to assign the proper value to its tabId. But since it's called asynchronously, during an instant the badge or icon will disappear and then reappear.
To avoid this blinking issue, the solution is setting the value both as the default and for the current tabId. The default needs to be updated when focusing another window, but it might still not be enough if you simultaneously focus a new window and open a new tab in it.
But if you set tab-specific values, when you want to change them, you need to update all tabs! This has very horrible performance when there are hundreds of tabs. Another approach is updating at onActivated event, but again the old value might appear for an instant.
Of course, onAttached and onDetached events are also needed, and the wrong value also appears for an instant.
In order to minimize these wrong instants, my event listeners need to be synchronous. This means I need to maintain my own data structures to keep track of which tabs are active, I can't use query.

So really, it would be much better if you allowed windowId.

	Caitlin Neiman [:caitmuenster]
	Comment 2 • 7 years ago

Hi Oriol, this has been added to the agenda for the December 5, 2017 WebExtensions APIs triage meeting. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1vH4wqJJZt1jk-cpx5NOq67b1UNekeQog6bz2HFOHe5E/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

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

I will try to join on irc, but not sure if I will be able to.

	Mark Striemer [:mstriemer]
	Comment 4 • 7 years ago

Hi Oriol,

You mentioned on IRC the use case of a tab counter extension. Is this what you're looking to implement? We had some difficulty coming up with what else adding to this API might enable the creation of in our meeting. Are there any other use cases you can think of?

I did some playing around with the current API and it seems like you can accomplish this without an update to the API, as you suggested, by setting the text for the active tab in each window and the fallback being the count for the active window. It does feel a bit more complicated than it needs to be with all the listeners though.

Do you still think you need this in the API for your use case or some other use cases you can come up with? I'm just not sure what this enables and it seems like it only saves about 20 lines of code if we add it.

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

(In reply to Mark Striemer [:mstriemer] from comment #4)
> You mentioned on IRC the use case of a tab counter extension. Is this what
> you're looking to implement?

Yes, I aim to port a perfect tab counter (https://addons.mozilla.org/firefox/addon/tab-counter/) into a webextension.
I have already found two approaches:

 - https://addons.mozilla.org/firefox/addon/tabcounter-1/ just uses a global browserAction. This ensures the counter will flow nicely, but it won't work if you have multiple windows side by side. It also has some other minor problems like not immediately updating the counter when closing a tab because of bug 1396758.

 - https://addons.mozilla.org/firefox/addon/tab-counter-webext/ uses tab-specific browserActions. When you open a new tab, the counter briefly disappears and displays "wait" (the global badge text value). This is not tolerable.

Using the approach described in comment 1, I have managed to do something better, but I noticed it would be much better with my proposal.

> Are there any other use cases you can think of?

I can perfectly imagine some kind of a script or ad blocker which allows the user to specify different policies for each window, and display the number of blocked items in that window.

Alternatively, maybe some add-on wants tab-specific browserActions but wants them to persist when an existing tab navigates to a new page. Instead of listening to onUpdated and add back the tab-specific browserAction, another approach would be setting window-specific browserActions and update them at onActivated. Depending on the add-on, one approach may be more suitable than the other.

> It does feel a bit more complicated than it needs to be with all the listeners though.

Yes, and in comment 1 I forgot an onUpdated listener is also needed, in case the page navigates.

There is also the problem that I don't like how badges are displayed, so currently I'm using a canvas to draw the counter as an icon. It may not be much important for text badges, but storing an image for each tab wastes memory uselessly when there is a huge amount of tabs.

That said, in bug 1419940 I will allow to clear tab-specific browserAction values. Then, when changing the active tab, it will be possible to delete the counter of the previous one. This would solve this memory problem and would avoid flickering when changing the active tab inside the same window. The downside is that it may add a flickering when changing the active tab of an unfocused window.

> Do you still think you need this in the API for your use case or some other
> use cases you can come up with? I'm just not sure what this enables and it
> seems like it only saves about 20 lines of code if we add it.

It's not absolutely needed. But for sure it simplifies various things, allows a more fluid behavior and requires lots of less event listener, so it has a smaller performance impact. And allowing window-specific browserActions seems a natural addition to the API.

And I believe this would be relatively easy to implement, I may be able to do it myself just by adding few lines of code.

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

I noticed the windowId parameter had been approved for sidebarAction.setPanel in bug 1390464.

	Mike Conca [:mconca]
	Comment 7 • 6 years ago

Allowing this to proceed as a P5 enhancement.  Patches welcome.

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

Great! I will adapt my patch from bug 1390464.

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

The only problem is that the browserAction.enable and browserAction.disable methods accept an integer tabId parameter instead of a details object. Therefore I can't make them target a specific window.

I can do it for browserAction.isEnabled. Seems pointless given the above, but it might be more consistent.

	Shane Caraveo (:mixedpuppy)
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8970501 [details]
Bug 1419893 - Add windowId parameter in browserAction methods

https://reviewboard.mozilla.org/r/234686/#review250400

::: browser/components/extensions/parent/ext-browserAction.js:112
(Diff revision 1)
> -    this.tabContext = new TabContext(tab => Object.create(this.globals),
> -                                     extension);
> +    this.tabContext = new TabContext(target => {
> +      let window = target.ownerGlobal;
> +      if (target === window) {
> +        return Object.create(this.globals);
> +      }
> +      return Object.create(this.tabContext.get(window));
> +    }, extension);

tabContext get/clear now alternately take a nativetab or window, so the original contract is broken.  That's already been done in sidebar.  There's no reason I can see that it must be a nativetab, but lets make sure the code is clear and understandable.

Change "nativeTab" in tabContext.get/clear to something else (suggestions welcome) such as keyObject.  Then add some real doc strings to the tabContext class.

Also verify we are not leaking in tabContext if a window is closed.

::: browser/components/extensions/parent/ext-browserAction.js:722
(Diff revision 1)
> -          browserAction.setProperty(tab, "badgeBackgroundColor", color);
> +          browserAction.setProperty(details, "badgeBackgroundColor", color);
>          },
>  
>          getBadgeBackgroundColor: function(details, callback) {
> -          let tab = getTab(details.tabId);
> -
> +          let color = browserAction.getProperty(details, "badgeBackgroundColor");
> +          return color || [0xd9, 0, 0, 255];

Add a followup bug to get the default color from the theme.  We have a theme meta bug somewhere...maybe block 1330328

::: browser/components/extensions/schemas/browser_action.json:481
(Diff revision 1)
>              "name": "details",
>              "type": "object",
>              "properties": {
>                "tabId": {
>                  "type": "integer",
>                  "optional": true,
> -                "description": "Specify the tab to get the enabledness from. If no tab is specified, the non-tab-specific enabledness is returned."
> +                "description": "Specify the tab to get the enabledness from. If no tab nor window is specified, the global enabledness is returned."
> +              },
> +              "windowId": {
> +                "type": "integer",
> +                "optional": true,
> +                "minimum": -2,
> +                "description": "Specify the window to get the enabledness from. If no tab nor window is specified, the global enabledness is returned."

Add a type and use it for those area's where we repeat the details object that only has tabId/windowId.

::: browser/components/extensions/test/browser/browser_ext_browserAction_context.js:10
(Diff revision 1)
>  async function runTests(options) {
>    async function background(getTests) {
> -    async function checkDetails(expecting, tabId) {
> -      let title = await browser.browserAction.getTitle({tabId});
> +    async function checkDetails(expecting, details) {
> +      let title = await browser.browserAction.getTitle(details);
>        browser.test.assertEq(expecting.title, title,
> -                            "expected value from getTitle");
> +                            "expected value from getTitle in " + JSON.stringify(details));

remove the stringify part of the test strings.

::: browser/components/extensions/test/browser/browser_ext_browserAction_context.js:582
(Diff revision 1)
> +      ];
> +    },
> +  });
> +});
> +
> +add_task(async function testMultipleWindows() {

This test file is getting huge.  Lets break it up, place new stuff into a new file.

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Change "nativeTab" in tabContext.get/clear to something else (suggestions
> welcome) such as keyObject.  Then add some real doc strings to the
> tabContext class.

Done!

> Also verify we are not leaking in tabContext if a window is closed.

The context data is stored in a weakmap, so window-specific data should be removed when the window is closed. Well, tab-specific data references strongly the window-specific data, but if the window is closed, its tabs should be removed, then tab-specific data is destroyed, and no more strong references should remain.

> Add a followup bug to get the default color from the theme.  We have a theme
> meta bug somewhere...maybe block 1330328

I don't really understand this, background badge colors don't seem to depend on the theme. And I don't see the relationship with this bug. Better file the bug yourself.

> Add a type and use it for those area's where we repeat the details object
> that only has tabId/windowId.

Done!

> remove the stringify part of the test strings.

I added this to know if a failure was in the tab, window or global data, but OK.

> This test file is getting huge.  Lets break it up, place new stuff into a
> new file.

But the test needs the runTests function, which is defined in this file. The test that could be moved is the testNavigationClearsData. Should I move that instead?

	Shane Caraveo (:mixedpuppy)
	Comment 14 • 6 years ago

> > Add a followup bug to get the default color from the theme.  We have a theme
> > meta bug somewhere...maybe block 1330328
> 
> I don't really understand this, background badge colors don't seem to depend
> on the theme. And I don't see the relationship with this bug. Better file
> the bug yourself.

Any color/etc should be themeable (not necessarily extension themeable).  When we hard code a color like this we kind of break that capability.  There is a larger set of theme/style guide stuff going on, so having a bug about this will keep track of the issue.   bug 1462416

	Shane Caraveo (:mixedpuppy)
	Comment 15 • 6 years ago
mozreview-review

Comment on attachment 8970501 [details]
Bug 1419893 - Add windowId parameter in browserAction methods

https://reviewboard.mozilla.org/r/234686/#review250824

::: browser/components/extensions/schemas/browser_action.json:80
(Diff revision 2)
> +            "description": "Sets a window-specific value."
> +          }
> +        }
> +      },
> +      {
> +        "id": "DetailsGet",

I think the descriptions can be more generic here to avoid having two types that are otherwise identical.

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

Comment on attachment 8970501 [details]
Bug 1419893 - Add windowId parameter in browserAction methods

Does it look good now? Should I do the same to sidebar_action.json?

Also, about leaking in tabContext if a window is closed, I was assuming that the ChromeWindow object would be destroyed. But it is not (and you can e.g. obtain it using Services.wm.getOuterWindowWithId). This means that the tabContext data won't be deleted when the window is closed.

But this is not a new problem, because the ChromeWindow has strong references to its tabs, so tabContext data for tabs is not deleted neither...

	Shane Caraveo (:mixedpuppy)
	Comment 19 • 6 years ago
mozreview-review

Comment on attachment 8970501 [details]
Bug 1419893 - Add windowId parameter in browserAction methods

https://reviewboard.mozilla.org/r/234686/#review251518

	Pulsebot
	Comment 20 • 6 years ago

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/331be24ce5e6
Add windowId parameter in browserAction methods r=mixedpuppy

	Narcis Beleuzu [:NarcisB]
	Comment 21 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/331be24ce5e6

	Victor Carciu
	Comment 22 • 6 years ago

I verified the issue using https://addons.mozilla.org/ro/firefox/addon/tab-counter-webext/ which was found when reading the bug and the "wait" text is still displayed in Latest Firefox 62 when opening new tabs.

@Oriol: is the webextension updated to use the latest changes? If not, can you please provide an updated testing extension?

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

(In reply to Victor Carciu from comment #22)
> I verified the issue using
> https://addons.mozilla.org/ro/firefox/addon/tab-counter-webext/ which was
> found when reading the bug and the "wait" text is still displayed in Latest
> Firefox 62 when opening new tabs.
> 
> @Oriol: is the webextension updated to use the latest changes? If not, can
> you please provide an updated testing extension?

That extension is not mine so no idea, probably not. Check the attachment instead. I guess I will publish it in AMO after some refinements, works much better.

	Victor Carciu
	Comment 24 • 6 years ago

With the attachment from Comment #23, everything works as expected. Marking the issue as verified.

	Irene Smith
	Comment 25 • 6 years ago

I could use a little guidance on documenting this.

From reading through the bug, it seems that I have to document a new object, the keyObject, and then change the signature of the existing method to use that object instead of tabId. I can't find the information for that object and I'm also not sure exactly how many methods need to be changed.

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

I would explain this like it was done for sidebarAction in bug 1390464, e.g. see https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/sidebarAction/setTitle

The methods that need to be changed are
 - browserAction.setTitle
 - browserAction.getTitle
 - browserAction.setIcon
 - browserAction.setPopup
 - browserAction.getPopup
 - browserAction.setBadgeText
 - browserAction.getBadgeText
 - browserAction.setBadgeBackgroundColor
 - browserAction.getBadgeBackgroundColor
 - browserAction.isEnabled

	Irene Smith
	Comment 27 • 6 years ago

(In reply to Oriol Brufau [:Oriol] from comment #26)
> I would explain this like it was done for sidebarAction in bug 1390464, e.g.
> see
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> sidebarAction/setTitle
> 
> The methods that need to be changed are
>  - browserAction.setTitle
>  - browserAction.getTitle
>  - browserAction.setIcon
>  - browserAction.setPopup
>  - browserAction.getPopup
>  - browserAction.setBadgeText
>  - browserAction.getBadgeText
>  - browserAction.setBadgeBackgroundColor
>  - browserAction.getBadgeBackgroundColor
>  - browserAction.isEnabled

Thanks for replying so quickly. That helps a lot!

	Irene Smith
	Comment 28 • 6 years ago

These pages have been updated:

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/setTitle
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/getTitle
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/setIcon
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/setPopup
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/getPopup
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/setBadgeText
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/getBadgeText
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/setBadgeBackgroundColor
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/getBadgeBackgroundColor
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/isEnabled

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

I did some extra modifications, e.g. it's no longer true that if you omit the tabId you will get or set a global value (this will only happen if you also omit the windowId), so I removed those sentences.

	Irene Smith
	Comment 30 • 6 years ago

Thank you for taking the time to review this so quickly. I really appreciate it!