340173 - Rework l10n part of NSIS installer

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Axel Hecht [:Pike]]

As agreed on in bug 326580 comment 14, the l10n part of the NSIS installer should
be reworked to be as trivial as possible.

I'm filing this so that we have it on our radar.

I'd favour a solution of using just a .inc file with #defines to be used to
preprocess a commonLocale.nsh.in in generic, much as we do for install.rdf.
If we can have the generated file go through iconv to check the dedicated
charset during build, that would make QA of the installer a lot less error prone
and will help the smaller locales in particular.

Comment 1

[Robert Strong (they/them - no direct email)]

I'll *look* into this as stated in bug 326580 comment 14 (e.g. I'll see what can be done to simplify the format of the file that contains our additional strings). It would be terrific if someone else could take this on especially since NSIS is very simple to work with and I am having to learn as I go as would anyone else would have to that took this on.

Axel, perhaps you could do this? I know you have made a couple of suggestions on how to accomplish this and since you have an interest in this it seems like it would be a good fit.

Comment 2

[Robert Strong (they/them - no direct email)]

The desired format for this file is the same as the properties file

Comment 3

[Axel Hecht [:Pike]]

Attachment: create commonLocales.nsh from Makefile and l10n defines

Here is the part that actually moves commonLocale.nsh to generic, 
commonLocale.nsh.in and preprocesses and converts it to the right charset.
The NSIS language is picked by language.mk, that's a makefile as I need this
in the postprocessing. It's just in there once though, I pass the language
and it's uppercase for the ID via -D to the preprocessor.

This patch does not fix the following things:
- separate browser from toolkit strings
- defend against code injection (a stricter compareDefines in compare-ocales.pl
would help)
- document the whole thing (I tried to not make things worse)

I took the charsets to convert to from NSIS 2.17, I didn't verify that those
are the same as the versions installed on the tinderboxens.

Charset statements on the nsis site are very discouraging, I hope the code is
better than they claim it to be. Otherwise we're up for some suprises.
QA should do some comparison tests of the xpinstall installer and the nsis
one once rob gets this sorted out.

Comment 4

[Robert Strong (they/them - no direct email)]

Axel, thank you for the patch! I talked with schrep about this and he wanted it to be in the same format as one of the existing locale formats. Would you prefer it if the input file was in the same format as a properties file? I should be able to put together a perl script to convert it but I want to make sure this is something you would want before I do so.

Comment 5

[Axel Hecht [:Pike]]

I guess what schrep meant to say was "use a well supported format like 
properties".

.inc files have
- a well defined syntax
- example usage in the l10n rep (defines.inc)
- a known encoding (utf-8)
- support in compare-locales.pl (though not as strict as we should, probably,
different bug)
- a clear mapping to key-value

property files don't bring an improvement here, and their encoding story is
a bit sad and historic, we shouldn't introduce new usages for those. bsmedberg
is talking about making stringbundles load from DTD files, too, so those
are on the fall.
It might be an interesting experiment to make the preprocessor.pl support
reading variable assignements from DTD files, too. Filed bug 340501.

Summary, we have all the tooling support we need as we don't introduce a new
format. Making those less is a different bug.

Given the comments we get from the l10n community about the quality of the
NSIS-internal l10n, we may have to be more aggressive about supporting our
own copies of that. What's the licensing story there, is gpl-compat a problem?

Comment 6

[Axel Hecht [:Pike]]

Attachment: add localization note with available languages to language.mk

I just saw the the comment about the available languages looks nice in generic,
but should really be in language.mk, too, so that localizers actually see it.

PS: bug 340513 tracks requests for updates to the nsis language files. I don't 
even see makensis picking up English.nlf, though, Rob?

Comment 7

[Mike Connor [:mconnor]]

-> Axel, since he's working on this already, and no one else has cycles left.

Comment 8

[Axel Hecht [:Pike]]

Attachment: merge bug 340167

I updated the comment and fixed the broken l10n file pickup from bug 340167. 
I didn't include any of my test changes for bug 340953.

I'm not sure if we should change the file format in l10n before we actually
get an nsis installer to pick up the localized strings. I would think that
both el and pl make decent test locales.

Comment 9

