Last Comment Bug 722263 - New Tab Page: changing browser.newtab.url require restart.
: New Tab Page: changing browser.newtab.url require restart.
Status: RESOLVED FIXED
[qa+]
: addon-compat
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 722660 (view as bug list)
Depends on: 908110
Blocks: 455553
  Show dependency treegraph
 
Reported: 2012-01-30 03:22 PST by tabmix.onemen
Modified: 2013-09-04 12:04 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified


Attachments
patch v1 (1.08 KB, patch)
2012-02-09 07:25 PST, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
patch v2 (1.29 KB, patch)
2012-03-12 04:26 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v3 (1.20 KB, patch)
2012-03-12 10:10 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description tabmix.onemen 2012-01-30 03:22:01 PST
Changing browser.newtab.url require restart.

In my opinion changing to browser.newtab.url need to be in effect immediately without restart.
Comment 1 Dão Gottwald [:dao] 2012-01-31 04:24:43 PST
*** Bug 722660 has been marked as a duplicate of this bug. ***
Comment 2 Mike Kaply [:mkaply] 2012-02-08 09:45:57 PST
This will be a major issue for add-ons that want to override the new tab page.
Comment 3 Tim Taubert [:ttaubert] 2012-02-09 07:25:05 PST
Created attachment 595730 [details] [diff] [review]
patch v1

Adding a pref observer to update the value when changed.
Comment 4 Dão Gottwald [:dao] 2012-02-09 07:39:17 PST
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.
Comment 5 Tim Taubert [:ttaubert] 2012-02-09 07:45:57 PST
Oops, right :/ How about using nsISupportsWeakReference for the observer instead of explicitly removing it?
Comment 6 Dão Gottwald [:dao] 2012-02-09 08:08:04 PST
I don't know, I think this may cause the observer to get lost when it shouldn't.
Comment 7 Tim Taubert [:ttaubert] 2012-03-12 04:26:40 PDT
Created attachment 604887 [details] [diff] [review]
patch v2
Comment 8 Dão Gottwald [:dao] 2012-03-12 09:18:34 PDT
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();
>+  }
Comment 9 Tim Taubert [:ttaubert] 2012-03-12 09:38:43 PDT
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 Dão Gottwald [:dao] 2012-03-12 09:41:45 PDT
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 Dão Gottwald [:dao] 2012-03-12 09:43:48 PDT
And yes, I don't think we need to worry about this and can just leave it writable.
Comment 12 Tim Taubert [:ttaubert] 2012-03-12 09:46:46 PDT
(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.
Comment 13 Tim Taubert [:ttaubert] 2012-03-12 10:10:45 PDT
Created attachment 604992 [details] [diff] [review]
patch v3

Removed the usage of Object.defineProperty().
Comment 14 Tim Taubert [:ttaubert] 2012-03-12 19:09:10 PDT
https://hg.mozilla.org/integration/fx-team/rev/bb271ef702c6
Comment 15 Tim Taubert [:ttaubert] 2012-03-13 03:37:15 PDT
https://hg.mozilla.org/mozilla-central/rev/bb271ef702c6
Comment 16 Tim Taubert [:ttaubert] 2012-04-02 03:09:29 PDT
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.
Comment 17 Alex Keybl [:akeybl] 2012-04-02 10:21:03 PDT
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.
Comment 18 Dão Gottwald [:dao] 2012-04-02 10:40:34 PDT
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.
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-02 15:25:02 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/f738e3a8091a
Comment 20 Ben Bucksch (:BenB) 2012-04-02 17:16:58 PDT
Thanks
Comment 21 Virgil Dicu [:virgil] [QA] 2012-04-05 08:00:49 PDT
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 Virgil Dicu [:virgil] [QA] 2012-04-24 07:59:11 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.