241729 - Preference- Advanced-Scripts and Plugins can not be locked

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Boying Lu]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4.1) Gecko/20040406
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4.1) Gecko/20040406

lock all the items in the Preference- Advanced-Scripts and Plugins
 in autoconfig.jsc by using prefLock()

But in fact, all these items are not disabled 




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

Comment 1

[Boying Lu]

Attachment: patch v0

Comment 2

[Boying Lu]

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

Can you give a r?
I have tested the patch and it works.

Thanks

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to review this for a while (probably weeks).  I suggest
asking an xpfe peer to do so.

Comment 4

[Boying Lu]

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

Can you give r?
Thanks

Comment 5

[[e-mail]]

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

Style nits:
>+function setState(var component, state) {
no "var" in parameter lists

>+  var isLocked;
>+  var prefString;
>+                                                                                
>+  prefString = component.getAttribute("prefstring");
>+  isLocked = parent.hPrefWindow.getPrefIsLocked(prefString);
combine declaration and definition

>+  setState(document.getElementById("allowScripts"),state);
>+  setState(document.getElementById("allowWindowMoveResize"),state);
>+  setState(document.getElementById("allowImageSrcChange"),state);
>+  setState(document.getElementById("allowWindowStatusChange"),state);
>+  setState(document.getElementById("allowWindowFlip"),state);
>+  setState(document.getElementById("allowHideStatusBar"),state);
>+  setState(document.getElementById("allowContextmenuDisable"),state);
space after comma

Could you move the document.getElementById into the setState function?

Comment 6

[Boying Lu]

Attachment: patch v1

This patch is made according to neil's sugguestion. 
Thanks neil.

Comment 7

[Boying Lu]

Comment on attachment 147286 [details] [diff] [review]
patch v1

Can you give r now?

Comment 8

[Boying Lu]

Comment on attachment 147286 [details] [diff] [review]
patch v1

Can you give sr?
Thanks

Comment 9

[Boying Lu]

Comment on attachment 147286 [details] [diff] [review]
patch v1

Hi, Bryner,
  Can you give sr?

Thanks

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 147286 [details] [diff] [review]
patch v1

I'm not all that familiar with this code.  However... why is this locking check
needed for this particular pref panel, when I don't see it being used in any
others?

Comment 11

[Boying Lu]

User can lock these preferences in autoconfig.jsc and if usr wants to do
this, Mozilla should disable this items.

I want a sr. Would you please tell me which super reviewer can I contract?

Thanks a lot

Comment 12

[Boying Lu]

Comment on attachment 147286 [details] [diff] [review]
patch v1

Hi, Byner,
  How about this patch now?

Comment 13

[Boying Lu]

Comment on attachment 147286 [details] [diff] [review]
patch v1

Can you give sr now?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Boying Lu, I'm unfortunately not doing sr's at the moment (see the reviewers
page).  I'd recommend jag or alecf as srs for this patch...

Note that bryner asked a question that never quite got answered (that being what
makes this pref different from _other_ prefs that may also be locked).  I assume
the answer has to do with something this pref panel does that would override the
WSM?

Comment 15

[Boying Lu]

BZ, 
  Thanks for your feedback. This is a common bug among preferences. The root
cause is that the prefstring's state (locked or unlocked) is not considered when
setting these prefernces states.

Comment 16

[Boying Lu]

Comment on attachment 147286 [details] [diff] [review]
patch v1

Can you give sr?
Thanks

Comment 17

[jag (Peter Annema)]

Comment on attachment 147286 [details] [diff] [review]
patch v1

I think we need this special handling here because we dynamically change the
"disabled" state for these preferences' UI. Most other places don't do that, so
they don't need any special handling for the locking mechanism.

One way to make it clear in the code is to change setState to something like:

function setState(id, state) {
  var component = document.getElementById(id);
  var prefString = component.getAttribute("prefstring");
  if (parent.hPrefWindow.getPrefIsLocked(prefString))
    return;

  component.disabled = state;
}

In other words, don't touch the disabled state (which _should_ be true) when
the pref is locked.

Looking at some other pref panels that call getPrefIsLocked, they could use a
similar helper function. But that'd be a different bug.

Though in practice equivalent, I'd prefer it if you were to change setState to
either do the early return like above, or change the last line to:

  component.disabled = isLocked || state;

to indicate that isLocked is the first thing to look at, and only if it's not
locked do we care about the state. (Nit picking? I guess so).

Also, perhaps the function name should be changed to setDisabled or
setDisabledState, setState implies something more generic than it really is.

sr=jag with those changes.

Comment 18

[Boying Lu]

Attachment: patch v2

Jay, 
  Thanks for your comments. I made the patch based on these comments.

  Can you give sr and check in the trunk now?

Thanks

Comment 19

[Boying Lu]

Hi, Jag,
  Can you give r/sr now?
Thanks

Comment 20

[Boying Lu]

Comment on attachment 148480 [details] [diff] [review]
patch v2

Hi, Neil,
  I have remade the patch according to Jag's comments.
  Would you please give r again?
Thanks

Comment 21

[Boying Lu]

Comment on attachment 148480 [details] [diff] [review]
patch v2

Hi, Neil,
  I have made some modifications on the patch based on Jag's comments.
  Would you please give r?
Thanks a lot

Comment 22

[Boying Lu]

Comment on attachment 148480 [details] [diff] [review]
patch v2

Hi, Neil,
  How about this patch?
Can you give r now?
Thanks

Comment 23

[Boying Lu]

Comment on attachment 148480 [details] [diff] [review]
patch v2

Hi, Neil,
  It seems that you have no time to reivew the new patch.
  If so, would you please tell me who can reiview the patch?
  Thanks

Comment 24

[[e-mail]]

Comment on attachment 148480 [details] [diff] [review]
patch v2

Trivial changes (renaming the function) don't need a new r. And I'm not CC'd on
the bug, so I didn't notice your requests, but sorry for the delay.

Comment 25

[Boying Lu]

Comment on attachment 148480 [details] [diff] [review]
patch v2

Jag,
  Can you give sr now?
thanks

Comment 26

[jag (Peter Annema)]

Comment on attachment 148480 [details] [diff] [review]
patch v2

sr=jag

Comment 27

[Boying Lu]

Hi, jag and nei,
  Thanks very much, Could you help me to checkin the patch? Because I have no
permission to that.

Thanks

Comment 28

[Boying Lu]

Neil,
  Would you please help me to checkin the patch? Because I have no permission to
do that.

Thanks

Comment 29

[Boying Lu]

Jag,
  Neil seems no time to checkin the patch. Would you please help me to do that?

Thanks

Comment 30

[Frank Wein [:mcsmurf]]

patch has been checked in:
2004-06-23 00:55	pete.zha%sun.com 	mozilla/ xpfe/ components/ prefwindow/
resources/ content/ pref-scripts.js 	1.17 	14/7  	bug 241729
Preference- Advanced-Scripts and Plugins can not be locked
neil.parkwaycc.co.uk: review+
jag: superreview+
patch by brian.lu%sun.com