Last Comment Bug 531526 - [SM2.0.1] Workaround browser.toolbars.showbutton.* prefs that should not have been migrated from 1.1
: [SM2.0.1] Workaround browser.toolbars.showbutton.* prefs that should not have...
Status: RESOLVED FIXED
: fixed-seamonkey2.0.1, fixed-seamonkey2.0.3
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: SeaMonkey 2.0 Branch
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Philip Chee
:
:
Mentors:
Depends on:
Blocks: 549395
  Show dependency treegraph
 
Reported: 2009-11-28 06:57 PST by Philip Chee
Modified: 2010-03-01 12:11 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 gButtonPrefListener (2.93 KB, patch)
2009-12-03 09:46 PST, Philip Chee
neil: review+
Details | Diff | Splinter Review
Patch 1.1 make the regexp match those buttons exactly (2.90 KB, patch)
2009-12-04 08:24 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch 1.2 use pref.clearUserPref() instead r=neil (2.90 KB, patch)
2009-12-04 08:34 PST, Philip Chee
philip.chee: review+
neil: superreview+
neil: approval‑seamonkey2.0.1+
Details | Diff | Splinter Review
[Checkin: comment 10] Patch v1.3 Nit really fixed. (963 bytes, patch)
2009-12-10 02:11 PST, Philip Chee
neil: review+
neil: approval‑seamonkey2.0.3+
Details | Diff | Splinter Review

Description Philip Chee 2009-11-28 06:57:01 PST
We seem to have accidentally migrated from 1.1 all the "browser.toolbars.showbutton.*" preferences when we shouldn't. The most common case appears to be the home button. Our button pref listener is still active because of the Go and Search buttons in the URL bar. One possible symptom is that the Home button is invisible anywhere including in the toolbar customization palette window. We since these prefs are already migrated we need a way to ignore them in 2.0.1
Comment 1 Philip Chee 2009-12-03 09:46:25 PST
Created attachment 415891 [details] [diff] [review]
Patch v1.0 gButtonPrefListener

> +        if (/(bookmarks|home|print)/.test(item.substr(this.domain.length+1)))
> +          Application.prefs.get(item).reset();

Clear userpref for those button prefs which we should not have migrated.

> -  MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.bookmarks",   Bool),
> -  MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.home",        Bool),
> -  MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.print",       Bool),

Fix the migrator code.
Comment 2 neil@parkwaycc.co.uk 2009-12-03 14:40:05 PST
Comment on attachment 415891 [details] [diff] [review]
Patch v1.0 gButtonPrefListener

In BrowserToolboxCustomizeDone() you call gButtonPrefListener.updateButton twice (although I can't remember why). Is there any point trying to do anything else here? Or maybe split it into two listeners, one for go and one for search?

>-  MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.bookmarks",   Bool),
>-  MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.home",        Bool),
>-  MAKESAMETYPEPREFTRANSFORM("browser.toolbars.showbutton.print",       Bool),
These are obviously correct, sigh...
Comment 3 neil@parkwaycc.co.uk 2009-12-04 08:22:41 PST
Comment on attachment 415891 [details] [diff] [review]
Patch v1.0 gButtonPrefListener

>+        if (/(bookmarks|home|print)/.test(item.substr(this.domain.length+1)))
Let's have a stronger regexp than this. (A good enough one will avoid the need for a substr.) r=me with this fixed.

>+          Application.prefs.get(item).reset();
Nit: I'd prefer pref.clearUserPref to match the rest of this object.
Comment 4 Philip Chee 2009-12-04 08:24:06 PST
Created attachment 416096 [details] [diff] [review]
Patch 1.1 make the regexp match those buttons exactly

As discussed over #irc I've changed the regexp to 
if (/\.(bookmarks|home|print)$/.test(item))
Comment 5 Philip Chee 2009-12-04 08:34:53 PST
Created attachment 416101 [details] [diff] [review]
Patch 1.2 use pref.clearUserPref() instead r=neil

>>+          Application.prefs.get(item).reset();
> Nit: I'd prefer pref.clearUserPref to match the rest of this object.

Fixed.
Comment 7 neil@parkwaycc.co.uk 2009-12-10 01:22:06 PST
(In reply to comment #5)
>Created an attachment (id=416101)
>Patch 1.2 use pref.clearUserPref() instead r=neil
>>>+          Application.prefs.get(item).reset();
>> Nit: I'd prefer pref.clearUserPref to match the rest of this object.
>Fixed.
Oh dear. Oh dear oh dear. How could I forget to check. D'oh!
Comment 8 Philip Chee 2009-12-10 02:04:21 PST
>>> Nit: I'd prefer pref.clearUserPref to match the rest of this object.
>>Fixed.
> Oh dear. Oh dear oh dear. How could I forget to check. D'oh!
Hmm. Aaargh. Sorry about the braino. I could have sworn I had fixed it.
....
In fact I have it fixed locally. Oh drat I think I forgot to do hg qrefresh.
Comment 9 Philip Chee 2009-12-10 02:11:21 PST
Created attachment 416881 [details] [diff] [review]
[Checkin: comment 10] Patch v1.3 Nit really fixed.

Really fixed. (fingers crossed)
Comment 10 Jens Hatlak (:InvisibleSmiley) 2009-12-10 14:37:23 PST
Comment on attachment 416881 [details] [diff] [review]
[Checkin: comment 10] Patch v1.3 Nit really fixed.

http://hg.mozilla.org/comm-central/rev/b228857ea6fb
http://hg.mozilla.org/releases/comm-1.9.1/rev/926cae605771
Comment 11 Jens Hatlak (:InvisibleSmiley) 2009-12-10 14:38:39 PST
Please change status as needed, probably after verification with a nightly that contains the change.
Comment 12 Philip Chee 2009-12-12 09:39:46 PST
Confirm fixed.
Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9.1.7pre) Gecko/20091212 Lightning/1.0b2pre SeaMonkey/2.0.2pre

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