Closed Bug 1390464 Opened 3 years ago Closed 2 years ago

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

Categories

(WebExtensions :: Frontend, enhancement, P5)

enhancement

Tracking

(firefox61 verified, firefox62 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: ke5trel, Assigned: Oriol)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved][sidebar])

Attachments

(3 files)

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.
Blocks: 1329022
Whiteboard: [design-decision-needed]
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#
Whiteboard: [design-decision-needed] → [design-decision-approved]
Severity: normal → enhancement
Priority: -- → P5
Whiteboard: [design-decision-approved] → [design-decision-approved][sidebar]
See Also: → 1419893
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 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.
Attachment #8958294 - Flags: feedback?(mixedpuppy)
Attached file 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.
Assignee: nobody → oriol-bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8958294 - Flags: review?(mixedpuppy)
Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

Tests of course, but this looks good.
Attachment #8958294 - Flags: review?(mixedpuppy) → feedback+
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`.
I forgot to assert some failures for bad parameters.
Shane, when will you be able to review this? Or do you prefer that bug 1451176 is fixed first?
Comment on attachment 8958294 [details]
Bug 1390464 - Add windowId parameter in sidebarAction methods

https://reviewboard.mozilla.org/r/227224/#review244768
Attachment #8958294 - Flags: review?(mixedpuppy) → review+
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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/958fff8c5f46
Add windowId parameter in sidebarAction methods r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/958fff8c5f46
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Keywords: dev-doc-needed
Attached image 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?
Flags: needinfo?(oriol-bugzilla)
Yes, it's expected. It's more clear when you use multiple windows (of the same profile), but there are already automated tests.
Flags: needinfo?(oriol-bugzilla)
Thanks Oriol for the confirmation!
Status: RESOLVED → VERIFIED
I added the definitions of window-specific icon, title and panel. Seems good now.
Flags: needinfo?(oriol-bugzilla)
Thank you, Oriol!
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.