Ian Neal Assignee
	Description • 17 years ago

Start with migrating fonts and colors pref panels.

	neil@parkwaycc.co.uk
	Comment 1 • 17 years ago

Comment on attachment 295858 [details] [diff] [review]
Patch for fonts and colors

Colours are fine, but fonts are seriously wrong... I'm seeing all manner of errors in the JavaScript console. Also, the old code used to build the popups lazily because otherwise they take seconds to refresh.

>+                  onsyncfrompreference="return document.getElementById('fonts_pane').readFontLanguageGroup();">
This unfortunately seems to fire before the script has loaded...

>+                      onsyncfrompreference="return document.getElementById('fonts_pane').readFontSelection(this);"/>
"this" is the preference element. (Maybe it shouldn't be...)

>+    // Both lists are sorted, and the Fonts-By-Type list is a subset of the
>+    // All-Fonts list, so walk both lists side-by-side, skipping values we've
>+    // already created menu items for.
>+    var builtItem = separator ? separator.nextSibling : popup.firstChild;
>+    var builtItemValue = builtItem ? builtItem.getAttribute("value") : null;
Why not use the fonts array?

>+    separator = document.createElement("menuseparator");
>+    popup.appendChild(separator);
This is incorrect if there are no specific fonts.

>+  aMenuList.appendChild(popup);    
Trailing spaces :-P

>+function readMinFontSize(aElement)
>+function readUseDocumentFonts()
>+function writeUseDocumentFonts(aUseDocumentFonts)
I don't see what use these are.

	Karsten Düsterloh
	Comment 2 • 17 years ago

Comment on attachment 295858 [details] [diff] [review]
Patch for fonts and colors

Also some code style nits for pref-fonts.js:
  - prefix globals with g
  - UpperCaseFunctionNames()
  - no spaces in or after empty {}

	Ian Neal Assignee
	Comment 3 • 17 years ago

Let's do this one pane at a time - split off pref-colors changes first.
Only change from this part of last patch is the removal of trailing space just before closing of <!DOCTYPE> element.

	Ian Neal Assignee
	Comment 4 • 17 years ago

Comment on attachment 297067 [details] [diff] [review]
Patch for pref-colors only v0.1 (Checked into trunk)

Checking in
preferences.xul;
new revision: 1.9; previous revision: 1.8
preftree.xul;
new revision: 1.113; previous revision: 1.112
pref-colors.xul;
new revision: 1.59; previous revision: 1.58
done

	Ian Neal Assignee
	Comment 5 • 17 years ago

Changes since v0.1
* Use the font array for building the second part of the font list
* Test for non-empty first part of the font list before adding a separator
* Removed trailing spaces and other extra spaces
* Prefixed global variables with g
* Started function names with a capital letter
* Builds the font list lazily

Read/WriteUseDocumentFonts are used to convert the integer pref to a boolean element (checkbox)
ReadMinFontPref is used to deal with when the pref does not exist and what default value to use.

	neil@parkwaycc.co.uk
	Comment 6 • 17 years ago

Comment on attachment 302358 [details] [diff] [review]
Patch for pref-fonts only v0.2

>+        <menulist id="selectLangs" preference="font.language.group"
>+                  onsyncfrompreference="return document.getElementById('fonts_pane').ReadFontLanguageGroup();">
There's a bug on having to use document.getElementById?

>+                      onsyncfrompreference="return document.getElementById('fonts_pane').ReadFontPref(this, null);">
I'd pass in the real default value (0) instead of "assuming" it.
(I'd also pass in the value as a number in the other cases.)

>+const kDefaultFontType          = "font.default.%LANG%";
>+const kFontNameFmtSerif         = "font.name.serif.%LANG%";
>+const kFontNameFmtSansSerif     = "font.name.sans-serif.%LANG%";
>+const kFontNameFmtMonospace     = "font.name.monospace.%LANG%";
>+const kFontNameFmtCursive       = "font.name.cursive.%LANG%";
>+const kFontNameFmtFantasy       = "font.name.fantasy.%LANG%";
>+const kFontNameListFmtSerif     = "font.name-list.serif.%LANG%";
>+const kFontNameListFmtSansSerif = "font.name-list.sans-serif.%LANG%";
>+const kFontNameListFmtMonospace = "font.name-list.monospace.%LANG%";
>+const kFontNameListFmtCursive   = "font.name-list.cursive.%LANG%";
>+const kFontNameListFmtFantasy   = "font.name-list.fantasy.%LANG%";
>+const kFontSizeFmtVariable      = "font.size.variable.%LANG%";
>+const kFontSizeFmtFixed         = "font.size.fixed.%LANG%";
>+const kFontMinSizeFmt           = "font.minimum-size.%LANG%";
There's no point defining contants for these strings, you only use them once, and the name of the constant is no more informative then the string.

>+  if (!gFontEnumerator) {
>+    gFontEnumerator = Components.classes["@mozilla.org/gfx/fontenumerator;1"]
>+                                .createInstance(Components.interfaces.nsIFontEnumerator);
>+    }
Nit: incorrect indentation of }

>+      if (fonts.indexOf(gAllFonts[i]) == -1) {
fonts.lastIndexOf(gAllFonts[i], 0) is cheaper. I'd also change this to == 0 and swap the else case around.

>+    var preference = document.getElementById(prefs[i].format.replace(/%LANG%/, languageGroup));
>+    if (!preference) {
>+      preference = document.createElement("preference");
>+      var name = prefs[i].format.replace(/%LANG%/, languageGroup);
Put the name up above since you need it there anyway. Also I'd simply chop the %LANG% off the format and concatenate the languageGroup instead.

>+  return undefined;
Nit: this is the default return value, and there are no other returns.

>+  var stripWhitespace = /^\s*(.*)\s*$/;
Nit: I prefer .replace(/^\s+|\s+$/g, "")

>+  for (var i = 0; i < fontNames.length; ++i) {
>+    var fontName = fontNames[i].replace(stripWhitespace, "$1");
>+    fontItems = aElement.getElementsByAttribute("value", fontName);
>+    if (fontItems.length)
>+      break;
>   }
>+  if (fontItems.length)
>+    return fontItems[0].getAttribute("value");
Why not return the value inside the loop?

>+function ReadFontPref(aElement, aDefaultValue)
>+{
>+  // Check to see if preference value exists, if not return
>+  // given default value if there is one or first in list.
>+  var preference = document.getElementById(aElement.getAttribute("preference"));
>+  if (!!preference.value)
>+    return undefined;
>+  return aDefaultValue || aElement.firstChild.firstChild.getAttribute("value");
>+}
Why the !!? Does return preference.value || aDefaultValue work?
(See above about not needing to "assume" the value.)

>+function ReadUseDocumentFonts()
>+{
>+  var preference = document.getElementById("browser.display.use_document_fonts");
>+  return preference.value == 1;
>+}
>+ 
>+function WriteUseDocumentFonts(aUseDocumentFonts)
>+{
>+  return aUseDocumentFonts.checked ? 1 : 0;
>+}
Strangely the checkbox seems to work without ReadUseDocumentFonts but not without WriteUseDocumentFonts :-(

	Robert Kaiser
	Comment 7 • 17 years ago

I've been looking into the main appearance panel in the last few hours, but the overlays for startup options coming from composer/mailnews/chatzilla give me headaches :(

	Robert Kaiser
	Comment 8 • 17 years ago

Here's a WIP patch for the main appearance pane, including bug 417319 work - note though that due to some yet unresolved issue with loading the overlays to pref-appearance.xul I commented out all of those overlay lines in the patch for now, so only the "browser" startup option appears on the pane with this WIP patch for now. If anyone finds out why the overlays (which are included) make the pane fail, I'd be happy about that...

	Robert Kaiser
	Comment 9 • 17 years ago

After some IRC talk with IanN about the problem I saw with the WIP patch in comment #8, I found out that the loadOverlay() call in http://mxr.mozilla.org/mozilla/source/toolkit/content/widgets/preferences.xml#692 never notifies the observer about this pane being loaded (we get past the loadOverlay() but never enter the observe() function of obs) - this might be an instance of bug 330458.

	Robert Kaiser
	Comment 10 • 17 years ago

I'd like to add the language selection to the main appearance panel (I have that in my local tree) and we already once had a problem with space in the appearance pane in bug 341949, so adding zoom or the aggressive favicon pref would probably also break that again, even if we didn't add locale selection there.

Because of that I propose to split off the parts that affect web and message content to a new content pref pane, for which I'm attaching a patch here.
We could probably make the icons part a groupbox that also includes http://kb.mozillazine.org/Browser.chrome.load_toolbar_icons if we keep supporting that on trunk.

	neil@parkwaycc.co.uk
	Comment 11 • 17 years ago

patching file `mozilla/suite/common/jar.mn'
Hunk #1 FAILED at 139.
1 out of 1 hunk FAILED -- saving rejects to mozilla/suite/common/jar.mn.rej
patching file `mozilla/suite/common/pref/preferences.xul'
patch: **** malformed patch at line 82:

	Robert Kaiser
	Comment 12 • 17 years ago

> patch: **** malformed patch at line 82:

Hrmm, yes, I had to manually edit the diff there, I may have introduced a bug there.

	Ian Neal Assignee
	Comment 13 • 17 years ago

(In reply to comment #10)
> We could probably make the icons part a groupbox that also includes
> http://kb.mozillazine.org/Browser.chrome.load_toolbar_icons if we keep
> supporting that on trunk.
> 
I'm happy for that to happen. Don't forget to update help as well if you are adding extra pref panel / pref options.

	Robert Kaiser
	Comment 14 • 17 years ago

(In reply to comment #13)
> Don't forget to update help as well if you are
> adding extra pref panel / pref options.

Yes, I'll look into that once I have review on the new panel. Should I do the work for that icons groupbox right now or can I do that in a followup?

	Ian Neal Assignee
	Comment 15 • 17 years ago

(In reply to comment #14)
> (In reply to comment #13)
> > Don't forget to update help as well if you are
> > adding extra pref panel / pref options.
> 
> Yes, I'll look into that once I have review on the new panel. Should I do the
> work for that icons groupbox right now or can I do that in a followup?
> 
Well you're adding at least one extra pref as it is so you might as well do this one too and group them how you intend.

	Robert Kaiser
	Comment 16 • 17 years ago

Here's an updated patch for the content panel, adding that groupbox (and reducing context on jar.mn so I can hopefully give you something that applies cleanly).

	Ian Neal Assignee
	Comment 17 • 17 years ago

(In reply to comment #16)
> Created an attachment (id=303386) [details]
> add content panel, v2 (including site icon groupbox)
> 
> Here's an updated patch for the content panel, adding that groupbox (and
> reducing context on jar.mn so I can hopefully give you something that applies
> cleanly).
> 

So you are planning to put in browser.chrome.load_toolbar_icons in a spin off bug?

	Robert Kaiser
	Comment 18 • 17 years ago

Here's another WIP patch for the appearance pane, which is almost read from my perspective - you need at least the part of bug 417319 for testing that removes the ChatZilla overlay from pref-appearance so that you aren't disturbed by the bug 330458 problem. I'd like a first review on this even though it cannot go in exactly as it is in the patch (workaround for the overlay problem).
Some Comments along with that:
1) Actually overlaying the mailnews and editor parts is commented out (cZ part also needs to be removed for testing this), but I added checkboxes manually in the pane so that the final layout can be tested. Of course, this needs to be corrected for checkin.
2) I put locale switching into this panel now (and it works!), if that's ok, I need to add removal of the old pref-locales as well.

	Ian Neal Assignee
	Comment 19 • 17 years ago

Comment on attachment 303714 [details] [diff] [review]
WIP2 patch for main appearance pane

This is on code inspection only.

>Index: mozilla/suite/common/pref/pref-appearance.xul
>===================================================================

>+    <groupbox id="generalStartupPreferences" align="start">
Because chatzilla still looks for "generalStartupPreferences" perhaps change the id to "generalStartupPrefs"

>+      <menulist id="switchLocales" flex="1" preference="general.useragent.locale"
preference could be on a new line.
>+                onsyncfrompreference="return document.getElementById('appearance_pane').SelectLocale(this);"/>
Hmmm, must log a bug on the need to call the function that way.

>Index: mozilla/suite/common/pref/preferences.xul
>===================================================================
>       <treeitem container="true"
>-                id="appearance"
>-                label="&appear.label;"
>+                id="appearance" label="&appear.label;"
id should be appearanceItem and label should be on next line.

>+                prefpane="appearance_pane"
>+                helpTopic="appearance_pref"
helpTopic usually comes before prefpane entries elsewhere in this file.
>                 url="chrome://communicator/content/pref/pref-appearance.xul">
Change needed for pref-locale as per your comment in bug

>Index: mozilla/suite/common/pref/preftree.xul
>===================================================================
Change needed for pref-locale as per your comment in bug

>Index: mozilla/suite/locales/en-US/chrome/common/pref/pref-appearance.dtd
>===================================================================
> <!ENTITY useSiteIcons.label                     "Show Web Site Icons">
> <!ENTITY useSiteIcons.accesskey                 "S">
> <!ENTITY enableAutomaticImageResizing.label     "Resize large images to fit in the browser window">
> <!ENTITY enableAutomaticImageResizing.accesskey "R">
> <!ENTITY useSmoothScroll.label                  "Use smooth scrolling">
> <!ENTITY useSmoothScroll.accesskey              "U">
>+<!ENTITY textZoomOnly.label                     "Zoom only text instead of full pages">
>+<!ENTITY textZoomOnly.accesskey                 "Z">
These entities will now be in the pref-content.dtd so should be removed.
> 
>Index: mozilla/mailnews/jar.mn
>===================================================================
>-% overlay chrome://communicator/content/pref/pref-appearance.xul               chrome://messenger/content/mailPrefsOverlay.xul
>+###% overlay chrome://communicator/content/pref/pref-appearance.xul               chrome://messenger/content/mailPrefsAppearanceOverlay.xul
I prefer mailPrefAppearanceOverlay.xul as the filename.

>+    content/messenger/mailPrefsAppearanceOverlay.xul                           (base/prefs/resources/content/mailPrefsAppearanceOverlay.xul)
See comment above.

/mailPrefsAppearanceOverlay.xul
>===================================================================
See comment above.
>+<overlay id="mailPrefsAppearanceOverlay"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
See comment above.

>+  <groupbox id="generalStartupPreferences">
See comment about changing to generalStartupPrefs earlier.

>Index: mozilla/editor/ui/jar.mn
>===================================================================
>-% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://editor/content/editorPrefsOverlay.xul
>+###% overlay chrome://communicator/content/pref/pref-appearance.xul chrome://editor/content/editorPrefsAppearanceOverlay.xul
I prefer editorPrefAppearanceOverlay.xul as a filename.

>+      content/editor/editorPrefsAppearanceOverlay.xul          (composer/content/editorPrefsAppearanceOverlay.xul)
See comment above.

>Index:  mozilla/editor/ui/composer/content/editorPrefsAppearanceOverlay.xul
>===================================================================
See comment above.

>+<!DOCTYPE overlay [
>+<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" >
>+%brandDTD;
Is brand.dtd still needed?

>+<overlay id="editorPrefsAppearanceOverlay"
See comment above.

>+  <groupbox id="generalStartupPreferences">
See comment about changing to generalStartupPrefs earlier.

	Robert Kaiser
	Comment 20 • 17 years ago

(In reply to comment #17)
> So you are planning to put in browser.chrome.load_toolbar_icons in a spin off
> bug?

Ideally, yes. I'm working on too many different things atm, I'd like to get those done step by step.

	Ian Neal Assignee
	Comment 21 • 17 years ago

Changes since v0.2:
* As per Neil's comments.
* Did some further tweaking of how preference name is generated.

Carrying forward sr= and requesting r=

	Ian Neal Assignee
	Comment 22 • 17 years ago

Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

Just on reading through the code...
>Index:  mozilla/suite/locales/en-US/chrome/common/pref/pref-content.dtd
>===================================================================
>+<!ENTITY pref.content.description               "Those settings influence how website and message content appears in &brandShortName;. See the separate preferences for colors, fonts and languages to further customize website appearance and the Privacy &amp; Security section for privacy-related website settings.">
"These" instead of "Those", also drop the "website" from the first part of the second sentence so it becomes "to further customize appearance". Perhaps say "separate preference panels" or "pages" or similar?

	Robert Kaiser
	Comment 23 • 17 years ago

(In reply to comment #19)
> >+      <menulist id="switchLocales" flex="1" preference="general.useragent.locale"
> preference could be on a new line.
> >+                onsyncfrompreference="return document.getElementById('appearance_pane').SelectLocale(this);"/>
> Hmmm, must log a bug on the need to call the function that way.

Not sure if that's possible. onsyncfrompreference get loaded into the XBL and runs in the scope of that instead of the pane scope, so the function isn't local to it.

	Robert Kaiser
	Comment 24 • 17 years ago

Sorry for generating another bugmail, missed this comment:

(In reply to comment #19)
> >+                prefpane="appearance_pane"
> >+                helpTopic="appearance_pref"
> helpTopic usually comes before prefpane entries elsewhere in this file.

Actually, I see it helpTopic after prefpane in most places in the file...

All other comments are addressed in the patch I'm about to attach (and which really works because of the fix for bug 419452, which makes the overlays work).

	Robert Kaiser
	Comment 25 • 17 years ago

Here's the first really working patch for the main pref-appearance panel, adressing the comments from the WIP versions.

	neil@parkwaycc.co.uk
	Comment 26 • 17 years ago

Comment on attachment 306108 [details] [diff] [review]
main appearance pane, v1

>-  <groupbox id="generalStartupPreferences" align="start">
Why rename this?

>+    <vbox class="box-padded" align="start">
>+      <separator class="thin" />
Nit: put the separator before the vbox

>+      <menulist id="switchLocales" flex="1"
>+                preference="general.useragent.locale"
I don't like the way this menulist consumes the width of the groupbox.

>diff -N mozilla/suite/common/pref/pref-appearance.js
Not added to jar.mn

>+  var popup = document.createElement("menupopup");
Use menulist.appendItem so you don't need this.

>+  var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                      .getService(Components.interfaces.nsIStringBundleService);
>+  var langNames = sbs.createBundle("chrome://global/locale/languageNames.properties");
>+  var regNames  = sbs.createBundle("chrome://global/locale/regionNames.properties");
These ought to be <stringbundle> elements.

>+      displayName = langNames.GetStringFromName(parts[0]);
I think I saw someone .toLowerCase() this too just in case.

>+          displayName += " (" + regNames.GetStringFromName(parts[1].toLowerCase()) + ")";
This is OK for l10n, right? ;-)

>-<!--
>-          <treeitem label="&locales.label;"
>-                    url="chrome://communicator/content/pref/pref-locales.xul"/>
>         </treechildren>
>       </treeitem>
>--->
>+
You need to remove the close comment too...

>-<!ENTITY languageList.label                     "Choose your preferred language for &brandShortName;:">
Heh, how long has that been there :-)

>+<!ENTITY restartOnLangChange.label              "Language preferences will take effect when you restart &brandShortName;.">
>+<!ENTITY selectLocale.label                     "Select the language for text that appears in dialog boxes, menus, toolbars and button labels:">
Nit: entries are in reverse order

>diff -N mozilla/mailnews/base/prefs/resources/content/mailPrefAppearanceOverlay.xul
Although not applicable in every case, if the overlay is small enough that every element has an id then it doesn't really matter how many times it is used.

sr- based on number of nits and questions.

	Robert Kaiser
	Comment 27 • 17 years ago

(In reply to comment #26)
> (From update of attachment 306108 [details] [diff] [review])
> >-  <groupbox id="generalStartupPreferences" align="start">
> Why rename this?

Because IanN suggested that, see comment #19

> >+      displayName = langNames.GetStringFromName(parts[0]);
> I think I saw someone .toLowerCase() this too just in case.

Why? The chrome registry should always gives us correct language IDs, and those have a lowercase language name per spec (RFC 3066).

> >+          displayName += " (" + regNames.GetStringFromName(parts[1].toLowerCase()) + ")";
> This is OK for l10n, right? ;-)

I hope so, I never heard of problems with the locale switcher extension, where this is copied from.

> >--->
> >+
> You need to remove the close comment too...

Erm, "--->" is just that, as the "-" in the first column is the diff "minus" sign.

> >-<!ENTITY languageList.label                     "Choose your preferred language for &brandShortName;:">
> Heh, how long has that been there :-)

No idea, I wondered as well.

> >diff -N mozilla/mailnews/base/prefs/resources/content/mailPrefAppearanceOverlay.xul
> Although not applicable in every case, if the overlay is small enough that
> every element has an id then it doesn't really matter how many times it is
> used.

IMHO, It's actually much cleaner to have separate overlays there which never are supposed to drop elements - not sure about load time though (or even how much of an issue that is).

	Ian Neal Assignee
	Comment 28 • 17 years ago

(In reply to comment #27)
> (In reply to comment #26)
> > (From update of attachment 306108 [details] [diff] [review] [details])
> > >-  <groupbox id="generalStartupPreferences" align="start">
> > Why rename this?
> 
> Because IanN suggested that, see comment #19
My understanding is that the change would only be needed if the same overlay file was used for both SM2.x and pre SM2.x versions of pref-appearance.xul for chatzilla (unless you do as Neil has done in the latest patch for that).

	Karsten Düsterloh
	Comment 29 • 17 years ago

Comment on attachment 303785 [details] [diff] [review]
Patch for pref-fonts only v0.3

>Index: pref-fonts.xul
>===================================================================
>+<!DOCTYPE overlay SYSTEM "chrome://communicator/locale/pref/pref-fonts.dtd">
>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

Add an empty line between these two.

>+    <groupbox>
>+      <caption>
>+        <hbox align="center">
>+          <label value="&language.label;" 
>+                 accesskey="&language.accesskey;" 
>+                 control="selectLangs"/>
>+        </hbox>

I don't see much point in this hbox (especially with the following menulist outside of it) since caption already is one itself. You can put the align onto the caption.

>+        <menulist id="selectLangs" preference="font.language.group"
>+                  onsyncfrompreference="return document.getElementById('fonts_pane').ReadFontLanguageGroup();">

There's not much point in returning the value of a function which doesn't give a return value. I think it's clearer that we'll hit the 'undefined' case with this pseudo handler if we drop the return here. 

>+        <rows>
>+          <row align="center">
>+            <spacer/>
>+            <hbox align="center">
>+              <label value="&typefaces.label;"/>
>+            </hbox>  
>+            <hbox align="center">
>+              <label value="&sizes.label;"/>
>+            </hbox>  
>+          </row>

The hboxes are not needed, row is already one itself.

> 
>-</page>
>+          <row>
>+            <separator class="thin"/>
>+          </row>
>+

Drop the empty lines around that row, we don't do that below either.

>+          <row align="center">
>+            <hbox align="center" pack="end">
>+              <label value="&monospace.label;" 
>+                     accesskey="&monospace.accesskey;" 
>+                     control="monospace"/>
>+            </hbox>

Add a thin separator row before the monospace row.

>+    <hbox align="center">
>+      <!-- Unchecking this removes the ability to select dynamic fonts -->
>+      <checkbox id="browserUseDocumentFonts" 

You do like hboxes, do you? ;-)
But this one is superfluous, too...

>Index: pref-fonts.js
>===================================================================

This file used the "curly braces go onto their own line" style. 
I'd rather like to see that retained (despite the overall rewrite).

>+function BuildFontList(aLanguage, aFontType, aMenuList, aPreference)
...
>+  var fonts = GetFontEnumerator().EnumerateFonts(aLanguage, aFontType, {});
>+  if (fonts.length > 0)
>+    defaultFont = GetFontEnumerator().getDefaultFont(aLanguage, aFontType);
>+  else {
>+    fonts = GetFontEnumerator().EnumerateFonts(aLanguage, "", {});
>+    if (fonts.length > 0)
>+      defaultFont = GetFontEnumerator().getDefaultFont(aLanguage, "");

Please always "embrace" both if-branches if at least one needs it.

Furthermore, we don't care if the length is actually > 0, it can't be < 0 anyways, so "if (fonts.length)" will suffice.
This is also true for all of the > 0 tests which follow.

>+  if (!gAllFonts)
>+    gAllFonts = GetFontEnumerator().EnumerateAllFonts({});
>+    

Non-empty empty line. ;-)

>+    for (var i = 0; i < fonts.length; ++i) {
>+      menuitem = document.createElement("menuitem");
>+      menuitem.setAttribute("value", fonts[i]);
>+      menuitem.setAttribute("label", fonts[i]);

You can just use 
  for each (var font in fonts)
  ...
  menuitem.setAttribute("value", font);
here.

>+    for (i = 0; i < gAllFonts.length; ++i) {

Ditto here and for other arrays(!) below.

>+function ReadFontLanguageGroup()
>+{
>+  var prefs = [{ format: "default",       type: "string",  element: "defaultFontType", fonttype: ""         },

No space after {, please, and align the closing }s.

>+        window.setTimeout(BuildFontList, i*100, languageGroup,
>+                          prefs[i].fonttype, element, preference);

First, the * should have spaces around.
Second, eww - do we really care about the order? We don't have control over it anyway, even with this. Can't we just hide the entire row until the dropdown is filled?

>+function ReadFontSelection(aElement)
...
>+    // There is a setting that actually is in the list. Respect it.
>+    if (fontItems.length > 0)
>+      return undefined;

As much as I usually dislike that, I agree that explicitly returning undefined here is more readable.


No real problems, r- just for the number of nits.

	Ian Neal Assignee
	Comment 30 • 17 years ago

(In reply to comment #29)
> (From update of attachment 303785 [details] [diff] [review])
> >Index: pref-fonts.js
> >===================================================================
> >+    for (var i = 0; i < fonts.length; ++i) {
> >+      menuitem = document.createElement("menuitem");
> >+      menuitem.setAttribute("value", fonts[i]);
> >+      menuitem.setAttribute("label", fonts[i]);
> 
> You can just use 
>   for each (var font in fonts)
>   ...
>   menuitem.setAttribute("value", font);
> here.
for...in should not be used for arrays - see MDC for details - http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Statements:for...in#Description

> >+        window.setTimeout(BuildFontList, i*100, languageGroup,
> >+                          prefs[i].fonttype, element, preference);
> 
> First, the * should have spaces around.
> Second, eww - do we really care about the order? We don't have control over it
> anyway, even with this. Can't we just hide the entire row until the dropdown is
> filled?
Hiding looks even more ugly than doing it this way. Patch with rest of changes coming up.

	Ian Neal Assignee
	Comment 31 • 17 years ago

Changes since v0.3:
* Now honour lock status of browser.display.languageList
* Made most of the changes per mnyromyr's review (see previous comment for exceptions)
* Used legacy pref-fonts' way of stopping menulists jumping once populated (setting an empty label on the menulist element) - alternatively could set it in the xul but I think it looks better if the menulists go blank and disabled when switching between language groups rather than just disabled.

	Ian Neal Assignee
	Comment 32 • 17 years ago

(In reply to comment #31)
> Created an attachment (id=306918) [details]
> pref-fonts patch v0.4
Hmmm, I meant to put something in about browser.display.languageList in either the xul or js files - any preference?

	Ian Neal Assignee
	Comment 33 • 17 years ago

Comment on attachment 306108 [details] [diff] [review]
main appearance pane, v1

>Index: mozilla/suite/common/pref/pref-appearance.xul
>===================================================================
>+<!DOCTYPE overlay [
>+  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD;
>+  <!ENTITY % prefAppearanceDTD SYSTEM "chrome://communicator/locale/pref/pref-appearance.dtd"> %prefAppearanceDTD;
> ]>
Add an empty line here, before the <overlay>.

>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

>Index: mozilla/suite/common/pref/pref-appearance.js
>===================================================================
>+function switchLocales_load() {
Functions should be properly Capitalised, so should be SwitchLocales_Load()

>+function SelectLocale(aElement)
>+{
Declare matchItems here, so you are not declaring it twice (strict JS warnings otherwise I think).
>+  var pref = document.getElementById(aElement.getAttribute("preference"));
>+  if (pref.value) {
>+    var matchItems = aElement.getElementsByAttribute("value", pref.value);
>+    // If the pref matches an entry that actually is in the list, use it.
>+    if (matchItems.length > 0)
Only need to do (matchItems.length) as if it exists it will always be > 0.

>+      return pref.value;
>+  }
>+
>+  if (pref.defaultValue) {
>+    var matchItems = aElement.getElementsByAttribute("value", pref.defaultValue);
>+    // If the pref's default matches an entry that actually is in the list, use it.
>+    if (matchItems.length > 0)
As above.
>+      return pref.defaultValue;
>+  }
>+
>+  // If prefs can't point us to a valid value and something is set, leave that.
>+  if (aElement.value)
>+    return aElement.value;
>+
>+  // If somehow we still have no value, return the first value in the list
>+  return aElement.firstChild.firstChild.getAttribute("value");
>+}
>Index: mozilla/suite/common/pref/preferences.xul
>===================================================================
>     <treechildren id="prefsPanelChildren">
>       <treeitem label="READ THIS!" prefpane="temp_readthis_pane"/>
>-<!-- commenting out yet unmigrated panels
>+      <!-- Appearance items -->
>       <treeitem container="true"
>-                id="appearance"
>+                id="appearanceItem"
>                 label="&appear.label;"
>+                prefpane="appearance_pane"
>+                helpTopic="appearance_pref"
>                 url="chrome://communicator/content/pref/pref-appearance.xul">
You are correct about position of prefpane/helpTopic but I think we have, already, become inconsistent in the layout of each treeitem...
Probably needs to be sorted.

	Ian Neal Assignee
	Comment 34 • 17 years ago

Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

>Index: mozilla/suite/common/pref/preferences.xul
>===================================================================
>+          <treeitem id="contentItem"
>+                    label="&content.label;"
>+                    helpTopic="appearance_pref_content"
>+                    prefpane="content_pane"
>+                    url="chrome://communicator/content/pref/pref-content.xul"/>
See your previous comments about position of helpTopic / prefpane, you might as well correct the ones for "colorsItem" too.
>+<!-- commenting out yet unmigrated panels
>           <treeitem label="&fonts.label;"
>                     url="chrome://communicator/content/pref/pref-fonts.xul"/>
> -->
>           <treeitem id="colorsItem"
>                     label="&colors.label;"
>                     helpTopic="appearance_pref_colors"
>                     prefpane="colors_pane"
>                     url="chrome://communicator/content/pref/pref-colors.xul"/>

Rest as per previous comment, r=me with those changes.

	Robert Kaiser
	Comment 35 • 17 years ago

(In reply to comment #22)
> >Index:  mozilla/suite/locales/en-US/chrome/common/pref/pref-content.dtd
> >===================================================================
> >+<!ENTITY pref.content.description               "Those settings influence how website and message content appears in &brandShortName;. See the separate preferences for colors, fonts and languages to further customize website appearance and the Privacy &amp; Security section for privacy-related website settings.">
> "These" instead of "Those", also drop the "website" from the first part of the
> second sentence so it becomes "to further customize appearance". Perhaps say
> "separate preference panels" or "pages" or similar?

I shortened it to "These settings influence how website and message content appears in &brandShortName;." - the rest can be explained in the help docs (done in a followup).

	neil@parkwaycc.co.uk
	Comment 36 • 17 years ago

(In reply to comment #28)
>(In reply to comment #27)
>>(In reply to comment #26)
>>>(From update of attachment 306108 [details] [diff] [review] [details] [details])
>>>>-  <groupbox id="generalStartupPreferences" align="start">
>>>Why rename this?
>>Because IanN suggested that, see comment #19
>My understanding is that the change would only be needed if the same overlay
>file was used for both SM2.x and pre SM2.x versions of pref-appearance.xul for
>chatzilla (unless you do as Neil has done in the latest patch for that).
Actually you don't even need it then; the checkboxes are almost identical.

	neil@parkwaycc.co.uk
	Comment 37 • 17 years ago

Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

>                     url="chrome://communicator/content/pref/pref-colors.xul"/>
>-<!--
>-          <treeitem label="&locales.label;"
>-                    url="chrome://communicator/content/pref/pref-locales.xul"/>
>         </treechildren>
>       </treeitem>
>--->
>       <treeitem container="true"
Not part of this patch.

>+    </groupbox>
>+
>+    <vbox class="box-padded" align="start">
>+      <separator class="thin" />
Nits: Separator between the groupbox and the vbox please.
      Also lose the space before the /.

>+<!ENTITY useFavIcons.label                      "Aggressively look for Web Site Icons when not defined in page">
Nit: I think this is bad grammar, but I'm not sure what I'd like this to say; maybe it would suffice to reduce it to "Aggressively look for Web Site Icons" (i.e. without mentioning that this only makes a difference when there's no icon link tag in the page) or maybe change the condition to "... when the page does not define one".

	Robert Kaiser
	Comment 38 • 17 years ago

Comment on attachment 303386 [details] [diff] [review]
add content panel, v2 (including site icon groupbox) [checked in]

Content panel is checked in, thanks.

	neil@parkwaycc.co.uk
	Comment 39 • 17 years ago

Comment on attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

>+      if (fonts.lastIndexOf(gAllFonts[i]) == 0)
Nit: I said fonts.lastIndexOf(gAllFonts[i], 0) == 0

	Robert Kaiser
	Comment 40 • 17 years ago

New version of the patch, all comments fixed and tested in current builds (yay, loading overlays works again).

	neil@parkwaycc.co.uk
	Comment 41 • 17 years ago

Comment on attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

>+    </preferences>
>+
>+  <stringbundleset id="prefAppearanceBundleset">
Nit: indentation of this and the next five lines is wrong

>+    <groupbox id="switchLocaleBox">
Nit: IMHO this groupbox needs align="start" too.

>Index: mozilla/suite/common/pref/pref-appearance.js
>===================================================================
>RCS file: mozilla/suite/common/pref/pref-appearance.js
>diff -N mozilla/suite/common/pref/pref-appearance.js
>--- /dev/null	2008-02-25 20:03:38.000000000 +0100
>+++ mozilla/suite/common/pref/pref-appearance.js	2008-03-07 23:25:07.000000000 +0100
Still no jar.mn change in the patch ;-)

	Karsten Düsterloh
	Comment 42 • 17 years ago

Comment on attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

Yeah, looks much better without flickering dropdown boxes. ;-)

	Ian Neal Assignee
	Comment 43 • 17 years ago

Comment on attachment 306918 [details] [diff] [review]
pref-fonts patch v0.4 (Patch checked in)

With Neil's final comment addressed.
Checking in to trunk
preftree.xul;
new revision: 1.117; previous revision: 1.116
done
preferences.xul;
new revision: 1.14; previous revision: 1.13
pref-fonts.xul;
new revision: 1.87; previous revision: 1.86
pref-fonts.js;
new revision: 1.54; previous revision: 1.53
done

	Ian Neal Assignee
	Comment 44 • 17 years ago

Comment on attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

>Index: mozilla/suite/common/pref/preftree.xul
>===================================================================
>-        <treeitem>
>-          <treerow>
>-            <treecell url="chrome://communicator/content/pref/pref-locales.xul" label="&locales.label;"/>
>-          </treerow>
>-        </treeitem>
In theory isn't this being migrated too even if it is to the main appearance pane?

>Index: mozilla/editor/ui/composer/content/editorPrefsOverlay.xul
>===================================================================
>   <!-- editor startup toggle -->
>-  <groupbox id="generalStartupPreferences">           
>-    <checkbox id="generalStartupEditor" wsm_persist="true"
>+  <preferences id="appearance_preferences">
>+    <preference id="general.startup.editor"
>+                name="general.startup.editor"
>+                type="bool"/>
>+  </preferences>
Nit: Preferences should come before the <!-- editor startup toggle --> comment

r=me with those addressed (and Neil's comments)

	Robert Kaiser
	Comment 45 • 17 years ago

Comment on attachment 308031 [details] [diff] [review]
main appearance pane, v2 [checked in]

Patch for main panel including removal of locales panel has been checked in now.

	Robert Kaiser
	Comment 46 • 17 years ago

Actually, from what I see, this completes the work in this bug, so marking this FIXED.
This is the first pref category (besides debug) to be completely ported to the new window!

	Tony Mechelynck [:tonymec]
	Comment 47 • 17 years ago

Verifying on tinderbox build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031507 SeaMonkey/2.0a1pre

Appearance
  Content
  Fonts
  Colors

are now on the "new" preferences UI. In the "(Legacy Prefwindow)" they now show an empty page, except "Content" which is absent.

	Ian Neal Assignee
	Comment 48 • 17 years ago

Don't forget updating help too.

	Robert Kaiser
	Comment 49 • 17 years ago

Help for this is done together with the content panel in bug 420730