Implement tabs.discard method for Desktop

RESOLVED FIXED in Firefox 58

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: kernp25, Assigned: u462496)

Tracking

(Blocks 3 bugs, {dev-doc-needed})

unspecified
mozilla58
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Depends on: 1284886
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Ever confirmed: true
Whiteboard: [tabs]
Summary: add chrome.tabs.discard method → Implement tabs.discard method

Updated

2 years ago
Priority: -- → P3
Whiteboard: [tabs] → [tabs] triaged
Duplicate of this bug: 1325484

Updated

2 years ago
webextensions: --- → +

Updated

2 years ago
Duplicate of this bug: 1270784
Duplicate of this bug: 1333047

Updated

2 years ago
See Also: → 1333046
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.

Updated

2 years ago
Assignee: nobody → bob.silverberg

Updated

2 years ago
See Also: → 1364610
Blocks: 1366759
No longer blocks: 1366759
Duplicate of this bug: 1367192
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.
Duplicate of this bug: 1377733
Depends on: 1387144

Comment 10

2 years ago
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?)

Comment 12

2 years ago
@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?

Comment 13

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

Comment 14

2 years ago
Posted patch 1322485_tabs.discard_V1.diff (obsolete) — Splinter Review
Attachment #8911042 - Flags: review?(kmaglione+bmo)

Comment 15

2 years ago
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");
(Assignee)

Comment 16

2 years ago
(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.

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
Posted patch 1322485_tabs.discard_V2.diff (obsolete) — Splinter Review
Updated per comments
Assignee: bob.silverberg → kevinhowjones
Attachment #8911042 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8911042 - Flags: review?(kmaglione+bmo)
Attachment #8911184 - Flags: review?(rob)
(Assignee)

Updated

2 years ago
No longer depends on: 1387144
Summary: Implement tabs.discard method → Implement tabs.discard method for Desktop

Comment 19

2 years ago
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 21

2 years ago
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)
Attachment #8911184 - Flags: review?(rob)
(Assignee)

Comment 22

2 years ago
Posted patch 1322485_tabs.discard_V3.diff (obsolete) — Splinter Review
Attachment #8911184 - Attachment is obsolete: true
Attachment #8918797 - Flags: review?(mixedpuppy)
Attachment #8918797 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 23

2 years ago
Posted patch 1322485_tabs.discard_V3.diff (obsolete) — Splinter Review
Added commit message.
Attachment #8918797 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Whiteboard: [tabs] triaged → [checkin-needed]

Comment 24

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d9710162f5
Implement tabs.discard method for Desktop. r=mixedpuppy
Keywords: checkin-needed
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.
Whiteboard: [checkin-needed]
Depends on: 1409411
No longer depends on: 1409411
oh ick, I missed the schema problem.  Can you do a followup and remove the callback from schema?
Flags: needinfo?(kevinhowjones)
(Assignee)

Comment 29

2 years ago
(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.
Flags: needinfo?(kevinhowjones)
Just set async: true.  I thought I had put that in review comments...something must have happened.
(Assignee)

Comment 31

2 years ago
(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 :$
(Assignee)

Comment 32

2 years ago
(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.
(Assignee)

Comment 34

2 years ago
Posted patch 1322485_tabs.discard_V7.diff (obsolete) — Splinter Review
Rewrote tests, fixed test_ext_all_apis.html error, rebased.
Attachment #8919008 - Attachment is obsolete: true
Attachment #8920934 - Flags: review?(mixedpuppy)
(Assignee)

Comment 36

2 years ago
(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
(Assignee)

Updated

2 years ago
Attachment #8920934 - Flags: review?(mixedpuppy)
(Assignee)

Comment 37

2 years ago
(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.
Flags: needinfo?(mixedpuppy)
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);
  }
});
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 39

2 years ago
Posted patch 1322485_tabs.discard_V9.diff (obsolete) — Splinter Review
(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)
Attachment #8920934 - Attachment is obsolete: true
Attachment #8923370 - Flags: review?(mixedpuppy)
(Assignee)

Comment 41

2 years ago
(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?
Flags: needinfo?(mixedpuppy)
(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().
Flags: needinfo?(mixedpuppy)
Attachment #8923370 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 44

2 years ago
Added commit message.
Attachment #8923370 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 45

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b855fe8469
Implement browser.tabs.discard API. r=mixedpuppy
Keywords: checkin-needed
(Assignee)

Comment 46

2 years ago
Any chance of uplifting this to 57?
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(amckay)

Comment 47

2 years ago
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.
Flags: needinfo?(amckay)
(Assignee)

Comment 48

2 years ago
(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 :-)
Flags: needinfo?(mixedpuppy)

Comment 49

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/41b855fe8469
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Comment 50

2 years ago
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
Flags: needinfo?(mixedpuppy)
(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.
Flags: needinfo?(mixedpuppy)

Comment 52

2 years ago
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.
Flags: needinfo?(kevinhowjones)
(Reporter)

Comment 54

2 years ago
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
(Reporter)

Comment 55

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

Updated

2 years ago
Flags: needinfo?(kevinhowjones)
(Reporter)

Comment 56

2 years ago
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

Comment 57

a year ago
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.
Flags: needinfo?(kevinhowjones)
Flags: needinfo?(kevinhowjones) → qe-verify-

Updated

a year ago
Duplicate of this bug: 1128502
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.
Flags: needinfo?(kevinhowjones)
(Assignee)

Comment 60

a year ago
(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!
Flags: needinfo?(kevinhowjones)

Updated

a year ago
Depends on: 1421479
(Assignee)

Comment 61

a year ago
(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.

Comment 62

a year ago
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?

Updated

a year ago
Depends on: 1422588

Updated

a year ago

Updated

a year ago
Depends on: 1451799

Updated

a year ago
No longer depends on: 1451799

Updated

11 months ago

Updated

11 months ago
No longer blocks: Session_managers
Blocks: 1466904

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.