Most of WebExtensions is built upon the windows and tabs api, therefore this is the first we need to implement. As a first iteration, I am exposing all tabs to WebExtensions that have a browser element. This is the start page on the mail tab, or a content page. Newly created tabs will be content pages. The patch I'm attaching still has some flaws that will need followups, but I wanted to get initial support in. Namely: - Moving tabs between windows does not work well - Windows of type popup have the tab bar hidden, but this causes our bindings to fail in various ways when loading new tabs Another big issue, there are no tests. Firefox uses browser chrome tests, which we don't have any of. We could do mozmill tests, but I'm not particularly happy writing extensive tests in a framework that is on its way out. Still, I believe we should land this to allow initial experimentation. I will also request uplifting to beta (needs some slight backports so separate patch) and work on a patch to allow WX experiments in release. I think this will allow us to maximize participation from the community.

I tried your patch together with the add-ons Gradientus, Weatherlicious and Quantum Lights (the last two needed to add a ID manually to get installed) and they worked. \o/ But what I'm seeing is that on start with the add-on enabled the main tab has no title. I need to switch to a other folder to make it visible. Then it works until the next start. Also when I open other tabs and start TB active on a other tab, the main tab text isn't visible. This happens with all three add-ons.

I was wrong in comment 2. The invisible title on the first tab is also the case without a enabled add-on. Checking in Inspector shows me that the tab has title=" ".

Should re-enable the test in bug 1464707 when fixing this. Magnus, how are we doing on the review? Would be great to get this in soon.

Comment on attachment 8969483 [details] [diff] [review] Fix - v1 Review of attachment 8969483 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks good. You've made a lot more progress than I did, so I'm happy I don't have to figure a lot of this stuff out. There's a few changes needed: it looks like nsIDOMXULElement is no longer a thing, and the manifest needs to be included when packaging. I made a try push with those changes: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c796d70985bd7357fab25da1c9d958b2146bf3cf ::: mail/components/extensions/extensions-mail.manifest @@ +1,3 @@ > +category webextension-modules mail chrome://messenger/content/ext-mail.json > + > +category webextension-scripts c-mail chrome://messenger/content/parent/ext-mail.js I think the "c-browser" in the file you copied this from is a typo. I can't find any reference to it or think of any reason for it.

