Last Comment Bug 753448 - [New Tab Page] preload newtab pages in the background and swap them in when opening a new tab
: [New Tab Page] preload newtab pages in the background and swap them in when o...
Status: RESOLVED FIXED
[Snappy:P1]
: perf
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: Firefox 17
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
Depends on: 786484 728294 758212 764931 765211 765226 765235 771892
Blocks: 725756 752839 780123 791670
  Show dependency treegraph
 
Reported: 2012-05-09 11:28 PDT by Tim Taubert [:ttaubert]
Modified: 2012-09-17 06:28 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screencast without the patch (2.63 MB, video/webm)
2012-05-09 11:29 PDT, Tim Taubert [:ttaubert]
no flags Details
screencast with the patch (571.80 KB, video/webm)
2012-05-09 11:30 PDT, Tim Taubert [:ttaubert]
no flags Details
screencast without the patch (656.47 KB, video/webm)
2012-05-09 11:32 PDT, Tim Taubert [:ttaubert]
no flags Details
patch v1 (10.31 KB, patch)
2012-05-09 11:40 PDT, Tim Taubert [:ttaubert]
gavin.sharp: feedback+
Details | Diff | Review
patch v2 (11.28 KB, patch)
2012-05-09 13:03 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v3 (11.85 KB, patch)
2012-05-10 11:42 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v4 (13.04 KB, patch)
2012-05-11 06:08 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v4b (13.13 KB, patch)
2012-05-11 06:25 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
patch v5 (13.37 KB, patch)
2012-05-11 07:48 PDT, Tim Taubert [:ttaubert]
gavin.sharp: feedback+
Details | Diff | Review
Part 1 - Add BrowserNewTabPreloader.jsm (10.06 KB, patch)
2012-06-15 07:38 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 2 - Make tabbrowser use the new tab preloader (3.47 KB, patch)
2012-06-15 07:39 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 3 - Adapt tests for the new tab page (1.39 KB, patch)
2012-06-15 07:44 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 1 - Add BrowserNewTabPreloader.jsm (10.03 KB, patch)
2012-07-05 05:41 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 2 - Make tabbrowser use the new tab preloader (3.06 KB, patch)
2012-07-05 05:43 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 3 - Adapt tests for the new tab page (1.39 KB, patch)
2012-07-05 05:44 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review
Part 2 - Make tabbrowser use the new tab preloader (3.07 KB, patch)
2012-07-06 05:33 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review
Part 1 - Add BrowserNewTabPreloader.jsm (10.07 KB, patch)
2012-07-06 07:34 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Review
Part 2 - Make tabbrowser use the new tab preloader (3.18 KB, patch)
2012-07-07 03:31 PDT, Tim Taubert [:ttaubert]
ttaubert: review+
Details | Diff | Review
Part 2 - Make tabbrowser use the new tab preloader (3.14 KB, patch)
2012-07-08 04:16 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review
Part 4 - Shutdown leak tests shouldn't include the preloaded newtab page (1.06 KB, patch)
2012-07-08 04:23 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Review
Part 1 - Add BrowserNewTabPreloader.jsm (10.16 KB, patch)
2012-07-08 04:29 PDT, Tim Taubert [:ttaubert]
gavin.sharp: review+
Details | Diff | Review
Part 4 - Shutdown leak tests shouldn't include the preloaded newtab page (1.34 KB, patch)
2012-07-10 03:05 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Review
Part 2 - Integrate the new tab preloader (2.68 KB, patch)
2012-08-03 03:46 PDT, Tim Taubert [:ttaubert]
jaws: review+
dao+bmo: review-
Details | Diff | Review
Part 1 - Add BrowserNewTabPreloader.jsm (11.95 KB, patch)
2012-08-05 08:30 PDT, Tim Taubert [:ttaubert]
gavin.sharp: review+
Details | Diff | Review
Part 2 - Integrate the new tab preloader (2.73 KB, patch)
2012-08-06 02:49 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review
Part 2 - Integrate the new tab preloader (3.18 KB, patch)
2012-08-08 02:13 PDT, Tim Taubert [:ttaubert]
jaws: review+
Details | Diff | Review

Description Tim Taubert [:ttaubert] 2012-05-09 11:28:04 PDT
Opening the newtab page with about:newtab (or any other URL besides about:blank) flickers because the user will first see a about:blank and then the loading of the newtab url will start. This causes bugs such as bug 752839.

