Closed Bug 722263 Opened 10 years ago Closed 9 years ago

New Tab Page: changing browser.newtab.url require restart.

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13
Tracking Status
firefox12 + verified
firefox13 --- verified

People

(Reporter: tabmix.onemen, Assigned: ttaubert)

References

Details

(Keywords: addon-compat, Whiteboard: [qa+])

Attachments

(1 file, 2 obsolete files)

Changing browser.newtab.url require restart.

In my opinion changing to browser.newtab.url need to be in effect immediately without restart.
Duplicate of this bug: 722660
This will be a major issue for add-ons that want to override the new tab page.
Attached patch patch v1 (obsolete) — Splinter Review
Adding a pref observer to update the value when changed.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #595730 - Flags: review?(dao)
Comment on attachment 595730 [details] [diff] [review]
patch v1

The observer needs to be removed when the window closes in order to not keep it alive.
Attachment #595730 - Flags: review?(dao) → review-
Oops, right :/ How about using nsISupportsWeakReference for the observer instead of explicitly removing it?
I don't know, I think this may cause the observer to get lost when it shouldn't.
Depends on: 729063
No longer depends on: 729063
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #595730 - Attachment is obsolete: true
Attachment #604887 - Flags: review?(dao)
Comment on attachment 604887 [details] [diff] [review]
patch v2

>+  let update = function () {
>+    let desc = {enumerable: true, value: getNewTabPageURL()};
>+    Object.defineProperty(this, "BROWSER_NEW_TAB_URL", desc);
>+  }.bind(this);

How exactly is this better than:

>+  function update() {
>+    BROWSER_NEW_TAB_URL = getNewTabPageURL();
>+  }
By defining a read-only getter we're not making anyone think BROWSER_NEW_TAB_URL is a setter that modifies the preference behind it. If you think we don't need to cover that case then of course just changing a property value is easier and has less overhead.
It won't be read-only at the time you're adding the observer. It would only be read-only once the pref changed once, which doesn't seem to make sense.
And yes, I don't think we need to worry about this and can just leave it writable.
(In reply to Dão Gottwald [:dao] from comment #10)
> It won't be read-only at the time you're adding the observer. It would only
> be read-only once the pref changed once, which doesn't seem to make sense.

Hmm, didn't know that defineLazyGetter() doesn't work that way.

(In reply to Dão Gottwald [:dao] from comment #11)
> And yes, I don't think we need to worry about this and can just leave it
> writable.

Ok, will change that part.
Attached patch patch v3Splinter Review
Removed the usage of Object.defineProperty().
Attachment #604887 - Attachment is obsolete: true
Attachment #604992 - Flags: review?(dao)
Attachment #604887 - Flags: review?(dao)
Attachment #604992 - Flags: review?(dao) → review+
https://hg.mozilla.org/integration/fx-team/rev/bb271ef702c6
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 12 → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/bb271ef702c6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Comment on attachment 604992 [details] [diff] [review]
patch v3

[Approval Request Comment]
User impact if declined: browser.newtab.url pref requires restart, addon-compat with the new pref affected
Testing completed (on m-c, etc.): no problems since its landing for fx13
Risk to taking this patch (and alternatives if risky): backing out the new way to define the url used for new tabs which is imo a bigger risk
String changes made by this patch: none

This would solve the issues raised in bug 740340. We're providing the new preference to define the url used for new tabs but add-on authors can't use it right now because it requires a restart to take effect. This is of course nonsense for restartless add-ons and might also confuse users that manually change this pref.
Attachment #604992 - Flags: approval-mozilla-beta?
Comment on attachment 604992 [details] [diff] [review]
patch v3

[Triage Comment]
This patch should be fairly low risk, and will prevent add-on compatibility fallout after FF12's release. Approved for Beta 12.

I believe this also affects Aurora 13, so approving for that branch as well.
Attachment #604992 - Flags: approval-mozilla-beta?
Attachment #604992 - Flags: approval-mozilla-beta+
Attachment #604992 - Flags: approval-mozilla-aurora+
Comment on attachment 604992 [details] [diff] [review]
patch v3

This landed on mozilla-central in the Firefox 13 phase, which means that this is currently on aurora.
Attachment #604992 - Flags: approval-mozilla-aurora+
Thanks
Whiteboard: [qa+]
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0

Verified in Firefox 12 beta 4 on Ubuntu 11.10, Mac Os 10.6 and Windows XP. The new tab page is accessible without a restart after changing the pref.
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120424 Firefox/13.0a2
Also verified on Aurora 13 with Windows 7, mac OS 10.6 and ubuntu 11.10. the pref can be toggled without restart.
You need to log in before you can comment on or make changes to this bug.