Do we also want a dedicated method to restore tabs again (use case e.g. https://addons.mozilla.org/en-US/android/addon/tabpreloader/), or just allow e.g. the reload method to handle that case (which might or might not still require some code changes to correctly restore unloaded tabs)?

No, there's a corresponding tab `discarded` property that they should be able to set via `tabs.update` if we want to support that functionality. I'm frankly annoyed that even this method exists, rather than just the ability to set that property via `tabs.update` in the first place. I'm considering just implementing it as a compatibility stub that calls `tabs.update`.

I've got no real experience of that API, so thanks very much for the answer, sounds okay.

Just FYI, since I'm guessing that people will be less familiar with Android: At least for the time being (until somebody gets around to implementing unlinked browsers on Android as well) an unloaded tab on Android can be recognised by looking for tab.browser.__SS_restore and/or tab.browser.getAttribute("pending"). As for the actual act of discarding/restoring a tab, the tab object on Android has "zombify"/"unzombify" methods (https://dxr.mozilla.org/mozilla-central/rev/96e18bec9fc8a5ce623c16167c12756bbe190d73/mobile/android/chrome/content/browser.js#3813) which will do all the necessary work. Also, I've no idea in how far this might be relevant, but zombifying a tab involves destroying the original browser, so any event listeners that were attached directly to that browser become invalid. We've got TabPreZombify/TabPostZombify events so anybody affected can remove and re-add its event listeners again.

I would like that tabs.create() allows to open discarded tab. I should probably open new bug for this, right? (Or does it already exist?)

@kernp25: thank you. I believe bug https://bugzilla.mozilla.org/show_bug.cgi?id=1128502 should be marked as resolved duplicate. Am I right? And can I do it myself?

The same goes for https://bugzilla.mozilla.org/show_bug.cgi?id=973720. Should be marked as resolved duplicate in my view.

Comment on attachment 8911042 [details] [diff] [review] 1322485_tabs.discard_V1.diff Review of attachment 8911042 [details] [diff] [review]: ----------------------------------------------------------------- Any plans for Android? ::: browser/base/content/tabbrowser.xml @@ +2506,5 @@ > tab.removeAttribute("linkedpanel"); > > this._createLazyBrowser(tab); > + > + var evt = new CustomEvent("TabBrowserDiscarded", { bubbles: true, detail: {} }); Remove `detail: {}`. It is not used anywhere. ::: browser/components/extensions/ext-tabs.js @@ +429,5 @@ > nativeTab.ownerGlobal.gBrowser.removeTab(nativeTab); > } > }, > > + async discard(tabs) { Rename the argument to `tabIds` (so that it cannot be confused with an array of Tab objects). @@ +435,5 @@ > + tabs = [tabs]; > + } > + > + for (let tabId of tabs) { > + let nativeTab = tabTracker.getTab(tabId); Use let tabs = tabIds.map(tabId => tabTracket.getTab(tabId)); So that if the tabId is invalid, that the whole call fails, and not that we are in a half-broken state where some of the tabs are discarded, and others aren't. ::: browser/components/extensions/test/browser/browser_ext_tabs_discarded.js @@ +36,5 @@ > + await browser.tabs.discard(tabs[2].id); > + let discardedTab = await browser.tabs.get(tabs[2].id); > + browser.test.assertEq(true, discardedTab.discarded, "discarded tab discard property"); > + browser.test.assertEq(true, discardedEventData[1], "discarded tab onUpdated discard property"); > + Also add a tests for: - an attempt to discard a non-existing tab - an attempt to discard([existingTabId, nonExistingTabId]) and verify that an error is thrown You can use await browser.test.assertRejects(browser.tabs.discard(...), /some expected error message/, "Description of expectation");

(In reply to Rob Wu [:robwu] from comment #15) > Comment on attachment 8911042 [details] [diff] [review] > 1322485_tabs.discard_V1.diff > > Review of attachment 8911042 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for the review... > Any plans for Android? AFAIK, there still isn't lazy tab support for Android. I believe there is a bug for implementing this, but I wasn't able to find it. Since "discard" in Firefox refers to de-lazifying (unlike Chrome), there is currently no support for this bug in Android. IAE, I would rather get this landed for desktop and attack Android in another bug.

(In reply to Kevin Jones from comment #16) > (In reply to Rob Wu [:robwu] from comment #15) > > Comment on attachment 8911042 [details] [diff] [review] > > 1322485_tabs.discard_V1.diff > > > > Review of attachment 8911042 [details] [diff] [review]: > > ----------------------------------------------------------------- > > Thanks for the review... > > > Any plans for Android? > > AFAIK, there still isn't lazy tab support for Android. I believe there is a > bug for implementing this, but I wasn't able to find it. Since "discard" in > Firefox refers to de-lazifying (unlike Chrome), there is currently no > support for this bug in Android. > > IAE, I would rather get this landed for desktop and attack Android in > another bug. Sounds reasonable. I think that the "other bug" is bug 1387144, which is marked as a dependency of this bug. Just don't forget to open a new bug requesting tabs.dicard for Android so that it can be fixed later.

Updated per comments

Comment on attachment 8911184 [details] [diff] [review] 1322485_tabs.discard_V2.diff Review of attachment 8911184 [details] [diff] [review]: ----------------------------------------------------------------- Although I can review the code and give feedback/comments, you still need a r+ from a WebExtension peer* before the patch can be landed (e.g. :mixedpuppy, :aswan, :kmag, :rpl). * https://wiki.mozilla.org/Modules ::: browser/components/extensions/test/browser/browser_ext_tabs_discarded.js @@ +42,5 @@ > + let discardedTab = await browser.tabs.get(tabs[2].id); > + browser.test.assertEq(true, discardedTab.discarded, "discarded tab discard property"); > + browser.test.assertEq(true, discardedEventData[1], "discarded tab onUpdated discard property"); > + > + let invalidTabId = getInvalidTabId(tabs); Let's simplify this to a hard-coded number, e.g. 999999999. @@ +46,5 @@ > + let invalidTabId = getInvalidTabId(tabs); > + await browser.test.assertRejects(browser.tabs.discard(invalidTabId), /Invalid tab ID/, > + "attempt to discard invalid tabId should throw"); > + await browser.test.assertRejects(browser.tabs.discard([invalidTabId, tabs[2].id]), /Invalid tab ID/, > + "attempt to discard a valid and invalid tabId should throw"); Also check that the other tab is still not discarded. And I wonder, what happens if you try to discard a tab that has already been discarded? Naturally, I would expect the call to pass without errors. Could you also add a test for this scenario?

(In reply to Kevin Jones from comment #16) > (In reply to Rob Wu [:robwu] from comment #15) > > Any plans for Android? > > AFAIK, there still isn't lazy tab support for Android. Yes and no. Fennec doesn't support completely <browser>-less tabs like Desktop does since recently, but it still supports unloading tabs ("Zombie" tabs in Fennec terms), which should still achieve most of the resource saving (except we still need an "about:blank" <browser>). The bug about full lazy tab support you're looking for is probably bug 1343090.

Comment on attachment 8911184 [details] [diff] [review] 1322485_tabs.discard_V2.diff (clearing r? flag because I've already provided feedback, and my review doesn't carry any weight towards landing the patch)

Added commit message.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/24d9710162f5 Implement tabs.discard method for Desktop. r=mixedpuppy

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2a13331ac302 for https://treeherder.mozilla.org/logviewer.html#?job_id=137389160&repo=mozilla-inbound

Also, rather a lot of https://treeherder.mozilla.org/logviewer.html#?job_id=137388352&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=137387947&repo=mozilla-inbound

Please don't put checkin-needed on the whiteboard in addition to the keyword. Our bug marking tools won't clear it automatically. Just the keyword is sufficient.

oh ick, I missed the schema problem. Can you do a followup and remove the callback from schema?

(In reply to Shane Caraveo (:mixedpuppy) from comment #28) > oh ick, I missed the schema problem. Can you do a followup and remove the > callback from schema? Oh good, I'm glad you know what's going on there, I didn't know where to start looking on that. I'm still having trouble getting rid of the other test errors, but will certainly do that in the next patch.

Just set async: true. I thought I had put that in review comments...something must have happened.

(In reply to Shane Caraveo (:mixedpuppy) from comment #30) > Just set async: true. I thought I had put that in review > comments...something must have happened. Copy and paste error :$

(In reply to Shane Caraveo (:mixedpuppy) from comment #28) > oh ick, I missed the schema problem. Can you do a followup and remove the > callback from schema? Removing the callback didn't fix https://treeherder.mozilla.org/logviewer.html#?job_id=137389160&repo=mozilla-inbound&lineNumber=2291. We're you thinking it would fix that? IAE, any suggestions on how to fix it?

(In reply to Kevin Jones from comment #32) > (In reply to Shane Caraveo (:mixedpuppy) from comment #28) > > oh ick, I missed the schema problem. Can you do a followup and remove the > > callback from schema? > > Removing the callback didn't fix > https://treeherder.mozilla.org/logviewer.html#?job_id=137389160&repo=mozilla- > inbound&lineNumber=2291. We're you thinking it would fix that? > > IAE, any suggestions on how to fix it? Oh, is that the only issue? You need to add tabs.discard to test_ext_all_apis.html. I keep forgetting about that test, bothersome it is.

Rewrote tests, fixed test_ext_all_apis.html error, rebased.

(In reply to Kevin Jones from comment #35) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e1e44bff8b3adca886a5695311a4e506ae9a2524 Still getting failure on Windows debug: https://treeherder.mozilla.org/logviewer.html#?job_id=138799356&repo=try&lineNumber=18049

(In reply to Kevin Jones from comment #36) > (In reply to Kevin Jones from comment #35) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=e1e44bff8b3adca886a5695311a4e506ae9a2524 > > Still getting failure on Windows debug: > > https://treeherder.mozilla.org/logviewer. > html#?job_id=138799356&repo=try&lineNumber=18049 Shane if you have anything to suggest on this I'm all ears.

My guess is that you are not waiting long enough for the tab to update. You have stuff like this: await browser.tabs.discard(tabs[0].id); let discardedTab = await browser.tabs.get(tabs[0].id); I think there may be a potential race here between the event that is dispatched and getting the tab, which would get aggravated by a debug build. Maybe something like this would work: let expect = new Promise(); browser.tabs.discard(tabs[0].id); let tabId = await expect; browser.test.assertEq(tabId, tabs[0].id); // you know you got updateInfo, no need to test further let discardedTab = await browser.tabs.get(tabs[0].id); browser.test.assertEq(true, discardedTab.discarded, "discarded tab discard property"); // reset expect to a new promise for each test browser.tabs.onUpdated.addListener((tabId, updatedInfo) => { if ("discarded" in updatedInfo) { expect.resolve(tabId); } });

(In reply to Kevin Jones from comment #36) > (In reply to Kevin Jones from comment #35) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=e1e44bff8b3adca886a5695311a4e506ae9a2524 > > Still getting failure on Windows debug: > > https://treeherder.mozilla.org/logviewer. > html#?job_id=138799356&repo=try&lineNumber=18049 Ah, I forgot that remote tabs cannot be discarded. I added `skip-if = !e10s` in browser.ini. I also went ahead and added code to wait for onUpdated event before checking for discarded state, as this seems like a good idea in any event. I did not use the construct you suggested however, as I couldn't get it to work. (AFAICT, there is no way to resolve a promise outside of the promise's callback)

(In reply to Kevin Jones from comment #40) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c8148f162f2ad03bf1412e82bed79a226d4da61d Shane, do you see any failures here that could be a result of this patch?

(In reply to Kevin Jones from comment #41) > (In reply to Kevin Jones from comment #40) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=c8148f162f2ad03bf1412e82bed79a226d4da61d > > Shane, do you see any failures here that could be a result of this patch? None of those failures look related to your changes to me. I'd call that a clean try run.

(In reply to Kevin Jones from comment #39) > (AFAICT, there is no way to resolve a promise outside of the promise's > callback) For future reference, look into PromiseUtils.defer().

Added commit message.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/41b855fe8469 Implement browser.tabs.discard API. r=mixedpuppy

Any chance of uplifting this to 57?

It's not a crash or a regression, so very unlikely at this stage. We've already said no to new APIs in 57 and we'd have to do the same with this one I'm afraid.

(In reply to Andy McKay [:andym] from comment #47) > It's not a crash or a regression, so very unlikely at this stage. We've > already said no to new APIs in 57 and we'd have to do the same with this one > I'm afraid. np :-)

I see the API is "async": true, should it support the callback form from Chrome as well ? https://developer.chrome.com/extensions/tabs#method-discard

(In reply to Tim Nguyen :ntim from comment #50) > I see the API is "async": true, should it support the callback form from > Chrome as well ? > > https://developer.chrome.com/extensions/tabs#method-discard No, we're not using callback for new APIs.

Th(In reply to Shane Caraveo (:mixedpuppy) from comment #51) > (In reply to Tim Nguyen :ntim from comment #50) > > I see the API is "async": true, should it support the callback form from > > Chrome as well ? > > > > https://developer.chrome.com/extensions/tabs#method-discard > > No, we're not using callback for new APIs. Chrome supports tabs.discard with a callback, I think it does make sense for compatibility.

(In reply to Tim Nguyen :ntim from comment #52) > Th(In reply to Shane Caraveo (:mixedpuppy) from comment #51) > > (In reply to Tim Nguyen :ntim from comment #50) > > > I see the API is "async": true, should it support the callback form from > > > Chrome as well ? > > > > > > https://developer.chrome.com/extensions/tabs#method-discard > > > > No, we're not using callback for new APIs. > > Chrome supports tabs.discard with a callback, I think it does make sense for > compatibility. argh. Yeah, for chrome compatibility it should have a callback. Kevin, sorry, I told you to remove that, it bypassed my brain that chrome supports the api.

Also, browser.tabs.discard is not the same as with chrome. In chrome, you can only pass a tab id where in firefox if you pass a tab id it will be converted to an array [1]. Of course, you can pass also an array with tab ids but i think passing just the tab id of one tab should be enough. I'm telling this because, i think it should be compatible with chrome's tabs.discard method. [1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#453

Because, it creates an array just for 1 tab everytime you call browser.tabs.discard. I'm also thinking for performance reasons.

But, i see tabs.remove [1] also does this so you can ignore my 2 comments above. [1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/remove

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

I've written a page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/discard Please let me know if this covers it.

(In reply to Will Bamberg [:wbamberg] from comment #59) > I've written a page on this: > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/discard > > Please let me know if this covers it. Hi Will, Minor details maybe, Non-remote tabs cannot be discarded, however maybe that doesn't even matter any longer. It appears that about: tabs are now out-of-process. Currently tabs with beforeunload handlers which **will show a prompt** cannot be discarded. (Tabs with beforeunload handlers which don't show a prompt can be discarded.) Note that a prompt won't be shown, they will just silently be ignored. Hopefully the option to override this and force discarding will be added, see bug 1420681. Thanks for your work!

(In reply to Kevin Jones from comment #60) > (In reply to Will Bamberg [:wbamberg] from comment #59) > > I've written a page on this: > > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/discard > > > > Please let me know if this covers it. > > Hi Will, > > Minor details maybe, > > Non-remote tabs cannot be discarded, however maybe that doesn't even matter > any longer. It appears that about: tabs are now out-of-process. > > Currently tabs with beforeunload handlers which **will show a prompt** > cannot be discarded. (Tabs with beforeunload handlers which don't show a > prompt can be discarded.) Note that a prompt won't be shown, they will just > silently be ignored. Hopefully the option to override this and force > discarding will be added, see bug 1420681. > > Thanks for your work! Er, these comments apply to Firefox. I don't know about the others, but Google Chrome forces discarding of tabs with blocking beforeunload handlers without warning.

Sorry, perhaps dumb question that I miss: if beforeunload handler fires, can't you just move user over there, than once taken care, switch him back?