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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Mathnerd314, Assigned: jaws)

Tracking

24 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

5 years ago
Blocks: 875257
(Reporter)

Comment 1

5 years ago
And my English failed; the sentence should read: I would like the preloading code *to* do the same.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
QA Contact: jaws
Summary: Don't hard-code about:newtab → Don't hard-code about:newtab in BrowserNewTabPreloader.jsm
Created attachment 762497 [details] [diff] [review]
Patch

Tim, what do you think about this?
Assignee: nobody → jaws
Attachment #762497 - Flags: review?(ttaubert)
Version: Trunk → 24 Branch
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.
(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.
(Reporter)

Comment 5

5 years ago
Maybe we just need another pref, browser.newtab.preload.url. It seems logical enough.
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 7

5 years ago
Or the check could be:
this._enabled = this._branch.getBoolPref("preload") && this._branch.getCharPref("url") == NEWTAB_URL;
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 9

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.