[Axel Hecht [:Pike]]

Attachement 224997 doesn't merge in the de-xpinstall that bsmedberg landed on the
trunk yet.
That's simple to do, though, the chunk in Makefile.in doesn't apply and needs
to be ported to
toolkit/mozapps/installer/windows/nsis/makensis.mk. 

Comment 10

[Robert Strong (they/them - no direct email)]

Axel, how would languages like Chinese be handled since iconv doesn't appear to handle the codepages for zh-CN and zh-TW or is there a way to accomplish this with these codepages?

Comment 11

[Axel Hecht [:Pike]]

TradChinese specifies CP950, SimpChinese CP936, Japanese CP932 in NSIS 2.17.

Just looked at the patch, I just forgot to scroll down after my grep foo and 
dropped all locales after Russian :-/. Oops, sorry.

Comment 12

[Axel Hecht [:Pike]]

The relevant helper macro looks like this (non-tested, though)

NSIS_CODEPAGE = $(if $(filter $(1), Albanian Arabic Basque Belarusian Breton Bulgarian Catalan Croatian Czech Danish Dutch English Estonian Farsi Finnish French German Hungarian Icelandic Indonesian Italian Kurdish Latvian Lithuanian Luxembourgish Macedonian Malay Mongolian Norwegian Portuguese PortugueseBR Slovak Slovenian Spanish Swedish Thai Turkish Ukrainian Welsh), CP1252, \
  $(if $(filter $(1), Japanese), CP932, \
  $(if $(filter $(1), SimpChinese), CP936, \
  $(if $(filter $(1), Korean), CP949, \
  $(if $(filter $(1), TradChinese), CP950, \
  $(if $(filter $(1), Bosnian Polish Romanian SerbianLatin), CP1250, \
  $(if $(filter $(1), Russian Serbian), CP1251 , \
  $(if $(filter $(1), Greek), CP1253, \
  $(if $(filter $(1), Hebrew), CP1255, CODEPAGE_NOTFOUND)) \
  )))))))

I'm not attaching a new patch, because rob is probably on the trunk, and this 
macro goes somewhere to makensis.mk there, anyway, AFAICT.

Comment 13

[Axel Hecht [:Pike]]

A short excerpt of what's making progress in rob's, bsmedberg's and mine inboxens.

We'll drop the NSIS-shipped localizations and replace them with our own strings.
The NSIS-ready files to be used for that will be generated by the build process,
the locale strings come from .properties files. If we make them support \u unicode
escapes is open.

Rob is working on this, so I assign this bug to him.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: sample locale file for custom strings

Axel and Benjamin, is this ok for a locale file?

I finished most of it tonight. I am going to finish splitting out the code so apps can customize their installers before submitting.

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 225850 [details]
sample locale file for custom strings

This looks good, I have some nits and questions though.

Could you start the comment with 

# LOCALIZATION NOTE:

># Accesskeys are defined by prefixing the letter that is to be used for the
># accesskey with an ampersand (e.g. &).
># Do not replace ${BrandShortName} or ${BrandFullName} with a custom string.
># Use \n to create a newline in the string
>APP_DESC=Required files for the ${BrandShortName} application
>DEV_TOOLS_DESC=A tool for inspecting the DOM of HTML, XUL, and XML pages, including the browser chrome.
>QFA_DESC=A tool for submitting crash reports to Mozilla.org.
>SAFE_MODE=Safe Mode
>OPTIONS_PAGE_TITLE=Setup Type
>OPTIONS_PAGE_SUBTITLE=Choose setup options
>SHORTCUTS_PAGE_TITLE=Set Up Shortcuts
>SHORTCUTS_PAGE_SUBTITLE=Create Program Icons

This comment was supposed to go, we want the uninstall survey unconditionally,
right?
># to enable the survey, see DO_UNINSTALL_SURVEY in appLocale.nsi
>SURVEY_TEXT=&Tell us what you thought of ${BrandShortName}
>LAUNCH_TEXT=&Launch ${BrandFullName} now
<...>
>OPTION_CUSTOM_DESC=You may choose individual options to be installed. Recommended for experienced\nusers.

The last string looks like you're using \n to make some strings fit into
the dialog size, is that right? That yields two questions, should the dialog
size be localizable, and, can we document which strings need manual line
breaking and which break automatically?