(In reply to Geoff Lankow (:darktrojan) from comment #5) > Comment on attachment 8969483 [details] [diff] [review] > Fix - v1 > > Review of attachment 8969483 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall this looks good. You've made a lot more progress than I did, so I'm > happy I don't have to figure a lot of this stuff out. There's a few changes > needed: it looks like nsIDOMXULElement is no longer a thing, and the > manifest needs to be included when packaging. Yes, this was likely removed, good catch. A bunch of other nsIDOM interfaces are also going away. > > I made a try push with those changes: > https://treeherder.mozilla.org/#/jobs?repo=try-comm- > central&revision=c796d70985bd7357fab25da1c9d958b2146bf3cf Thanks! > > ::: mail/components/extensions/extensions-mail.manifest > @@ +1,3 @@ > > +category webextension-modules mail chrome://messenger/content/ext-mail.json > > + > > +category webextension-scripts c-mail chrome://messenger/content/parent/ext-mail.js > > I think the "c-browser" in the file you copied this from is a typo. I can't > find any reference to it or think of any reason for it. The c-* causes the script to be loaded in e10s child processes.

The other eslint issues are hiding this, but there is an indent issue with shouldSwitchTo, and an unused PrivateBrowsingUtils in ext-windows.js

This is the same patch with fixes I've since made to: mail/base/content/specialTabs.js (whitespace) mail/components/extensions/parent/ext-mail.js (nsIDOMXULElement) mail/components/extensions/parent/ext-windows.js (PrivateBrowsingUtils) mail/installer/package-manifest.in (packaging) I think with these fixed, this should be checked in.

Comment on attachment 8987010 [details] [diff] [review] 1455471-webext-windowstabs-1.diff Review of attachment 8987010 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

(In reply to Philipp Kewisch [:Fallen] from comment #4) > Should re-enable the test in bug 1464707 when fixing this. Yes, we should submit an M-C patch re-enabling a bunch of tests. There's one already here: "Bug 1468667 - [webext] Enable webextensions tests for Thunderbird".

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/95b0887a1895 Implement Thunderbird WebExtensions tabs and windows API. r=darktrojan DONTBUILD

Daily will build this in 14 minutes ;-)

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/7f6264949d3f Backed out changeset 95b0887a1895 for test failures. a=jorgk DONTBUILD

That caused various test failures, see: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=95b0887a1895ab7e8c1b55152a91c347abbd335b&selectedJob=184663799

Test failures in MozMill: folder-display/test-deletion-with-multiple-displays.js folder-display/test-deletion-from-virtual-folders.js So in case you don't know how to run those tests: 1) cd obj* 2) mozmake SOLO_TEST=folder-display/test-deletion-with-multiple-displays.js mozmill-one mozmake SOLO_TEST=folder-display/test-deletion-from-virtual-folders.js mozmill-one I suggest a try run before requesting check-in again :-(

I see two things going on here: At https://hg.mozilla.org/comm-central/rev/7f6264949d3f#l5.83 , NS_ERROR_FAILURE is thrown. This doesn't actually seem to stop the tests passing. According to the docs for nsIWebProgress.idl, this means the listener is already added, but I don't see how it could have been. I haven't yet investigated how commenting out that line affects what the patch is trying to achieve. At https://hg.mozilla.org/comm-central/rev/7f6264949d3f#l5.384 , it seems titleChangeFunc needs to be called. I think we can just move the early return down and forget about why.

(In reply to Geoff Lankow (:darktrojan) from comment #16) > > At https://hg.mozilla.org/comm-central/rev/7f6264949d3f#l5.384 , it seems > titleChangeFunc needs to be called. I think we can just move the early > return down and forget about why. Could this be the cause why the title in tab is empty on startup (comment 2 and 3)?

I'll push this to try and assuming that passes it should be checked in.

Comment on attachment 8988432 [details] [diff] [review] 1455471-webext-windowstabs-2.diff How about uplifting this to TB 62 and TB 60 (where the patch also applies)? I thought the idea was to get WE add-ons like uBlock Origin supported in TB 60 ESR? Has anyone tried that add-on with the patch applied?

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/25adf10ec9f9 Implement Thunderbird WebExtensions tabs and windows API. r=darktrojan

Comment on attachment 8988432 [details] [diff] [review] 1455471-webext-windowstabs-2.diff Review of attachment 8988432 [details] [diff] [review]: ----------------------------------------------------------------- Late to the party, but anyway, some nits here ::: mail/base/content/msgMail3PaneWindow.js @@ +756,2 @@ > for (let i = 0; i < tab.tabs.length; i++) > + if("tabType" in tab.tabs[i]) nit: space before parenthesis ::: mail/base/content/tabmail.xml @@ +867,5 @@ > + let droppedTab = targetTabmail.tabInfo[targetTabmail.tabInfo.length - 1]; > + targetTabmail.moveTabTo(droppedTab, aTargetPosition); > + } > + return aTargetWindow; > + } else { no else after return, please ::: mail/components/extensions/parent/ext-mail.js @@ +69,5 @@ > +} > +global.getTabBrowser = getTabBrowser; > + > +/** > + * Thw window tracker tracks opening and closing Thunderbird windows. Each window has an id, which Typo: The @@ +525,5 @@ > + get tabmail() { > + return this.browser.ownerDocument.getElementById("tabmail"); > + } > + > + /** Returns the frame loader for the tab */ This and other comments in this file lack a period at the end. For oneliner comments, you might also prefer tripple-dash comment style. /// ::: mail/components/extensions/parent/ext-tabs.js @@ +83,5 @@ > +]); > +const restricted = new Set(["url", "favIconUrl", "title"]); > + > +/** > + * An EventManager for the tabs.onUpdated listener . @@ +369,5 @@ > + : windowTracker.getWindow(createProperties.windowId, context); > + let tabmail = window.document.getElementById("tabmail"); > + > + let url; > + if (createProperties.url !== null) { why not just a truthy check? @@ +423,5 @@ > + let nativeTabInfo = getTabOrActive(tabId); > + let browser = getTabBrowser(nativeTabInfo); > + let tabmail = browser.ownerDocument.getElementById("tabmail"); > + > + if (updateProperties.url !== null) { here too @@ +439,5 @@ > + }; > + browser.loadURI(url, options); > + } > + > + if (updateProperties.active !== null) { and here (there's more later, I'll refrain from commenting on those) ::: mail/components/extensions/parent/ext-windows.js @@ +65,5 @@ > + return Promise.resolve(windowManager.convert(window, getInfo)); > + }, > + > + getAll: function(getInfo) { > + let doNotCheckTypes = getInfo === null || getInfo.windowTypes === null; here too similarly, why not just let doNotCheckTypes = !getInfo || !getInfo.windowTypes; @@ +71,5 @@ > + function typeFilter(win) { > + return doNotCheckTypes || getInfo.windowTypes.includes(win.type); > + } > + > + let windows = Array.from(windowManager.getAll(), win => win.convert(getInfo)).filter(typeFilter); IMHO for cases like this, nicer to inline the typeFilter function @@ +130,5 @@ > + let nativeTabInfo = tabTracker.getTab(createData.tabId); > + let tabmail = getTabBrowser(nativeTabInfo).ownerDocument.getElementById("tabmail"); > + let targetType = wantNormalWindow ? null : "popup"; > + window = tabmail.replaceTabWithWindow(nativeTabInfo, targetType)[0]; > + } else if (createData.url !== null) { // eslint-disable-line no-negated-condition eslint seems to agree with me ;)

