[New Tab Page] preload newtab pages in the background and swap them in when opening a new tab

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {perf})

Trunk
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(5 attachments, 21 obsolete attachments)

571.80 KB, video/webm
Details
656.47 KB, video/webm
Details
1.39 KB, patch
jaws
: review+
Details | Diff | Splinter Review
11.95 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
3.18 KB, patch
jaws
: review+
Details | Diff | Splinter Review
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.
Created attachment 622429 [details]
screencast without the patch
Created attachment 622430 [details]
screencast with the patch
Created attachment 622433 [details]
screencast without the patch
Attachment #622429 - Attachment is obsolete: true
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.
Attachment #622438 - Flags: feedback?(gavin.sharp)
Attachment #622438 - Flags: feedback?(dao)
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?
Attachment #622438 - Flags: feedback?(gavin.sharp) → feedback+
(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.
Created attachment 622475 [details] [diff] [review]
patch v2

Alright, I think the patch is quite ready for review :)
Attachment #622438 - Attachment is obsolete: true
Attachment #622438 - Flags: feedback?(dao)
Attachment #622475 - Flags: review?(gavin.sharp)
Summary: [New Tab Page] opening a new tab flickers and loads in multiple steps → [New Tab Page] preload newtab pages in the background and swap them in when opening a new tab
(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 on attachment 622475 [details] [diff] [review]
patch v2

Cancelling review while investigating try failures. Will refactor the patch a bit.
Attachment #622475 - Flags: review?(gavin.sharp)
Created attachment 622818 [details] [diff] [review]
patch v3

Refactored the patch and solved some (hopefully all) test failures.
Attachment #622475 - Attachment is obsolete: true
Attachment #622818 - Flags: review?(gavin.sharp)
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.
Attachment #622818 - Attachment is obsolete: true
Attachment #622818 - Flags: review?(gavin.sharp)
Attachment #623112 - Flags: review?(gavin.sharp)
Created attachment 623116 [details] [diff] [review]
patch v4b

SecurityUI.init() needs to be wrapped in a try/catch block.
Attachment #623112 - Attachment is obsolete: true
Attachment #623116 - Flags: review?(gavin.sharp)
Attachment #623112 - Flags: review?(gavin.sharp)
Comment on attachment 623116 [details] [diff] [review]
patch v4b

This is not the fix we were looking for.
Attachment #623116 - Attachment is obsolete: true
Attachment #623116 - Flags: review?(gavin.sharp)
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...
Attachment #623140 - Flags: review?(gavin.sharp)
Comment on attachment 623140 [details] [diff] [review]
patch v5

Passes try now.
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.
Attachment #623140 - Flags: review?(gavin.sharp) → feedback+
Blocks: 725756
Depends on: 758212

Comment 17

5 years ago
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.
Depends on: 764931
Depends on: 765211
Depends on: 765226
Depends on: 765235
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.
Attachment #623140 - Attachment is obsolete: true
Attachment #633518 - Flags: review?(gavin.sharp)
Created attachment 633519 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader
Attachment #633519 - Flags: review?(gavin.sharp)
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.
Attachment #633521 - Flags: review?(gavin.sharp)
Try is green:

https://tbpl.mozilla.org/?tree=Try&rev=0abbdf55a236
Created attachment 639304 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm
Attachment #633518 - Attachment is obsolete: true
Attachment #633518 - Flags: review?(gavin.sharp)
Created attachment 639305 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader
Attachment #633519 - Attachment is obsolete: true
Attachment #633519 - Flags: review?(gavin.sharp)
Created attachment 639306 [details] [diff] [review]
Part 3 - Adapt tests for the new tab page
Attachment #633521 - Attachment is obsolete: true
Attachment #633521 - Flags: review?(gavin.sharp)
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.
Attachment #639306 - Flags: review?(jaws)
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.
Attachment #639305 - Flags: review?(jaws)
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?
(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.
Created attachment 639631 [details] [diff] [review]
Part 2 - Make tabbrowser use the new tab preloader

Changed to window.toolbar and fixed a syntax error.
Attachment #639305 - Attachment is obsolete: true
Attachment #639305 - Flags: review?(jaws)
Attachment #639631 - Flags: review?(jaws)
Created attachment 639659 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm
Attachment #639304 - Attachment is obsolete: true
Attachment #639659 - Flags: review?(mak77)
Attachment #639659 - Flags: review?(gavin.sharp)
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 :)
Attachment #639631 - Flags: review?(jaws) → review+
Attachment #639306 - Flags: review?(jaws) → review+
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.
Attachment #639631 - Attachment is obsolete: true
Attachment #639951 - Flags: review+
Depends on: 771892
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.
Attachment #639951 - Attachment is obsolete: true
Attachment #640050 - Flags: review?(jaws)
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.
Attachment #640051 - Flags: review?(dao)
Created attachment 640052 [details] [diff] [review]
Part 1 - Add BrowserNewTabPreloader.jsm

Patch updated.
Attachment #639659 - Attachment is obsolete: true
Attachment #639659 - Flags: review?(mak77)
Attachment #639659 - Flags: review?(gavin.sharp)
Attachment #640052 - Flags: review?(gavin.sharp)
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.
Attachment #640051 - Flags: review?(dao) → review-
Attachment #640050 - Flags: review?(jaws) → review+
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.
Attachment #640051 - Attachment is obsolete: true
Attachment #640545 - Flags: review?(dao)

Updated

5 years ago
Blocks: 759711
No longer blocks: 759711
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?
Attachment #640545 - Flags: review?(dao) → review-
(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.
(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?
(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.
(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...
Attachment #640545 - Attachment is obsolete: true
Depends on: 728294

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:p1]

Updated

5 years ago
Blocks: 778612
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?
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 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.
Attachment #640052 - Flags: review?(gavin.sharp) → review+
(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.
(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.
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.
Attachment #640050 - Attachment is obsolete: true
Attachment #648657 - Flags: review?(jaws)
Blocks: 780123
(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 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.
Attachment #648657 - Flags: review?(jaws)
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.
Attachment #640052 - Attachment is obsolete: true
Attachment #649101 - Flags: review?(gavin.sharp)
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).
Attachment #648657 - Flags: review?(jaws)
Attachment #648657 - Flags: review?(jaws) → review+
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.
Attachment #648657 - Flags: review-
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.
Attachment #648657 - Attachment is obsolete: true
Attachment #649225 - Flags: review?(jaws)
(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.
(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 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!
Attachment #649101 - Flags: review?(gavin.sharp) → review+
Attachment #649225 - Flags: review?(jaws) → review+
(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.
(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.
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.
(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.
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().
Attachment #649225 - Attachment is obsolete: true
Attachment #650001 - Flags: review?(jaws)
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().
(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.
Attachment #650001 - Flags: review?(jaws) → review+

Updated

5 years ago
No longer blocks: 778612
https://hg.mozilla.org/integration/fx-team/rev/c767ba96126f
https://hg.mozilla.org/integration/fx-team/rev/a5b58ae99ac9
https://hg.mozilla.org/integration/fx-team/rev/274f20ffcc5d
Whiteboard: [Snappy:p1] → [Snappy:P1][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c767ba96126f
https://hg.mozilla.org/mozilla-central/rev/a5b58ae99ac9
https://hg.mozilla.org/mozilla-central/rev/274f20ffcc5d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P1][fixed-in-fx-team] → [Snappy:P1]
Target Milestone: --- → Firefox 17

Comment 67

5 years ago
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).
(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 :)

Updated

5 years ago
Depends on: 790205

Updated

5 years ago
No longer depends on: 790205

Updated

5 years ago
Depends on: 786484

Updated

5 years ago
Blocks: 786476

Updated

5 years ago
No longer blocks: 786476
Depends on: 786476

Updated

5 years ago
No longer depends on: 786476
Blocks: 791670
You need to log in before you can comment on or make changes to this bug.