441403 - Migrate SeaMonkey's popup preferences to new pref window

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Ian Neal]

Attachment: pref-popups patch v0.1

This patch:
* migrates the popup preferences to new pref window
* adds a new pref to control whether the sound is a system beep or a custom sound
* migrates to the new version of popup preferences for selecting the sound (currently only done in pref pane but either needs to be done in notification.xml as well or moved to somewhere in the startup of a browser window)
* adds filefield element functionality to the new pref window
* use filefield element for custom sound url

I'll make relevant help changes once this patch is reviewed and finalised.

Comment 1

[[e-mail]]

Comment on attachment 326386 [details] [diff] [review]
pref-popups patch v0.1

>+// used to decide whether to migrate popup prefs
>+pref("privacy.popups.prefs_version",              0);
> pref("privacy.popups.sound_enabled",              false);
>+pref("privacy.popups.sound_type",                 0);
Why not just default the sound type to 1? If you're really worried the place to do migration is in the migrator.

> <!-- ***** BEGIN LICENSE BLOCK *****
>-   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
What happened here?

>+        <button id="exceptionsButton"
>+                label="&popupExceptions.label;"
>+                accesskey="&popupExceptions.accesskey;"
>+                preference="pref.advanced.popups.disable_button.view_popups"/>
Shouldn't this button do something?

