1390464 - sidebarAction.setPanel option to set sidebar document for a specific window

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Kestrel]

Similar to the current ability to specify a tabId in sidebarAction.setPanel, it would be nice to be able to specify a windowId so that changing the sidebar document in one window does not affect other windows.

Comment 1

[Caitlin Neiman [:caitmuenster]]

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

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/13gmYyN0qCjzV7YAsqGpbeeHr3al0yiWP7ayqKJPLS2w/edit#

Comment 2

[Oriol Brufau [:Oriol]]

I agree with this, but

 1. This should not be limited to the panel. Someone may want to also change the title or the icon per window, or to only open or close the sidebar in a specific window.
 2. If these values can be set for specific windows, one should be also able to get them using getPanel, getTitle, etc.
 3. It's not clear what should happen if both a tabId and a windowId are specified.

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: Bug 1390464 - Add windowId parameter in sidebarAction methods

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

Comment 4

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Needs test, of course, but does this look good?
This allows the windowId parameter in setTitle, getTitle, setIcon, setPanel and getPanel.
Specifying both a tabId and a windowId will reject the promise.

Comment 5

[Oriol Brufau [:Oriol]]

Attachment: test.xpi

This can be used to test.
Open multiple windows with multiple tabs.
Install the add-on in about:debugging.
Click the browserAction icon.
This will set a window-specific panel with the windowId and a tab-specific panel with the tabId.
You can switch tabs to see how it works.
There is also a random number which can be used to know when the sidebar is reloaded.

Comment 6

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227224/diff/1-2/

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Tests of course, but this looks good.

Comment 8

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227224/diff/2-3/

Comment 9

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

https://reviewboard.mozilla.org/r/227222/#review239048

I refactored the tests so that they make more sense. Instead of the various `expect` and `expectDefaults` functions, now it's `expect(tab, window, global, default)`. Inherited values don't have to be repeated everywhere. It would be more reliable if bug 1427306 had been approved, but seems good enough.

::: browser/components/extensions/test/browser/browser_ext_sidebarAction_context.js:564
(Diff revision 3)
> +        },
> +        async expect => {
> +          browser.test.log("Move tab from old window to the new one.");
> +          await browser.tabs.move(tabs[1], {windowId: windows[1], index: -1});
> +          await browser.tabs.update(tabs[1], {active: true});
> +          expect(null, details[2], null, details[0]);

This should be `expect(details[3], details[2], null, details[0])`. Tab data is lost when the tab is moved into another window, but this is another bug not related to `windowId`.

Comment 10

[Oriol Brufau [:Oriol]]

I forgot to assert some failures for bad parameters.

Comment 11

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227224/diff/3-4/

Comment 12

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227224/diff/4-5/

Comment 13

[Oriol Brufau [:Oriol]]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227224/diff/5-6/

Comment 14

[Oriol Brufau [:Oriol]]

Shane, when will you be able to review this? Or do you prefer that bug 1451176 is fixed first?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

https://reviewboard.mozilla.org/r/227224/#review244768

Comment 16

[Oriol Brufau [:Oriol]]

OK, then let's land this first and in bug 1451176 think about the best way to preserve tab-specific data while inheriting from the new window.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/958fff8c5f46
Add windowId parameter in sidebarAction methods r=mixedpuppy

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/958fff8c5f46

Comment 19

[CosminB]

Attachment: Bug1390464.gif

I tested on Firefox 59.0.3 (20180427210249),  Firefox 61.0b3 (20180507191226) and Firefox 62.0a1 (20180503220110) under Win 7 64-bit and Mac OS X 10.13.3 and noticed that:

- On FF61 & FF62 on the active tab, a tabId is displayed in the sidebar and the other opened tabs will have a windowId = 5.
  If I click on the browser_action for any of the tabs that have windowId = 5 displayed, it will change into tabId.

- On FF59 after clicking on the browser_action, the current active tab will display the tabId.
  The other tabs don’t have windowId = 5 and only after you click on the browser_action, the tabId is displayed in the sidebar.

Is this the expected behavior?

Comment 20

[Oriol Brufau [:Oriol]]

Yes, it's expected. It's more clear when you use multiple windows (of the same profile), but there are already automated tests.

Comment 21

[CosminB]

Thanks Oriol for the confirmation!

Comment 22

[Will Bamberg [:wbamberg]]

I've updated MDN to include this new argument:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/getPanel
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/getTitle
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/setPanel
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/setTitle
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/sidebarAction/setIcon

Please let me know if we need anything else though!

Comment 23

[Oriol Brufau [:Oriol]]

I added the definitions of window-specific icon, title and panel. Seems good now.

Comment 24

[Will Bamberg [:wbamberg]]

Thank you, Oriol!