Do we need to take care about quoting in the strings? ' vs ". If so, could
you add how to do so to the localization note?

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to comment #15)
> Could you start the comment with 
> 
> # LOCALIZATION NOTE:
Done


> This comment was supposed to go, we want the uninstall survey unconditionally,
> right?
Yes

> The last string looks like you're using \n to make some strings fit into
> the dialog size, is that right? That yields two questions, should the dialog
> size be localizable, and, can we document which strings need manual line
> breaking and which break automatically?
Yes... I need to work on this some more before committing to one way or the other.

> Do we need to take care about quoting in the strings? ' vs ". If so, could
> you add how to do so to the localization note?
This is handled by the pre-processing so including " or ' in strings is covered.

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: patch

I gave up on trying to make the installer.nsi generic enough to keep in toolkit. This also allows each app to override the default locale files in toolkit.

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 226313 [details] [diff] [review]
patch

>Index: browser/installer/windows/nsis/installer.nsi
...
>+; empty files - except for the comment line - for generating custom pages.
>+!system 'echo ; > options.ini'
>+!system 'echo ; > shortcuts.ini'
Instead of having these in CVS I generate them in the installer itself. I believe there may be a way to not have to create them as this does but had difficulty getting that to work so I went with this method.

>+ReserveFile options.ini
>+ReserveFile shortcuts.ini
>+
>+!define MUI_COMPONENTSPAGE_SMALLDESC
>+;!define MUI_COMPONENTSPAGE_NODESC
>+
>+!define MUI_ICON setup.ico
>+!define MUI_UNICON setup.ico
>+
>+!define MUI_WELCOMEPAGE_TITLE_3LINES
>+!define MUI_WELCOMEFINISHPAGE_BITMAP wizWatermark.bmp
>+!define MUI_UNWELCOMEFINISHPAGE_BITMAP wizWatermark.bmp
>+
>+!define MUI_HEADERIMAGE
>+!define MUI_HEADERIMAGE_RIGHT
>+!define MUI_HEADERIMAGE_BITMAP wizHeader.bmp
>+
>+!define MUI_ABORTWARNING
>+
>+/**
>+ * Installation Pages
>+ */
>+; Welcome Page
>+!insertmacro MUI_PAGE_WELCOME
>+
>+; License Page
>+;LicenseData /LANG=${LANG_ENGLISH} license.txt
>+;LicenseData /LANG=${LANG_ITALIAN} license.txt
>+LicenseForceSelection radiobuttons
>+;LicenseForceSelection checkbox
bah! I forgot to take these commented out lines. I just removed it in my tree.

>+; Setup the survey controls, functions, etc. except when the application has
>+; defined NO_UNINSTALL_SURVEY
>+!ifndef NO_UNINSTALL_SURVEY
>+!define MUI_FINISHPAGE_SHOWREADME_TEXT $(SURVEY_TEXT)
>+!define MUI_FINISHPAGE_SHOWREADME_FUNCTION un.survey
>+!endif
I reversed the logic here so the uninstall survey will be available by default and an app can choose to override it and not have an uninstall survey.

>+Section "-Application" Section1
This hides the app on the optional components page since it will always be installed.

>+  ${If} ${FileExists} "$EXEDIR\removed-files.log"
>+    ${LogHeader} "Removing Obsolete Files and Directories"
>+    ${LineFind} "$EXEDIR\removed-files.log" "/NUL" "1:-1" "onInstallDeleteFile"
>+    ${LineFind} "$EXEDIR\removed-files.log" "/NUL" "1:-1" "onInstallRemoveDir"
>+  ${EndIf}
The changes made to remove the xpinstall based installer made it so that the config directory is no longer created and removed-files.log is now alongside the setup.exe so I changed this accordingly.

