This is a support bug for bug 906076, lazy-browser-tabs, which deals with tabbrowser tabs in order to provide startup optimization and resource conservation. This bug is the first phase in creating the framework for the lazy-browser-tab, in tabbrowser.xml. This bug splits the code currently in `addTab` which links the xul:browser and other elements into a separate internal method, `_linkBrowserToTab`. `addTab` will now only contain the minimal code to create the xul:tab; `_linkBrowserToTab` will link the xul:browser and related structure to the tab in a separate action. See https://bugzilla.mozilla.org/show_bug.cgi?id=906076#c205 for more details.

David, I sent you an email

David, I'm unable to push to reviewboard right now due to problems on the site. I would have to be L3. The other option is to upload my patch to the bug. When asking how long this would be, I was told "quite a while".

In that case, let's start with Bugzilla until Reviewboard is fixed.

Comment on attachment 8714151 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_01.diff Review of attachment 8714151 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but then, I have read that code 20 times already. Let's ask jaws what he thinks of it with fresh eyes.

Comment on attachment 8714151 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_01.diff Review of attachment 8714151 [details] [diff] [review]: ----------------------------------------------------------------- For this patch, it would have been easier to review if you kept the renaming changes (var -> let, t -> tab, b -> browser) and style fixes (curly brackets around single-line conditional blocks) to a separate patch. ::: browser/base/content/tabbrowser.xml @@ +1749,5 @@ > + "use strict"; > + > + // This is called from addTab linkedBrowser proxy, to lazily > + // instantiate the browser and related elements and link to > + // the tab. A property hasLinkedBrowser will be set on the tab. Please define this property on the binding. @@ +1762,5 @@ > + let fromExternal = aParams.fromExternal; > + let allowMixedContent = aParams.allowMixedContent; > + let forceNotRemote = aParams.forceNotRemote; > + let noReferrer = aParams.noReferrer; > + let userContextId = aParams.userContextId; I'm indifferent, but this could be written as: let { referrerURI, referrerPolicy, charset, postData, ... } = aParams; The plus side is not having to duplicate the property name on the right-hand side.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6) > Comment on attachment 8714151 [details] [diff] [review] > bug_1243707_lazy_browser_tabs_framework_01.diff > ::: browser/base/content/tabbrowser.xml > @@ +1749,5 @@ > > + "use strict"; > > + > > + // This is called from addTab linkedBrowser proxy, to lazily > > + // instantiate the browser and related elements and link to > > + // the tab. A property hasLinkedBrowser will be set on the tab. > > Please define this property on the binding. I don't understand what you are asking here... (Though I can see that the comment is worded prematurely, since there is no linkedBrowser proxy yet.)

I was asking to define the 'hasLinkedBrowser' property on the binding. The property is set a bit lower in the patch. It is premature since it will always be true, but it should be defined since it is already being set.

Made changes as requested.

Why does this patch set hasLinkedBrowser at all? It's not used anywhere else. This seems like it should be added in a later patch.

Removed `hasLinkedBrowser` declaration.

I ran the full suite of mochitest on Mac OS, I only get one error that traces through code in the patch. However, I get the same error when running mochitest without the patch. Error: TEST-START | toolkit/components/thumbnails/test/browser_thumbnails_bug726727.js ... 77961 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x804b0013 (NS_ERROR_PORT_ACCESS_NOT_ALLOWED) [nsIWebNavigation.loadURIWithOptions]" nsresult: "0x804b0013 (NS_ERROR_PORT_ACCESS_NOT_ALLOWED)" location: "JS frame :: chrome://browser/content/browser.js :: _loadURIWithFlags :: line 862" data: no]" {file: "chrome://browser/content/tabbrowser.xml" line: 1859}] ...

Comment on attachment 8716691 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_03.diff I don't see a bug on file for the test failure that you're seeing, but if you get the same failure without the patch then I think it may not be related. Did you try sending your patch to tryserver and seeing if it fails there?

Comment on attachment 8716691 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_03.diff Please avoid changing t to tab and the bracing style in addTab. This is critical code and I think having relevant, less deeply buried annotations for the affected lines outweighs code style aspects in this case.

Made changes requested by Dao. Also minimized format changes in code transferred from `addTab` to `_linkBrowserToTab`.

