561749 - Make pref and event for "new tab" page

Status: VERIFIED DUPLICATE of bug 455553

(Firefox :: Tabbed Browser, enhancement)

Description

[Ben Bucksch (:BenB)]

When you open a new tab, a blank page opens. Arguably, a user might want to have his homepage loaded instead, or some other page.

Opera natively and many Firefox extensions try improve the user experience by showing a page with most recently visited pages etc.. That includes Mozilla Lab's own "Auto Dial" extension as well as many others like "Fast Dial", 

Opera "Speed dial"
http://help.opera.com/Windows/9.20/en/speeddial.html
http://www.opera.com/browser/tips/?feature=speeddial
http://www.opera.com/browser/features/

Mozilla Labs
Auto Dial <https://addons.mozilla.org/de/firefox/addon/8615>
http://ed.agadak.net/2008/08/auto-dial-beta-add-on-for-quick-page-access
https://mozillalabs.com/blog/2008/08/new-tab-concepts/
https://mozillalabs.com/blog/2009/03/firefox-new-tab-page-cognitive-shield/
https://mozillalabs.com/blog/2009/03/firefox-new-tab-next-iteration/

Other extensions
New Tab Homepage <https://addons.mozilla.org/de/firefox/addon/777>
New Tab Jumpstart <https://addons.mozilla.org/de/firefox/addon/8914>
Fast Dial <https://addons.mozilla.org/de/firefox/addon/5721>
New Tab King, ..., you name it.

Generally, it's not understandable why you can configure your homepage as page that loads on new windows, but not as page that loads in new tabs, *not even as an option*, if you really want that.
So, the need is clearly there.

Problem:
Unfortunately, the Firefox code hardcodes "about:blank" in the source code as "new tab page":

Firefox source, browser.js:
<http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#1801>

function BrowserOpenTab()
{
  if (!gBrowser) {
    // If there are no open browser windows, open a new one
    window.openDialog("chrome://browser/content/", "_blank",
                      "chrome,all,dialog=no", "about:blank");
    return;
  }
  gBrowser.loadOneTab("about:blank", {inBackground: false});
  focusAndSelectUrlBar();
}


That leads to extensions doing ugly stuff like:

Implementation "Auto Dial" extension from labs.mozilla.com :
<?xml version="1.0"?>
<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <script type="application/javascript">
  <![CDATA[
  // Quick hack to open up about:tab on a new tab
  let (newTabURI = "chrome://autodial/content/autodial.xul") {
    // Make BrowserOpenTab point to the new tab uri instead of about:blank
    eval(BrowserOpenTab.toSource().replace(/about:blank/g, newTabURI));
    // Clear out the new tab url
    eval(URLBarSetURI.toSource().replace(/}$/, 'if (gURLBar.value == "' + newTabURI + '") gURLBar.value = ""; $&'));
  }
  ]]>
  </script>
</overlay>

eval(BrowserOpenTab.toSource().replace(/about:blank/g, newTabURI));

is not exactly safe coding, neither is overwriting the whole BrowserOpenTab(), yet these seems the obvious options for extensions. Nevermind users!

Solution:
Make a pref
browser.tabs.newtab_url
which is akin to "browser.startup.homepage" and defines the URL to load when a new tab is opened. The default is still "about:blank".
Later, we could have pref UI for it, together with the homepage, or not.
More importantly, advanced users and extensions could easily set it without altering browser code or breaking each other or even the browser code.

Comment 1

[Ben Bucksch (:BenB)]

browser.tabs.newTabPage, actually

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Implement pref, v2

- Clear URLbar for internal newtab URLs
- Cache pref, incl. pref observer (not sure if cache is necessary)

Comment 4

[Ben Bucksch (:BenB)]

A "NewTab" event (which used to exist, but was removed in bug 494152 for other reasons) would be useful for other extensions which want to do other things, but I guess the code would need to be refactored more for that.

Comment 5

[Ben Bucksch (:BenB)]

My extension probably also needs to do other things on "new tab" (particularly, fill the "speed dial" page), so the "NewTab" event is needed. I'll supply a new patch which adds that.
You can nevertheless review the current patch already. I'll keep the pref in any case.

Comment 6

[Ben Bucksch (:BenB)]

I tried using "TabOpen" event and checking for event.target.linkedBrowser.userTypedValue !== null, as New Tab Jumpstart does, but that doesn't work either, it fires for all new tabs, including those loaded by link clicks (middle-click, context menu) or websites (target="_blank", window.open()), possibly due to bug 569047.

Comment 7

[Ben Bucksch (:BenB)]

I tried "New Tab Jumpstart", it apparently works there, but only apparently: The new tab page of "New Tab Jumpstart" does load even for link middle clicks, but is then replaced with the page. So, this is overzealous and wastes performance.

Comment 8

[Ben Bucksch (:BenB)]