>+  ; Create Start Menu shortcuts
>+  ${LogHeader} "Adding Shortcuts"
>+  ${If} $AddStartMenuSC == 1
>+    CreateDirectory "$SMPROGRAMS\$StartMenuDir"
>+    CreateShortCut "$SMPROGRAMS\$StartMenuDir\${BrandFullName}.lnk" "$INSTDIR\${FileMainEXE}" "" "$INSTDIR\${FileMainEXE}" 0
>+    ${LogUninstall} "File: $SMPROGRAMS\$StartMenuDir\${BrandFullName}.lnk"
>+    CreateShortCut "$SMPROGRAMS\$StartMenuDir\${BrandFullName} ($(SAFE_MODE)).lnk" "$INSTDIR\${FileMainEXE}" "-safe-mode" "$INSTDIR\${FileMainEXE}" 0
>+    ${LogUninstall} "File: $SMPROGRAMS\$StartMenuDir\${BrandFullName} ($(SAFE_MODE)).lnk"
>+  ${EndIf}
Install will no longer remove shortcuts during a custom install when shortcuts have been de-selected (bug 341659).

>+Function ListWindowsInfo
I forgot to remove this.

>+Function preComponents
>+  Call CheckCustom
>+  ; If DOMi isn't available skip the options page
>+  ${Unless} ${FileExists} "$EXEDIR\optional\extensions\[e-mail]"
>+    ; If talkback exists always install it enabled.
>+    ${If} ${FileExists} "$EXEDIR\optional\extensions\[e-mail]"
>+      SectionSetFlags 2 1
>+    ${EndIf}
>+    Abort
>+  ${EndUnless}
>+FunctionEnd
This hides talkback on the optional components list if it isn't in the package (e.g. tbox builds other than the nightly build). If DOMi isn't in the package the components page is skipped and talkback will be installed enabled for custom installs.

>+Function .onInit
>+  StrCpy $LANGUAGE 0
>+;Call ListWindowsInfo
>+  !insertmacro MUI_INSTALLOPTIONS_EXTRACT "options.ini"
>+  !insertmacro MUI_INSTALLOPTIONS_EXTRACT "shortcuts.ini"
>+  !insertmacro createShortcutsINI
>+  !insertmacro createBasicCustomOptionsINI
>+;  !insertmacro createBasicCompleteCustomOptionsINI
>+  ; Hide DOMi in the components page if it isn't available
>+  ${Unless} ${FileExists} "$EXEDIR\optional\extensions\[e-mail]"
>+    SectionSetText 1 ""
>+  ${EndUnless}
>+  ; Hide Talkback in the components page if it isn't available
>+  ${Unless} ${FileExists} "$EXEDIR\optional\extensions\[e-mail]"
>+    SectionSetText 2 ""
>+  ${EndUnless}
>+FunctionEnd
Language ID 0 is the default language and by using 0 for the language ID throughout a single language installer will use that language. I removed the commented out call to ListWindowsInfo from my tree.

>Index: browser/installer/windows/nsis/SetProgramAccess.nsi
Made it so that if a shortcut has arguments this will not remove it in set program access - hide and fixed a bug in the cwd of the shortcut due to our installer not being located next to the main exe.

>Index: toolkit/mozapps/installer/windows/nsis/common.nsh
Added MOZ_ macros for the MUI.

>+!macro createShortcutsINI
...
>+!macro createBasicCompleteCustomOptionsINI
Adds the appropriate values to the ini files so we no longer need them in cvs.

> !macro GetParentDir
>    Exch $R0
>    Push $R1
>    Push $R2
>    Push $R3
>    StrLen $R3 $R0
>   ${DoWhile} 1 > 0
>     IntOp $R1 $R1 - 1
>@@ -116,19 +373,20 @@
>   StrCpy $R0 "$R0" "" 1
> 
>   StrCpy $R9 "$R0" "" -1
>   StrCmp $R9 "\" 0 +2
>   StrCpy $R0 "$R0" -1
> 
>   ClearErrors
>   GetFullPathName $R8 $R0
>-  IfErrors 0 +2
>+  IfErrors +2 0
This was a bug in the logic.

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch w/ removed cruft

Comment 20

[Axel Hecht [:Pike]]

I'm looking at the l10n stuff first, so

# To make the l10n tinderboxen see changes to this file you can change a value
# name by adding - to the end of the name followed by chars (e.g. Branding-2).

What's the usecase for this? NSIS changes the sematics of a string without
renaming it, but we would want to?
I guess that's a followup bug, but we should probably check the NSIS version
to be compatible with what we do.

