202388 - runtime switch between system prefs and mozilla prefs

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Bolian Yin]

bug#195708 give mozilla the ability to use system preferences, and that feature
take effect when mozilla starts up.

It is better that user can switch between mozilla prefs and system prefs when
mozilla is running. This bug will do that work. All the UI and code are only
have effect when the system pref feature is enabled.

Comment 1

[Bolian Yin]

Attachment: patch

alecf, would you like to review this bug also?

Comment 2

[Alec Flett]

Comment on attachment 120810 [details] [diff] [review]
patch 

this seems like a lot of work and I'm not sure of the value - I mean when you
save the current mozilla prefs, aren't you going to lose that as soon as you
shut down? Or maybe that's the intention? This makes me think that prefs needs
a way to have runtime overriding of prefs before even hitting its own
hashtable.

I also don't see the advantage to the changes to nsPrefWindow.js (nor comments
explaining whats going on there)

+<!ENTITY systemPrefDescription.label	    "By checking this, system
preferences will be used in stead of that of mozilla">

"instead" is one word... I think this needs to be rephrased ("By checking this,
" sounds awkward) but actually I can't think of a decent way of phrasing this
extra description, such that it will be more informative to the user. I will
think about it and see if I can help come up with a better description.

I'm marking this r- for now because of the changes to nsPrefWindow - I don't
see how they're necessary.

Comment 3

[Bolian Yin]

Alecf, Thanks for your comments.
We don't need to save current mozilla prefs at shutdown. We know which prefs
(system's or mozilla's) should be read at the next time startup. The saved
current mozilla prefs are only used when change back to mozilla prefs at runtime.

Sorry for the lack of comments in the nsPrefWindow.js changes. I will add in the
next revision of the patch. Now, I explain below:

//////////////////////////////////////////////////////////////
// This make sure the pref are only saved when the pref value is changed and 
// the pref is not locked. (it did not deal with the lock issure before)
//////////////////////////////////////////////////////////////////
-                        if( value != this.getPref( preftype,
itemObject.prefstring ) )
+                        if( !this.getPrefIsLocked(itemObject.prefstring) &&
+							(value != this.getPref( preftype, itemObject.prefstring)))
                           {
                             this.setPref( preftype, itemObject.prefstring, value );
                           }


////////////////////////////////////////////////////////////////////////////////
// This will make the page update widgets states not only when it is first 
// loaded, but also when there are system pref switch. (for example, 
// refect the lock status change).
// 
///////////////////////////////////////////////////////////////////////////////
-            if( !(aPageTag in this.wsm.dataManager.pageData) )
+            if(!(aPageTag in this.wsm.dataManager.pageData) ||
+               (this.pagePrefChanged && (!(aPageTag in this.pagePrefUpdated))))
               {

I believe this patch will make the system pref reading feature easy to use. 
I will wait for your better Description on the UI.

Comment 4

[Bolian Yin]

Attachment: patch

Hi Alecf, I made a new patch, which includes the explain of the changes in
nsPrefwindow.js. I also make a new description of the switch in the UI, which
is:

<!ENTITY systemPrefDescription.label	   "check this item to use preferences
from the system, the related mozilla preferences will be shadowed">

Can you review again?  Thanks.

Comment 5

[Alec Flett]

Comment on attachment 123473 [details] [diff] [review]
patch 

I don't like "config.system-pref" as a pref name - it doesn't explain to me
what it actually means.

Also, you can use the NS_LossyConvertUCS2toASCII here - prefnames are always
ASCII (I really need to document that)

as for the description, you need to capitalize "Check". the comma is also
wrong. how about

"Check this item to inherit preferences from the system. The system settings
will override the mozilla preferences"

(though you have to get "mozilla" from brand.dtd!)

-			 if( value != this.getPref( preftype,
itemObject.prefstring ) )
+			 // make sure the pref are only saved when the pref
+			 // value is changed and the pref is not locked.
+			 if( !this.getPrefIsLocked(itemObject.prefstring) &&
+			    (value != this.getPref( preftype,
itemObject.prefstring)))

I'm not sure I understand (or at least agree with) the behavior you're defining
in nsPrefWindow.js - you're not setting the pref if it is locked - we still
want to update the UI, even if it is locked!

Comment 6

[Bolian Yin]

How about use "system.preference" instead of "config.system-pref"? or you
suggest another one?

Where to use NS_LossyConvertUCS2toASCII, my chars in pref string are all ASCII.

I agree your new description.

+			 // make sure the pref are only saved when the pref
+			 // value is changed and the pref is not locked.

The comments here are bad, how about change it to:

+			 // the pref is not saved, if the pref value is not
+                        // changed or the pref is locked.

is that clear for you?

Comment 7

[Bolian Yin]

How about use "system.preference" instead of "config.system-pref"? or you
suggest another one?

Where to use NS_LossyConvertUCS2toASCII, my chars in pref string are all ASCII.

I agree your new description.

+			 // make sure the pref are only saved when the pref
+			 // value is changed and the pref is not locked.

The comments here are bad, how about change it to:

+			 // the pref is not saved if the pref value is not
+                        // changed or the pref is locked.

Do you think it is better?

Comment 8

[Alec Flett]

Oh, I thought "config." was good - but it needs to be more descriptive. How
about "config.use-system-prefs" ?

you can use NS_LossyConvertUCS2toASCII() in the Observe() methods, instead of
NS_ConvertUCS2toUTF8()

I'll go over the patch once more, but perhaps with those comments it will make
more sense

Comment 9

[Bolian Yin]

Ok, Alecf.  I will make a new patch after you finish the review.

Comment 10

[Alec Flett]

Comment on attachment 123473 [details] [diff] [review]
patch 

ok, this is reasonable for 1.5alpha.. the only thing is the pref name. I meant
something that describes more accurately what this is

how about
"config.use_system_prefs"?

sr=alecf with that (

Comment 11

[Bolian Yin]

Ok, I will change that name. Thanks for the sr=  :)

Comment 12

[Bolian Yin]

Attachment: patch for checkin(add Alecf's comments)

Comment 13

[Bolian Yin]

checked in trunk.

Comment 14

[Alec Flett]

bolin, you need both a super reviewer AND a reviewer before you check in
patches. You should not have checked this in until you had a reviewer.

Comment 15

[Bolian Yin]

Sorry, Alecf. I thought you review through all the details and that is enough
for both r= and sr=. 
reopen the bug, and wait for a review.

Comment 16

[Kyle Yuan]

Comment on attachment 125212 [details] [diff] [review]
patch for checkin(add Alecf's comments)

r=kyle with some style nits:

>+    delete [] mSysPrefs;
I'm not sure if deleting a null pointer is safe on all platforms.

>+        if (enabled != mEnabled) {
>+            if (mEnabled)
>+                //read prefs from system
>+                rv = UseSystemPrefs();
>+            else
>+                //roll back to mozilla prefs
>+                rv = UseMozillaPrefs();
>+        }
How about 
rv = mEnabled ? UseSystemPrefs() : UseMozillaPrefs();

>+    if (!mSysPrefService) {
>+            return NS_ERROR_FAILURE;
Indent problem (or you are using tab).

>+    for (PRInt16 index = 0; index < sysPrefCount; ++index) {
I prefer to use PRInt32 as the loop counter, because it is the fastest data
type for 32-bit CPU.

>     if (mGConfLib) {
>-        PR_UnloadLibrary(mGConfLib);
>+        // PR_UnloadLibrary(mGConfLib);
Please either remove this line or add comment to explain why you comment it
out.

Comment 17

[timeless]

delete [] 0 is defined as safe by c++ ...

> I prefer to use PRInt32 as the loop counter, because it is the fastest data
> type for 32-bit CPU.

instead use PRIntn if that's your intent.

Comment 18

[Alec Flett]

(timeless is right, but thanks for the thorough review kyle!)

Comment 19

[Bolian Yin]

I will backout the patch and make a new one.

Comment 20

[Bolian Yin]

Attachment: additional patch

I made an additional patch.  Hope I can carry the sr= and r= for it. If no
objection, I want to check it in the next day. Thanks for all the reviews.

Comment 21

[Bolian Yin]

The additional patch checked in Trunk.

Comment 22

[Darin Fisher]

i'm not sure this is completely fixed.  it doesn't seem like network.proxy.type
is getting updated correctly.  i see network.proxy.http getting updated, but not
the type.  without that... it doesn't work.  mine is stuck at direct connection.

is this a known problem?  should i file a new bug?  thx!

Comment 23

[Darin Fisher]

btw, i meant to add that i am using my own build of mozilla 1.6 alpha from a few
days ago, running on RH9 linux.

Comment 24

[Bolian Yin]

Currenly, this feature only works for the "Manual Proxy Configuration", because
there is no related setting in gconf for mozilla's "network.proxy.type". So
mozilla only try its best to read the proxy settings from gconf, and these
proxies only work when you choose "Manual Proxy Configuration".

If gconf add more info in the future, this feature can be improved.

Comment 25

[Darin Fisher]

hmm.. interesting.  yet, my preferences dialog under RH9 has options for Direct,
Manual, and PAC.  how can it be that they don't store the selected option? 
peeking in gconf-editor, i see that there is a key under /system/proxy/mode
which gives the equivalent of network.proxy.type.  seems to me that the
necessary key does indeed exist in gconf.  perhaps this is a recent addition?

Comment 26

[Bolian Yin]

/system/proxy/mode?  It seems I missed it. Then we have a way to make it better.
Need a new bug for it. I'd like to fix but I am not sure when I can get the time. 
Fixing from others are welcome.

Comment 27

[Ganesh]

Attachment: corrected Patch for ftp and other new settings

The keys we look for ftp seem to be incorrect. This patch has corrected that
and added new keys like ss,socks,noproxy and manual proxy.