882992 - Don't hard-code about:newtab in BrowserNewTabPreloader.jsm

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Description

[Allan Gardner (:Mathnerd314)]

Bug 875257 hard-coded about:newtab in https://hg.mozilla.org/mozilla-central/rev/b649a45e1b9a as the URL of the new tab. It also left the enable condition at prefHasUserValue(browser.newtab.url). Bug 724239 avoided hard-coding about:newtab by using getdefault(browser.newtab.url); I would like the preloading code cannot do the same.

Steps to reproduce:
1. Change the default of browser.newtab.url to something other than about:newtab
2. Open two new tabs quickly; the first will be about:newtab and the second will be the alternate page

Comment 1

[Allan Gardner (:Mathnerd314)]

And my English failed; the sentence should read: I would like the preloading code *to* do the same.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Tim, what do you think about this?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 762497 [details] [diff] [review]
Patch

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

I'm worried about two things here. First, if you change browser.newtab.url with preload enabled and a couple of preloaded pages in the background, we practically disable preloading until the end of the session. We'd need to update the hidden browsers' URLs as well.

The second thing is that I think not supporting pages other than about:newtab might actually be a good thing. Now that we're planning to make preload=true the default, users and add-on authors can run into interesting problems when they set their own newtab URL. The page content might be stale and not at all what you would expect from a page being loaded when opening a new tab. Add-on authors would have to implement synchronization between running instances of their new tab pages (like about:newtab does) to ensure this all still makes sense with preloading enabled.

I think we might be better off not supporting preloading for custom pages as this could save us lots of interesting bug reports from users and developers.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> I think we might be better off not supporting preloading for custom pages as
> this could save us lots of interesting bug reports from users and developers.

I agree.

Comment 5

[Allan Gardner (:Mathnerd314)]

Maybe we just need another pref, browser.newtab.preload.url. It seems logical enough.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mathnerd314 from comment #5)
> Maybe we just need another pref, browser.newtab.preload.url. It seems
> logical enough.

I think the reasoning in comment #3 is sound. Opening up preloading to non-about:newtab pages can invite unwanted issues and is something that (at least at the moment) is not prioritized heavily enough to have the resources necessary to fix issues that may come forward.

(In reply to Mathnerd314 from comment #0)
> 2. Open two new tabs quickly; the first will be about:newtab and the second
> will be the alternate page

You could also disable the preloading feature as a workaround.

Comment 7

[Allan Gardner (:Mathnerd314)]

Or the check could be:
this._enabled = this._branch.getBoolPref("preload") && this._branch.getCharPref("url") == NEWTAB_URL;

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mathnerd314 from comment #7)
> Or the check could be:
> this._enabled = this._branch.getBoolPref("preload") &&
> this._branch.getCharPref("url") == NEWTAB_URL;

I don't see how that would help at all. At that point we might as well keep using the NEWTAB_URL constant anyways.

Comment 9

[Allan Gardner (:Mathnerd314)]

The user might change browser.newtab.url to about:blank, then change it back to about:newtab. In such a case browser.newtab.url would have a user value and preloading would be silently disabled.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mathnerd314 from comment #9)
> The user might change browser.newtab.url to about:blank, then change it back
> to about:newtab. In such a case browser.newtab.url would have a user value
> and preloading would be silently disabled.

That would be incorrect, since https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPrefBranch?redirectlocale=en-US&redirectslug=nsIPrefBranch#prefHasUserValue%28%29 says that if the preference value is changed back to it's default value, prefHasUserValue will return false.