Regarding the ordering, base.properties is brittle there, custom.properties and
mui.properties are not?
That aspect worries me a little, we never compare with ordering in case, and
not all tools care about it.

In base.properties, you have a few lines with =:, i.e.
LicenseSubCaption=: License Agreement
Is that intentional?

Re
Branding=Nullsoft Install System %s
As we use BrandingText " " to hide that, do we need that string?

The fallback to localized toolkit properties in preprocess-locale.pl won't
work, it's either
$topsrcdir/toolkit/locales/en-US/installer/windows/base.properties or
$topsrcdir/../l10n/$AB_CD/toolkit/installer/windows/base.properties
I don't think we should pick up en-US as emergency fallback so I guess a small
perl port of EXPAND_LOCALE_SRCDIR is the best way to solve that.

locales.nsi says that en_US should 11033, but uses 1033. That looks a tad
confusing. Do we have to keep the locales.nsi information central or could
we pick it up from base.properties? Or a lang.properties next to it? I'm not
sure that locales.nsi scales with the dozen new locales coming in.
Do we want to explicitly allow empty IDs? I guess we still need to fill out
the empty codepages, at least?

Comment 21

[Benjamin Smedberg]

Comment on attachment 226317 [details] [diff] [review]
patch w/ removed cruft

>Index: toolkit/locales/en-US/installer/windows/base.properties

>+# To make the l10n tinderboxen see changes to this file you can change a value
>+# name by adding - to the end of the name followed by chars (e.g. Branding-2).

Hrm?

>+# The order of the strings below are important and must not be changed.

That's going to cause havoc with automated tools. Why is this necessary?

>Index: toolkit/mozapps/installer/windows/nsis/locales.nsi

>+; English, Great Britain
>+;!define en-GB_id   ""
>+;!define en-GB_cp   "CP1252"

Why commented out?

>Index: toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl

>+print "iconv -f UTF-8 -t $langCP instgen/$AB_CD-base.properties > instgen/$AB_CD.nlf\n";
>+system("iconv -f UTF-8 -t $langCP instgen/$AB_CD-base.properties > instgen/$AB_CD.nlf") &&
>+  die "Error converting codepage for instgen/$AB_CD-base.properties";

You repeat this pattern at least twice: perhaps you should have a little utility sub to print a command, run it, and die if failed?

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to comment #20)
> What's the usecase for this? NSIS changes the sematics of a string without
> renaming it, but we would want to?
> I guess that's a followup bug, but we should probably check the NSIS version
> to be compatible with what we do.
We currently change the name of the string when we change the string itself in our properties files so the l10n tboxes see the changes. This is for if we change a string whether it is due to an update we want from NSIS or otherwise.

> Regarding the ordering, base.properties is brittle there, custom.properties and
> mui.properties are not?
> That aspect worries me a little, we never compare with ordering in case, and
> not all tools care about it.
I agree and considered forcing the output of the strings in the correct order via the perl script. Perhaps a followup bug.

> In base.properties, you have a few lines with =:, i.e.
> LicenseSubCaption=: License Agreement
> Is that intentional?
Yes, they are the same as the NSIS strings.

> Re
> Branding=Nullsoft Install System %s
> As we use BrandingText " " to hide that, do we need that string?
Yes, all strings are necessary for it to be read in correctly.

> The fallback to localized toolkit properties in preprocess-locale.pl won't
> work, it's either
> $topsrcdir/toolkit/locales/en-US/installer/windows/base.properties or
> $topsrcdir/../l10n/$AB_CD/toolkit/installer/windows/base.properties
> I don't think we should pick up en-US as emergency fallback so I guess a small
> perl port of EXPAND_LOCALE_SRCDIR is the best way to solve that.
Bummer, perhaps just having each app then supply the files.

> locales.nsi says that en_US should 11033, but uses 1033. That looks a tad
> confusing. Do we have to keep the locales.nsi information central or could
> we pick it up from base.properties? Or a lang.properties next to it? I'm not
> sure that locales.nsi scales with the dozen new locales coming in.
> Do we want to explicitly allow empty IDs? I guess we still need to fill out
> the empty codepages, at least?
We are going to need something along these lines for when the installer supports multiple languages as in CD distributions and this is when the ID's will be used. We could use a separate file to create this on a per locale basis and just increment the ID's, etc.