Attachment: Add property to "TabOpen" event, v1

This patch adds 2 properties to the "TabOpen" event object:
- uri : if the new tab is going to load a page, this contains the URI
- isNewEmptyTab: true, if the user opened a new, fresh, empty tab without
  any page in it.

uri and isNewEmptyTab are mutually exclusive, but I think it's cleaner and safer that way than just checking for |uri == null|.
One example why it's safer is:

The current patch does not work yet for target="_blank" links and window.open(). It only works for "open link in new tab" or middle-click on links. And the "New Tab" button and menu item.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 441508 [details] [diff] [review]
Implement pref, v2

>+var gBrowserNewTabURL = null;
> function BrowserOpenTab()
> {
>+  if (!gBrowserNewTabURL)
>+  {
>+    gBrowserNewTabURL = "about:blank";
>+    try {
>+      let prefname = "browser.tabs.newTabPage";
>+      gBrowserNewTabURL = gPrefService.getCharPref(prefname);
>+      //gBrowserNewTabURL = gPrefService.getComplexValue(prefname,
>+      //    Ci.nsIPrefLocalizedString).data;
>+      gPrefService.addObserver(prefname,
>+      {
>+        observe : function(subject, topic, data)
>+        {
>+          gBrowserNewTabURL = gPrefService.getCharPref(prefname);
>+        },
>+        QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
>+            Ci.nsISupportsWeakReference]),
>+      }, false);
>+    } catch (e) {}
>+  }

You don't need to cache this, and you shouldn't catch exceptions (since you don't expect any).

>   if (!gBrowser) {
>     // If there are no open browser windows, open a new one
>     window.openDialog("chrome://browser/content/", "_blank",
>-                      "chrome,all,dialog=no", "about:blank");
>+                      "chrome,all,dialog=no", gBrowserNewTabURL);

This seems inconsistent with OpenBrowserWindow, which will open your home page...

>@@ -2170,16 +2191,23 @@ function URLBarSetURI(aURI, aValid) {
>       value = losslessDecodeURI(uri);
> 
>     let isBlank = (uri.spec == "about:blank");
>     valid = !isBlank && (!aURI || aValid);
>   }
> 
>   gURLBar.value = value;
>   SetPageProxyState(valid ? "valid" : "invalid");
>+
>+  // Clear URLBar when it's the new tab URL and it's an internal URL
>+  if (gBrowserNewTabURL &&
>+      gURLBar.value == gBrowserNewTabURL &&
>+      (gBrowserNewTabURL.substr(0, 7) == "chrome:" ||
>+       gBrowserNewTabURL.substr(0, 6) == "about:"))
>+    gURLBar.value = "";

Seems like the extension should just add that value to gInitialPages.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #6)
> I tried using "TabOpen" event and checking for
> event.target.linkedBrowser.userTypedValue !== null, as New Tab Jumpstart does,
> but that doesn't work either, it fires for all new tabs, including those loaded
> by link clicks (middle-click, context menu) or websites (target="_blank",
> window.open()), possibly due to bug 569047.

It's because userTypedValue is set after TabOpen has been dispatched.

Comment 11

[Ben Bucksch (:BenB)]

> You don't need to cache this

Why not? Getting prefs takes time.

> and you shouldn't catch exceptions (since you don't expect any).

I do: A pref can always not exist (or be in the wrong type). I don't want to break the whole browser just because somebody doesn't have the default pref for this. IMHO, get*Pref should always be in try/catch. Alternatively, I could use FUEL prefs API, which doesn't throw.

>     window.openDialog("chrome://browser/content/", "_blank",
>-                      "chrome,all,dialog=no", "about:blank");
>+                      "chrome,all,dialog=no", gBrowserNewTabURL);

This seems inconsistent with OpenBrowserWindow, which will open your home
page...

But we do "New tab" here, not "open window".

> Seems like the extension should just add that value to gInitialPages.

This is a pref, which the user can set. I could add the pref value to gInitialPages, though.

> It's because userTypedValue is set after TabOpen has been dispatched.

Maybe we should fix that.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> > You don't need to cache this
> 
> Why not? Getting prefs takes time.

It should be cheap enough for this case.

> > and you shouldn't catch exceptions (since you don't expect any).
> 
> I do: A pref can always not exist (or be in the wrong type). I don't want to
> break the whole browser just because somebody doesn't have the default pref for
> this. IMHO, get*Pref should always be in try/catch. Alternatively, I could use
> FUEL prefs API, which doesn't throw.

The pref is going to exist, since you set it in firefox.js.

> >     window.openDialog("chrome://browser/content/", "_blank",
> >-                      "chrome,all,dialog=no", "about:blank");
> >+                      "chrome,all,dialog=no", gBrowserNewTabURL);
> 
> This seems inconsistent with OpenBrowserWindow, which will open your home
> page...
> 
> But we do "New tab" here, not "open window".