We can land a follow-up to address these.

Is this correct? -function typeFilter(win) { - return doNotCheckTypes || getInfo.windowTypes.includes(win.type); } - -let windows = Array.from(windowManager.getAll(), win => win.convert(getInfo)).filter(typeFilter); +let windows = Array.from(windowManager.getAll(), win => win.convert(getInfo)) + .filter(win => doNotCheckTypes || getInfo.windowTypes.includes(win.type));

(In reply to Jorg K (GMT+2) from comment #19) > Comment on attachment 8988432 [details] [diff] [review] > 1455471-webext-windowstabs-2.diff > > How about uplifting this to TB 62 and TB 60 (where the patch also applies)? > I thought the idea was to get WE add-ons like uBlock Origin supported in TB > 60 ESR? Has anyone tried that add-on with the patch applied? 60 ESR will need some minor changes to how the EventManager is used, but otherwise it should work fine. Essentially bug 1450388 part 1 in reverse for the uplift. I don't know if uBlock will be supported given it may need more APIs, but I would love to see this patch in 60 so authors can do experiments on the release version (we'd also need to enable wx experiments on release but that is another bug) (In reply to Magnus Melin from comment #21) > > @@ +525,5 @@ > > + get tabmail() { > > + return this.browser.ownerDocument.getElementById("tabmail"); > > + } > > + > > + /** Returns the frame loader for the tab */ > > This and other comments in this file lack a period at the end. > For oneliner comments, you might also prefer tripple-dash comment style. /// I'd prefer /** comments, I think you need jsdoc plugins to support tripple slash. > @@ +369,5 @@ > > + : windowTracker.getWindow(createProperties.windowId, context); > > + let tabmail = window.document.getElementById("tabmail"); > > + > > + let url; > > + if (createProperties.url !== null) { > > why not just a truthy check? This is likely copied from m-c code, not sure what their reasoning was. If there isn't a need I'd keep it for consistency. I'd assume it is something about undefined having a different meaning. > > @@ +71,5 @@ > > + function typeFilter(win) { > > + return doNotCheckTypes || getInfo.windowTypes.includes(win.type); > > + } > > + > > + let windows = Array.from(windowManager.getAll(), win => win.convert(getInfo)).filter(typeFilter); > > IMHO for cases like this, nicer to inline the typeFilter function Probably also copied from m-c. I'd be ok with inlining it if necessary. > > @@ +130,5 @@ > > + let nativeTabInfo = tabTracker.getTab(createData.tabId); > > + let tabmail = getTabBrowser(nativeTabInfo).ownerDocument.getElementById("tabmail"); > > + let targetType = wantNormalWindow ? null : "popup"; > > + window = tabmail.replaceTabWithWindow(nativeTabInfo, targetType)[0]; > > + } else if (createData.url !== null) { // eslint-disable-line no-negated-condition > > eslint seems to agree with me ;) In this case it was less logical to flip the condition. The else case as it stands is the catch-all, which opens about:blank. If you don't negate the condition, you'd have to put that catch-all in an else if (createData.url === null).

Comment on attachment 8988523 [details] [diff] [review] 1455471-follow-up.patch Review of attachment 8988523 [details] [diff] [review]: ----------------------------------------------------------------- I'm okay with this, there's some more things that eslint picked up on but aren't the end of the world. The sooner we have eslint running by on all the things, the better. ::: mail/base/content/msgMail3PaneWindow.js @@ +758,1 @@ > tabmail.openTab(tab.tabs[i].tabType,tab.tabs[i].tabParams); There's a space missing after the comma here. @@ +791,5 @@ > //First get default account > try > { > > + if (initialUri) Please add curly braces here.

Geoff, thanks for the review. Can you make sense of the last line of comment #24: In this case it was less logical to flip the condition. The else case as it stands is the catch-all, which opens about:blank. If you don't negate the condition, you'd have to put that catch-all in an else if (createData.url === null). I just went ahead and changed } else if (createData.url !== null) { to } else if (createData.url) { so is this correct or not?

Yeah, I get it. He's saying it's more logical have those two blocks of code in that order (and it is - we're dealing with what we have, not what we don't have). The linter is complaining because there's a negation in the test, but that's only there because somebody decided to compare with null. (It would probably also complain if you'd switched the blocks and made the test "!createData.url".) You've changed it correctly.

(In reply to Magnus Melin from comment #21) > ::: mail/base/content/tabmail.xml > @@ +867,5 @@ > > + let droppedTab = targetTabmail.tabInfo[targetTabmail.tabInfo.length - 1]; > > + targetTabmail.moveTabTo(droppedTab, aTargetPosition); > > + } > > + return aTargetWindow; > > + } else { > > no else after return, please I re-checked the code here, dropping the else after return does make it a bit more complicated to read imho. Before, we had: if (thing) { ... return resultA; } else { ... return resultB; } now, we have an early return right in the middle of the function (which is not good style aiui): if (thing) { ... return resultA; } ... return resultB; If you want to avoid the else after return, then I'd prefer if we could go with: let targetWindow; ... if (aTargetWindow && aTargetWindow !== "popup") { ... targetWindow = aTargetWindow; } else { ... targetWindow = window.openDialog("chrome://messenger/content/", "_blank", ...); } return targetWindow;

(In reply to Philipp Kewisch [:Fallen] from comment #28) > now, we have an early return right in the middle of the function (which is > not good style aiui): Early returns *is* the preferred coding style. Quoting the Mozilla coding style guide: "Don't put an else right after a return (or a break). Delete the else, it's unnecessary and increases indentation level." Early returns are always in the middle of the function, what else would it mean?

Addressed comments: Missing space, added {}, re-established if{}else{}return. Everyone happy now? ;-)

Just in case: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f827bdea1906072b8291b9c44670f3713517f946

Well regarding the early return, the way you changed it to (let targetWindow...) is even worse than the original I think. Pointless assignment.

OK, back to version 1 :-(

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/1d4bfcd37323 comment and white-space fixes, minor style tweaks. r=darktrojan

Comment on attachment 8988845 [details] [diff] [review] 1455471-follow-up.patch (v1b) Review of attachment 8988845 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the post-push comments, but I think we can't make this change without consequences: ::: mail/components/extensions/parent/ext-tabs.js @@ +475,5 @@ > }, > > async query(queryInfo) { > if (!extension.hasPermission("tabs")) { > + if (queryInfo.url || queryInfo.title) { I don't think we can use this kind of check here, this would let a title 0 through the check. Same for all the other checks. @@ +523,5 @@ > tabIds = [tabIds]; > } > > let destinationWindow = null; > + if (moveProperties.windowId) { Not sure where windowId starts, but this would not allow a window id of 0. ::: mail/components/extensions/parent/ext-windows.js @@ +74,5 @@ > }, > > create: function(createData) { > + let needResize = (createData.left || createData.top || > + createData.width || createData.height); Sizes may very well be 0, this needs to keep !== null. @@ +159,5 @@ > > update: function(windowId, updateInfo) { > + if (updateInfo.state && updateInfo.state != "normal") { > + if (updateInfo.left || updateInfo.top || > + updateInfo.width || updateInfo.height) { Same here

OK, this restores the title and numerical ones you mentioned.

Comment on attachment 8989078 [details] [diff] [review] 1455471-follow-up.patch Review of attachment 8989078 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine from inspection. I didn't check how the code looks afterwards, but it is an improvement. Thanks for the patch!

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/e47ad86f6ca6 Follow-up: Restore some of the null checks removed in previous follow-up. r=philipp

Comment on attachment 8988432 [details] [diff] [review] 1455471-webext-windowstabs-2.diff We will need to ship a TB 62 beta at some stage with working Calendar.

Beta (TB 62): https://hg.mozilla.org/releases/comm-beta/rev/4182dcaada29 https://hg.mozilla.org/releases/comm-beta/rev/93b52182117c https://hg.mozilla.org/releases/comm-beta/rev/86f37ebf3992

> I will also ... work on a patch to allow WX experiments in release. Since this doesn't appear to have happened (please correct me if I'm wrong), I've been asked to backport the tabs API to Thunderbird 60 ESR. This is the result.

ESR60 try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b4a8cc7fdf88f7583d566f93da88b60ab71dc355

Linux opt Z1 test 7 failures are in composition, so I'm assuming they're unrelated. Linux debug Z3 test and Linux64 opt Z3 test fails in test_toggling_modes, looks like bug 1490639 Windows debug Z1 test fails and crashes in test_crashed_plugin_notiifcation_inline, looks like bug 1493807 All the Bs jobs failed, but I take it that's an infra problem?

Don't worry, your try looks good. No need to explain every single failure ;-)

You may know it looks good, but I don't!

Comment on attachment 9014039 [details] [diff] [review] comm-esr60 backport Review of attachment 9014039 [details] [diff] [review]: ----------------------------------------------------------------- Any chance you can give me a diff between c-c and esr60 to review? I tried the bugzilla interdiff but it was throwing errors. Assuming you just debitrotted it and there were no other changes necessary, r+wc below. If there are further changes I'd like to take a quick look. ::: mail/components/extensions/parent/ext-tabs.js @@ +325,5 @@ > + > + onRemoved: new EventManager({ > + context, > + name: "tabs.onRemoved", > + register: fire => { EventManager arguments were different in ESR60, see e.g. https://dxr.mozilla.org/mozilla-esr60/source/browser/components/extensions/ext-tabs.js#138

Also, thank you for doing this! It was on my list but I hadn't gotten around to it.

(In reply to Philipp Kewisch from comment #46) > EventManager arguments were different in ESR60, see e.g. > https://dxr.mozilla.org/mozilla-esr60/source/browser/components/extensions/ext-tabs.js#138 In that case someone needs to fix https://dxr.mozilla.org/comm-central/source/comm/mail/components/extensions/parent/ext-mail.js#184

(In reply to Philipp Kewisch from comment #46) > Any chance you can give me a diff between c-c and esr60 to review? I tried > the bugzilla interdiff but it was throwing errors. Assuming you just > debitrotted it and there were no other changes necessary, r+wc below. If > there are further changes I'd like to take a quick look. Trying to interdiff in Bugzilla probably wouldn't work anyway because I rolled in some subsequent fixes (accidentally including the EventManager changes of course...) but the differences between this patch and trunk are as follows: I didn't include the .eslintignore changes. The context of the messenger.xul hunks changed significantly, so I had to manually patch that file. Two ExtensionUtils APIs moved to ExtensionCommon so there's a minor change to ext-mail.js to move them back.

This shows the ext-mail.js changes mentioned in my previous comment and also the ext-tabs.js changes that you requested.

Oops, forgot ext-windows.js

Comment on attachment 9018404 [details] [diff] [review] Backport to TB 60 - Updated for review comments @Philipp: Neil created an interdiff for you, for the backport. I've looked at the interdiff, and it looks good to me. I haven't tested it myself. But Neil assured me that he tested it with our extension (which uses the API) in TB 60 and it worked fine now, with his latest event changes.

I came here to look at possible uplifts into TB 60.3. (In reply to neil@parkwaycc.co.uk from comment #48) > In that case someone needs to fix > https://dxr.mozilla.org/comm-central/source/comm/mail/components/extensions/ > parent/ext-mail.js#184 Can someone please explain this sentence. Is there something wrong on trunk? Also, there was a tiny regression caused by this, bug 1472494 (see "Depends on" above). Can you get that fix ready for uplift as well or include it here.

Comment on attachment 8988432 [details] [diff] [review] 1455471-webext-windowstabs-2.diff (In reply to Jorg K from comment #54) > (In reply to neil from comment #48) > > In that case someone needs to fix > > https://dxr.mozilla.org/comm-central/source/comm/mail/components/extensions/parent/ext-mail.js#184 > Can someone please explain this sentence. Is there something wrong on trunk? >+global.WindowEventManager = class extends EventManager { >+ constructor(context, name, event, listener) { >+ super(context, name, fire => { ESR60 syntax (wrong for this trunk patch!) >+ super({ >+ context, >+ name: "tabs.onUpdated", >+ register, >+ }); Correct trunk syntax. > Also, there was a tiny regression caused by this, bug 1472494 (see "Depends > on" above). Can you get that fix ready for uplift as well or include it here. It's already included!

Comment on attachment 9018404 [details] [diff] [review] Backport to TB 60 - Updated for review comments Geoff, can you please give this another glance, particularly the interdiff https://bugzilla.mozilla.org/attachment.cgi?oldid=9014039&action=interdiff&newid=9018404&headers=1 to the r+ version. If OK, please cancel r?philipp. I'd like to move this forward.

Comment on attachment 9018404 [details] [diff] [review] Backport to TB 60 - Updated for review comments I'm happy with this.

Comment on attachment 9018404 [details] [diff] [review] Backport to TB 60 - Updated for review comments Let's take in then.

s/in/it/ :-(

TB 60.3: https://hg.mozilla.org/releases/comm-esr60/rev/04059d1aba5da3dc5943770feb23d1bb8e67c7f5

Thanks!

It seems to me that the description of this API - to interact with browser windows could be improved. See: https://thunderbird-webextensions.readthedocs.io/en/latest/windows.html I can't find a way to create a window that is covered by this API. When I open what I think of as windows, the compose or the address book windows onCreate is not fired. I've also had a lot of fun with changeFocus. It is fired when I change focus but the WinID is invalid whether I change focus to the address book, a new or open compose window or the error console. The only valid WinID returned is 3 for a normal window, the main window. Interesting that this, the only valid window has a null title. Blessings and thanks for all the work so far!

Graeme, I think we discussed this on the mailing list. This API here is coming from Firefox and is currently concerned with browser windows and tabs only. Can you please file an enhancement request to make it work with other mail window types as well? We need to pay attention when combining with the tabs API, because e.g. composer windows cannot open webpage tabs.

My concern here is not what it does and doesn't do but on the documentation. I think it needs to be clear what windows it handles on the documentation to avoid more frustrated, giving up addon authors.

That's fair enough. Note that you have a button on top right to edit the document and propose changes. I did that for you: https://github.com/thundernest/webext-docs/pull/2

https://github.com/thundernest/webext-docs/pull/2/commits/4f529a27be0e5318b447ae4c2f3faaf7a24b490a (not yet approved)