Comment 23

[Robert Strong (they/them - no direct email)]

(In reply to comment #21)
> (From update of attachment 226317 [details] [diff] [review] [edit])
> >Index: toolkit/locales/en-US/installer/windows/base.properties
> 
> >+# To make the l10n tinderboxen see changes to this file you can change a value
> >+# name by adding - to the end of the name followed by chars (e.g. Branding-2).
> 
> Hrm?
I made the assumption that there was a comparison routine to make the l10n tboxes see the changes.

> >+# The order of the strings below are important and must not be changed.
> 
> That's going to cause havoc with automated tools. Why is this necessary?
NSIS reads those strings in order and applies them. I believe they can just be overridden instead so I'll look into it.

> >Index: toolkit/mozapps/installer/windows/nsis/locales.nsi
> 
> >+; English, Great Britain
> >+;!define en-GB_id   ""
> >+;!define en-GB_cp   "CP1252"
> 
> Why commented out?
The locales we have that aren't available from NSIS are commented out since I don't have an id for it yet. I'll add one.

> >Index: toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl
> 
> >+print "iconv -f UTF-8 -t $langCP instgen/$AB_CD-base.properties > instgen/$AB_CD.nlf\n";
> >+system("iconv -f UTF-8 -t $langCP instgen/$AB_CD-base.properties > instgen/$AB_CD.nlf") &&
> >+  die "Error converting codepage for instgen/$AB_CD-base.properties";
> 
> You repeat this pattern at least twice: perhaps you should have a little
> utility sub to print a command, run it, and die if failed?
Sure.

Comment 24

[Robert Strong (they/them - no direct email)]

How about just reading the name value pairs in base.properties into vars and forcing them to print out in the correct order?

Comment 25

[Axel Hecht [:Pike]]

(In reply to comment #24)
> How about just reading the name value pairs in base.properties into vars and
> forcing them to print out in the correct order?
> 

Sounds good.

Regarding the language IDs, I'm still puzzled about the additional 10000.
As this stuff won't be used until we do the multiple locale installers, can't we
just drop them for now? I'm not sure if we'll be gaining much if we have some
numbers being the same, others not, unless there is something magic with the 
en-US being 1033 inside NSIS.
I'd favor to see the encoding to be defined in the l10n reps, though. There will
be languages for which that is going to change from 2.0 to 3.0, esp indic ones,
just FYI.

Comment 26

[Robert Strong (they/them - no direct email)]

Are you ok with keeping the font name, font size, and whether the language is right to left outside of the locale since these values are for the most part not overridden from the defaults and will seldom if ever change?

Comment 27

[Robert Strong (they/them - no direct email)]

I found a way to just override the values so the order won't be important.

Comment 28

[Robert Strong (they/them - no direct email)]

Attachment: patch

Changes from last patch:
Require each app to provide the locale files
Removal of most of the contents of locales.nsi. I want to keep these values in this file in part due to the multi-language installer work that will need to be done.
Retrieve codepage from charset.mk
Generate a minimal nlf for NSIS and then override the values. This allows for any order of the values in override.properties.
Removal of code to support multi-language installer.

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 226433 [details] [diff] [review]
patch

>Index: toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl
>+  if ($line =~ m|^!define$AB_CD\_rtl|) {
>+    $RTL = "RTL";
>+  }
that should be
>+  if ($line =~ m|^!define $AB_CD\_rtl|) {

other than that I think this should be good to go.

Comment 30

[Robert Strong (they/them - no direct email)]

Axel, I forgot to mention that we can remove some of the strings from override.properties though I'd prefer to keep them so if we decide to use functionality that requires the string it will already be available. Also, I was considering writing a utility for converting an existing NSIS main and mui language file into the properties format... would that be worthwhile to do?

Comment 31

[Axel Hecht [:Pike]]

First off, this looks good over all.
I see a warning and a few ERRORs in the build log, but those don't seem to
be locale string related.
I didn't test to create a localized installer, yet, I guess I need to whack
my branch tree for that.

There are tons of unused strings in those files, right? I think there are
four ways to accept a license, one of which we use, and then there are slightly
different versions of that in override.properties, all of which are unused?

The problem I see, if we don't use those strings, we can't QA them, and given
the recent push for QA in l10n, I'm not sure we could actually use them without
QA. Given that I don't understand $_CLICK about what they do, it's probably
better that way.
If we want those strings in, we should probably have a dummy-installer using
them, so that localizers and QA can actually see them in action.

I created a full set of screen shots to be used in a devmo document on how
to localize the installer, I don't think we should make big quality assumptions
on any string that doesn't show up on those, talkback (and reporter?) aside, I 
don't have that but we of course want those to be good.

Comment 32

[Robert Strong (they/them - no direct email)]

Axel, I'll remove the strings that aren't used. With that change are you ok with +'ing the patch otherwise?

Comment 33

[Axel Hecht [:Pike]]

Comment on attachment 226433 [details] [diff] [review]
patch

Yes, perfectly. Thanks a lot.

Comment 34

[Robert Strong (they/them - no direct email)]

Attachment: patch for checkin

Here is the patch I will be checking in shortly. The only strings that aren't readily apparent in the ui are the error message strings and the logging strings. Thanks Axel and Benjamin for the reviews!

Comment 35

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 226557 [details] [diff] [review]
patch for checkin

Please give it a day or two before requesting branch approval (trying to resurrect trunk baking, per the announcement)

Comment 37

[Axel Hecht [:Pike]]

Adding the repackaging bug 339714 to the dependency chain.

Comment 38

[Axel Hecht [:Pike]]

Comment on attachment 226557 [details] [diff] [review]
patch for checkin

re-requesting approval, we want this patch and bug 339714 to be on the branch by Friday. We're short on trunk baking time, and we have only very few locales on trunk compared to branch. The l10n-baking of this will need to happen on the branch, sadly.

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: remove appLocale.nsi

Forgot to include the removal of this file. :(

Comment 40

[Axel Hecht [:Pike]]

Comment on attachment 226622 [details] [diff] [review]
remove appLocale.nsi

I was about to file that as a follow up bug :-).

Comment 41

[Robert Strong (they/them - no direct email)]

(In reply to comment #40)
> (From update of attachment 226622 [details] [diff] [review] [edit])
> I was about to file that as a follow up bug :-).
Thanks and I just checked it in.

Comment 42

[Mike Connor [:mconnor]]

Comment on attachment 226557 [details] [diff] [review]
patch for checkin

So, just because we have less coverage on trunk doesn't mean we shouldn't make sure we get a couple days of builds, even on en-US to make sure the changes are reasonably solid.  The l10n testing is the second step, IMO.

Comment 43

[Robert Strong (they/them - no direct email)]

Checked in on MOZILLA_1_8_BRANCH for Firefox 2.0

Comment 44

[dynamis (Tomoya ASAI)]

Hi, I have a question.
Why ab-CD_font and ab-CD_size in locales.nsi are defined only for Chinese?

I checked all locale files contributed to NSIS:
http://nsis.cvs.sourceforge.net/nsis/NSIS/Contrib/Language%20files/
And result is, below four locales specified their own font/size.
http://nsis.cvs.sourceforge.net/nsis/NSIS/Contrib/Language%20files/Japanese.nlf?view=markup
http://nsis.cvs.sourceforge.net/nsis/NSIS/Contrib/Language%20files/Korean.nlf?view=markup
http://nsis.cvs.sourceforge.net/nsis/NSIS/Contrib/Language%20files/SimpChinese.nlf?view=markup
http://nsis.cvs.sourceforge.net/nsis/NSIS/Contrib/Language%20files/TradChinese.nlf?view=markup
So, I think we should write font settings not only for zh-CN/zh-CN but also for ja/ko locales.
# IMHO, it's better if we can define this font setting in a L10N CVS file.

See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=343449#c2
# I think that lack of this definition is the cause of bug 343449

Comment 45

[dynamis (Tomoya ASAI)]

While writting previous post, Axel already answered in bug 343449.
https://bugzilla.mozilla.org/show_bug.cgi?id=343449#c3
Sorry.

Comment 46

[Robert Strong (they/them - no direct email)]

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