A simple idea would be to preload the newtab page in the background and when gBrowser.addTab() is called move the newly loading docShell to the background and swap it with the preloaded docShell.

The user can now interact with the instantly loaded tab or just navigate away. When another tab is opened we just swap again because the previous tab should now be loaded in the background. If it's not loaded yet (which should almost never be the case) we can just not swap and let the new tab load normally.
Comment 1 Tim Taubert [:ttaubert] 2012-05-09 11:29:34 PDT
Created attachment 622429 [details]
screencast without the patch
Comment 2 Tim Taubert [:ttaubert] 2012-05-09 11:30:15 PDT
Created attachment 622430 [details]
screencast with the patch
Comment 3 Tim Taubert [:ttaubert] 2012-05-09 11:32:15 PDT
Created attachment 622433 [details]
screencast without the patch
Comment 4 Tim Taubert [:ttaubert] 2012-05-09 11:40:32 PDT
Created attachment 622438 [details] [diff] [review]
patch v1

That's the patch you see in the screencast. It works as described in comment #0.

I actually wanted to put the preloading <browser> into tabbrowser.xml because that's where it should be I think but this wasn't easy. The tabbrowser received all events dispatched by it and of course didn't find the tab for the target browser.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-09 11:56:18 PDT
Comment on attachment 622438 [details] [diff] [review]
patch v1

This looks promising!

What are the browser.xml form fill changes about? Was it not a problem before because of some tabbrowser logic? Or is it just a bug fix not specific to this change?

Do we not need to worry about ever reloading about:newtab because it's always self-updating? Even for changes in the frequency of most-visited sites (as opposed to explicit user changes)? 

In addTab, shouldn't we avoid doing the load entirely when we're going to do the swap?
Comment 6 Tim Taubert [:ttaubert] 2012-05-09 12:48:47 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> What are the browser.xml form fill changes about? Was it not a problem
> before because of some tabbrowser logic? Or is it just a bug fix not
> specific to this change?

That's a fix, now that swapDocShells supports swapping between a standalone browser and one contained in a tabbrowser. We need to call attachFormFill() for the docShell that's going to be swapped in from the background here. The soon-to-be-hidden one needs to be detached.

> Do we not need to worry about ever reloading about:newtab because it's
> always self-updating? Even for changes in the frequency of most-visited
> sites (as opposed to explicit user changes)? 

Not a problem with about:newtab. Changes are synchronized between all open instances. We should probably introduce a flag like browser.newtab.preload so that addon authors can disable preloading if they don't want to support it.

> In addTab, shouldn't we avoid doing the load entirely when we're going to do
> the swap?

We shouldn't stop the load or otherwise there's nothing to swap in for the next new tab that is opened.
Comment 7 Tim Taubert [:ttaubert] 2012-05-09 13:03:41 PDT
Created attachment 622475 [details] [diff] [review]
patch v2

Alright, I think the patch is quite ready for review :)
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-09 14:37:00 PDT
(In reply to Tim Taubert [:ttaubert] from comment #6)
> We shouldn't stop the load or otherwise there's nothing to swap in for the
> next new tab that is opened.

OK, I was temporarily confused about how this worked ("swapping" rather than "cloning").
Comment 9 Tim Taubert [:ttaubert] 2012-05-10 06:42:16 PDT
Comment on attachment 622475 [details] [diff] [review]
patch v2

Cancelling review while investigating try failures. Will refactor the patch a bit.
Comment 10 Tim Taubert [:ttaubert] 2012-05-10 11:42:04 PDT
Created attachment 622818 [details] [diff] [review]
patch v3

Refactored the patch and solved some (hopefully all) test failures.
Comment 11 Tim Taubert [:ttaubert] 2012-05-11 06:08:37 PDT
Created attachment 623112 [details] [diff] [review]
patch v4

Some securityUI tests were failing. Turns out we need to re-initialize the securityUI after swapping the docShells so that we keep the securityUI state but remove and add the web progress listeners.
Comment 12 Tim Taubert [:ttaubert] 2012-05-11 06:25:47 PDT
Created attachment 623116 [details] [diff] [review]
patch v4b

SecurityUI.init() needs to be wrapped in a try/catch block.
Comment 13 Tim Taubert [:ttaubert] 2012-05-11 06:56:32 PDT
Comment on attachment 623116 [details] [diff] [review]
patch v4b

This is not the fix we were looking for.
Comment 14 Tim Taubert [:ttaubert] 2012-05-11 07:48:22 PDT
Created attachment 623140 [details] [diff] [review]
patch v5

Removed all the nonsense touching nsSecureBrowserUI. All we need is a small fix in the secure browser UI test suite that should've actually been necessary since about:newtab landed...
Comment 15 Tim Taubert [:ttaubert] 2012-05-11 15:38:36 PDT
Comment on attachment 623140 [details] [diff] [review]
patch v5

Passes try now.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-14 17:54:50 PDT
Comment on attachment 623140 [details] [diff] [review]
patch v5

Seems like uninit needs to handle being called when init() wasn't, given that BROWSER_NEW_TAB_URL can change during a window's lifetime.

BrowserNewTabPreloader could be defined in a module, and initialized with a window (new BrowserNewTabPreloader(window)). I don't really like adding more #included files in browser.js.

Dispatching a "TabNew" event might be confusing to extensions (how does it differ from TabOpen? Why is it not fired consistently?). I'm not sure how to best address that - perhaps with a more verbose name? Or maybe avoid dispatching the event and just have tabbrowser call gBrowserNewTabPreloader.newTab directly?

Looks like swapNewTabWithBrowser could share some code with swapBrowsersAndCloseOther with a bit of refactoring?

Why do the newtab tests require disabling preloading?

Using the return value of indexOf for filter() is confusing, throw a != -1 or == 0 in there :) Why do those tests need changing?

