445015 - Migrate SeaMonkey Validation to the new prefwindow

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Ian Neal]

Comment 1

[Philip Chee]

Taking.

Comment 2

[Philip Chee]

Re-assignment didn't seem to work?? Trying again.

Comment 3

[Philip Chee]

Attachment: Patch 1.0 mozilla-central part

This part of the patch touches files in mozilla-central

>  <dialog id="crlviewer"
> +        windowtype="mozilla:crlmanager"

The Thunderbird and Firefox showCRLs() function never found an existing window since the CRL Manager window didn't have a window type and would open a new window every time that button was clicked.

> -      signersURL.setAttribute("value", ocspEntry.serviceURL);
> +      serviceURL.value = ocspEntry.serviceURL;
> +      document.getElementById("validation_pane").userChangedValue(serviceURL);

For some reason the preference isn't updated if the value of the textbox is changed programmatically via javascript.

Comment 4

[Philip Chee]

Attachment: Patch 1.0 comm-central part

> -    document.documentElement.openWindow("Mozilla:CRLManager", "chrome://pippki/content/crlManager.xul",
> +    document.documentElement.openWindow("mozilla:crlmanager", "chrome://pippki/content/crlManager.xul",

I used all lower-case for the windowtype in the pippki CRL Manager since the windowtypes in the other windows all had lower-case names. This fixes the Thunderbird Preferences (Advanced Pane) which never worked anyway.

Comment 5

[Philip Chee]

Comment on attachment 335713 [details] [diff] [review]
Patch 1.0 mozilla-central part

Requesting review from a security peer.

Comment 6

[Philip Chee]

Attachment: Patch 1.1 mozilla-central part

>> +      document.getElementById("validation_pane").userChangedValue(serviceURL);
>For some reason the preference isn't updated if the value of the textbox is
>changed programmatically via javascript.

mfinkle points out on irc that I should just update the <preference> element directly and the change will propagate back to the textbox automatically.

Comment 7

[Kai Engert (:KaiE:)]

Philip, you have requested review on two attachments:
- attachment 335836 [details] [diff] [review]
- attachment 335714 [details] [diff] [review]

But:
- both look nearly identical!
- both appear to be in mozilla-central

The only difference is:
https://bugzilla.mozilla.org/attachment.cgi?oldid=335714&action=interdiff&newid=335836&headers=1

I think one of your attachments is wrong.

Can you please verify your attachments and request review on the latest versions of your patch?

Comment 8

[Philip Chee]

Attachment: Patch 1.0 comm-central part (for real)

> I think one of your attachments is wrong.
Sorry. I hope this one is correct.

Comment 9

[Kai Engert (:KaiE:)]

In PrefOverlay you seem to add the english word "Migrated" to the UI, but you keep the treecell in the old UI. Why?

Why do you introduce "let" into the JS code? Is that necessary, or do you consider that to be more elegant?

Reviewing code that has white space changes is hard. One constantly tries to compare code, finding out whether it's a real change, or just whitespace change. Please assist reviewers by attaching (in addition) whitespace-ignore versions of patches.

Comment 10

[Kai Engert (:KaiE:)]

I have not worked with Xul/JS/Preferences code in a while. You are changing from "prefstring" to "preference". I assume you know what you do, and you have tested that your code still stores the preferences correctly.

Comment 11

[Kai Engert (:KaiE:)]

When you work on code and make changes, please try to avoid unnecessary white space changes. And avoid unnnecessary, confusing changes. You are moving attributes around, so I see changes where they are none.

Please look at your patches yourself before you ask for review, and ask yourself, do you quickly see the change, or are you overwhelmed by lots of distracting changes?

Comment 12

[Kai Engert (:KaiE:)]

I have looked at your patches, and they look reasonable to me.
But before I give r+ I'd like to your answers to my questions from comment 9 and comment 10. Thanks!

Comment 13

[Philip Chee]

> In PrefOverlay you seem to add the english word "Migrated" to the UI, but you
> keep the treecell in the old UI. Why?

This is how all the other prefpane migration bugs do it.
<http://mxr.mozilla.org/comm-central/search?string=Migrated%3A&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central>
FYI, this bug is a sub-bug of  Bug 394522.

> Why do you introduce "let" into the JS code? Is that necessary, or do you
> consider that to be more elegant?

let has lexical scoping. I am using let in two ways:

1. To define variables inside a block.
See "let definitions" in <http://developer.mozilla.org/en/New_in_JavaScript_1.7#let_definitions>

2. To bind variables locally in the scope of for loops.
See "let-scoped variables in for loops" in <http://developer.mozilla.org/en/New_in_JavaScript_1.7#let-scoped_variables_in_for_loops>

> Reviewing code that has white space changes is hard. One constantly tries to
> compare code, finding out whether it's a real change, or just whitespace
> change. Please assist reviewers by attaching (in addition) whitespace-ignore
> versions of patches.

I am using MQ (Mercurial Queues). Unfortunately 'hg qdiff' doesn't support any of the -w -b -B switches.

> I have not worked with Xul/JS/Preferences code in a while. You are changing
> from "prefstring" to "preference". I assume you know what you do, and you have
> tested that your code still stores the preferences correctly.

This is how the toolkit Preferences system works. For how the preference attribute is used see:
<http://developer.mozilla.org/index.php?title=En/XUL/Attribute/Preference>
For an overview see:
<http://developer.mozilla.org/index.php?title=En/Preferences_System>

> When you work on code and make changes, please try to avoid unnecessary white
> space changes. And avoid unnnecessary, confusing changes. You are moving
> attributes around, so I see changes where they are none.

I did try, however since the Prefpane migration is a significant rewrite I am taking the opportunity to fix stylistic nits and the other bugs in this migration are also doing this.

See the following patches for example: Attachment 330771 [details] [diff], Attachment 325201 [details] [diff], Attachment 325313 [details] [diff], Attachment 329081 [details] [diff], Attachment 318139 [details] [diff], Attachment 318140 [details] [diff]

Comment 14

[Philip Chee]

Attachment: hg diff -w -b -B -r qparent -r qtip

Hmm> I found out how to use hg diff when using MQ but -w -b -B doesn't seem to do anything?

Comment 15

[Philip Chee]

Comment on attachment 335931 [details] [diff] [review]
Patch 1.0 comm-central part (for real)

Mark: This patch touches one file in thunderbird (/mail/components/preferences/advanced.js) so asking for review. You'll need to look at the mozilla-central patch in attachment 335836 [details] [diff] [review] for the reason for the change.

Comment 16

[Ian Neal]

Comment on attachment 335836 [details] [diff] [review]
Patch 1.1 mozilla-central part

General Nits:
functions should start with a capital letter
could you give at least a context of 8

>+++ b/security/manager/pki/resources/content/pref-validation.js
>+function Startup()
> {
>+  const nsIX509CertDB = Components.interfaces.nsIX509CertDB;
>+  const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
> 
>+  var certdb = Components.classes[nsX509CertDB].getService(nsIX509CertDB);
>   ocspResponders = certdb.getOCSPResponders();
> 
>   var signersMenu = document.getElementById("signingCA");
Why not assign it to be document.getElementById("signingCA").firstChild?
>+  for (let i = 0; i < ocspResponders.length; i++) {
>+    let ocspEntry = ocspResponders.queryElementAt(i, nsIOCSPResponder);
again, why not do:
let responseSigner = ocspResponders.queryElementAt(i, nsIOCSPResponder).responseSigner;?
>     var menuItemNode = document.createElement("menuitem");
>     menuItemNode.setAttribute("value", ocspEntry.responseSigner);
>     menuItemNode.setAttribute("label", ocspEntry.responseSigner);
>     signersMenu.firstChild.appendChild(menuItemNode);
Possibly instead of this you could use the .appendItem method (description argument is optional on it).
>   }
> 
> function doEnabling(called_by)
> {
>+  var signingCA = document.getElementById("security.OCSP.signingCA");
>+  var serviceURL = document.getElementById("security.OCSP.URL");
>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>+  var requireWorkingOCSP = document.getElementById("security.OCSP.require");
>   var enableOCSPBox = document.getElementById("enableOCSPBox");
>   var certOCSP = document.getElementById("certOCSP");
>   var proxyOCSP = document.getElementById("proxyOCSP");
More work needs to be done on doEnabling function - at the moment it does a number of things which should be done by onsyncto/frompreference nor does it honour pref locks. Instant apply needs to be allowed for.

> function changeURL()
You should be able to call with this.value as an argument thus avoiding having to find it out.
> {
>+  var CA = document.getElementById("signingCA").value;
>+  var serviceURL = document.getElementById("security.OCSP.URL");
> 
>+  for (let i = 0; i < ocspResponders.length; i++) {
>+    let ocspEntry = ocspResponders.queryElementAt(i, nsIOCSPResponder);
>     if (CA == ocspEntry.responseSigner) {
>+      serviceURL.value = ocspEntry.serviceURL;
>       break;
>     }
>   }
>+++ b/security/manager/pki/resources/content/pref-validation.xul
>+<!DOCTYPE overlay [
>   <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
>+  %brandDTD;
Is brand.dtd still used?
>   <!ENTITY % prefValidationDTD SYSTEM "chrome://pippki/locale/pref-validation.dtd">
>   %prefValidationDTD;
> ]>
> 
>+  <prefpane id="validation_pane"
>+            label="&pref.validation.title;"
>+            script="chrome://pippki/content/pref-validation.js">
>+    <preferences id="validation_preferences">
>+      <preference id="security.OCSP.disable_button.managecrl"
>+                  name="security.OCSP.disable_button.managecrl"
>+                  type="bool"/>
>+      <preference id="security.OCSP.enabled"
>+                  name="security.OCSP.enabled"
>+                  type="int"
>+                  onchange="doEnabling(0);"/>
onchange probably not needed when you move to using onsyncto/frompreference
>+      <preference id="security.OCSP.URL"
>+                  name="security.OCSP.URL"
>+                  type="string"/>
>+      <preference id="security.OCSP.signingCA"
>+                  name="security.OCSP.signingCA"
>+                  type="string"/>
>+      <preference id="security.OCSP.require"
>+                  name="security.OCSP.require"
>+                  type="bool"/>
>+    </preferences>
> 
>+    <groupbox>
>+      <caption label="&validation.crl.caption;"/>
>+      <description>&validation.crl.description;</description>
>+      <hbox align="center">
>+        <button id="managecrlbutton"
>+                label="&validation.managecrls.button;"
>+                accesskey="&validation.managecrls.accesskey;"
>+                oncommand="openCrlManager();"
>+                preference="security.OCSP.disable_button.managecrl"/>
>+      </hbox>
>+    </groupbox>
> 
>+    <groupbox align="start">
>+      <caption label="&validation.ocsp.caption;"/>
>+      <checkbox id="enableOCSPBox"
>+                label="&enableOCSP.label;"
>+                accesskey="&enableOCSP.accesskey;"
Add preference and onsyncto/frompreference attributes.
>+                oncommand="doEnabling(1);"/>

>+                <label value="&signingCA.label;"
>+                       accesskey="&signingCA.accesskey;"
>+                       control="signingCA"/>
>+                <menulist id="signingCA"
>+                          preference="security.OCSP.signingCA"
>+                          flex="1"
>+                          oncommand="changeURL()">
changeURL should probably be on the preference element and pass in this.value

Comment 17

[Ian Neal]

Comment on attachment 335931 [details] [diff] [review]
Patch 1.0 comm-central part (for real)

>>@@ -157,16 +158,19 @@
>                 url="chrome://communicator/content/pref/pref-security.xul">
>         <treechildren id="securityChildren">
>           <treeitem id="cookiesItem" label="&cookies.label;"
>                     prefpane="cookies_pane" helpTopic="cookies_prefs"
>                     url="chrome://communicator/content/pref/pref-cookies.xul"/>
>           <treeitem id="imagesItem" label="&images.label;"
>                     prefpane="images_pane" helpTopic="images_prefs"
>                     url="chrome://communicator/content/pref/pref-images.xul"/>
>+          <treeitem id="validationItem" label="&validation.label;"
>+                    prefpane="validation_pane"
>+                    url="chrome://pippki/content/pref-validation.xul"/>
One attribute per line please (yes, it's not correct above, and yes, I did it!)
r=me with that change

Comment 18

[Philip Chee]

Attachment: Patch 1.1 comm-central part

> One attribute per line please (yes, it's not correct above, and yes, I did it!)
> r=me with that change

Carrying forward r+=iann_bugzilla

Comment 19

[Philip Chee]

Comment on attachment 336361 [details] [diff] [review]
Patch 1.1 comm-central part

This patch touches /mail/components/preferences/advanced.js

Comment 20

[Philip Chee]

Attachment: Patch 1.2 mozilla-central part

> functions should start with a capital letter
Any particular reason? I thought camelcase was the way to go for function and that inicaps was for Objects. Certainly many if not most of the functions in the seamonkey codebase are camelcase.

> could you give at least a context of 8
Sorry, I forgot to update the .hgrc in mozilla-central.

>>   var signersMenu = document.getElementById("signingCA");
> Why not assign it to be document.getElementById("signingCA").firstChild?
[...]
>>     signersMenu.firstChild.appendChild(menuItemNode);
> Possibly instead of this you could use the .appendItem method (description
> argument is optional on it).
Switched to .appendItem() so no need for .firstChild.

>>+    let ocspEntry = ocspResponders.queryElementAt(i, nsIOCSPResponder);
> again, why not do:
> let responseSigner = ocspResponders.queryElementAt(i,
> nsIOCSPResponder).responseSigner;?
Fixed.

> More work needs to be done on doEnabling function - at the moment it does a
> number of things which should be done by onsyncto/frompreference nor does it
> honour pref locks. Instant apply needs to be allowed for.
Fixed. Instant apply seems to work without me doing anything special.

>> function changeURL()
> You should be able to call with this.value as an argument thus avoiding having
> to find it out.
Fixed.

>>+  %brandDTD;
> Is brand.dtd still used?
No it isn't. Removed.

>>+      <preference id="security.OCSP.enabled"
>>+                  name="security.OCSP.enabled"
>>+                  type="int"
>>+                  onchange="doEnabling(0);"/>
> onchange probably not needed when you move to using onsyncto/frompreference
Fixed.

>>+      <checkbox id="enableOCSPBox"
>>+                label="&enableOCSP.label;"
>>+                accesskey="&enableOCSP.accesskey;"
> Add preference and onsyncto/frompreference attributes.
Fixed.

>>+                <menulist id="signingCA"
>>+                          preference="security.OCSP.signingCA"
>>+                          flex="1"
>>+                          oncommand="changeURL()">
> changeURL should probably be on the preference element and pass in this.value
Fixed. But I had to use a "onchange" on the preference element. For some reason "oncommand" didn't work.

Comment 21

[[e-mail]]

Comment on attachment 336361 [details] [diff] [review]
Patch 1.1 comm-central part

>+          <treeitem id="validationItem"
>+                    label="&validation.label;"
>+                    prefpane="validation_pane"
>+                    url="chrome://pippki/content/pref-validation.xul"/>
> <!--
>           <treeitem id="popupspref"
>                     label="&popups.label;"
>                     url="chrome://communicator/content/pref/pref-popups.xul"/>
> -->
Two points:
Validation was, I believe, the very last subcategory under Privacy & Security
Validation does in fact have help, the topic being validation_prefs obviously
sr=me with those fixed.

Comment 22

[Philip Chee]

> Validation was, I believe, the very last subcategory under Privacy & Security
> Validation does in fact have help, the topic being validation_prefs obviously

Actually all four have help. Thanks for pointing this out. Also I only just noticed (thanks to KaiRo) that I can't move this item to the preftree in preferences.xul as there is an #IFDEF MOZ_PSM somewhere in the build system. KaiRo will be splitting the pki prefOverlay tree into two in Bug 450257. I'll update my patch as soon as that bug lands.

Comment 23

[Philip Chee]

Attachment: Patch 2.0 updated comm-central (post file mv)

Updated patch after KaiRo's file reorg

Carrying forward r+=kaie

Comment 24

[Philip Chee]

Attachment: [checked-in] Patch 2.0T Thunderbird Prefs fix

This is the Thunderbird prefs fix part. Carrying forward r+=standard8

Comment 25

[Philip Chee]

Attachment: [checked-in] (Mozilla Central) Patch 2.0C fix windowtype in crlManager.xul

Fix windowtype in /security/manager/pki/resources/content/crlManager.xul
carrying forward r+=kaie

Comment 26

[Kai Engert (:KaiE:)]

Comment on attachment 336834 [details] [diff] [review]
[checked-in] (Mozilla Central) Patch 2.0C fix windowtype in crlManager.xul

r=kaie

Comment 27

[Robert Kaiser]

Comment on attachment 336827 [details] [diff] [review]
Patch 2.0 updated comm-central (post file mv)

>diff --git a/suite/common/pref/preferences.xul b/suite/common/pref/preferences.xul
>                     url="chrome://communicator/content/pref/pref-images.xul"/>
>+          <treeitem id="validationItem"
>+                    label="&validation.label;"
>+                    prefpane="validation_pane"
>+                    url="chrome://pippki/content/pref-validation.xul"
>+                    helpTopic="validation_prefs"/>
> <!--
>           <treeitem id="popupspref"
>                     label="&popups.label;"
>                     url="chrome://communicator/content/pref/pref-popups.xul"/>
>           <treeitem id="masterpassItem"
>                     label="&masterpass.label;"
>                     prefpane="masterpass_pane"
>                     url="chrome://pippki/content/pref-masterpass.xul"
>@@ -176,21 +185,16 @@
>                     prefpane="ssl_pane"
>                     url="chrome://pippki/content/pref-ssl.xul"
>                     helpTopic="ssl_prefs"/>
>           <treeitem id="certItem"
>                     label="&certs.label;"
>                     prefpane="certs_pane"
>                     url="chrome://pippki/content/pref-certs.xul"
>                     helpTopic="certs_prefs"/>
>-          <treeitem id="validationItem"
>-                    label="&validation.label;"
>-                    prefpane="validation_pane"
>-                    url="chrome://pippki/content/pref-validation.xul"
>-                    helpTopic="validation_prefs"/>
> -->
>         </treechildren>

Could you please just move the end of the comment to be after the certItem? we should leave the order of panes intact, even if some are still commented atm.

Comment 28

[Ian Neal]

Comment on attachment 336827 [details] [diff] [review]
Patch 2.0 updated comm-central (post file mv)

>--- a/suite/common/pref/preferences.xul
>+          <treeitem id="validationItem"
>+                    label="&validation.label;"
>+                    prefpane="validation_pane"
>+                    url="chrome://pippki/content/pref-validation.xul"
>+                    helpTopic="validation_prefs"/>
As KaiRo said, do not change the order of the treeitems.
> <!--
>           <treeitem id="popupspref"
>                     label="&popups.label;"
>                     url="chrome://communicator/content/pref/pref-popups.xul"/>
>           <treeitem id="masterpassItem"
>                     label="&masterpass.label;"
>                     prefpane="masterpass_pane"
>                     url="chrome://pippki/content/pref-masterpass.xul"
>@@ -176,21 +185,16 @@
>                     prefpane="ssl_pane"
>                     url="chrome://pippki/content/pref-ssl.xul"
>                     helpTopic="ssl_prefs"/>
>           <treeitem id="certItem"
>                     label="&certs.label;"
>                     prefpane="certs_pane"
>                     url="chrome://pippki/content/pref-certs.xul"
>                     helpTopic="certs_prefs"/>
>-          <treeitem id="validationItem"
>-                    label="&validation.label;"
>-                    prefpane="validation_pane"
>-                    url="chrome://pippki/content/pref-validation.xul"
>-                    helpTopic="validation_prefs"/>
> -->

>--- a/suite/security/prefs/pref-validation.js
> var ocspResponders;
> var cacheRadio = 0;
Global variables should start with a 'g'

>+  // Make it easier to access the pref pane from onsync.
>+  document.getElementById("managecrlbutton").pane = this;
>+  document.getElementById("enableOCSPBox").pane = this;
>+  document.getElementById("securityOCSPEnabled").pane = this;
> 
>+  doEnabling();
> }
> 
>+function doEnabling(val)
Name of function should start with a capital letter, passed variables should start with an 'a' so in this case variable would be 'aVal'
> {
>+  var signingCA = document.getElementById("security.OCSP.signingCA");
>+  var serviceURL = document.getElementById("security.OCSP.URL");
>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>+  var requireWorkingOCSP = document.getElementById("security.OCSP.require");
>   var certOCSP = document.getElementById("certOCSP");
>   var proxyOCSP = document.getElementById("proxyOCSP");
> 
>+  var OCSPPrefValue = (val != undefined) ? val : parseInt(securityOCSPEnabled.value);
Does the pref security.OCSP.enabled ever return anything other than an integer in toolkit?

>+  var isLocked = (OCSPPrefValue == 0) || securityOCSPEnabled.locked;
>+
>+  certOCSP.disabled = isLocked;
>+  proxyOCSP.disabled = isLocked;
>+  signingCA.disabled = OCSPPrefValue == 0 || OCSPPrefValue == 1 || signingCA.locked;
>+  serviceURL.disabled = OCSPPrefValue == 0 || OCSPPrefValue == 1 || serviceURL.locked;
>+  requireWorkingOCSP.disabled = (OCSPPrefValue == 0) || requireWorkingOCSP.locked;
>+  return OCSPPrefValue;
You should be able to use EnableElementById function here for all four elements. Does changing the disabled status of a radiogroup not affect its children?
>+}
>+
>+function enableCRL(button)
Not needed, see later.
>+function syncToOCSPBox(box)
Name of function should start with a capital letter. The element's check attribute gets set to what you return in this case so no point passing an argument., passed variables should start with an 'a' so in this case variable would be 'aBox'
>+{
>+  // the radio button changed, or we init the stored value from prefs
>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>+  var OCSPPrefValue = (parseInt(securityOCSPEnabled.value));
security.OCSP.enabled should always return an integer?
>+  box.checked = (OCSPPrefValue != 0);
>+  return doEnabling(OCSPPrefValue);
>+}
>+
>+function syncFromOCSPBox(box)
Name of function should start with a capital letter, passed variables should start with an 'a'. Could pass the element's checked value so in this case variable would be 'aChecked'.
>+{
>+  // the user toggled the checkbox to enable/disable OCSP
>+  var newVal = 0;
>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>   var OCSPPrefValue = parseInt(securityOCSPEnabled.value);
security.OCSP.enabled should always return an integer?
>+  if (box.checked) {
Bracing style should be brace on new line.
>+    // now enabled. if we have a cached radio val, restore it.
>+    // if not, use the first setting
>+    newVal = (cacheRadio > 0) ? cacheRadio : 1;
>   }
>   else {
Bracing style should be brace on new line.
>+    // now disabled. remember current value
>+    cacheRadio = OCSPPrefValue;
>   }
>+  return doEnabling(newVal);
> }
> 
>+function changeURL(CA)
Name of function should start with a capital letter, passed variables should start with an 'a' so in this case variable would be 'aCA'
> {
>+  var serviceURL = document.getElementById("security.OCSP.URL");
> 
>+  for (let i = 0; i < ocspResponders.length; i++) {
Bracing style should be brace on new line.
>+    let ocspEntry = ocspResponders.queryElementAt(i, nsIOCSPResponder);
>     if (CA == ocspEntry.responseSigner) {
Bracing style should be brace on new line.
>+      serviceURL.value = ocspEntry.serviceURL;
>       break;
>     }
>   }
> }
> 
> function openCrlManager()
Name of function should start with a capital letter.

>diff --git a/suite/security/prefs/pref-validation.xul b/suite/security/prefs/pref-validation.xul
>--- a/suite/security/prefs/pref-validation.xul
>+      <preference id="security.OCSP.disable_button.managecrl"
>+                  name="security.OCSP.disable_button.managecrl"
>+                  type="bool"
>+                  inverted="true"/>
inverted attribute not needed, preferences on buttons are treated different in toolkit, they only disable a button when locked, the actual preference value makes no difference.

>+    <groupbox>
>+      <caption label="&validation.crl.caption;"/>
>+      <description>&validation.crl.description;</description>
>+      <hbox align="center">
>+        <button id="managecrlbutton"
>+                label="&validation.managecrls.button;"
>+                accesskey="&validation.managecrls.accesskey;"
>+                oncommand="openCrlManager();"
>+                onsyncfrompreference="return this.pane.enableCRL(this);"
onsyncfrompreference not needed here, see earlier.
>+                preference="security.OCSP.disable_button.managecrl"
>+                preference-editable="true"/>
This attribute not needed.

>+    <groupbox align="start">
>+      <caption label="&validation.ocsp.caption;"/>
>+      <checkbox id="enableOCSPBox"
>+                label="&enableOCSP.label;"
>+                accesskey="&enableOCSP.accesskey;"
>+                onsyncfrompreference="return this.pane.syncToOCSPBox(this);"
>+                onsynctopreference="return this.pane.syncFromOCSPBox(this);"
Use this.checked as the argument.
>+                preference="security.OCSP.enabled"/>
>+      <!-- Prefs -->
>+      <radiogroup id="securityOCSPEnabled"
>+                  preference="security.OCSP.enabled"
>+                  onsynctopreference="return this.pane.doEnabling(this.value);"
>+                  >
Not sure you need the onsynctopreference on the radiogroup, the onsyncto/from on the checkbox should be sufficient.
>+          <radio value="0"
>+                 hidden="true"/>
>+          <radio id="certOCSP"
>+                 value="1"
>+                 label="&certOCSP2.label;"
>+                 accesskey="&certOCSP2.accesskey;"/>
>+          <radio id="proxyOCSP"
>+                 value="2"
>+                 label="&proxyOCSP2.label;"
>+                 accesskey="&proxyOCSP2.accesskey;"/>
Probably don't need the ids on the radio elements.

r- for the moment.

Comment 29

[Philip Chee]

Attachment: Patch 2.1 (comm-central)

> As KaiRo said, do not change the order of the treeitems.
Fixed.

> Global variables should start with a 'g'
Fixed.

>>+function doEnabling(val)
> Name of function should start with a capital letter, passed variables should
> start with an 'a' so in this case variable would be 'aVal'
Fixed.

>>+  var OCSPPrefValue = (val != undefined) ? val : parseInt(securityOCSPEnabled.value);
> Does the pref security.OCSP.enabled ever return anything other than an integer
> in toolkit?
As long as there is a default pref it should return an integer. parseInt() removed.

>>+  var isLocked = (OCSPPrefValue == 0) || securityOCSPEnabled.locked;
>>+
>>+  certOCSP.disabled = isLocked;
>>+  proxyOCSP.disabled = isLocked;
>>+  signingCA.disabled = OCSPPrefValue == 0 || OCSPPrefValue == 1 || signingCA.locked;
>>+  serviceURL.disabled = OCSPPrefValue == 0 || OCSPPrefValue == 1 || serviceURL.locked;
>>+  requireWorkingOCSP.disabled = (OCSPPrefValue == 0) || requireWorkingOCSP.locked;
>>+  return OCSPPrefValue;
> You should be able to use EnableElementById function here for all four
> elements.
Fixed.
> Does changing the disabled status of a radiogroup not affect its
> children?
Fixed.

>>+function enableCRL(button)
> Not needed, see later.
Fixed.

>>+function syncToOCSPBox(box)
> Name of function should start with a capital letter. The element's check
> attribute gets set to what you return in this case so no point passing an
> argument., passed variables should start with an 'a' so in this case variable
> would be 'aBox'
Fixed.

>>+  var OCSPPrefValue = (parseInt(securityOCSPEnabled.value));
> security.OCSP.enabled should always return an integer?
Fixed.

>>+  box.checked = (OCSPPrefValue != 0);
>>+  return doEnabling(OCSPPrefValue);
>>+}
>>+
>>+function syncFromOCSPBox(box)
> Name of function should start with a capital letter, passed variables should
> start with an 'a'. Could pass the element's checked value so in this case
> variable would be 'aChecked'.
Fixed.

>>   var OCSPPrefValue = parseInt(securityOCSPEnabled.value);
> security.OCSP.enabled should always return an integer?
Fixed.

>>+  if (box.checked) {
> Bracing style should be brace on new line.
Fixed.
>>+    // now enabled. if we have a cached radio val, restore it.
>>+    // if not, use the first setting
>>+    newVal = (cacheRadio > 0) ? cacheRadio : 1;
>>   }
>>   else {
> Bracing style should be brace on new line.
Fixed.

>>+function changeURL(CA)
> Name of function should start with a capital letter, passed variables should
> start with an 'a' so in this case variable would be 'aCA'
Fixed.

>> {
>>+  var serviceURL = document.getElementById("security.OCSP.URL");
>> 
>>+  for (let i = 0; i < ocspResponders.length; i++) {
> Bracing style should be brace on new line.
Fixed.

>>+    let ocspEntry = ocspResponders.queryElementAt(i, nsIOCSPResponder);
>>     if (CA == ocspEntry.responseSigner) {
> Bracing style should be brace on new line.
Fixed.

>> function openCrlManager()
> Name of function should start with a capital letter.
Fixed.

>>+                  inverted="true"/>
> inverted attribute not needed, preferences on buttons are treated different in
> toolkit, they only disable a button when locked, the actual preference value
> makes no difference.
Fixed.

>>+                onsyncfrompreference="return this.pane.enableCRL(this);"
> onsyncfrompreference not needed here, see earlier.
Fixed.

>>+                preference="security.OCSP.disable_button.managecrl"
>>+                preference-editable="true"/>
> This attribute not needed.
Fixed.

>>+                onsynctopreference="return this.pane.syncFromOCSPBox(this);"
> Use this.checked as the argument.
Fixed.

>>+                  onsynctopreference="return this.pane.doEnabling(this.value);"
>>+                  >
> Not sure you need the onsynctopreference on the radiogroup, the onsyncto/from
> on the checkbox should be sufficient.
You're right. Removed.

> Probably don't need the ids on the radio elements.
I'd like to keep the ids. With my extension author hat on I have to say removing element IDs tend to break extension overlays that expect certain anchor points. Although in this particular case it's hard to see anyone wanting to overlay this radiogroup.

Comment 30

[[e-mail]]

Comment on attachment 337606 [details] [diff] [review]
Patch 2.1 (comm-central)

>+  for (let i = 0; i < gOcspResponders.length; i++) {
>+    let responseSigner = gOcspResponders.queryElementAt(i, nsIOCSPResponder)
>+                                       .responseSigner;
>+    signersMenu.appendItem(responseSigner, responseSigner);
Consider setting the serviceURL as an attribute on the menuitem too, to save having to look it up later. (I asked kaie whether we should list response signers without a URL, but he wasn't sure.)

>+function gDoEnabling(aVal)
This isn't a variable.

>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>+  var OCSPPrefValue = (aVal != undefined) ? aVal : securityOCSPEnabled.value;
Please fix all your callers to pass the correct value.

>+  return OCSPPrefValue;
Don't bother returning the value, most of your callers don't need it and they already know what it is anyway.

>+function SyncFromOCSPBox(aChecked)
Bah, why did security/ have to go for this b****** hybrid?

>+    newVal = (gCacheRadio > 0) ? gCacheRadio : 1;
gCacheRadio || 1

>+      <hbox align="center">
Nit: don't need align="center"

>+          <radio id="certOCSP"
>+          <radio id="proxyOCSP"
You're not using these ids.

>+          <grid class="indent" flex="1">
>+            <columns>
>+            <column/>
>+            <column flex="1"/>
>+            </columns>
Nit: indentation wrong for the <column>s

>+                          flex="1">
This flex was wrong. Please remove it.

Comment 31

[Ian Neal]

Comment on attachment 337606 [details] [diff] [review]
Patch 2.1 (comm-central)

>+++ b/suite/security/prefs/pref-validation.js
>+  for (let i = 0; i < gOcspResponders.length; i++) {
Nit: bracing style, opening brace should be on a new line.
>+    let responseSigner = gOcspResponders.queryElementAt(i, nsIOCSPResponder)
>+                                       .responseSigner;
>+    signersMenu.appendItem(responseSigner, responseSigner);
As Neil suggests, appendItem method returns the item created so you should be able to add serviceURL as an attribute, which does mean reversing some of the previous changes as you now need ocspEntry again but should make ChangeURL function simpler.

Comment 32

[Robert Kaiser]

adding blocking flag as a blocker depends on this fix.

Comment 33

[Philip Chee]

Attachment: Patch 2.2 (comm-central)

>>+  for (let i = 0; i < gOcspResponders.length; i++) {
>>+    let responseSigner = gOcspResponders.queryElementAt(i, nsIOCSPResponder)
>>+                                       .responseSigner;
>>+    signersMenu.appendItem(responseSigner, responseSigner);
>Consider setting the serviceURL as an attribute on the menuitem too, to save
>having to look it up later. (I asked kaie whether we should list response
>signers without a URL, but he wasn't sure.)
Fixed.

>>+function gDoEnabling(aVal)
>This isn't a variable.
Fixed.

>>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>>+  var OCSPPrefValue = (aVal != undefined) ? aVal : securityOCSPEnabled.value;
>Please fix all your callers to pass the correct value.
Fixed.

>>+  return OCSPPrefValue;
>Don't bother returning the value, most of your callers don't need it and they
>already know what it is anyway.
Fixed.

>>+function SyncFromOCSPBox(aChecked)
>Bah, why did security/ have to go for this b****** hybrid?
I used a CVS build as a baseline comparison and wondered about the "frankencheckbox" as well.

>>+    newVal = (gCacheRadio > 0) ? gCacheRadio : 1;
>gCacheRadio || 1
Fixed.

>>+      <hbox align="center">
>Nit: don't need align="center"
Fixed.

>>+          <radio id="certOCSP"
>>+          <radio id="proxyOCSP"
>You're not using these ids.
Fixed.

>>+          <grid class="indent" flex="1">
>>+            <columns>
>>+            <column/>
>>+            <column flex="1"/>
>>+            </columns>
>Nit: indentation wrong for the <column>s
Fixed.

>>+                          flex="1">
>This flex was wrong. Please remove it.
Fixed.

Comment 34

[[e-mail]]

Comment on attachment 337835 [details] [diff] [review]
Patch 2.2 (comm-central)

 const nsIOCSPResponder = Components.interfaces.nsIOCSPResponder;
>+var gOcspResponders;
These don't need to be global any more.

>+  const nsIX509CertDB = Components.interfaces.nsIX509CertDB;
>+  const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
> 
>+  var certdb = Components.classes[nsX509CertDB].getService(nsIX509CertDB);
>+  gOcspResponders = certdb.getOCSPResponders();
I didn't spot this before, but you could consider:
var ocspResponders = Components.classes["@mozilla.org/security/x509certdb;1"]
                               .getService(Components.interfaces.nsIX509CertDB)
                               .getOCSPResponders();
Or maybe keeping the certdb temporary.

>+function doEnabling(aOCSPPrefValue)
I think IanN wanted DoEnabling

>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>+  var OCSPPrefValue = securityOCSPEnabled.value;
You only use these in the else case. (In fact OCSPPrefValue is redundant.)

>+      <preference id="security.OCSP.signingCA"
>+                  name="security.OCSP.signingCA"
>+                  type="string"
>+                  onchange="ChangeURL()"/>
I've decided that it's not a good idea to do this on arbitrary pref changes, so please move this to be oncommand on the menulist.

>+                <textbox id="serviceURL"
>+                         preference="security.OCSP.URL"/>
Is it me or does this look awkward when it would fit on one line anyway?

Comment 35

[Karsten Düsterloh]

> >+                <textbox id="serviceURL"
> >+                         preference="security.OCSP.URL"/>
> Is it me or does this look awkward when it would fit on one line anyway?

// Enter uncalled comment.

No, it does. 
(But if it doesn't, I prefer one line per attribute.)

// Off uncalled comment.

Comment 36

[Philip Chee]

Attachment: [checked-in] Patch 2.3 (comm-central)

carrying forward sr+ from Neil.

>>+var gOcspResponders;
>These don't need to be global any more.
Fixed.

>I didn't spot this before, but you could consider:
>var ocspResponders = Components.classes["@mozilla.org/security/x509certdb;1"]
>                               .getService(Components.interfaces.nsIX509CertDB)
>                               .getOCSPResponders();
>Or maybe keeping the certdb temporary.
Fixed (keeping certdb).

>>+function doEnabling(aOCSPPrefValue)
>I think IanN wanted DoEnabling
Fixed.

>>+  var securityOCSPEnabled = document.getElementById("security.OCSP.enabled");
>>+  var OCSPPrefValue = securityOCSPEnabled.value;
>You only use these in the else case. (In fact OCSPPrefValue is redundant.)
Fixed.

>>+                  onchange="ChangeURL()"/>
>I've decided that it's not a good idea to do this on arbitrary pref changes, so
>please move this to be oncommand on the menulist.
Fixed. I just hope Ian doesn't want it moved back to the preference again.

>>+                <textbox id="serviceURL"
>>+                         preference="security.OCSP.URL"/>
>Is it me or does this look awkward when it would fit on one line anyway?
It's just you. Putting it all on one line would introduce a stylistic inconsistency into the file.

Comment 37

[Robert Kaiser]

mozilla-central part pushed as http://hg.mozilla.org/mozilla-central/rev/8fcf92cfd811 and comm-central part pushed as http://hg.mozilla.org/comm-central/rev/317d694415d6 - Philip, I'll leave it up to you to mark fixed.