1377733 - Add discarded state to tabs.Tab

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P5)

Description

[u462496]

WebExtensions should have a way of knowing whether tabs are lazy or not, so they can be handled without inappropriately instantiating lazy tabs.

Presumably via browser.tabs.query et al.

Comment 2

[u462496]

I'm sorry, I am failing to see how this is a duplicate of bug 1322485.  All I am asking for here is to expose a property in tabs.query to tell whether a tab is lazy or not.  bug 1322485 appears to be an API for suspending tabs.  While bug 1322485 would most likely include what I am asking here, must we wait for the development of a more involved API for something that seems would be fairly simple to implement?  Can we just expose the property in tabs.query in the meanwhile?

Comment 3

[u462496]

I think something like this would do it:

ext-utils.js:

  get status() {
    if (!this.nativeTab.linkedPanel) {
      return "suspended";
    }
    if (this.nativeTab.getAttribute("busy") === "true") {
      return "loading";
    }
    return "complete";
  }

It would also keep from returning "complete" on an unloaded tab which isn't very meaningful.

You could even take it a step further, since it is possible to do a non-lazy restore:

  get status() {
    if (!this.nativeTab.linkedPanel) {
      return "suspended";
    }
    if (this.nativeTab.getAttribute("pending") === "true") {
      return "pending";
    }
    if (this.nativeTab.getAttribute("busy") === "true") {
      return "loading";
    }
    return "complete";
  }

Comment 4

[Tomislav Jovanovic :zombie]

Hi Kevin,

If you are volunteering to do this, I guess it can be done independently from bug 1322485 then, so feel free to reopen it and assign it to yourself.

Comment 5

[u462496]

Attachment: 1377733_tabs_Tab_status_V1.diff

Did not see analogues for promiseBrowserState and promiseTabState in webextension libraries.

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8885568 [details] [diff] [review]
1377733_tabs_Tab_status_V1.diff

Review of attachment 8885568 [details] [diff] [review]:
-----------------------------------------------------------------

(Kris asked me to give feedback here)

This looks as a good start, though there are things that need to be addressed.
I understand Fennec supports suspended tabs as well, so we should implement it at the same time.

Also, would you mind switching to mozreview, as it makes it easier to track changes between patches?

::: browser/components/extensions/ext-utils.js
@@ +535,5 @@
>    }
>  
>    get status() {
> +    if (!this.nativeTab.linkedPanel) {
> +      return "suspended";

These new states need to be added to the json schema.

@@ +538,5 @@
> +    if (!this.nativeTab.linkedPanel) {
> +      return "suspended";
> +    }
> +    if (this.nativeTab.getAttribute("pending") === "true") {
> +      return "pending";

What is a pending state?  Your answer can be in the description of the json schema.. ;)

::: browser/components/extensions/test/browser/browser_ext_tabs_query.js
@@ +39,5 @@
>  
>          tabs.sort((tab1, tab2) => tab1.index - tab2.index);
>  
>          browser.test.assertEq(tabs[0].url, "about:blank", "first tab blank");
>          tabs.shift();

Can you please remove this, so that we align tab indexes inside and outside of the extension, make this code a bit easier to follow.

@@ +51,5 @@
>          browser.test.assertEq(tabs[0].url, "about:robots", "tab 0 url correct");
>          browser.test.assertEq(tabs[1].url, "about:config", "tab 1 url correct");
>  
>          browser.test.assertEq(tabs[0].status, "complete", "tab 0 status correct");
> +        browser.test.assertEq(tabs[1].status, "suspended", "tab 1 status correct");

I don't understand what would make about:config be in the suspended state here (while about:robots is complete)?

Comment 7

[Tomislav Jovanovic :zombie]

And please add me as a reviewer for the next version.

Comment 8

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #6)
> Comment on attachment 8885568 [details] [diff] [review]
> 1377733_tabs_Tab_status_V1.diff
> 
> Review of attachment 8885568 [details] [diff] [review]:
> -----------------------------------------------------------------
> I understand Fennec supports suspended tabs as well, so we should implement
> it at the same time.

I am a part-time helper with limited time to contribute.  Adding another dimension/learning curve to this would not be good right now.  Can we just land this for Desktop and add the Fennec stuff in another bug?  (Also, I was not aware that Fennec now supports lazy-browser tabs.  Can you tell me the bug tracking this?)

> Also, would you mind switching to mozreview, as it makes it easier to track
> changes between patches?

Using mozreview puts a kink in my workflow.  I would prefer to stick with uploading patches in the bug.

> These new states need to be added to the json schema.
> 
> What is a pending state?  Your answer can be in the description of the json
> schema.. ;)

Noted.

> ::: browser/components/extensions/test/browser/browser_ext_tabs_query.js
> @@ +39,5 @@
> >  
> >          tabs.sort((tab1, tab2) => tab1.index - tab2.index);
> >  
> >          browser.test.assertEq(tabs[0].url, "about:blank", "first tab blank");
> >          tabs.shift();
> 
> Can you please remove this, so that we align tab indexes inside and outside
> of the extension, make this code a bit easier to follow.

This was in the original code, so I assumed it something being tested for, but if you don't think it is necessary, sure, I can remove it.

> @@ +51,5 @@
> >          browser.test.assertEq(tabs[0].url, "about:robots", "tab 0 url correct");
> >          browser.test.assertEq(tabs[1].url, "about:config", "tab 1 url correct");
> >  
> >          browser.test.assertEq(tabs[0].status, "complete", "tab 0 status correct");
> > +        browser.test.assertEq(tabs[1].status, "suspended", "tab 1 status correct");
> 
> I don't understand what would make about:config be in the suspended state
> here (while about:robots is complete)?

about:config tab has never been selected so it remains suspended.  about:robots tab was selected earlier.

Comment 9

[u462496]

Attachment: 1377733_tabs_Tab_status_V2.diff

I forgot to thank you for looking at my patch - thank you :-)

