The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: tabmix.onemen, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, {addon-compat})

Trunk
Firefox 13
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12+ verified, firefox13 verified)

Details

(Whiteboard: [qa+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Changing browser.newtab.url require restart.

In my opinion changing to browser.newtab.url need to be in effect immediately without restart.

Updated

5 years ago
Duplicate of this bug: 722660

Comment 2

5 years ago
This will be a major issue for add-ons that want to override the new tab page.
Keywords: addon-compat
(Assignee)

Comment 3

5 years ago
Created attachment 595730 [details] [diff] [review]
patch v1

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-
(Assignee)

Comment 5

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

Updated

5 years ago
Depends on: 729063
(Assignee)

Updated

5 years ago
No longer depends on: 729063
(Assignee)

Comment 7

5 years ago
Created attachment 604887 [details] [diff] [review]
patch v2
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();
>+  }
(Assignee)

Comment 9

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

Comment 12

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

Comment 13

5 years ago
Created attachment 604992 [details] [diff] [review]
patch v3

Removed the usage of Object.defineProperty().
Attachment #604887 - Attachment is obsolete: true
Attachment #604992 - Flags: review?(dao)
Attachment #604887 - Flags: review?(dao)

Updated

5 years ago
Attachment #604992 - Flags: review?(dao) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/bb271ef702c6
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 12 → Firefox 13
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bb271ef702c6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

5 years ago
tracking-firefox12: --- → ?
(Assignee)

Comment 16

5 years ago
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+

Updated

5 years ago
tracking-firefox12: ? → +
tracking-firefox13: --- → +
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+

Updated

5 years ago
tracking-firefox13: + → ---
https://hg.mozilla.org/releases/mozilla-beta/rev/f738e3a8091a
status-firefox12: --- → fixed

Comment 20

5 years ago
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.
status-firefox12: fixed → verified
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.
status-firefox13: --- → verified
Depends on: 908110
You need to log in before you can comment on or make changes to this bug.