236782 - all preference items in "HTTP Networking" is not lockable;

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Boying Lu]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; zh-CN; rv:1.4.1) Gecko/20040102
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; zh-CN; rv:1.4.1) Gecko/20040102

If the items in the HTTP Networking are locked by autoconfig.jsc. They are not
actually locked. i.e. user can still edit these items in preferences panel.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Boying Lu]

Attachment: patch v0

Comment 2

[Boying Lu]

Comment on attachment 143265 [details] [diff] [review]
patch v0

Can you give a "r" 
Thanks

Comment 3

[Boris Zbarsky [:bzbarsky]]

Please don't add consumers of nsIPref.  It's deprecated and needs to be removed.
 Use nsIPrefService.

Comment 4

[David Hallowell]

Comment on attachment 143265 [details] [diff] [review]
patch v0

I'm not an official patch reviewer so won't be able to give you r=, Boris would
be a better person to ask

Comment 5

[Boying Lu]

Attachment: I changed nsIPref to nsIPrefService

Comment 6

[Boying Lu]

Comment on attachment 146084 [details] [diff] [review]
I changed nsIPref to nsIPrefService

Can you give r?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146084 [details] [diff] [review]
I changed nsIPref to nsIPrefService

Brian, did you test this?  There is no PrefIsLocked() method on nsIPrefService.
 You want to get that contractid as an nsIPrefBranch, then use the method on
that.

Next time please test the patch before posting it, ok?

Comment 8

[Boying Lu]

Attachment: Change nsIPrefService to nsIPref

Thanks 

I modified the patch and test it. It works now.
Can you give it r?

Comment 9

[Boying Lu]

Comment on attachment 146166 [details] [diff] [review]
Change nsIPrefService to nsIPref

Can you give a r?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146166 [details] [diff] [review]
Change nsIPrefService to nsIPref

This is just the first patch you posted.  I've already said that that's not
acceptable.

Comment 11

[Boying Lu]

Attachment: Remove the code using nsIPref

How about this one? I have tested it and it works

Comment 12

[Boying Lu]

Comment on attachment 146458 [details] [diff] [review]
Remove the code using nsIPref

Can you give r now?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146458 [details] [diff] [review]
Remove the code using nsIPref

This looks much better!

>+    var doDisable = !(enableHTTP11.selected && enableKeepAlive.checked)|| isLocked;

Space before the "||", please?

What happened to disabling the pipelining proxy pref too?  That was in the
earlier patches, right?

Comment 14

[Boying Lu]

Attachment: I added the codes for "proxy" section

Thanks for your comments. I have re-made the patch accroding to your comments

Comment 15

[Boying Lu]

Comment on attachment 146823 [details] [diff] [review]
I added the codes for "proxy" section

Can you give r now?
Thanks

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146823 [details] [diff] [review]
I added the codes for "proxy" section

r/sr=bzbarsky; looks good.  Thanks for making the changes.  Neil should
probably give this a once-over.

Comment 17

[Boying Lu]

Hi,BZ,
  Would you please help me to checkin the path? Because I have no such right yet.
Thanks 

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sure; once Neil OKs it I can check it in.

You may want to file a parallel bug on firefox to fix their copy of this pref
panel...

Comment 19

[Boying Lu]

Ok

Comment 20

[[e-mail]]

Fix checked in to the trunk.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

can someone explain why the patch wasn't as simple as changing:
     enablePipelining.disabled = doDisable;
to:
     enablePipelining.disabled |= doDisable;
?

Comment 22

[Boying Lu]

Suppose preference P's prefstring is "p", user can set a lock on "p", which
means that prefernce P should be disabled after launching Mozilla. That is what
the patch does.

If we just replace 
    enablePipelining.disabled = doDisable;
with:
    enablePipelining.disabled |= doDisable;
There will be two issues:
1. We can't disable P if p is locked.
2. if p is not locked in autoconfig.jsc and once P is disabled by changing some
other preference say Q, P will not enable again even user change the settings of Q.

Comment 23

[Boying Lu]

*** Bug 241416 has been marked as a duplicate of this bug. ***

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #22)
> If we just replace 
>     enablePipelining.disabled = doDisable;
> with:
>     enablePipelining.disabled |= doDisable;
> There will be two issues:
> 1. We can't disable P if p is locked.

Maybe I have misunderstood some things in preferences... But doesn't the
widgetstatemanager usually disable items that are locked? And wasn't, therefore,
your patch only needed because this assignment to .disabled overrode it?

> 2. if p is not locked in autoconfig.jsc and once P is disabled by changing some
> other preference say Q, P will not enable again even user change the settings
of Q.

Oh, yeah, that is a good point. so nevermind.