It might be worth splitting off the browser.xml change to a separate bug.
Comment 17 timbugzilla 2012-06-04 18:29:41 PDT
Would be great to see this land soon, as it (animation stuttering on opening a new tab) is probably the most obvious/frequent example of UI jank for me.
Comment 18 Tim Taubert [:ttaubert] 2012-06-15 07:38:33 PDT
Created attachment 633518 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> BrowserNewTabPreloader could be defined in a module, and initialized with a
> window (new BrowserNewTabPreloader(window)). I don't really like adding more
> #included files in browser.js.

Done.

> Dispatching a "TabNew" event might be confusing to extensions (how does it
> differ from TabOpen? Why is it not fired consistently?). I'm not sure how to
> best address that - perhaps with a more verbose name? Or maybe avoid
> dispatching the event and just have tabbrowser call
> gBrowserNewTabPreloader.newTab directly?

Done.

> Looks like swapNewTabWithBrowser could share some code with
> swapBrowsersAndCloseOther with a bit of refactoring?

Done.
Comment 19 Tim Taubert [:ttaubert] 2012-06-15 07:39:52 PDT
Created attachment 633519 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader
Comment 20 Tim Taubert [:ttaubert] 2012-06-15 07:44:24 PDT
Created attachment 633521 [details] [diff] [review]
Part 3 - Adapt tests for the new tab page

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16)
> Why do the newtab tests require disabling preloading?

Reverted, all I did now was to not wait for the 'load' event if the new tab page has already loaded (because it was swapped in).

> Using the return value of indexOf for filter() is confusing, throw a != -1
> or == 0 in there :) Why do those tests need changing?

Removed all this. I think it was fixed by bug 764931.
Comment 21 Tim Taubert [:ttaubert] 2012-06-15 15:05:26 PDT
Try is green:

https://tbpl.mozilla.org/?tree=Try&rev=0abbdf55a236
Comment 22 Tim Taubert [:ttaubert] 2012-07-05 05:41:05 PDT
Created attachment 639304 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm
Comment 23 Tim Taubert [:ttaubert] 2012-07-05 05:43:05 PDT
Created attachment 639305 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader
Comment 24 Tim Taubert [:ttaubert] 2012-07-05 05:44:40 PDT
Created attachment 639306 [details] [diff] [review]
Part 3 - Adapt tests for the new tab page
Comment 25 Tim Taubert [:ttaubert] 2012-07-05 05:48:10 PDT
Comment on attachment 639306 [details] [diff] [review]
Part 3 - Adapt tests for the new tab page

Now that new tab pages might be swapped in, they're instantly loaded after gBrowser.addTab() and the load event will of course never occur. By checking the document's readyState we can just bypass that and continue testing.
Comment 26 Tim Taubert [:ttaubert] 2012-07-05 05:50:16 PDT
Comment on attachment 639305 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

Quite simple patch that makes use of the JSM introduced in the first part. The most important part is the .newTab() call that swap the newly created docShell with the one preloaded in the background.
Comment 27 Jared Wein [:jaws] (please needinfo? me) 2012-07-05 13:32:53 PDT
Comment on attachment 639305 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

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

