Closed Bug 740340 Opened 12 years ago Closed 12 years ago

Remove half finished new tab code from Firefox 12

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Version:
12 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox12 - ---

People

(Reporter: mkaply, Unassigned)

References

Details

We're trying to get our add-on that overrides the new tab page working on Firefox 12.

Unfortunately because half of the features of the new new tab page are in the code, things are breaking.

It's very difficult to build an add-on that will work in 11, 12, and 13 that does anything to the new tab.

In particular, if you set browser.newtab.url to a value, even though browser.newtabpage.enabled is false, the page is still displayed as the new tab.

The reason we had to set that value is because the new code in utilityOverlay.js - http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#73 checks that value to see if it is a "blank" URL.

so the new tab code is actively interfering with about:blank in Firefox 12.

Also, about:newtab should not be in gInitialPages in browser.js yet, since it's not a valid page.

This feature should not have landed half done.

It should have been completed on a branch before dropping into Firefox.
The primary function that needs to be changed is this one:

function BrowserOpenTab()
{
  openUILinkIn(BROWSER_NEW_TAB_URL, "tab");
}

This should go back to about:blank until Firefox 13.
Blocks: 455553
This bug effectively asks to back out bug 455553 "Part 3" until it's working.
tracking-firefox12: --- → ?
(In reply to Ben Bucksch (:BenB) from comment #2)
> This bug effectively asks to back out bug 455553 "Part 3" until it's working.

Tim - is there any benefit to leaving bug 455553 in FF12? How risky would it be to perform the backout? Thanks!
tracking-firefox12: ? → +
(In reply to Michael Kaply (mkaply) from comment #0)
> Unfortunately because half of the features of the new new tab page are in
> the code, things are breaking.

The main difference between Fx12 and 13 is that about:newtab in 13 has a new layout and some (mostly) papercut bugs fixed.

> In particular, if you set browser.newtab.url to a value, even though
> browser.newtabpage.enabled is false, the page is still displayed as the new
> tab.

browser.newtab.url is for add-ons and everybody. browser.newtabpage.enabled is for the built-in about:newtab. Don't use that - why would you want to? This won't work in Fx13 as well.

> The reason we had to set that value is because the new code in
> utilityOverlay.js -
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> utilityOverlay.js#73 checks that value to see if it is a "blank" URL.

What exactly isn't working? This bug report is too vague to determine if it justifies backing out the newtab changes.

> Also, about:newtab should not be in gInitialPages in browser.js yet, since
> it's not a valid page.

You can load it - it's valid page.
Yeah, I really don't understand this bug. Can you clarify exactly what you're trying to do, and why it isn't working?
I misunderstood what browser.newtabpage.enabled was for I assumed (and I think logically so) that enables any and all new tab functionality (including the use of the new browser.newtab.url preference).

A better name might have been browser.aboutnewtab.enabled

My problem with the code as it stands is that by making browser.newtab.url available in Firefox 12, you're implying that it works.

It doesn't.

If you set browser.newtab.url it does nothing until you restart the browser.

It works properly in Firefox 13.

So you've created a situation where an add-on should not actually use browser.newtab.url until Firefox 13.

I'd like to build an add-on that uses the new tab code that works in Firefox 12 and Firefox 13 without a version check.

That's not possible with the state of things right now.

You should remove the code that uses browser.newtab.url for Firefox 12 (one function - BrowserOpenTab) so that add-ons aren't tempted to use browser.newtab.url to get new tab functionality and so add-ons can properly use browser.newtab.url without the risk of breaking their add-on in Firefox 12 to get it working in Firefox 13.
(In reply to Michael Kaply (mkaply) from comment #6)
> If you set browser.newtab.url it does nothing until you restart the browser.

That was fixed in bug 722263. We can backport it to 12 if you think that's important.

Is that the only issue you have with the new tab page functionality? "Half finished" implies there are more serious issues, but I'm not aware of any.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> (In reply to Michael Kaply (mkaply) from comment #6)
> > If you set browser.newtab.url it does nothing until you restart the browser.
> 
> That was fixed in bug 722263. We can backport it to 12 if you think that's
> important.

No, I don't think that should be backported. That would only encourage people to use the partial new tab functionality in Firefox 12 when they should wait until Firefox 13. The preferences and none of the other UI is available.

I've already indicated the change I think should be made. The browser.newtab.url should be ignored in Firefox 12.

> Is that the only issue you have with the new tab page functionality? "Half
> finished" implies there are more serious issues, but I'm not aware of any.

Yeah, my apologies. I was frustrated out how much the new tab code was interfering with custom new tab functionality without itself being functional.

I'm actually quite happy about the new tab because it removes the zillion hacks that add-on developers do to get a new tab page. (You would probably cry if you saw some of them.)

I still stand by my statement that this was dropped too early. There should have been more work before this much unused code ended up in a release.
I think I understand what you're saying now. It sounds like you want to use browser.newtab.url to override our start page, but then also somehow piggy-back on the built-in about:newtab code. You're saying that you can do this in 13, but can't do it in 12 because the code that's in 12 isn't "complete enough" (but it's not quite clear to me which parts of it you're relying on exactly - being explicit about this might clarify things further).

The intended use case for browser.newtab.url was to completely replace our tab page, without relying on any of the Firefox about:newtab page code. I don't understand why removing this capability from 12 would help you.
Well, in our case we were overriding BrowserOpenTab explicitly and replacing about:blank with our URL (not my idea, but that's how it was done). 

I've already worked around my problem.

I think you're misunderstanding what I'm saying though.


The problem here is that you've given add-on developers just enough rope to hang themselves (and users) in Firefox 12.

You've given them a preference that if they set, it looks like they've replaced the new tab page, but in reality it's broke and a user can't change it back via the UI.

If I wanted to modify my add-on today in anticipation of the Firefox 13 new tab page, I simply can't do it. Because if I use browser.newtab.url my add-on is broken. Because I can't enable and disable it on the fly.

Please don't allow browser.newtab.url to do anything in Firefox 12. It should not be functional until the rest of the new tab code has dropped.
(In reply to Michael Kaply (mkaply) from comment #10)
> Because I can't enable and disable it on the fly.

So it sounds to me as bug 722263 is exactly what you need (and should want). Other than that you didn't name any problems that "break" your add-on. That's iirc the only bug that really changed any behavior between Fx12 and Fx13 regarding the browser.newtab.url preference. It's still not clear to me which problems you see in Fx12 that are fixed in Fx13 (except bug 722263).
(In reply to Michael Kaply (mkaply) from comment #10)
> Well, in our case we were overriding BrowserOpenTab explicitly and replacing
> about:blank with our URL (not my idea, but that's how it was done). 

The goal of browser.newtab.url was to make that kind of hack unnecessary.

> You've given them a preference that if they set, it looks like they've
> replaced the new tab page, but in reality it's broke and a user can't change
> it back via the UI.

This is the part that I don't understand. If an addon is customizing the new tab page URL, why would it do so using a URL that is "broken"?

> If I wanted to modify my add-on today in anticipation of the Firefox 13 new
> tab page, I simply can't do it. Because if I use browser.newtab.url my
> add-on is broken. Because I can't enable and disable it on the fly.

There's no preference UI in any version of Firefox (13 or otherwise) that lets users control which new tab page they get, or re-set browser.newtab.url to its default value. If your add-on wants to provide UI for enabling/disabling its custom new tab page functionality "on the fly", it's free to do that...
Sorry, I was going by the original spec that did have preference UI. I didn't realize that had been removed.

So when an add-on replaces the new tab page, it's going to be the add-ons responsibility to maintain that. And provide a way to unto. It's keyword.URL all over again :)

> This is the part that I don't understand. If an addon is customizing the new tab page URL, why would it do so using a URL that is "broken"?

What I'm saying is that when an add-on sets this URL, nothing happens. They have to restart the browser. And when they unset it, nothing happens. They have to restart the browser. So if an add-on was using this pref, it would not be clear to the user why it wasn't working.


So I was always under the impression that browser.newtab.url was somehow tied to about:newtab. If that's not the case, by all means, just backport the fix.

My only point in all this was that the state it is in today is broke. So other remove the broken code or fix it.

But you should be warned that if you make this available in Firefox 12, lots of add-on developers are going to see it.

So people might not even see the pretty new new tab page :)
I agree with Mike's last comment. Please either backport bug 722263 or backout bug 455553 "Part 3". My preference being the former, this is would be quite useful and do the same as patch in bug 561749.
Let's just backport bug 722263. Tim, can you do that?
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Yes, will do.
You need to log in before you can comment on or make changes to this bug.