Requested changes made.

Comment 10

[Tomislav Jovanovic :zombie]

(In reply to Kevin Jones from comment #8)
> Can we just land this for Desktop and add the Fennec stuff in another bug?
> (Also, I was not aware that Fennec now supports lazy-browser tabs.
> Can you tell me the bug tracking this?)

Sorry, I wasn't exact with my terminology, fennec supports tabs "not-yet-session-restored", and turns out they are using the same attribute "pending" for that [1] (lazy tabs work tracked in bug 1343090).

(also, the terminology seems backward to me, "suspended" sounded more like "not-yest-session-restored", and "pending" sounded like "without-an-initialized-<browser>" -- but that issue might be moot, see review).

1) http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3643-3647


> Using mozreview puts a kink in my workflow.  I would prefer to stick with
> uploading patches in the bug.

Ok, but note that by doing so you put kinks into workflows of everyone else who needs to read your code (including other part-time contributors like me).


> This was in the original code, so I assumed it something being tested for,
> but if you don't think it is necessary, sure, I can remove it.

Yeah, your addition only made it apparent that it's hard to follow, I understand it wasn't your code, thanks for improving it.

Comment 11

[Tomislav Jovanovic :zombie]

(In reply to Kevin Jones from comment #8)
> Can we just land this for Desktop and add the Fennec stuff in another bug?

bug 1387144

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8892946 [details] [diff] [review]
1377733_tabs_Tab_status_V2.diff

Review of attachment 8892946 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good, but we should probably go for compatibility with Chrome.

::: browser/components/extensions/schemas/tabs.json
@@ +69,5 @@
>            "mutedInfo": {"$ref": "MutedInfo", "optional": true, "description": "Current tab muted state and the reason for the last state change."},
>            "url": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL the tab is displaying. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "title": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The title of the tab. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "favIconUrl": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL of the tab's favicon. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission. It may also be an empty string if the tab is loading."},
> +          "status": {"type": "string", "optional": true, "description": "The status of the tab.  Can be: <em>suspended</em> - tab is lazy, <em>pending</em> - tab is no longer lazy but has never been loaded, <em>loading</em> or <em>complete</em>."},

Sorry I didn't check before, but Chrome actually has a separate flag called "discarded" on the Tab object instead of a new status [1].

Also, is there a difference for the extensions developers if a tab is suspended vs pending?  If not, that is implementation detail and we should just expose it as one state (or flag, for Chrome compatibility).

1) https://developer.chrome.com/extensions/tabs#type-Tab

Comment 13

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #12)
> Comment on attachment 8892946 [details] [diff] [review]
> 1377733_tabs_Tab_status_V2.diff
> 
> Review of attachment 8892946 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the speedy review sir.

A few comments and questions so I can continue on...