It still seems inconsistent. I don't see why "New Tab" and "New Window" should behave differently when there's no browser window.

> > Seems like the extension should just add that value to gInitialPages.
> 
> This is a pref, which the user can set. I could add the pref value to
> gInitialPages, though.

The user isn't going to invent new chrome: or about: URIs, so this seems irrelevant.

Comment 13

[Ben Bucksch (:BenB)]

> It still seems inconsistent. I don't see why "New Tab" and "New Window" should
> behave differently when there's no browser window.

They do now (without my patch), too. I don't know why. But that's not part of this bug.

> The pref is going to exist, since you set it in firefox.js.

I think it's unwise to count on *any* default pref.
But if you want to break the browser in such cases, fine.

Attaching new patch with that changed.

Comment 14

[Ben Bucksch (:BenB)]

Attachment: Implement pref, v4

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #13)
> > The pref is going to exist, since you set it in firefox.js.
> 
> I think it's unwise to count on *any* default pref.
> But if you want to break the browser in such cases, fine.

Which cases concretely?

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 448194 [details] [diff] [review]
Implement pref, v4

>+  var newTabURL = gPrefService.getCharPref("browser.tabs.newTabPage");

You should probably use gPrefService.getComplexValue(..., Ci.nsISupportsString).data to support unicode values.

>+  //var newTabURL = gPrefService.getComplexValue("browser.tabs.newTabPage",
>+  //    Ci.nsIPrefLocalizedString).data;

What's this about?

>+  if (gInitialPages.indexOf(newTabURL) == -1)
>+    gInitialPages.push(newTabURL);

Please avoid as per comment 12.

Comment 17

[Ben Bucksch (:BenB)]

> You should probably use gPrefService.getComplexValue(...,
> Ci.nsISupportsString).data to support unicode values.

I don't see a reason to support Unicode URLs.

> +  //    Ci.nsIPrefLocalizedString).data;
> What's this about?

I'll remove it.

>+  if (gInitialPages.indexOf(newTabURL) == -1)
>+    gInitialPages.push(newTabURL);

> Please avoid as per comment 12.

What's the problem with these 2 lines?
It should be easy for extensions to hook up, and they shouldn't mess with browser-internal variables.
I think that should stay.

Comment 18

[Ben Bucksch (:BenB)]

Attachment: Implement pref, v5

Removed commented-out code.
Keeing gInitialPages population, so that extensions don't have to overlay just for that, and mess with browser-internal variables.

Comment 19

[Ben Bucksch (:BenB)]

Re comment 8:
> The current [TabOpen] patch does not work yet for target="_blank" links and
> window.open().

Any idea where to hook up to fix that?

Comment 20

[Dão Gottwald [:dao]]

(In reply to comment #17)
> > You should probably use gPrefService.getComplexValue(...,
> > Ci.nsISupportsString).data to support unicode values.
> 
> I don't see a reason to support Unicode URLs.

That you expect users to set custom URLs seems like a good reason.

> >+  if (gInitialPages.indexOf(newTabURL) == -1)
> >+    gInitialPages.push(newTabURL);
> 
> > Please avoid as per comment 12.
> 
> What's the problem with these 2 lines?

They don't make much sense, and it's not just two lines but those in URLBarSetURI too.

> It should be easy for extensions to hook up, and they shouldn't mess with
> browser-internal variables.

It's not "internal". It's intentionally global and extensible, like many of the things that extensions have access to.

Comment 21

[Ben Bucksch (:BenB)]

> it's not just two lines but those in URLBarSetURI too.

These are very intentional and must stay either way. I do NOT want extensions to set a http: URL and hide that fact, for privacy reasons.

Comment 22

[Ben Bucksch (:BenB)]

Attachment: Implement pref, v6

Support Unicode URLs.

Comment 23

[Ben Bucksch (:BenB)]

> They don't make much sense,

They do, because the point of the pref is to make it very easy to hook up.

Comment 24

[Ben Bucksch (:BenB)]

Attachment: Implement pref, v7, without gInitialPages fix

Comment 25

[Ben Bucksch (:BenB)]

Any idea wrt comment 19?

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #21)
> > it's not just two lines but those in URLBarSetURI too.
> 
> These are very intentional and must stay either way. I do NOT want extensions
> to set a http: URL and hide that fact, for privacy reasons.

It doesn't effectively prevent that. It makes it slightly harder for extensions to do that, and harder for us to detect it as well (e.g. via <https://mxr.mozilla.org/addons/search?string=ginitialpages>).

Comment 27

[Dão Gottwald [:dao]]

bug 455553 added browser.newtab.url

Comment 28

[Ben Bucksch (:BenB)]

Cool, thanks!

Comment 30

[Ben Bucksch (:BenB)]

See also bug 722263.
2 years later... :)