::: browser/base/content/browser.js
@@ +1386,5 @@
>      gSyncUI.init();
>  #endif
>  
> +    if (toolbar.visible)
> +      gBrowserNewTabPreloader.init();

I'm having a hard time following this here. Where is |toolbar| declared? Doing a plaintext search in browser.js for " toolbar " doesn't show any results that would make sense here. My other question is why does init'ing the preloader depend on toolbar visibility?
Comment 28 Tim Taubert [:ttaubert] 2012-07-06 04:52:10 PDT
(In reply to Jared Wein [:jaws] from comment #27)
> I'm having a hard time following this here. Where is |toolbar| declared?
> Doing a plaintext search in browser.js for " toolbar " doesn't show any
> results that would make sense here.

'toolbar' is the same as 'window.toolbar', like here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1201

I can add 'window.' if you think that's a bit clearer.

> My other question is why does init'ing
> the preloader depend on toolbar visibility?

window.toolbar.visible=false for popup windows. We don't want to preload a new tab page in the background of a popup window if it never gets used anyway.
Comment 29 Tim Taubert [:ttaubert] 2012-07-06 05:33:10 PDT
Created attachment 639631 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

Changed to window.toolbar and fixed a syntax error.
Comment 30 Tim Taubert [:ttaubert] 2012-07-06 07:34:22 PDT
Created attachment 639659 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm
Comment 31 Jared Wein [:jaws] (please needinfo? me) 2012-07-06 22:52:35 PDT
Comment on attachment 639631 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

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

::: browser/base/content/browser.js
@@ +1385,5 @@
>      // initialize the sync UI
>      gSyncUI.init();
>  #endif
>  
> +    if (window.toolbar.visible)

Please add a comment here about the relationship with toolbar and popup windows? I don't think this connection will be immediately obvious for others :)
Comment 32 Tim Taubert [:ttaubert] 2012-07-07 03:31:30 PDT
Created attachment 639951 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

Forwarding Jared's r+.

(In reply to Jared Wein [:jaws] from comment #31)
> Please add a comment here about the relationship with toolbar and popup
> windows? I don't think this connection will be immediately obvious for
> others :)

Yeah, fixed.
Comment 33 Tim Taubert [:ttaubert] 2012-07-08 04:16:36 PDT
Created attachment 640050 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

Asking for review again, had to correct the patch a bit.

The gBrowserNewTabPreloader.init() call does now pass and store the window. It's theoretically possible to access gBrowserNewTabPreloader even if the delayed initialization of the browser window never happened. As a result of this gBrowserNewTabPreloader.uninit() wouldn't be called to clean up.

Also we don't really need to set gBrowserNewTabPreloader=null at window unload. This way we can call .uninit() again even if it has already been uninitialized - which is really useful for part 4.
Comment 34 Tim Taubert [:ttaubert] 2012-07-08 04:23:59 PDT
Created attachment 640051 [details] [diff] [review]
Part 4 - Shutdown leak tests shouldn't include the preloaded newtab page

When a test creates a new tab with about:newtab as the initial URL, the docShell created by the test is swapped with a preloaded one. The newly created one is now kept in the background and waits until the next new tab is opened.

The log parser detects this as a document that is kept alive until shutdown which is actually done intentionally. We can avoid this "leak" by calling gBrowserNewTabPreloader.uninit() before the mochitest suite shuts down.
Comment 35 Tim Taubert [:ttaubert] 2012-07-08 04:29:10 PDT
Created attachment 640052 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm

Patch updated.
Comment 36 Dão Gottwald [:dao] 2012-07-08 15:55:28 PDT
Comment on attachment 640051 [details] [diff] [review]
Part 4 - Shutdown leak tests shouldn't include the preloaded newtab page

This isn't going to work for SeaMonkey which has gBrowser but not gBrowserNewTabPreloader.
Comment 37 Tim Taubert [:ttaubert] 2012-07-10 03:05:12 PDT
Created attachment 640545 [details] [diff] [review]
Part 4 - Shutdown leak tests shouldn't include the preloaded newtab page

(In reply to Dão Gottwald [:dao] from comment #36)
> This isn't going to work for SeaMonkey which has gBrowser but not
> gBrowserNewTabPreloader.

Good catch. Corrected.
Comment 38 Dão Gottwald [:dao] 2012-07-11 09:11:14 PDT
Comment on attachment 640545 [details] [diff] [review]
Part 4 - Shutdown leak tests shouldn't include the preloaded newtab page

This should be outside the window.gBrowser check, since it isn't subject to the above comment ("Many tests randomly add and remove tabs, resulting in [...]").

In principle, I don't like the test framework messing with specific features like this. Running uninit code that doesn't run when a real user closes the last browser window could hide other issues. Is there some other way to get the leak ignored?
Comment 39 Tim Taubert [:ttaubert] 2012-07-12 03:53:08 PDT
(In reply to Dão Gottwald [:dao] from comment #38)
> In principle, I don't like the test framework messing with specific features
> like this. Running uninit code that doesn't run when a real user closes the
> last browser window could hide other issues. Is there some other way to get
> the leak ignored?

I tried closing the test window before gathering leaked docShell/windows but I think that isn't actually correct as we don't catch things that are erroneously leaked until the browser window is closed. Or would that work because we're only searching for stuff leaking until app shutdown?

If we really do want to find all objects kept alive until the windows is closed, then Panorama and the NewTabPreloader are exceptions and do unfortunately need special treatment, I think.
Comment 40 Dão Gottwald [:dao] 2012-07-12 06:17:31 PDT
(In reply to Tim Taubert [:ttaubert] from comment #39)
> I tried closing the test window before gathering leaked docShell/windows but
> I think that isn't actually correct as we don't catch things that are
> erroneously leaked until the browser window is closed. Or would that work
> because we're only searching for stuff leaking until app shutdown?

We've seen many leaks that are tied to the window. To what extent this is the case after bug 695480, I'm not sure. Does closing the window before analyzing the leak statistics hide any of the leaks that we're currently see (bug 754804)?

> If we really do want to find all objects kept alive until the windows is
> closed, then Panorama and the NewTabPreloader are exceptions and do
> unfortunately need special treatment, I think.

How about setting an attribute on these iframes and browsers to indicate that their final window and docshell should be excluded from the statistics?
Comment 41 Tim Taubert [:ttaubert] 2012-07-12 08:46:52 PDT
(In reply to Dão Gottwald [:dao] from comment #40)
> We've seen many leaks that are tied to the window. To what extent this is
> the case after bug 695480, I'm not sure.

Me neither but I *think* we should continue testing for it.

> Does closing the window before
> analyzing the leak statistics hide any of the leaks that we're currently see
> (bug 754804)?

Yes, I managed to get rid of bug 759711 by closing the test window, running CC+GC and then checking for leaks.

> How about setting an attribute on these iframes and browsers to indicate
> that their final window and docshell should be excluded from the statistics?

I unfortunately don't think this is going to work. We currently just track debug output created by nsDocShell and the leak logger has no way of knowing about the docShell's container. We can't even access the docShell itself. I'm still trying to come up with a better solution... this is tricky.
Comment 42 Dão Gottwald [:dao] 2012-07-12 09:07:50 PDT
(In reply to Tim Taubert [:ttaubert] from comment #41)
> > Does closing the window before
> > analyzing the leak statistics hide any of the leaks that we're currently see
> > (bug 754804)?
> 
> Yes, I managed to get rid of bug 759711 by closing the test window, running
> CC+GC and then checking for leaks.

Right, but did any of the leaks that we don't want to ignore go away?

> > How about setting an attribute on these iframes and browsers to indicate
> > that their final window and docshell should be excluded from the statistics?
> 
> I unfortunately don't think this is going to work. We currently just track
> debug output created by nsDocShell and the leak logger has no way of knowing
> about the docShell's container. We can't even access the docShell itself.
> I'm still trying to come up with a better solution... this is tricky.

Maybe we can use recorded docshell leaks as a reason to ignore the accompanying DOM window leaks, since docshell leaks nowadays seem to indicate that the respective document is actually loaded in some iframe (bug 734172, bug 759711). Of course this would cause us to miss cases where the iframe sticks around unintentionally...
Comment 43 Jared Wein [:jaws] (please needinfo? me) 2012-08-01 14:22:09 PDT
Any update on this bug? Is it just waiting on Gavin to review or is there some more work to do with respect to leaks?
Comment 44 Tim Taubert [:ttaubert] 2012-08-01 14:27:10 PDT
Both! It's ready for review and I think bug 728294 will be fixed soon. We can't land this without it being fixed.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 00:12:45 PDT
Comment on attachment 640052 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <method name="swapNewTabWithBrowser">

>+            // Update the new tab's title.
>+            this.setTabTitle(aNewTab);

Don't we also need to call updateCurrentBrowser if aNewTab == this.selectedTab, as swapBrowsersAndCloseOther does?

>diff --git a/browser/modules/BrowserNewTabPreloader.jsm b/browser/modules/BrowserNewTabPreloader.jsm

>+BrowserNewTabPreloader.prototype = {

>+  _createBrowser: function Preloader_createBrowser() {

>+    this.browser.setAttribute("type", "content-targetable");

I don't recall whether this would matter in practice, but it seems like we don't want this window to be targetable, since it's hidden. So just use "content"?

>+    let style = this.browser.style;
>+    style.visibility = "hidden";
>+    style.height = "0";

Can you just use this.browser.collapsed = true?

It would be nice to have a single pre-loaded new tab page browser shared across all windows, but that seems more complicated (maybe we could investigate using the hidden window in a followup?). Hopefully the footprint overhead of one extra hidden browser per window isn't too significant.
Comment 46 Tim Taubert [:ttaubert] 2012-08-03 00:45:12 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #45)
> It would be nice to have a single pre-loaded new tab page browser shared
> across all windows, but that seems more complicated (maybe we could
> investigate using the hidden window in a followup?). Hopefully the footprint
> overhead of one extra hidden browser per window isn't too significant.

Yes, my first thought was to move that to the hidden windows as well but iirc that is only privileged on Mac but a content page on Linux/Windows for some reason. That's also the cause for some other funny code I've seen - we should maybe think about streamlining this to a XUL page on every OS, unless there's something holding us back I don't know about.
Comment 47 Tim Taubert [:ttaubert] 2012-08-03 03:28:57 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #45)
> Don't we also need to call updateCurrentBrowser if aNewTab ==
> this.selectedTab, as swapBrowsersAndCloseOther does?

Yes, good catch.

> I don't recall whether this would matter in practice, but it seems like we
> don't want this window to be targetable, since it's hidden. So just use
> "content"?

Fixed.

> Can you just use this.browser.collapsed = true?

Indeed, that's better.
Comment 48 Tim Taubert [:ttaubert] 2012-08-03 03:46:21 PDT
Created attachment 648657 [details] [diff] [review]
Part 2 - Integrate the new tab preloader

One little correction: I just realized that calling gBrowserNewTabPreloader.newTab() from gBrowser.addTab() doesn't quite feel right as the latter is such a generic method use by so many places.

I think it's better to call it in BrowserOpenTab() as that's the only place where we actually want the preloader to take effect: only when opening a new tab with the newtab url that is not a background tab. We don't have all that information in addTab() and I think it does make a little more sense to put it into browser.js.
Comment 49 Tim Taubert [:ttaubert] 2012-08-03 05:24:06 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #45)
> It would be nice to have a single pre-loaded new tab page browser shared
> across all windows, but that seems more complicated (maybe we could
> investigate using the hidden window in a followup?).

Filed bug 780123.
Comment 50 Jared Wein [:jaws] (please needinfo? me) 2012-08-04 10:31:40 PDT
Comment on attachment 648657 [details] [diff] [review]
Part 2 - Integrate the new tab preloader

Clearing review and including the conversation from IRC for posterity.

<jaws> ttaubert: hey for your preloading the new tab patch, should we only preload the about:newtab page, and not custom new tab urls?
<jaws> i'm worried about perf/memory overhead here, if a user has a heavy site set as their new tab page
<jaws> also, external sites that are tracking loads will get inflated numbers based on the preloading
<ttaubert> jaws: yeah that's a valid concern. we could probably restrict that to about:newtab but I actually wanted to give add-on authors the chance to do the same with their custom speed dial stuff
<gavin> you could limit it to default values of newtab.url, similar to the session history patch
<ttaubert> true, that wouldn't help add-on authors but distributors
<gavin> well, addons can specify a default pref
<ttaubert> oh
<gavin> it would just exclude about:config tweakers
<ttaubert> ok let's do that then
<ttaubert> jaws: I think I may need to change part 1 + 2, I'll ask you for review again.
Comment 51 Tim Taubert [:ttaubert] 2012-08-05 08:30:07 PDT
Created attachment 649101 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm

Changed the patch so that we're now only swapping preloaded tabs when *all* of the following are true:

1) browser.newtab.preload == true
2) browser.newtab.url && browser.newtab.url != "about:blank"
3) !prefHasUserValue("browser.newtab.url")

So we're making sure to only preload valid default newtab urls.
Comment 52 Tim Taubert [:ttaubert] 2012-08-05 08:31:28 PDT
Comment on attachment 648657 [details] [diff] [review]
Part 2 - Integrate the new tab preloader

Didn't need to touch this patch. gBrowserNewTabPreloader.newTab() returns early if preloading newtab pages is disabled (e.g. the user has a custom newtab url set).
Comment 53 Dão Gottwald [:dao] 2012-08-06 00:19:59 PDT
Comment on attachment 648657 [details] [diff] [review]
Part 2 - Integrate the new tab preloader

openUILinkIn doesn't always open the tab in the current window. BrowserOpenTab can be called in popup windows, for instance.
Comment 54 Tim Taubert [:ttaubert] 2012-08-06 02:49:00 PDT
Created attachment 649225 [details] [diff] [review]
Part 2 - Integrate the new tab preloader

(In reply to Dão Gottwald [:dao] from comment #53)
> openUILinkIn doesn't always open the tab in the current window.
> BrowserOpenTab can be called in popup windows, for instance.

openLinkIn(where = "tab") does not open a new tab in the current window if (!toolbar.visible), i.e. we have a popup window.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#236

That's not really a problem since we don't initialize the newtab preloader for popup windows but we can be a little more explicit here and add another check for toolbar.visible.
Comment 55 Dão Gottwald [:dao] 2012-08-06 11:22:57 PDT
(In reply to Tim Taubert [:ttaubert] from comment #54)
> That's not really a problem since we don't initialize the newtab preloader
> for popup windows but we can be a little more explicit here and add another
> check for toolbar.visible.

Well, this still won't make the preloading work in that case. BrowserOpenTab is also called in non-browser windows on OS X.
Comment 56 Tim Taubert [:ttaubert] 2012-08-06 11:26:11 PDT
(In reply to Dão Gottwald [:dao] from comment #55)
> (In reply to Tim Taubert [:ttaubert] from comment #54)
> > That's not really a problem since we don't initialize the newtab preloader
> > for popup windows but we can be a little more explicit here and add another
> > check for toolbar.visible.
> 
> Well, this still won't make the preloading work in that case. BrowserOpenTab
> is also called in non-browser windows on OS X.

Right but that's ok I think. To make this work we first need to fix bug 780123 as a follow-up. This bug should implement it then.
Comment 57 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-06 13:19:55 PDT
Comment on attachment 649101 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm

>diff --git a/browser/modules/BrowserNewTabPreloader.jsm b/browser/modules/BrowserNewTabPreloader.jsm

>+let Preferences = {

>+  get _branch() {

I prefer the delete this.lazyProp; return this.lazyProp = foo; style as discussed on IRC.

>+  get enabled() {

>+    let url = this._branch.getCharPref("url");

let url = this.url; ?

Only doing this for default values of newtab.url was more complicated than I thought, but I think you covered all of the cases properly!
Comment 58 Dão Gottwald [:dao] 2012-08-07 10:01:37 PDT
(In reply to Tim Taubert [:ttaubert] from comment #56)
> (In reply to Dão Gottwald [:dao] from comment #55)
> > (In reply to Tim Taubert [:ttaubert] from comment #54)
> > > That's not really a problem since we don't initialize the newtab preloader
> > > for popup windows but we can be a little more explicit here and add another
> > > check for toolbar.visible.
> > 
> > Well, this still won't make the preloading work in that case. BrowserOpenTab
> > is also called in non-browser windows on OS X.
> 
> Right but that's ok I think. To make this work we first need to fix bug
> 780123 as a follow-up. This bug should implement it then.

I don't see how that bug matters for what I said. You need to call gBrowserNewTabPreloader.newTab in the window where the tab is opened instead of the random, sometimes non-browser window where BrowserOpenTab is called.
Comment 59 Tim Taubert [:ttaubert] 2012-08-07 11:12:51 PDT
(In reply to Dão Gottwald [:dao] from comment #58)
> (In reply to Tim Taubert [:ttaubert] from comment #56)
> > (In reply to Dão Gottwald [:dao] from comment #55)
> > > (In reply to Tim Taubert [:ttaubert] from comment #54)
> > > > That's not really a problem since we don't initialize the newtab preloader
> > > > for popup windows but we can be a little more explicit here and add another
> > > > check for toolbar.visible.
> > > 
> > > Well, this still won't make the preloading work in that case. BrowserOpenTab
> > > is also called in non-browser windows on OS X.
> > 
> > Right but that's ok I think. To make this work we first need to fix bug
> > 780123 as a follow-up. This bug should implement it then.
> 
> I don't see how that bug matters for what I said. You need to call
> gBrowserNewTabPreloader.newTab in the window where the tab is opened instead
> of the random, sometimes non-browser window where BrowserOpenTab is called.

Right, but we can implement that when we support it. gBrowserInit.onLoad() can call gBrowserNewTabPreloader.newTab() when we pass "about:newtab" to the new window. This would return early if there's no preloaded tab, yet.

Calling gBrowserNewTabPreloader.newTab() in the few places we need it sounds better to me than just putting it in gBrowser.addTab() or the like. Also, the call from gBrowserInit.onLoad() could be wrapped in #ifdef XP_MACOSX because we just don't support that on Linux and Windows.

I'm not sure that there is a basic place to put the .newTab() call to as the first tab in new windows isn't added via .addTab(). It's just there.
Comment 60 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-07 12:01:40 PDT
Dao's point is that calling gBrowserNewTabPreloader.newTab() in the hidden window doesn't make any sense, even though it might not have any effect due to other unrelated checks (location bar visibility). You should put that call in a place that knows that the passed-in tab is a new tab, and that it will be loading the newtab URL.
Comment 61 Dão Gottwald [:dao] 2012-08-07 12:15:35 PDT
(In reply to Tim Taubert [:ttaubert] from comment #59)
> (In reply to Dão Gottwald [:dao] from comment #58)
> > (In reply to Tim Taubert [:ttaubert] from comment #56)
> > > (In reply to Dão Gottwald [:dao] from comment #55)
> > > > (In reply to Tim Taubert [:ttaubert] from comment #54)
> > > > > That's not really a problem since we don't initialize the newtab preloader
> > > > > for popup windows but we can be a little more explicit here and add another
> > > > > check for toolbar.visible.
> > > > 
> > > > Well, this still won't make the preloading work in that case. BrowserOpenTab
> > > > is also called in non-browser windows on OS X.
> > > 
> > > Right but that's ok I think. To make this work we first need to fix bug
> > > 780123 as a follow-up. This bug should implement it then.
> > 
> > I don't see how that bug matters for what I said. You need to call
> > gBrowserNewTabPreloader.newTab in the window where the tab is opened instead
> > of the random, sometimes non-browser window where BrowserOpenTab is called.
> 
> Right, but we can implement that when we support it. gBrowserInit.onLoad()
> can call gBrowserNewTabPreloader.newTab() when we pass "about:newtab" to the
> new window.

I wasn't talking about a new window being created but a tab being opened in an existing window, due to BrowserOpenTab being called in some other window where tabs can't be added directly.
Comment 62 Tim Taubert [:ttaubert] 2012-08-08 02:13:41 PDT
Created attachment 650001 [details] [diff] [review]
Part 2 - Integrate the new tab preloader

Sorry, Jared. Asking for review again. As Dão pointed out calling gBrowserNewTabPreloader.newTab() from BrowserOpenTab() is obviously a bad idea. I moved this call back to gBrowser.addTab().
Comment 63 Tim Taubert [:ttaubert] 2012-08-08 02:34:23 PDT
As expected, this patch is massively regressing talos tests, particularly ts_paint and all ts_places_* tests.

http://bit.ly/RoGwRt

Not sure what to do about that. gBrowserNewTabPreloader.init() is called from delayedStartup().
Comment 64 Tim Taubert [:ttaubert] 2012-08-09 05:54:11 PDT
(In reply to Tim Taubert [:ttaubert] from comment #63)
> As expected, this patch is massively regressing talos tests, particularly
> ts_paint and all ts_places_* tests.

I talked to Gavin and we'll land this feature pref'ed off and leave it that way until the talos regressions are figured out. It's easier to handle this in chunks i.e. follow-up bugs.
Comment 67 Thomas Stache 2012-08-14 13:13:20 PDT
A side effect of this is that new thumbnails now appear only for the 2nd new tab after visiting a page from the "top pages" list, whose thumbnail was missing for some reason (e.g. high frecency score synched from another client).
Comment 68 Jared Wein [:jaws] (please needinfo? me) 2012-08-14 14:08:45 PDT
(In reply to Thomas Stache from comment #67)

I'm ok with that, we just need to fix the original source of the problem and not lose thumbnails :)

Note You need to log in before you can comment on or make changes to this bug.