Closed
Bug 722263
Opened 12 years ago
Closed 12 years ago
New Tab Page: changing browser.newtab.url require restart.
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 13
People
(Reporter: tabmix.onemen, Assigned: ttaubert)
References
Details
(Keywords: addon-compat, Whiteboard: [qa+])
Attachments
(1 file, 2 obsolete files)
1.20 KB,
patch
|
dao
:
review+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Changing browser.newtab.url require restart. In my opinion changing to browser.newtab.url need to be in effect immediately without restart.
Comment 2•12 years ago
|
||
This will be a major issue for add-ons that want to override the new tab page.
Updated•12 years ago
|
Keywords: addon-compat
Assignee | ||
Comment 3•12 years ago
|
||
Adding a pref observer to update the value when changed.
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Oops, right :/ How about using nsISupportsWeakReference for the observer instead of explicitly removing it?
Comment 6•12 years ago
|
||
I don't know, I think this may cause the observer to get lost when it shouldn't.
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #595730 -
Attachment is obsolete: true
Attachment #604887 -
Flags: review?(dao)
Comment 8•12 years ago
|
||
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•12 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.
Comment 10•12 years ago
|
||
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.
Comment 11•12 years ago
|
||
And yes, I don't think we need to worry about this and can just leave it writable.
Assignee | ||
Comment 12•12 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•12 years ago
|
||
Removed the usage of Object.defineProperty().
Attachment #604887 -
Attachment is obsolete: true
Attachment #604992 -
Flags: review?(dao)
Attachment #604887 -
Flags: review?(dao)
Updated•12 years ago
|
Attachment #604992 -
Flags: review?(dao) → review+
Assignee | ||
Comment 14•12 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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bb271ef702c6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•12 years ago
|
tracking-firefox12:
--- → ?
Assignee | ||
Comment 16•12 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 17•12 years ago
|
||
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•12 years ago
|
tracking-firefox13:
--- → +
Comment 18•12 years ago
|
||
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•12 years ago
|
tracking-firefox13:
+ → ---
Comment 19•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/f738e3a8091a
status-firefox12:
--- → fixed
Comment 20•12 years ago
|
||
Thanks
Comment 21•12 years ago
|
||
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.
Comment 22•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•