Closed Bug 1289370 Opened 5 years ago Closed 4 years ago

Lazy-browser-tabs: modify tabbrowser call sites to handle lazy-browser tabs

Categories

(Firefox :: Tabbed Browser, enhancement, P2)

enhancement

Tracking

()

RESOLVED DUPLICATE of bug 1345098

People

(Reporter: u462496, Unassigned)

References

Details

Attachments

(9 files)

Support bug for bug 906076

Before implementing full optimization of lazy-browser tabs in SessionStore, call sites which access tab.linkedBrowser must be prepared so tabs in lazy-browser state are handled properly.

This bug will provide a series of patches to do that.
Blocks: lazytabs
Priority: -- → P2
Attachment #8774678 - Flags: review?(dao+bmo)
Attachment #8774679 - Flags: review?(dao+bmo)
Attachment #8774680 - Flags: review?(dao+bmo)
Attachment #8774681 - Flags: review?(dao+bmo)
Attachment #8774683 - Flags: review?(dao+bmo)
Attachment #8774685 - Flags: review?(dao+bmo)
Comment on attachment 8774678 [details] [diff] [review]
Patch - browser.js

Rather than changing all gBrowser.browsers loops to gBrowser.tabs loops, I think we should add a flag on the browser proxies to indicate that they're not real browsers.
Attachment #8774678 - Flags: review?(dao+bmo) → review-
Attachment #8774686 - Flags: review?(dao+bmo)
Comment on attachment 8774679 [details] [diff] [review]
Patch - browser-addons.js

same here
Attachment #8774679 - Flags: review?(dao+bmo) → review-
Attached patch Patch - devtoolsSplinter Review
Attachment #8774687 - Flags: review?(dao+bmo)
Attachment #8774688 - Flags: review?(dao+bmo)
Comment on attachment 8774680 [details] [diff] [review]
Patch - WebContentConverter.js

Ugh, we should probably make getTabForBrowser actually return the tab.
Attachment #8774680 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 8774678 [details] [diff] [review]
> Patch - browser.js
> 
> Rather than changing all gBrowser.browsers loops to gBrowser.tabs loops, I
> think we should add a flag on the browser proxies to indicate that they're
> not real browsers.

Oops, didn't see your comment before uploading all of these :-/
(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 8774678 [details] [diff] [review]
> Patch - browser.js
> 
> Rather than changing all gBrowser.browsers loops to gBrowser.tabs loops, I
> think we should add a flag on the browser proxies to indicate that they're
> not real browsers.

What about using the return value from getTabForBrowser as a flag instead, like I did in WebContentConverter.js? (and of course returning the tab like you said)
How could getTabForBrowser be used as a flag when it returns the tab as it should? And how would that be better than an explicit flag on the proxy?
(In reply to Dão Gottwald [:dao] from comment #15)
> How could getTabForBrowser be used as a flag when it returns the tab as it
> should? And how would that be better than an explicit flag on the proxy?

getTabForBrowser will return `undefined` if the browser is not real.

Putting a flag on the proxy is tricky, because any property on the browser will force instantiation.  But I suppose we could use a trap in the `linkedBrowser` proxy to ignore it?
(In reply to Allasso Travesser from comment #16)
> (In reply to Dão Gottwald [:dao] from comment #15)
> > How could getTabForBrowser be used as a flag when it returns the tab as it
> > should? And how would that be better than an explicit flag on the proxy?
> 
> getTabForBrowser will return `undefined` if the browser is not real.

It shouldn't. It should return the tab.
(In reply to Dão Gottwald [:dao] from comment #17)
> > getTabForBrowser will return `undefined` if the browser is not real.
> 
> It shouldn't. It should return the tab.

getTabForBrowser returns tabbrowser._tabForBrowser.get(aBrowser).  If a tab is lazy, _tabForBrowser never gets set, because there is no browser to set it with.
I suppose we could move _tabForBrowser.set to addTab and set it with the linkedBrowser proxy?
(In reply to Allasso Travesser from comment #19)
> I suppose we could move _tabForBrowser.set to addTab and set it with the
> linkedBrowser proxy?

Hmmm... not sure if that would work.
Actually I think it will work.

We could move _tabForBrowser.set to addTab and set it with the linkedBrowser proxy.  Then when we iterate over `browsers` we could use getTabForBrowser to retrieve the tab and test for tab.hasBrowser.  Then we wouldn't need to introduce another flag in the `browsers` proxy and deal with the complications of that.

What do you think?
Getting the tab from a browser in order to figure out whether the browser is real is just cumbersome. We should have a better API for that.
Well actually, it wouldn't be that hard I guess.  I don't think we need to do anything in the `browsers` proxy.  We just need a `isRealBrowser` property for the browser, and in the linkedBrowser proxy just return false without instantiating the browser.  When the real browser is created, we set `isRealBrowser` on the browser.

Does that sound okay?  Have a suggestion what to call the flag?
My suggestion would be isProxy.
Attachment #8774681 - Flags: review?(dao+bmo)
Attachment #8774683 - Flags: review?(dao+bmo)
Attachment #8774688 - Flags: review?(dao+bmo)
Attachment #8774685 - Flags: review?(dao+bmo)
Attachment #8774686 - Flags: review?(dao+bmo)
Attachment #8774687 - Flags: review?(dao+bmo)
The strategy of lazy-browsers has changed significantly since this bug was started.  I am closing this bug in favor of bug 1345098.
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1345098
You need to log in before you can comment on or make changes to this bug.