Some background:  "pending" existed long before lazy tabs, thus meant, pending restore - used as an attribute on a xul:tab.  "suspended" comes from the proposal of the term in bug 1284886.  I know it can get difficult to describe the subtleties between the states with single off-the-cuff terms.  Then Chrome has their own term "discarded" (Ugh - don't get me started on that one - what are we throwing away the tab?).  Nonetheless, Chrome's "discarded" (currently) correlates to Firefox "pending", thus a tab with the `pending` attribute set would be "discarded" in Chrome's terminology.

(Quotes not in order:)
> Also, is there a difference for the extensions developers if a tab is
> suspended vs pending?  If not, that is implementation detail and we should
> just expose it as one state (or flag, for Chrome compatibility).

I don't think it would matter at this point since [1] the `pending` attribute is set on lazy tabs, [2] currently the only way to un-suspend (de-lazify) a tab is to select it, thus removing the `pending` attribute.

> Sorry I didn't check before, but Chrome actually has a separate flag called
> "discarded" on the Tab object instead of a new status [1].

So here is a thought, to keep in line with Chrome, add the `discarded` (shudder) flag for a `tabs.Tab`.  The test would be whether the `pending` attribute is set.

Since AFAICT, a separate "suspended" state would not be necessary, then the work in ext-utils.js would not be necessary and would shift to ext-tabs.js.

At some point, the distinction between a "pending/discarded" tab and a "suspended" tab may disappear; the work of bug 1284886 will make traditional "unloading" of tabs functionally returning them to their lazy state.  Thus there will only be 2 states, lazy, and has-been-loaded.  The only contradiction to any of this is that there still is `browser.sessionstore.restore_tabs_lazily` but AFAIK that is only used for tests.[3]

[3] This is all notwithstanding some future use/feature which would find it useful to have a non-lazy tab which has not yet been loaded - which is theoretically possible.

Comment 14

[u462496]

(In reply to Kevin Jones from comment #13)
> (In reply to Tomislav Jovanovic :zombie from comment #12)
> > Comment on attachment 8892946 [details] [diff] [review]
> > 1377733_tabs_Tab_status_V2.diff
> > 
> > Review of attachment 8892946 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thanks for the speedy review sir.
> 
> A few comments and questions so I can continue on...
> 
> blah blah blah....

I just saw bug 1322485, so maybe this will just end up being a duplicate of that bug.

Comment 15

[Tomislav Jovanovic :zombie]

(In reply to Kevin Jones from comment #13)
> A few comments and questions so I can continue on...

I agree with everything you said (including the dislike for the Chrome nomenclature), so you can continue with:

> Since AFAICT, a separate "suspended" state would not be necessary, then the
> work in ext-utils.js would not be necessary and would shift to ext-tabs.js.



(In reply to Kevin Jones from comment #14)
> I just saw bug 1322485, so maybe this will just end up being a duplicate of that bug.

Err, yes, that's what this bug was marked as at first (see above ;)

Comment 16

[u462496]

Attachment: 1377733_tabs_Tab_discarded_V1.diff

tabs.Tab.discard et al

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8894011 [details] [diff] [review]
1377733_tabs_Tab_discarded_V1.diff

Review of attachment 8894011 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thank you.

You are missing a proper commit message, and I have a few small comments below.

Adding Kris for final review.

::: browser/components/extensions/schemas/tabs.json
@@ +70,5 @@
>            "url": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL the tab is displaying. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "title": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The title of the tab. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "favIconUrl": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL of the tab's favicon. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission. It may also be an empty string if the tab is loading."},
>            "status": {"type": "string", "optional": true, "description": "Either <em>loading</em> or <em>complete</em>."},
> +          "discarded": {"type": "boolean", "optional": true, "description": "Whether the tab is loaded with content."},

This sounds the opposite of what's going on.  Since this is currently only used for tabs pending restore, let's use that as a description for now (we can update it when that changes).

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
@@ +52,5 @@
> +                expectedSequence[i].status,
> +                collectedSequence[i].status,
> +                "check updatedInfo status"
> +              );
> +              if (expectedSequence[i].url || collectedSequence[i].url) {

nit: I think this can be outside the outer `if`, and the `discarded` check below doesn't need to be behind the `else` (basically, everything can be slightly simpler).

::: browser/components/extensions/test/browser/browser_ext_tabs_query.js
@@ +32,2 @@
>  
> +        browser.test.assertTrue(tabs[1].active, "tab 0 active");

please update the assert messages as well to reflect the tab indexes.

Comment 18

[u462496]

Attachment: 1377733_tabs_Tab_discarded_V2.diff

Updated per requested changes.

Comment 19

[Tomislav Jovanovic :zombie]

Comment on attachment 8894150 [details] [diff] [review]
1377733_tabs_Tab_discarded_V2.diff

Review of attachment 8894150 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please push to try and make sure to run the debug android mochitests (which would include tabs tests), since I think that might fail with the current patch?  

(It may still end up easier to add android support at the same time)

Taking back the r for a moment.

::: browser/components/extensions/schemas/tabs.json
@@ +70,5 @@
>            "url": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL the tab is displaying. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "title": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The title of the tab. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "favIconUrl": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL of the tab's favicon. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission. It may also be an empty string if the tab is loading."},
>            "status": {"type": "string", "optional": true, "description": "Either <em>loading</em> or <em>complete</em>."},
> +          "discarded": {"type": "boolean", "optional": true, "description": "Whether the tab is loaded with content."},

you didn't update the description.

::: toolkit/components/extensions/ExtensionTabs.jsm
@@ +488,5 @@
>        highlighted: this.selected,
>        active: this.selected,
>        pinned: this.pinned,
>        status: this.status,
> +      discarded: this.discarded,

Hmm, thinking about this again, this might fail for android _debug_ builds where we check the return values of functions against the schema, and this would be an "unknown" property.

Comment 20

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6673d3d878a921533627fe0cb43203d6f5a8cd5

Comment 21

[u462496]

Anything in the try results that raise any flags for you Tomislav?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6673d3d878a921533627fe0cb43203d6f5a8cd5

Comment 22

[Tomislav Jovanovic :zombie]

Yeah, as expected:

    Error: Type error for result value (Error processing 0: Unexpected property \"discarded\") for tabs.query.

so let's just implement that on android as well, as it should be pretty easy/similar (it's a "pending" flag on the browser, not on the tab [1]), though we can skip the test since I couldn't figure out how to setup a tab in the right state on android.

1) http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3655

Comment 23

[u462496]

Attachment: 1377733_tabs_Tab_discarded_rebased1_V1.diff

Comment 24

[u462496]

Attachment: 1377733_tabs_Tab_discarded_android_V1.diff

Comment 25

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4552e705165b5df9e13dc8d5565273483503883a

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Tomislav Jovanovic :zombie from comment #22)
> so let's just implement that on android as well, as it should be pretty
> easy/similar (it's a "pending" flag on the browser, not on the tab [1]),
> though we can skip the test since I couldn't figure out how to setup a tab
> in the right state on android.

By looking at the Tab prototype (http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/mobile/android/chrome/content/browser.js#3504) it seems that we have two options to create a tab in the right state on android
so that we can test the property also on Android:

- create a new tab with BrowserApp.addTab using "delayLoad: true" in the tab parameters

- call zombify on an existent tab (e.g. one retrieved using BrwserApp.selectedTab or one returned by BrowserApp.addTab), e.g. as when Firefox for Android needs to reduce the memory usage and all the tabs have been "zombified" (http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/mobile/android/chrome/content/MemoryObserver.js#21-31)

Comment 27

[Tomislav Jovanovic :zombie]

Thanks Luca, I actually found this test, but it seems I forgot to click submit in one of my open tabs.. ;)

http://searchfox.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_session_zombification.html#63-64

Comment 28

[u462496]

Attachment: 1377733_tabs_Tab_discarded_rebased1_V2.diff

Fixed description.

Comment 29

[u462496]

Attachment: 1377733_tabs_Tab_discarded_android_V2.diff

Fixed description.

Comment 30

[Tomislav Jovanovic :zombie]

Comment on attachment 8897534 [details] [diff] [review]
1377733_tabs_Tab_discarded_rebased1_V2.diff

Review of attachment 8897534 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-tabs.js
@@ +261,5 @@
>              } else if (event.type == "TabPinned") {
>                needed.push("pinned");
>              } else if (event.type == "TabUnpinned") {
>                needed.push("pinned");
> +            } else if (event.type == "TabBrowserInserted") {

hm, I think this event fires when the tab becomes not-lazy, which is often the same as when it becomes not-pending/not-discarded (starting to load) but it happens in in other cases as well.  Is there an event better correlated to "pending" changing state?

::: browser/components/extensions/schemas/tabs.json
@@ +70,5 @@
>            "url": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL the tab is displaying. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "title": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The title of the tab. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission."},
>            "favIconUrl": {"type": "string", "optional": true, "permissions": ["tabs"], "description": "The URL of the tab's favicon. This property is only present if the extension's manifest includes the <code>\"tabs\"</code> permission. It may also be an empty string if the tab is loading."},
>            "status": {"type": "string", "optional": true, "description": "Either <em>loading</em> or <em>complete</em>."},
> +          "discarded": {"type": "boolean", "optional": true, "description": "Whether the tab is not loaded with content."},

nit: I'm not a native speaker, but this doesn't sound right to me.  How about "True while the tab is not loaded with content."

::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
@@ +22,5 @@
>      background: function() {
>        let pageURL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context_tabs_onUpdated_page.html";
>  
>        let expectedSequence = [
> +        {discarded: false},

Does this mean all tabs, both from session and new ones, effectively start in "discarded" state?

Comment 31

[Tomislav Jovanovic :zombie]

Comment on attachment 8897537 [details] [diff] [review]
1377733_tabs_Tab_discarded_android_V2.diff

Review of attachment 8897537 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but drop the event listener.

::: mobile/android/components/extensions/ext-tabs.js
@@ +199,5 @@
>                  let {BrowserApp} = event.target.ownerGlobal;
>                  nativeTab = BrowserApp.getTabForBrowser(event.originalTarget);
>                  needed.push("audible");
>                  break;
> +              case "TabBrowserInserted": {

This event is browser only, and it looks like android doesn't have a corresponding event.  :/

::: mobile/android/components/extensions/ext-utils.js
@@ +377,5 @@
>      return this.nativeTab.browser;
>    }
>  
> +  get discarded() {
> +    return this.nativeTab.browser.getAttribute("pending") === "true";

nit: this.browser

Comment 32

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #30)
> Comment on attachment 8897534 [details] [diff] [review]
> 1377733_tabs_Tab_discarded_rebased1_V2.diff
> 
> Review of attachment 8897534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/extensions/ext-tabs.js
> @@ +261,5 @@
> >              } else if (event.type == "TabPinned") {
> >                needed.push("pinned");
> >              } else if (event.type == "TabUnpinned") {
> >                needed.push("pinned");
> > +            } else if (event.type == "TabBrowserInserted") {
> 
> hm, I think this event fires when the tab becomes not-lazy, which is often
> the same as when it becomes not-pending/not-discarded (starting to load) but
> it happens in in other cases as well.  Is there an event better correlated
> to "pending" changing state?

I pondered this for quite some time.  There is no event which is directly related to switching "pending" state.  However "TabBrowserInserted" will always always remove the "pending" state.  AFAICT, this is the only initializing action which sets a tab to "pending".  "TabBrowserInserted" appears to be the most suitable event.

At this point, the life-cycle is:

Tabs start out as "pending" (lazy/discarded)
Tab gets selected, browser is instantiated, "TabBrowserInserted" gets fired, pending/discarded state is removed
Tab stays in this state until it closes.

This will be the case until bug 1284886 lands, where a tab can return to its pending/discarded state.  Then we can add "TabSuspend" to the listeners.
 
> nit: I'm not a native speaker, but this doesn't sound right to me.  How
> about "True while the tab is not loaded with content."

Sounds good.

> ::: browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js
> @@ +22,5 @@
> >      background: function() {
> >        let pageURL = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context_tabs_onUpdated_page.html";
> >  
> >        let expectedSequence = [
> > +        {discarded: false},
> 
> Does this mean all tabs, both from session and new ones, effectively start
> in "discarded" state?

That is correct.

Comment 33

[u462496]

Attachment: 1377733_tabs_Tab_discarded_rebased1_V3.diff

Updated per comments

Comment 34

[u462496]

Attachment: 1377733_tabs_Tab_discarded_android_V3.diff

Updated per comments, removed listener.

Comment 35

[Tomislav Jovanovic :zombie]

Comment on attachment 8897990 [details] [diff] [review]
1377733_tabs_Tab_discarded_rebased1_V3.diff

Review of attachment 8897990 [details] [diff] [review]:
-----------------------------------------------------------------

Please add Kris for final review

Comment 36

[Tomislav Jovanovic :zombie]

Comment on attachment 8897991 [details] [diff] [review]
1377733_tabs_Tab_discarded_android_V3.diff

Review of attachment 8897991 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/extensions/ext-utils.js
@@ +373,5 @@
>      return this.nativeTab.playingAudio;
>    }
>  
>    get browser() {
> +    return this.browser;

err, recursion

Comment 37

[u462496]

Attachment: 1377733_tabs_Tab_discarded_android_V4.diff

Comment 38

[u462496]

(In reply to Tomislav Jovanovic :zombie from comment #36)
> Comment on attachment 8897991 [details] [diff] [review]
> 1377733_tabs_Tab_discarded_android_V3.diff
> 
> Review of attachment 8897991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/components/extensions/ext-utils.js
> @@ +373,5 @@
> >      return this.nativeTab.playingAudio;
> >    }
> >  
> >    get browser() {
> > +    return this.browser;
> 
> err, recursion

/me embarrassed

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8897990 [details] [diff] [review]
1377733_tabs_Tab_discarded_rebased1_V3.diff

Review of attachment 8897990 [details] [diff] [review]:
-----------------------------------------------------------------

This mostly looks good, but I want to make sure we don't get onUpdated events for the discarded property when we create non-lazy tabs, and that we have tests for both that and the case where a lazy tab transitions to non-lazy. I believe that the current tests test for the opposite behavior for initially-created non-lazy tabs, and don't test at all for the non-lazification case.

::: browser/components/extensions/ext-browser.js
@@ +555,5 @@
>      return this.nativeTab.linkedBrowser;
>    }
>  
> +  get discarded() {
> +    return this.nativeTab.getAttribute("pending") === "true";

Would be nice to add this as a property to the tab binding rather than checking the attribute everywhere, but I suppose this is fine for now.

::: browser/components/extensions/ext-tabs.js
@@ +262,5 @@
>                needed.push("pinned");
>              } else if (event.type == "TabUnpinned") {
>                needed.push("pinned");
> +            } else if (event.type == "TabBrowserInserted") {
> +              needed.push("discarded");

This should be fine for the normal case, but can we make sure we don't immediately fire an onUpdated event when a non-lazy tab is created? Tab events are fairly expensive, and the extra updated events are likely to confuse people.

Maybe just add a property to the TabBrowserInserted detail saying whether the pending property was set prior to insertion?

Comment 40

[u462496]

(In reply to Kris Maglione [:kmag] from comment #39)
> Comment on attachment 8897990 [details] [diff] [review]
> 1377733_tabs_Tab_discarded_rebased1_V3.diff
> 
> Review of attachment 8897990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This mostly looks good, but I want to make sure we don't get onUpdated
> events for the discarded property when we create non-lazy tabs, and that we
> have tests for both that and the case where a lazy tab transitions to
> non-lazy. I believe that the current tests test for the opposite behavior
> for initially-created non-lazy tabs, and don't test at all for the
> non-lazification case.

Currently, the only non-lazification case is when a tab is removed.  This will be the case until bug 1284886 lands, where a tab can return to its pending/discarded state.  Then we can add "TabSuspend" to the listeners.

But yes, we can add the property to the TabBrowserInserted detail as you suggested to avoid firing on tabs created as non-lazy.

> ::: browser/components/extensions/ext-browser.js
> @@ +555,5 @@
> >      return this.nativeTab.linkedBrowser;
> >    }
> >  
> > +  get discarded() {
> > +    return this.nativeTab.getAttribute("pending") === "true";
> 
> Would be nice to add this as a property to the tab binding rather than
> checking the attribute everywhere, but I suppose this is fine for now.
> 
> ::: browser/components/extensions/ext-tabs.js
> @@ +262,5 @@
> >                needed.push("pinned");
> >              } else if (event.type == "TabUnpinned") {
> >                needed.push("pinned");
> > +            } else if (event.type == "TabBrowserInserted") {
> > +              needed.push("discarded");
> 
> This should be fine for the normal case, but can we make sure we don't
> immediately fire an onUpdated event when a non-lazy tab is created? Tab
> events are fairly expensive, and the extra updated events are likely to
> confuse people.
> 
> Maybe just add a property to the TabBrowserInserted detail saying whether
> the pending property was set prior to insertion?

Thanks Kris.  That sounds good.  Have a thought on what to call it?

Comment 41

[u462496]

(In reply to Kevin Jones from comment #40)
> (In reply to Kris Maglione [:kmag] from comment #39)
> > Comment on attachment 8897990 [details] [diff] [review]
> > 1377733_tabs_Tab_discarded_rebased1_V3.diff
> > 
> > Review of attachment 8897990 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This mostly looks good, but I want to make sure we don't get onUpdated
> > events for the discarded property when we create non-lazy tabs, and that we
> > have tests for both that and the case where a lazy tab transitions to
> > non-lazy. I believe that the current tests test for the opposite behavior
> > for initially-created non-lazy tabs, and don't test at all for the
> > non-lazification case.
> 
> Currently, the only non-lazification case is when a tab is removed.  This
> will be the case until bug 1284886 lands, where a tab can return to its
> pending/discarded state.  Then we can add "TabSuspend" to the listeners.

I am needing to rethink my statements a bit.  I realized that there is a situation where a non-lazy, non-pending tab can be make pending, but not lazy.  An example of this would be creating a non-lazy tab (starts off non-pending) and then restoring the tab via setTabState().  This will result in a non-lazy tab which is pending.  setTabState() is used in tab switching.

Maybe that is what you were referring to in your comment that I wasn't seeing.

So my thinking that lazy == pending is not always true.  This also makes some of my statements in comment 32 not quite correct.

There are no events sent for this AFAIK and I can't think offhand of any events which would correlate to this.  SessionStore:restoreHistory message is sent, but I don't see any mechanism for listening for messages in the windowTracker API which ext-tabs.js is using for detecting events.  However I'm quite unfamiliar with the workings of this area.

Perhaps someone can advise on all of this.

Comment 42

[Jan Zavřel]

Do I understand it correctly that this API will allow develoepr/user to open tabs progressively? Like with this addon -> https://addons.mozilla.org/en-US/firefox/addon/load-tabs-progressively-fixed/?

Comment 43

[Kris Maglione [:kmag]]

Sorry for the delay. I've been busy/out.

(In reply to Kevin Jones from comment #41)
> (In reply to Kevin Jones from comment #40)
> > (In reply to Kris Maglione [:kmag] from comment #39)
> > > This mostly looks good, but I want to make sure we don't get onUpdated
> > > events for the discarded property when we create non-lazy tabs, and that we
> > > have tests for both that and the case where a lazy tab transitions to
> > > non-lazy. I believe that the current tests test for the opposite behavior
> > > for initially-created non-lazy tabs, and don't test at all for the
> > > non-lazification case.
> > 
> > Currently, the only non-lazification case is when a tab is removed.  This
> > will be the case until bug 1284886 lands, where a tab can return to its
> > pending/discarded state.  Then we can add "TabSuspend" to the listeners.
> 
> I am needing to rethink my statements a bit.  I realized that there is a
> situation where a non-lazy, non-pending tab can be make pending, but not
> lazy.  An example of this would be creating a non-lazy tab (starts off
> non-pending) and then restoring the tab via setTabState().  This will result
> in a non-lazy tab which is pending.  setTabState() is used in tab switching.
> 
> Maybe that is what you were referring to in your comment that I wasn't
> seeing.
> 
> So my thinking that lazy == pending is not always true.  This also makes
> some of my statements in comment 32 not quite correct.

I think for these purposes, we should think of lazy and pending as the same
thing. But, ideally, any pending tab should be made lazy.

In any case, my main concern was that we handle the following cases correctly:

1) We create a new non-lazy tab. The onCreated event for that tab has the
correct "discarded" value (false), and we don't get an onUpdated event for the
"discarded" property.

2) We create a lazy tab. That tab starts with the "discarded": true property. When
it's selected, we get an onUpdated event for the "discarded" property changing
to false.

I believe that we don't currently test for either of these cases.

Comment 44

[u462496]

Attachment: 1377733_tabs_Tab_discarded_rebased2_V1.diff

Rebased and updated per discussion.

Comment 45

[u462496]

Attachment: 1377733_tabs_Tab_discarded_android_V5.diff

Updated commit message.

Comment 46

[Kris Maglione [:kmag]]

Comment on attachment 8903157 [details] [diff] [review]
1377733_tabs_Tab_discarded_rebased2_V1.diff

Review of attachment 8903157 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/components/extensions/test/browser/browser_ext_tabs_discarded.js
@@ +12,5 @@
> +
> +    background: async function() {
> +      let onCreatedTabData = [];
> +      let discardedEventData = [];
> +      

Nit: Trailing whitespace. Same below.

@@ +52,5 @@
> +  await extension.startup();
> +
> +  let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com");
> +
> +  let tab2 = BrowserTestUtils.addTab(gBrowser, "about:blank", { createLazyBrowser: true });  

Nit: Trailing whitespace. And please remove whitespace inside curly braces. ESLint requires this.

Comment 47

[u462496]

Attachment: 1377733_tabs_Tab_discarded_rebased2_V2.diff

Fixed eslint errors.

Comment 48

[u462496]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f792d9a3b83ff293f5adf117992cecb1cd3a78ec

Comment 49

[Ryan VanderMeulen [:RyanVM]]

What's the relationship of the two non-obsolete patches to each other here? I can't reason about that from the Try links in the bug. Do they both need to land?

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Aha, the commit messages were misleading since one of the patches is clearly for desktop even though it says Android.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e21f475e05
Add `discarded` property to tabs.Tab on desktop. r=zombie, r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1474bb83406
Add `discarded` property to tabs.Tab on Android. r=zombie, r=kmag

Comment 52

[u462496]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #50)
> Aha, the commit messages were misleading since one of the patches is clearly
> for desktop even though it says Android.

Oh my, you're right.  Sorry for the confusion, and thanks for fixing that.

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e2e21f475e05
https://hg.mozilla.org/mozilla-central/rev/b1474bb83406

Comment 54

[Mike Hommey [:glandium]]

While I can see how this relates to future work on bug 1322485, this, on its own, appears to be unusable. Tabs restored from the sessionstore but not loaded yet are not marked discarded (it's always false).

Comment 55

[Will Bamberg [:wbamberg]]

I've documented this in:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query

Let me know if this covers it.

Comment 56

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #54)
> While I can see how this relates to future work on bug 1322485, this, on its
> own, appears to be unusable. Tabs restored from the sessionstore but not
> loaded yet are not marked discarded (it's always false).

FWIW, this was fixed somewhere between beta 3 and beta 9.

Comment 57

[u462496]

(In reply to Will Bamberg [:wbamberg] from comment #55)
> I've documented this in:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onUpdated
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query
> 
> Let me know if this covers it.

Technically, a discarded tab has more than its content removed; it is one whose browser is in a lazy state.  The way it is worded now puts it neck-and-neck with Chrome.  But someone unfamiliar with lazy-browser technology may not find that very meaningful.  Trying to describe that state in a meaningful yet concise way may be difficult.

Comment 58

[marius.santa]

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.

Comment 59

[u462496]

(In reply to marius.santa from comment #58)
> 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.

Manual testing shouldn't be required on this bug.  I am not familiar with setting the “qe-verify-“ flag.  The trailing hyphen hints that there is something else that goes there.

Comment 60

[Tomislav Jovanovic :zombie]

That's a negative "-" for "qe-verify" flag.

Comment 61

[Will Bamberg [:wbamberg]]

Thanks for looking!

> Technically, a discarded tab has more than its content removed; it is one whose browser is in a lazy state.  The way it is worded now puts it neck-and-neck with Chrome.

I don't really know what to do with this information. Could you elaborate on what "one whose browser is in a lazy state" means, in a way that would be useful for an add-on developer?

Comment 62

[u462496]

(In reply to Will Bamberg [:wbamberg] from comment #61)
> Thanks for looking!
> 
> > Technically, a discarded tab has more than its content removed; it is one whose browser is in a lazy state.  The way it is worded now puts it neck-and-neck with Chrome.
> 
> I don't really know what to do with this information. Could you elaborate on
> what "one whose browser is in a lazy state" means, in a way that would be
> useful for an add-on developer?

Hi Will, sorry for the late reply.  As I said in my original comment, I don't know if there is a concise way to describe this in the context, other than to maybe refer to bug 906076?

It is not critical however, and I think if it were left the way you have it would be fine.

Comment 63

[Tomislav Jovanovic :zombie]

Agreed, as noted above, this distinction is more of an implementation detail, not something extension devs need to know about.

Comment 64

[Will Bamberg [:wbamberg]]

Thanks people! Marking dev-doc-complete then.