Comment on attachment 8718308 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_04.diff >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >@@ -1739,8 +1739,140 @@ > ]]> > </body> > </method> > >+ <method name="_linkBrowserToTab"> >+ <parameter name="aTab"/> >+ <parameter name="aURI"/> >+ <parameter name="aParams"/> >+ <body> >+ <![CDATA[ >+ "use strict"; >+ >+ const NS_XUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; NS_XUL doesn't seem to be used here. Please remove it. Feel free to rename b to browser in this method. Since you're moving the code, this affects line annotations either way.

Comment on attachment 8718314 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_05.diff >+ // No preloaded browser found, create one. >+ browser = this._createBrowser({remote: remote, >+ uriIsAboutBlank: uriIsAboutBlank, >+ userContextId: aUserContextId}); nit: indentation is off >+ // wire up a progress listener for the new browser object. >+ let tabListener = this.mTabProgressListener(aTab, browser, uriIsAboutBlank, usingPreloadedContent); >+ const filter = Components.classes["@mozilla.org/appshell/component/browser-status-filter;1"] >+ .createInstance(Components.interfaces.nsIWebProgress); >+ filter.addProgressListener(tabListener, Components.interfaces.nsIWebProgress.NOTIFY_ALL); >+ browser.webProgress.addProgressListener(filter, Components.interfaces.nsIWebProgress.NOTIFY_ALL); Oh, and while you're at it, you could use Ci and Cc instead of Components.interfaces and Components.classes. >+ browser.loadURIWithFlags(aURI, { >+ flags: flags, >+ referrerURI: aNoReferrer ? null: aReferrerURI, >+ referrerPolicy: aReferrerPolicy, >+ charset: aCharset, >+ postData: aPostData, >+ }); indentation is off again

FYI, bug 1247920 overlaps slightly with your patch, so this will need an update.

Comment on attachment 8718856 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_06.diff To take care of comment 19, you'll have to create the patch on fx-team tip or wait till that other patch is merged from fx-team to mozilla-central, or apply it locally before that.

Updated patch per comment 21

Same as previous patch, but used -U 4 to be consistent with previous patches and not to break interdiff.

Sorry, interdiff doesn't work on this patch, I suppose because of the updates in the repos.

Comment on attachment 8719635 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_08.diff >+ // Note: Only do this of we still have a docShell. The TabOpen >+ // event was dispatched above and a gBrowser.removeTab() call from >+ // one of its listeners could cause us to fail here. >+ if (!remote && browser.docShell) { >+ this._outerWindowIDBrowserMap.set(browser.outerWindowID, browser); >+ } This is now obsolete as well, TabOpen is dispatched after this code runs. But I guess bug 906076 would change this again?

AFAIK, 906076 shouldn't change this, as far as `TabOpen` event goes. So I assume that we just need to remove the test for browser.docShell here? (and the comment)

Per comment 25

(In reply to Allasso Travesser from comment #26) > AFAIK, 906076 shouldn't change this, as far as `TabOpen` event goes. I don't get it. Bug 906076 would move the _linkBrowserToTab call to some later point, after TabOpen, right?

(In reply to Dão Gottwald [:dao] from comment #28) > (In reply to Allasso Travesser from comment #26) > > AFAIK, 906076 shouldn't change this, as far as `TabOpen` event goes. > > I don't get it. Bug 906076 would move the _linkBrowserToTab call to some > later point, after TabOpen, right? Sorry, yes that is true, I interpreted your question differently. However in the last patch, I removed the test for browser.docShell, since at this stage where we are just getting the framework in place, the browser is being linked before TabOpen event is fired (to prevent breakage in SessionStore). But yes, when we are at the stage in bug 906076 of actually lazily linking the browser, the test will have to be put back in.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11f231511081

... and you've got a failure: browser/base/content/test/general/browser_bug521216.js | got events and progress notifications in expected order - Got onStateChange,TabOpen,onLocationChange,onLinkIconAvailable, expected TabOpen,onStateChange,onLocationChange,onLinkIconAvailable

Dao, what do you think of this solution? `onStateChange` is sent when browser.loadURIWithFlags runs. It should work well with bug 906076 since in that bug when calling `addTab` with aURL we force the browser to be linked immediately, because lazily linking the browser in that case normally wouldn't make sense anyway. So that code would always run when needed.

Comment on attachment 8721786 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_10.diff Sorry, this needs another update since bug 1014185 landed and deleted a few lines from addTab.

(In reply to Dão Gottwald [:dao] from comment #33) > Comment on attachment 8721786 [details] [diff] [review] > bug_1243707_lazy_browser_tabs_framework_10.diff > > Sorry, this needs another update since bug 1014185 landed and deleted a few > lines from addTab. No problem, the update actually helps.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90988cc2e0fd

Comment on attachment 8723143 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_11.diff Can you make _linkBrowserToTab return an object like { usingPreloadedContent: usingPreloadedContent } and use that instead of browser.usingPreloadedContent? Looks good otherwise. Thanks!

Removed trailing whitespace to silence complaint by eslint.

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #41) > Removed trailing whitespace to silence complaint by eslint. arrrrg!

Backed out for jetpack failures. Backout: https://hg.mozilla.org/integration/fx-team/rev/96341def118f Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=dd5aaa1e47ad Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7940229&repo=fx-team 12:47:53 INFO - TEST-PASS | jetpack-package/addon-sdk/source/test/test-tab.js.testBug844492 | tab index is not undefined 12:47:53 INFO - TEST-PASS | jetpack-package/addon-sdk/source/test/test-tab.js.testBug844492 | the tab was not found as expected 12:47:53 INFO - An exception occurred. 12:47:53 INFO - Traceback (most recent call last): 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/timers.js", line 40, in notify 12:47:53 INFO - callback.apply(null, args); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/deprecated/unit-test.js", line 389, in done/</< 12:47:53 INFO - timer.setTimeout(_ => onDone(this)); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/deprecated/unit-test.js", line 563, in start/this.onDone 12:47:53 INFO - options.onDone(self); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/deprecated/unit-test.js", line 532, in promise callback*runNextTest 12:47:53 INFO - return tests.getNext().then((test) => { 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/deprecated/unit-test.js", line 538, in startMany/runNextTest/< 12:47:53 INFO - self.start({test: test, onDone: runNextTest}); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/deprecated/unit-test.js", line 576, in start 12:47:53 INFO - this.test.testFunction(this); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/test.js", line 67, in 12:47:53 INFO - test(assert, function() { 12:47:53 INFO - File "resource://jetpack-package-tests/jetpack-package/addon-sdk/source/test/test-tab.js", line 104, in exports.testBug844492 12:47:53 INFO - tabs.open({ url: 'about:mozilla' }); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/tabs/tabs-firefox.js", line 92, in Tabs<.open 12:47:53 INFO - return activeWindow.tabs.open(options); 12:47:53 INFO - File "resource://gre/modules/commonjs/sdk/windows/firefox.js", line 115, in WindowTabs<.open 12:47:53 INFO - domWindow.gBrowser.addTab(options.url); 12:47:53 INFO - File "chrome://browser/content/tabbrowser.xml", line 2020, in addTab 12:47:53 INFO - b.userTypedValue = aURI; 12:47:53 INFO - TypeError: b is null 12:48:08 WARNING - TEST-UNEXPECTED-FAIL | jetpack-package/addon-sdk/source/test/test-tab.js.testBug844492 | Test timed out (after: the tab was not found as expected) 12:48:08 INFO - TEST-INFO | Traceback (most recent call last): 12:48:08 INFO - File "resource://gre/modules/commonjs/sdk/timers.js", line 40, in notify 12:48:08 INFO - callback.apply(null, args); 12:48:08 INFO - File "resource://gre/modules/commonjs/sdk/deprecated/unit-test.js", line 506, in tiredOfWaiting 12:48:08 INFO - self.console.testMessage(false, false, self.test.name, 12:48:08 INFO - File "resource://gre/modules/commonjs/sdk/test/harness.js", line 541, in testMessage 12:48:08 INFO - this.trace();

Backout: https://hg.mozilla.org/integration/fx-team/rev/eb0465a53cb4

Sorry for the long delay: Updated patch fixes testBug844492 failures. Essentially moved one line: let b = t.linkedBrowser; to immediately after the `_linkBrowserToTab` call.

Comment on attachment 8739204 [details] [diff] [review] bug_1243707_lazy_browser_tabs_framework_13.diff (In reply to Allasso Travesser from comment #45) > Sorry for the long delay: I was on vacation anyway :)