>+/* File Field Widget */
>+filefield {
>+  margin: 2px 4px;
>+  -moz-appearance: textfield;
Hmm, do all our platforms support -moz-appearance?

>+.fileFieldContentBox {
>+  background-color: -moz-Dialog;
>+  color: -moz-DialogText;
I don't see this color doing anything useful; textboxes already have colours.

>+  margin: 1px;
I don't see the point of this; the icon has a margin, and the textbox padding.

>+filefield[disabled="true"] .fileFieldContentBox {
>+  opacity: 0.5;
>+}
This is just plain wrong.

>+filefield[disabled="true"] .fileFieldIcon {
>+ opacity: 0.2;
>+}
Ideally .fileFieldIcon would xbl:inherit .disabled here; in the meantime, please use > .fileFieldContentBox > .fileFieldIcon

>+.fileFieldLabel {
>+  -moz-appearance: none;
>+  background-color: transparent;
>+  border: none;
>+  padding: 1px 0px 0px 0px;
I'd like to see this with its default padding, to see how it compares.

>+   -moz-border-top-colors: #98A5B2 #000000;
>+   -moz-border-right-colors: #98A5B2 #000000;
>+   -moz-border-bottom-colors: #98A5B2 #000000;
>+   -moz-border-left-colors: #98A5B2 #000000;
These are the focused colours which readonly textfields never use.
Please use the standard textbox colours!

>+filefield[disabled="true"] .fileFieldContentBox {
>+  background-color: #C7D0D9;
>+  color: #999999;
>+  cursor: default !important;
Again, I think the textbox can handle its text colours and cursors.

>+  margin-top: 2px;
>+  margin-bottom: 2px;
>+  -moz-margin-start: 2px;
>+  -moz-margin-end: 4px;
Modern isn't even trying to be RTL-safe yet. margin: 2px 4px 2px 2px;
(Out of interest, what are the margins on autocomplete icons?)

>+.fileFieldLabel {
>+  -moz-appearance: none;
>+  background-color: transparent;
Aren't these unnecessary in Modern?

>+  padding: 1px 0px 0px 0px;
Again, try it without this. Or with whatever autocomplete uses, if different.

>+function ReadSoundLocation(aElement)
>+{
>+  var pref = document.getElementById(aElement.getAttribute("preference"));
>+  aElement.value = pref.value;
>+  if (aElement.value)
>+  {
>+    aElement.label = gFileHandler.getFileFromURLSpec(aElement.value).leafName;
>+    aElement.image = "moz-icon://" + aElement.label + "?size=16";
filefield actually has custom methods to get this right. Sadly they work on nsIFile, not a URI, but at least you can pass the whole URI to the image.

>+    initialDir = gFileHandler.getFileFromURLSpec(gSoundUrlBox.value)
>+                             .QueryInterface(nsILocalFile).parent;
You don't need to QI to nsILocalFile to get the parent.

Comment 2

[Ian Neal]

Attachment: pref-popups patch v0.2

Changes since v0.1:
* Default for privacy.popups.sound_type now 1
* Added code to migrator for when moving from 1.x to 2.x
* Removed format changing to boilerplate for pref-popups.xul
* Added missing oncommand from exceptionsButton
* Updated classic as per review (-moz-appearance seems to be supported on all platforms)
* Updated modern as per review - -moz-border-*-colors are the correct ones
* Modern now changes border colors on textbox when disabled
* Made use of filefield's file property in pref-popups.js

Comment 3

[[e-mail]]

Comment on attachment 327710 [details] [diff] [review]
pref-popups patch v0.2

>+// used to decide whether to migrate popup prefs
>+pref("privacy.popups.prefs_version",              0);
IMHO we don't need this. Anyone who's been using SM2.0a will get a sound_type of 1, but because their sound_url is blank they'll just get a beep anyway.

Even if you rewrote the panel so that the user chose between no sound, system beep and custom sound radio buttons I still wouldn't upgrade existing SM2.0a users although migration code would still be necessary of course.

>+    if (!PL_strncasecmp(aTransform->stringValue, "file://", 7)) {
>+      rv |= aBranch->SetCharPref("privacy.popups.sound_url",
>+                                 aTransform->stringValue);
>+    }
>+    else {
>+      char fileUrl[500];
>+      PR_snprintf(fileUrl, sizeof(fileUrl),
>+                  "file://%s", aTransform->stringValue);
>+      rv |= aBranch->SetCharPref("privacy.popups.sound_url", fileUrl);
I don't think this is reasonable. If you need to transform the pref, better perhaps would be to convert the URL to a file and check that it exists?

>   for (i = 0; i < NS_ARRAY_LENGTH(branchNames); ++i)
>+++ suite/themes/classic/communicator/preferences.css   1 Jul 2008 22:30:24 -0000
[patch corruption]

>+filefield[disabled="true"] > .fileFieldContentBox > .fileFieldIcon {
>+  opacity: 0.2;
I think this opacity might be a little low; try 0.4 perhaps (both themes)?

>+filefield {
>+  margin: 2px 4px;
>+  border: 2px solid;
>+  background-color: #FFFFFF;
>+  color: #000000;
>+  font: inherit;
>+  -moz-border-top-colors: #BEC3D3 #5D616E;
>+  -moz-border-right-colors: #F8FAFE #5D616E;
>+  -moz-border-bottom-colors: #F8FAFE #5D616E;
>+  -moz-border-left-colors: #BEC3D3 #5D616E;
No, no, no... these border colours are only used for focused textboxes! You don't need to set the text colour either, the textbox does that. In fact you don't even need to set the background colour, the content box already has one.

>+filefield[disabled="true"] {
>+  color: #999999;
>+  -moz-border-top-colors: #BEC3D3 #9DA1AE;
>+  -moz-border-right-colors: #E8EAEE #9DA1AE;
>+  -moz-border-bottom-colors: #E8EAEE #9DA1AE;
>+  -moz-border-left-colors: #BEC3D3 #9DA1AE;
You shouldn't have to change anything for a disabled filefield.

>+.fileFieldIcon {
>+  width: 16px;
>+  height: 16px;
>+  margin: 2px 4px 2px 2px;
>+}
I think this could be tweaked to 1/4/1/1px (and appropriately for Classic).

> textbox[disabled="true"] {
>   background-color: #C7D0D9;
>   color: #999999;
>   cursor: default !important;
>+  -moz-border-top-colors: #BEC3D3 #9DA1AE;
>+  -moz-border-right-colors: #E8EAEE #9DA1AE;
>+  -moz-border-bottom-colors: #E8EAEE #9DA1AE;
>+  -moz-border-left-colors: #BEC3D3 #9DA1AE;
Disabled textboxes already have these border colours.

>+  // Check to see if prefs need migrating
The migrator already did this.

Comment 4

[Ian Neal]

(In reply to comment #3)
> (From update of attachment 327710 [details] [diff] [review])
> 
> >+filefield {
> >+  margin: 2px 4px;
> >+  border: 2px solid;
> >+  background-color: #FFFFFF;
> >+  color: #000000;
> >+  font: inherit;
> >+  -moz-border-top-colors: #BEC3D3 #5D616E;
> >+  -moz-border-right-colors: #F8FAFE #5D616E;
> >+  -moz-border-bottom-colors: #F8FAFE #5D616E;
> >+  -moz-border-left-colors: #BEC3D3 #5D616E;
> No, no, no... these border colours are only used for focused textboxes!
Have you seen what a filefield looks like without these border colours? With the above border colours the filefield matches the look of a readonly textbox

> >+filefield[disabled="true"] {
> >+  color: #999999;
> >+  -moz-border-top-colors: #BEC3D3 #9DA1AE;
> >+  -moz-border-right-colors: #E8EAEE #9DA1AE;
> >+  -moz-border-bottom-colors: #E8EAEE #9DA1AE;
> >+  -moz-border-left-colors: #BEC3D3 #9DA1AE;
> You shouldn't have to change anything for a disabled filefield.
> 
> > textbox[disabled="true"] {
> >   background-color: #C7D0D9;
> >   color: #999999;
> >   cursor: default !important;
> >+  -moz-border-top-colors: #BEC3D3 #9DA1AE;
> >+  -moz-border-right-colors: #E8EAEE #9DA1AE;
> >+  -moz-border-bottom-colors: #E8EAEE #9DA1AE;
> >+  -moz-border-left-colors: #BEC3D3 #9DA1AE;
> Disabled textboxes already have these border colours.
No, they do not, they have different colour borders (#BEC3D3 #5D616E and #F8FAFE #5D616E are the enabled ones).

Comment 5

[[e-mail]]

(In reply to comment #4)
I've no idea what prompted me to make those comments. I'd even looked at textbox.css several times! (I won't bore you with my full command history.)

Why do you think we need a different border for disabled textboxes/filefields (very few widgets change their border when disabled) and how did you come to pick that particular colour? It's not used anywhere else within Modern.

Comment 6

[Ian Neal]

(In reply to comment #5)
> (In reply to comment #4)
> I've no idea what prompted me to make those comments. I'd even looked at
> textbox.css several times! (I won't bore you with my full command history.)
> 
> Why do you think we need a different border for disabled textboxes/filefields
> (very few widgets change their border when disabled) and how did you come to
> pick that particular colour? It's not used anywhere else within Modern.
> 
Just seemed out of place for the textbox/filefield not to change when the buttons next to it does. I think it is good it changes to show that its status is changing as well as the buttons. (Also in classic filefields change according to disability status).
As for the colour I just picked one that was slightly lighter shade of grey - I could not spot one suitable being used elsewhere in modern.

Comment 7

[Ian Neal]

Two approaches:
Using fixup:
Have to alter Makefile.in to include docshell
Have to add:
#include <nsIURIFixup.h>
#include <nsCDefaultURIFixup.h>
#include "nsIFileURL.h"
Code itself:
    nsCOMPtr<nsIFile> aFile;
    nsresult res = NS_ERROR_FAILURE;
    nsCOMPtr<nsIURIFixup> fixup = do_GetService(NS_URIFIXUP_CONTRACTID);
    if (fixup) {
      nsCOMPtr<nsIURI> uri;
      res = fixup->CreateFixupURI(nsDependentCString(aTransform->stringValue),
                                  nsIURIFixup::FIXUP_FLAG_NONE,
                                  getter_AddRefs(uri));
      if (NS_SUCCEEDED(res) && uri) {
        nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(uri, &res);
        if (fileURL) {
          res = fileURL->GetFile(getter_AddRefs(aFile));
        }
      }
    }
    if (NS_FAILED(res)) return rv;

Using NS_GetFileFromURLSpec and NS_NewNativeLocalFile (no changes to Makefile.in or adding #include(s))
Code itself:
    nsCOMPtr<nsIFile> aFile;
    nsresult res = NS_ERROR_FAILURE;
    if (!PL_strncasecmp(aTransform->stringValue, "file://", 7)) {
      res = NS_GetFileFromURLSpec(nsDependentCString(aTransform->stringValue),
                                  getter_AddRefs(aFile));
    }
    if (NS_FAILED(res)) {
      nsCOMPtr<nsILocalFile> localFile;
      res = NS_NewNativeLocalFile(nsDependentCString(aTransform->stringValue),
                                  PR_FALSE, getter_AddRefs(localFile));
      if (NS_FAILED(res)) return rv;
      aFile = localFile;
    }

Comment 8

[[e-mail]]

(In reply to comment #6)
> (Also in classic filefields change according to disability status).
I don't see that on W2K but I guess XP/Vista might differ - screenshot?

> As for the colour I just picked one that was slightly lighter shade of grey - I
> could not spot one suitable being used elsewhere in modern.
Well, maybe there's a reason for that ;-)

Comment 9

[[e-mail]]

(In reply to comment #7)
>     nsCOMPtr<nsIFile> aFile;
>     nsresult res = NS_ERROR_FAILURE;
>     if (!PL_strncasecmp(aTransform->stringValue, "file://", 7)) {
>       res = NS_GetFileFromURLSpec(nsDependentCString(aTransform->stringValue),
>                                   getter_AddRefs(aFile));
>     }
>     if (NS_FAILED(res)) {
>       nsCOMPtr<nsILocalFile> localFile;
>       res = NS_NewNativeLocalFile(nsDependentCString(aTransform->stringValue),
>                                   PR_FALSE, getter_AddRefs(localFile));
>       if (NS_FAILED(res)) return rv;
>       aFile = localFile;
>     }
I asked biesi and this was the way he suggested, although I don't think you need the file:// check, you seems to have a duplicate nsDependentCString and for some reason you have a return rv; ;-)

Comment 10

[Ian Neal]

Attachment: Zip file contain screenshots of pref-popups

This zip contains screenshots of the new pref-popups preference page for both classic and modern themes showing the filefield in enabled and disabled states.
These are taken on a Fedora 8 box.

Comment 11

[Ian Neal]

(In reply to comment #9)
> (In reply to comment #7)
> >     nsCOMPtr<nsIFile> aFile;
> >     nsresult res = NS_ERROR_FAILURE;
> >     if (!PL_strncasecmp(aTransform->stringValue, "file://", 7)) {
> >       res = NS_GetFileFromURLSpec(nsDependentCString(aTransform->stringValue),
> >                                   getter_AddRefs(aFile));
> >     }
> >     if (NS_FAILED(res)) {
> >       nsCOMPtr<nsILocalFile> localFile;
> >       res = NS_NewNativeLocalFile(nsDependentCString(aTransform->stringValue),
> >                                   PR_FALSE, getter_AddRefs(localFile));
> >       if (NS_FAILED(res)) return rv;
> >       aFile = localFile;
> >     }
> I asked biesi and this was the way he suggested, although I don't think you
> need the file:// check, you seems to have a duplicate nsDependentCString and
> for some reason you have a return rv; ;-)
> 
I'm returning rv (which is the result of the last pref setting) as just because we've not been able to get a valid file should not make us return an error.

Comment 12

[[e-mail]]

(In reply to comment #10)
>Created an attachment (id=328041)
>Zip file contain screenshots of pref-popups
Do Classic readonly textfields look like normal textfields or disabled ones?

(In reply to comment #11)
>I'm returning rv (which is the result of the last pref setting) as just because
>we've not been able to get a valid file should not make us return an error.
Sorry, I thought this was some sort of helper function, given the aFile result.

Comment 13

[Ian Neal]

(In reply to comment #12)
> (In reply to comment #10)
> >Created an attachment (id=328041) [details]
> >Zip file contain screenshots of pref-popups
> Do Classic readonly textfields look like normal textfields or disabled ones?
Classic readonly textfields look like disabled ones.

Comment 14

[Ian Neal]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > >Created an attachment (id=328041) [details] [details]
> > >Zip file contain screenshots of pref-popups
> > Do Classic readonly textfields look like normal textfields or disabled ones?
> Classic readonly textfields look like disabled ones.
Other than any text being greyed out in disabled ones.

Comment 15

[Ian Neal]

Attachment: pref-popups patch v0.2a

Changes since v0.2:
* Got rid of the new pref privacy.popups.prefs_version
* Now convert the url to a file, check it exists and is a file before setting the pref to the url spec
* Made various css changes

Comment 16

[[e-mail]]

Comment on attachment 328270 [details] [diff] [review]
pref-popups patch v0.2a

>+filefield {
>+  margin: 2px 4px;
>+  border: 2px solid;
>+  font: inherit;
Nit: unnecessary; to inherit the font is already the default.

> textbox[disabled="true"] {
>   background-color: #C7D0D9;
>   color: #999999;
>   cursor: default !important;
>+  -moz-border-top-colors: #BEC3D3 #9DA1AE;
>+  -moz-border-right-colors: #E8EAEE #9DA1AE;
>+  -moz-border-bottom-colors: #E8EAEE #9DA1AE;
>+  -moz-border-left-colors: #BEC3D3 #9DA1AE;
Nit: theme style is to put the border before the background/colour/cursor.

I still don't like your choice of disabled border colours; my suggestion is:
>+  -moz-border-top-colors: #BEC3D3 #98A5B2;
>+  -moz-border-right-colors: #F8FAFE #98A5B2;
>+  -moz-border-bottom-colors: #F8FAFE #98A5B2;
>+  -moz-border-left-colors: #BEC3D3 #98A5B2;

Comment 17

[Ian Neal]

Attachment: pref-popups patch v0.2b

Changes since v0.2a:
* Made suggested changes to modern theme

Carrying forward sr= and requesting r=

Comment 18

[Ian Neal]

Comment on attachment 328398 [details] [diff] [review]
pref-popups patch v0.2b

>Index: common/pref/pref-popups.xul
>===================================================================
>+      <preference id="privacy.popups.prefs_version"
>+                  name="privacy.popups.prefs_version"
>+                  type="int"/>
I will of course remove this unused pref before checkin (already done locally).

Comment 19

[[e-mail]]

You forgot to include the mailnews changes in your latest patches...

Comment 20

[Ian Neal]

Attachment: pref-popups patch v0.2c

Changes since v0.2b:
* Adds back missing mailnews parts of patch (from 0.2) - thanks for the catch Neil
* Removes unneeded privacy.popups.prefs_version as mentioned in comment#18

Carrying forward sr= and requesting r=

Comment 21

[[e-mail]]

(In reply to comment #1)
>(From update of attachment 326386 [details] [diff] [review])
>>+filefield[disabled="true"] .fileFieldIcon {
>>+ opacity: 0.2;
>>+}
>Ideally .fileFieldIcon would xbl:inherit .disabled here;
Note: Fixed in hg by bug 443410. If this patch doesn't need to land in CVS, you can use .fileFieldIcon[disabled="true"] instead.

Comment 22

[Ian Neal]

Comment on attachment 328968 [details] [diff] [review]
pref-popups patch v0.2c

>Index: suite/common/pref/pref-popups.js
>===================================================================
>+function EnableSoundUrl(aCustomSelected, aNoSkip)
>+{
>+  if (aNoSkip)
>+  {
>+    EnableElement(gSoundUrlBox, aCustomSelected, false);
I probably need to insert:
var pref = document.getElementById(gSoundUrlBox.getAttribute("preference"));
>+    EnableElement(gSelectSound, aCustomSelected, false);
then alter this line to:
EnableElement(gSelectSound, aCustomSelected && !pref.locked, false);
>+  }
>+  EnableElementById("previewSound", aCustomSelected, false);
>+}
This is so if the sound url pref is locked the user will not be able to browse for another sound.

Comment 23

[[e-mail]]

(In reply to comment #22)
> This is so if the sound url pref is locked the user will not be able to browse
> for another sound.
Ah, but that's easily fixed by locking the sound button pref.

Comment 24

[Ian Neal]

(In reply to comment #23)
> (In reply to comment #22)
> > This is so if the sound url pref is locked the user will not be able to browse
> > for another sound.
> Ah, but that's easily fixed by locking the sound button pref.
> 
Shouldn't we honour both though?

Comment 25

[[e-mail]]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > This is so if the sound url pref is locked the user will not be able to browse
> > > for another sound.
> > Ah, but that's easily fixed by locking the sound button pref.
> Shouldn't we honour both though?
Having two prefs that lock the same button would be silly, but I don't mind which one you choose to use.

Comment 26

[[e-mail]]

Comment on attachment 328968 [details] [diff] [review]
pref-popups patch v0.2c

>+  var ret = filepicker.show();
>+  if (ret == nsIFilePicker.returnOK)
>+  {
>+    var prefString = gSoundUrlBox.getAttribute("preference");
>+    var pref = document.getElementById(prefString);
>+    pref.value = filepicker.fileURL.spec;
>+    EnableSoundUrl(true, false);
>+  }
Looking at bug 444582 I see that the better way to do this is to base the code on updating the preference element rather than the sound url box, and also move the call to EnableSoundUrl to its onchange handler.

Comment 27

[[e-mail]]

Comment on attachment 328968 [details] [diff] [review]
pref-popups patch v0.2c

>+  var initialDir = null;
>+  if (gSoundUrlBox.value)
>+  {
>+    initialDir = gFileHandler.getFileFromURLSpec(gSoundUrlBox.value).parent;
>+  }
>+
>+  filepicker.init(window, gSelectSound.getAttribute("filepickertitle"),
>+                  nsIFilePicker.modeOpen);
>+  if (initialDir)
>+    filepicker.displayDirectory = initialDir;
Thought: why not write
filepicker.init(...);
if (soundUrlPref.value)
  filepicker.displayDirectory = gFileHandler.getFileFromURLSpec(soundUrlPref.value).parent;

Comment 28

[Ian Neal]

Attachment: pref-popups patch v0.2d

Changes since v0.2c:
* As Neil suggested change to using preference value than UI value
* Use preference onchange handler instead of calling from SelectSound function
* Removed gSoundUrlBox as a global and made gSoundUrlPref one instead
* All of the above allowed simplification of other functions

Carrying forward sr= and requesting r= again

Comment 29

[Ian Neal]

(In reply to comment #28)
> Created an attachment (id=329534) [details]
> pref-popups patch v0.2d
> 
> Changes since v0.2c:
> * As Neil suggested change to using preference value than UI value
> * Use preference onchange handler instead of calling from SelectSound function
> * Removed gSoundUrlBox as a global and made gSoundUrlPref one instead
> * All of the above allowed simplification of other functions
> 
> Carrying forward sr= and requesting r= again
> 
Part of this patch (pref-viewing_messages.xul bit) is bit rotted. The patch on bug 447696, once landed, will mean all that has to be changed are the EnableTextbox calls to EnableElementById calls on two lines.

Comment 30

[Ian Neal]

Attachment: pref-popups patch v0.2e

Changes since v0.2d:
* Now depends on patches from dependent bugs
* Reflects change of transform method name (SetFile instead of SetSound) and now have to pass both preference strings method needs to manipulate.

Carrying forward sr=

Comment 31

[Ian Neal]

Comment on attachment 331426 [details] [diff] [review]
pref-popups patch v0.2e

>--- a/suite/browser/browser-prefs.js
>+++ b/suite/browser/browser-prefs.js
>@@ -394,6 +394,7 @@
> pref("browser.downloadmanager.behavior", 0);
> 
> pref("privacy.popups.sound_enabled",              false);
>+pref("privacy.popups.sound_type",                 1);
This should probably be 0 now that the migration patch now assumes it is.

Comment 32

[Karsten Düsterloh]

Comment on attachment 331426 [details] [diff] [review]
pref-popups patch v0.2e

>+++ b/suite/common/bindings/notification.xml
>+              if (soundType == 1) {

Please use helpfully named constants for such values.

>+++ b/suite/common/pref/pref-popups.js
>+function SetLists()

Thanks for already uppercasing functions and bracing style used. :)

>+{
>+  const popupType = "popup";

Please use the k prefix for constants, ...

>+  const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
>+  const nsIPermission = Components.interfaces.nsIPermission;

... although these traditional definitions are probably acceptable (although I don't like them).

>+  var pref = document.getElementById("privacy.popups.remove_blacklist");
>+  if (pref.value)
>+  {
>+    var enumerator = permissionManager.enumerator;
>+    var hosts = [];

Please use let for subscope variables, here and everywhere else below.
You may want to consider using let in global scope as well.

>+    while (enumerator.hasMoreElements())
>+    {                
>+      var permission = enumerator.getNext();
>+      if (permission)
>+      {
>+        permission = permission.QueryInterface(nsIPermission);

if (permission instanceof nsIPermission)

>+        if ((permission.type == popupType) &&
>+            (permission.capability == nsIPermissionManager.DENY_ACTION))
>+          hosts[hosts.length] = permission.host;

hosts.push(permission.host);

>+    for (var i in hosts)
>+    {
>+      permissionManager.remove(hosts[i], popupType);
>+    }    

No need for braces.

>+      for (var i in hosts)
>+      {
>+        var host = hosts[i];
>+        host = "http://" + host;

No need for two assignments.

>+function EnableSoundRadio(aSoundChecked)
>+{
>+  const kCustomSound = 1;

:-)

>+function SelectSound()
...
>+  filepicker.init(window, gSelectSound.getAttribute("filepickertitle"),
>+                  nsIFilePicker.modeOpen);

Either one parameter per line or all on one line.

>+  var ret = filepicker.show();
>+  if (ret == nsIFilePicker.returnOK)
>+    gSoundUrlPref.value = filepicker.fileURL.spec;

No need for variable ret.

>+++ b/suite/common/pref/pref-popups.xul
>-   - and other provisions required by the LGPL or the GPL. If you do not delete
>+   - and other provisions required by the GPL or the LGPL. If you do not delete

*g*

>+      <hbox align="center">      

Why do all your top level hboxes have this useless align attribute set?

>+        <radiogroup id="popupSoundType"
>+                    orient="vertical"

Radiogroups are already vertical by courtesy of xul.css.

>+          <radio id="system"

Tinderbox orange alert! ;-)

>+          <radio id="custom"

Tinderbox orange alert! ;-)
Both ids are already in use in the preferences window.
Consider building with tests and running

cd $objdir/mozilla/_tests/testing/mochitest
python runtests.py --chrome --test-path=suite/common/tests/chrome/test_idcheck.xul

after finishing a patch touching XUL.

>+++ b/suite/common/pref/preferences.xul
>+          <treeitem id="popupsItem" label="&popups.label;"
>+                    prefpane="popups_pane" helpTopic="pop_up_blocking"
>                     url="chrome://communicator/content/pref/pref-popups.xul"/>

One attribute per line, pleasse, despite some other elements in the vicinity don't do that yet.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-popups.dtd
> <!ENTITY selectSound.label              "Browse…">
> <!ENTITY selectSound.accesskey          "r">

How about o or w? They tend to be wider than r.

r- solely for the number of nits.

Comment 33

[Ian Neal]

Attachment: pref-popups patch v0.2f (Checked in: Comment 34)

Changes since v0.2e (as per review comments):
* added constant for readability in notification.xml
* const name is now kPopupType in pref-popups.js
* used instanceof and .push
* removed extraneous braces
* tweaked host assignment
* one parameter per line for filepicker.init
* inlined filepicker.show()
* removed unneeded aligns
* made radio ids more unique
* one attribute per line for preferences.xul
* accesskey is now "o" for selectSound

Did not make changes from var to let as at the moment JS Debugger does not play well with them when debugging.

Comment 34

[Ian Neal]

Comment on attachment 335948 [details] [diff] [review]
pref-popups patch v0.2f (Checked in: Comment 34)

http://hg.mozilla.org/comm-central/rev/08a2d48078be