Currently, our installer on Windows shows garbled strings when the codepage (character encoding) of an installer is different from the codepage for 'non-Unicode' applications on Windows 2x/XP (and the default system codepage on Windows 9x/ME). That's because it uses 'A' versions of Windows APIs even when 'W' APIs are available on Win 9x/ME (let alone Win 2k/XP). (e.g. |ExtTextOut|). There are two consequences. The first is what's mentioned at the beginning of this report. The other is that we can't make installers for languages that are not supported by any of legacy Windows codepages. For instance, Hindi or Tamil installers can't be made. To fix this, we need to replace |ExtTextOut| and alike with the corresponding W APIs. In addition, translated strings for each localized installer need to be converted to UTF-8. A trickier part is how to deal with Windows APIs whose 'W' versions are not available on Windows 9x/ME unless MSLU is present (e.g. SendMessage). It might be possible to define 'UNICODE' in 'Makefile' or equivalent and to rely on the fact that MSLU is very likely to be available on virtually all Windows 9x/ME (even then, we might have to know its location, which is not easy)

I would like us to hold off on this until some of the xulrunner-installer stuff is sorted out: I think I'm going to propose that we define UNICODE universally and use the W APIs consistently throughout the app (and drop win95 if MSLU is not already present on the system).

Are you talking about installer only or all of our applications here? I guess you meant the latter. I'd love to use 'W' APIs consistently throughout, but it may not be so simple (recall our discussion in bug 239279) Anyway, I won't be able to work on this (even if I want to go ahead) until sometime later so that it shouldn't be a worry to you.

Yes, I meant our entire app story. Simply require MSLU be pre-installed or obtained by external means.

If the "non-Unicode" encoding is not set properly for Windows 2K/XP/etc, then the installer message are not readable. However, once the installation completes, Firefox shows the UI messages in the correct language. As a short term plan I would see fit to fix the installer to display UI messages as Unicode/UTF-8, just like the Firefox/Thunderbird applications do. Otherwise, the installer should detect if the "non-Unicode" encoding is not suitable and warn the user. We have quite a few end-users (living abroad) who were affected by this bug and I believe that other languages apart from Greek are affected.

You could use the Nullsoft Installer, http://nsis.sourceforge.net/

Tamil unicode characters are not rendered correctly.

(In reply to comment #5) > You could use the Nullsoft Installer, > http://nsis.sourceforge.net/ We do use it beginning with FF 2.0. Did that solve this problem?

Yes, this does solve the issue. Many thanks!

(In reply to comment #8) > Yes, this does solve the issue. Thanks for the confirmation. With that new installer, as long as firefox is installed in a folder whose name does not contain any character outside the system default code page, you can install and *run* Japanese (Chinese, Arabic, Hebrew, Korean) version of firefox on English (French, Russian) Windows. I'll resolve this as fixed after trying to install either Gujarati or Punjabi firefox 2.0 on Korean Win2k.

Soon we are releasing Firefox 3, and still this issue is open and problematic for Windows users who didn't changed the default charset for non-unicode. I've attached some screenshots.

At #8 I mention that the issue has been solved. I verify that the issue is *not* solved in Firefox 3RC2 for the Greek language (nor Russian, Ukranian and any language that does not fit in iso-8859-1).

(In reply to comment #8) > Yes, this does solve the issue. I beg to differ (Firefox 3rc2).

(In reply to comment #17) > (In reply to comment #8) > > Yes, this does solve the issue. > > I beg to differ (Firefox 3rc2). This bug which is to use unicode in the installer is not fixed and yes, it is not solved / fixed yet.

Use the Unicode version of NSIS for the Unicode language installers. http://www.scratchpaper.com.

Jim: thanks, we've already packaged that in MozillaBuild, there's just some extra work to be done to support it.

(In reply to comment #20) > Jim: thanks, we've already packaged that in MozillaBuild, there's just some > extra work to be done to support it. Ted, how can we know when it is ready?

Filip, when this bug is marked fixed it will be fixed on the trunk.

Can we get this into Firefox 3.1? Reassigning to nobody, I assume that jshin won't work on this.

Robert, IIRC, we'd be looking at either this or MSI, right? I haven't seen too much progress lately on the msi bug, I wonder what we should go for. What'd be your take?

I highly suspect getting the Unicode NSIS installer working will be much easier.

Could you take that on for fx3.1? Unless you're scared of a hug-feast at the next summit ;-)

Not sure at this time... I've got a couple of bugs for 3.1 and this is definitely on my radar if I can get those done in time.

Could I get an update, how are things?

This isn't on my radar for 3.1

So, I'm willing to give this a shot. What's this going to take in order to get fixed? Is it enough to switch to the Unicode NSIS, and remove the iconv conversions for locale files (we already require them to be in UTF-8, and the iconv command expects them to as well, so I guess it's safe to assume that they're Unicode)?

(In reply to comment #32) > So, I'm willing to give this a shot. What's this going to take in order to get > fixed? Is it enough to switch to the Unicode NSIS, and remove the iconv > conversions for locale files (we already require them to be in UTF-8, and the > iconv command expects them to as well, so I guess it's safe to assume that > they're Unicode)? No regarding being able to just switch to the Unicode version and remove the use of iconv or it would have been done a long time ago ;) NSIS made changes to the MUI wizard in newer versions of NSIS which will require changes to our installer code. Also, the latest version of MozillaBuild contains the 2.33 Unicode version of NSIS that was available at the time of the last MozillaBuild release.

(In reply to comment #33) > NSIS made changes to the MUI wizard in newer versions of NSIS which will > require changes to our installer code. Also, the latest version of MozillaBuild > contains the 2.33 Unicode version of NSIS that was available at the time of the > last MozillaBuild release. Any specific changes made to the MUI wizard that you may be aware of? Also, would we need to change the Unicode version of NSIS to an older/newer version in order to be able to build Unicode installers if needed?

Changing the unicode nsis version would require shipping and distributing a new version of MozillaBuild. I'd surely lobby for doing that, if we have good reasons to do so.

(In reply to comment #35) > Changing the unicode nsis version would require shipping and distributing a new > version of MozillaBuild. I'd surely lobby for doing that, if we have good > reasons to do so. Well, in current situation, we are not able to ship a localized installer for many locales, because they either may not have a code-page available, or a non-Unicode code-paged version may not be helpful because the majority of users don't run Windows under that code-page. Considering the fact that the installer is the first thing many users see in the application, I guess it would be very nice to have it fully localized. Does this reasoning look strong enough?

You're confusing two things: We include version 2.33 of NSIS with unicode support in MozillaBuild 1.3, which is what all our build slaves should have installed. That's a different version than the non-unicode NSIS we're using, 2.22. Those two have different versions because the patched versions of NSIS aren't available for the version we use with codepages. Thus, in order to use the unicode version, one has to update our nsis code from 2.22 to 2.33. See http://mxr.mozilla.org/mozilla/source/tools/build-environment/win32/ for what we ship currently. If you happen to find that a later version of nsis 2.33 would be easier to port to, and has unicode support patches available, we could go for that. That would then require us to actually update MozillaBuild, though, and deploy that on our windows build slaves, at least.

(In reply to comment #34) > (In reply to comment #33) > > NSIS made changes to the MUI wizard in newer versions of NSIS which will > > require changes to our installer code. Also, the latest version of MozillaBuild > > contains the 2.33 Unicode version of NSIS that was available at the time of the > > last MozillaBuild release. > > Any specific changes made to the MUI wizard that you may be aware of? The code that I know is broken is the custom wizard code in common.nsh (the section starting with the "Modern User Interface (MUI) override macros" comment) we had to include to satisfy the l10n requirement of not having unused strings in the locale files. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/common.nsh#173 > Also, would we need to change the Unicode version of NSIS to an older/newer > version in order to be able to build Unicode installers if needed? This is as Axel stated.

OK, here's the first WIP patch for you to test and comment on. This patch converts all files in objdir/browser/installer/windows/instgen to UTF-16LE (with the correct BOM injected using a helper script, because iconv doesn't generate them), and makes the necessary changes in order for the install scripts to be compiled with nsis 2.33. With this patch, a working installer and uninstaller is generated, which is capable of displaying Unicode text. I have not tested the installer as fully as I'd like to, but there is at least one major problem: the welcome screen of the installer turns up empty with a big white box in the middle. I have not yet spent any time to track this problem, but any help is certainly appreciated. Sounds like we're going to have Unicode support for the installer soon! :-)

The major change in this patch is that I have compiled all of the used plugins from source in Unicode mode, and updated the DLL files we store in the tree. Still no progress on the problem mentioned before. It seems that the bitmaps, some strings, and some formatting options don't make it into the final installer. I tried the latest Unicode NSIS build as well, with the same result. I've posted this problem to the Unicode NSIS thread on Winamp forums: <http://forums.winamp.com/showthread.php?s=&postid=2455303#post2455303>. If you have any suspicions on what may be going wrong, please let me know.

Happy to see a patch here :-) I can't really comment on the NSIS part, but on the build foo ... I'm having this odd feeling of saying "python". But that might be overdoing it. But I think that adding a perl script for prepending two bytes would be overdoing it, too. Though, codecs.utf_16_encode in python automatically adds the BOM, or you could use codecs.utf_16_le_encode and add the codecs.BOM_UTF16_LE by hand.

I don't know much Python, but I had another solution in mind: having a binary file containing only two bytes of the UTF-16LE BOM, and using cat to prepend it to the installer files generated instead of preprend-bom.pl. How does that sound?

By The way, this can also be done by simple `copy` command from windows: copy source + source destination

Ehsan, very awesome and thanks!

Not sure what exactly is going on but the ioSpecial.ini isn't fully populated with all of the fields required to display the welcome page.

OK, the patch is now ready for review! The problem I described in comment 39 was caused by the fact that we were !defining MUI_INSERT, which was not safe, since the symbol name was started by MUI_, where it should have used MOZ_MUI_ really. Anyway, that !define caused the MUI_INSERT !macro to become a no-op when being !insertmacro'ed, hence the problem. I also made a few more random fixes as well. This should produce a fully Unicode installer.

Here is the modified version of the plugins source code which I used to generate the Unicode binary plugins. This doesn't need to be reviewed -- I'm just attaching it as a reference. The original plugins source packages were downloaded from the NSIS wiki.

(In reply to comment #45) > Not sure what exactly is going on but the ioSpecial.ini isn't fully populated > with all of the fields required to display the welcome page. Yes. I explained why this happened in comment 46 (didn't see your comment then). The v1 patch fixes that issue.

(In reply to comment #47) > Created an attachment (id=352198) [details] > Unicode plugins source > > Here is the modified version of the plugins source code which I used to > generate the Unicode binary plugins. This doesn't need to be reviewed -- I'm > just attaching it as a reference. > > The original plugins source packages were downloaded from the NSIS wiki. When we modify source we typically have to add the source to the tree which can be a bit of a pain. I've updated the AppAssocReg plugin and updated the wiki (http://nsis.sourceforge.net/Application_Association_Registration_plug-in) for it so that one isn't a problem. I'll check if there are Unicode versions of the rest (UAC plugin doesn't appear to have one) to see if we can't lessen the amount of code that we'll have to add / maintain. Also, I recall creating a plug-in awhile back and compiling it with VC 7 which added dependencies on a newer crt which isn't always available... whichever ones we have to compile I can compile them with VC 6 just in case. They are slightly smaller this way as well. Once again, thank you for taking this on.

(In reply to comment #49) > When we modify source we typically have to add the source to the tree which can > be a bit of a pain. I've updated the AppAssocReg plugin and updated the wiki > (http://nsis.sourceforge.net/Application_Association_Registration_plug-in) for > it so that one isn't a problem. I'll check if there are Unicode versions of the > rest (UAC plugin doesn't appear to have one) to see if we can't lessen the > amount of code that we'll have to add / maintain. The UAC plugin was already Unicode-compatible. The only thing I did there is to uncomment the #define UNICODE line in uac.h, and re-compile. The remaining plugins don't have a clear license for their sources, so can we actually include them in the tree? Should we put them inside other-licenses/? Another approach which we could take to have non-Unicode plugins working for us would be to write a simple plugin to convert UTF-16LE strings to ANSI and vice versa, and use that to convert what we pass to the non-Unicode plugins (and possibly what they return). Here is the status of our plugins usage, if I'm reading our NSIS code correctly: processes: we don't seem to use this plugin in the code at all - do we just include its DLL? Can we drop it? nsProcess: we use nsProcess::_FindProcess in two places in common.nsh. This function takes a string parameter and returns strings as well. ShellLink: we use GetShortCutArgs, GetShortCutTarget, and SetShortCutWorkingDirectory in shared.nsh. These all take string parameters, and return strings as well. AppAssocReg and UAC are already Unicode compatible. > Also, I recall creating a > plug-in awhile back and compiling it with VC 7 which added dependencies on a > newer crt which isn't always available... whichever ones we have to compile I > can compile them with VC 6 just in case. They are slightly smaller this way as > well. I have taken great care about that, and made sure they're all compiled with a static CRT library. I have verified their dependencies using the Depends.exe tool <http://www.dependencywalker.com/> to make sure they're not dependent on VC CRT DLLs. Anyway, we can compile them with VC6 as well - I just don't have a VC6 installation handy. > Once again, thank you for taking this on. Sure thing. How does the patch itself look like?

This patch removes the dependency on toolkit/locales/installer/windows/charset.mk.

One minor glitch is I see the following warning when building the uninstaller: warning: LangString "MUI_UNTEXT_WELCOME_INFO_TITLE" is not set in language table of language baseLocale From what I can tell, MUI_UNTEXT_WELCOME_INFO_TITLE is there in baseLocale.nsh. I'm not sure if this warning is caused by a mistake in the patch, or in the localized file I'm using...

(In reply to comment #50) >... > The UAC plugin was already Unicode-compatible. The only thing I did there is > to uncomment the #define UNICODE line in uac.h, and re-compile. > > The remaining plugins don't have a clear license for their sources, so can we > actually include them in the tree? Should we put them inside other-licenses/? > > Another approach which we could take to have non-Unicode plugins working for us > would be to write a simple plugin to convert UTF-16LE strings to ANSI and vice > versa, and use that to convert what we pass to the non-Unicode plugins (and > possibly what they return). This might very well be worth doing. > processes: we don't seem to use this plugin in the code at all - do we just > include its DLL? Can we drop it? Likely... just need to verify other apps (Seamonkey, Thunderbird, and Sunbird) aren't using it and update them or workaround this if they are. > nsProcess: we use nsProcess::_FindProcess in two places in common.nsh. This > function takes a string parameter and returns strings as well. It might be possible to workaround this one. > ShellLink: we use GetShortCutArgs, GetShortCutTarget, and > SetShortCutWorkingDirectory in shared.nsh. These all take string parameters, > and return strings as well. This one will > AppAssocReg and UAC are already Unicode compatible. It is likely that a precompiled Unicode distribution for UAC would be required but I'll check. > > Once again, thank you for taking this on. > > Sure thing. How does the patch itself look like? From what I have looked at so far the patch is in really good shape. I'm going to try to verify everything is working properly today and hopefully have the review done by tomorrow.

(In reply to comment #53) >... > > ShellLink: we use GetShortCutArgs, GetShortCutTarget, and > > SetShortCutWorkingDirectory in shared.nsh. These all take string parameters, > > and return strings as well. > This one will bah... I suspect this one would be best converted. iirc a path can potentially contain multiple charsets.

The CloseApp macro which uses nsProcess::_FindProcess and nsProcess::_KillProcess is only used by Sunbird and Sunbird should be updated to not use it. After this is done the nsProcess.dll can be removed.

In addition !insertmacro CloseApp should be removed from all of the apps including Firefox

The migration to unicode sounds like a great point to drop that then?

It isn't the migration that makes this possible... it is that it would be best to remove nsProcess.dll so it doesn't have to be converted to Unicode and the only app holding back the removal is Sunbird.

(In reply to comment #52) > One minor glitch is I see the following warning when building the uninstaller: > > warning: LangString "MUI_UNTEXT_WELCOME_INFO_TITLE" is not set in language > table of language baseLocale > > From what I can tell, MUI_UNTEXT_WELCOME_INFO_TITLE is there in baseLocale.nsh. > I'm not sure if this warning is caused by a mistake in the patch, or in the > localized file I'm using... Changing the following in common.nsh should fix this - !insertmacro MOZ_MUI_LANGUAGEFILE_LANGSTRING_DEFINE WELCOME "MUI_UNTEXT_WELCOME_INFO_TITLE" + !insertmacro MOZ_MUI_LANGUAGEFILE_UNLANGSTRING_PAGE WELCOME "MUI_UNTEXT_WELCOME_INFO_TITLE"

So, what further things I can do on this bug? I have not tested Samonkey, Thunderbird and Sunbird with this patch, but the changes they need to make in order to support this should be fairly trivial (like the browser/ changes in my patch). Do you want me to take a shot at developing the mentioned NSIS plugin? (Haven't written an NSIS plugin so far, so no idea on the learning curve, but it shouldn't be that hard). Or perhaps we could split some of these stuff to followup bugs to enable our localizers to experiment with the new Unicode installer asap?

(In reply to comment #59) > Changing the following in common.nsh should fix this > > - !insertmacro MOZ_MUI_LANGUAGEFILE_LANGSTRING_DEFINE WELCOME > "MUI_UNTEXT_WELCOME_INFO_TITLE" > + !insertmacro MOZ_MUI_LANGUAGEFILE_UNLANGSTRING_PAGE WELCOME > "MUI_UNTEXT_WELCOME_INFO_TITLE" Thanks for the tip, here's the updated patch with that problem fixed.

Since the other apps rely on common.nsh at minimum they will need the installer/windows/Makefile.in changes and I'd like Sunbird to no longer rely on the CloseApp macro prior to this being checked in so they don't break. As a side note, they are currently using trunk on comm-central and don't have a 1.9.1 of the toolkit. I am pretty sure that converting the strings for ShellLink won't be ideal since a path can contain multiple charsets which when converted from Unicode to ANSI won't work properly in some cases so I am leaning towards just getting Unicode versions of ShellLink, UAC, and AppAssocReg.dll checked into the tree but first I need to find out what licensing issues there may be. I'm going to test Thunderbird and Seamonkey tonight. If you are up to try removing the CloseApp macro from Sunbird - actually replace it with ManualCloseAppPrompt which is already used by all of the other apps - that would be great. If not, I can try to do it sometime in the next couple of days.

(In reply to comment #62) > Since the other apps rely on common.nsh at minimum they will need the > installer/windows/Makefile.in changes and I'd like Sunbird to no longer rely on > the CloseApp macro prior to this being checked in so they don't break. As a > side note, they are currently using trunk on comm-central and don't have a > 1.9.1 of the toolkit. This can be done in another bug, right? > I am pretty sure that converting the strings for ShellLink won't be ideal since > a path can contain multiple charsets which when converted from Unicode to ANSI > won't work properly in some cases so I am leaning towards just getting Unicode > versions of ShellLink, UAC, and AppAssocReg.dll checked into the tree but first > I need to find out what licensing issues there may be. OK. > I'm going to test Thunderbird and Seamonkey tonight. If you are up to try > removing the CloseApp macro from Sunbird - actually replace it with > ManualCloseAppPrompt which is already used by all of the other apps - that > would be great. If not, I can try to do it sometime in the next couple of days. Well, I don't even have a comm-central clone yet. :-) But I may be able to give this a shot tomorrow, if I can figure out the comm-central stuff quickly enough. I'll update this bug if I come up with something, otherwise feel free to handle it yourself when you have the time. Thanks!

(In reply to comment #63) > (In reply to comment #62) > > Since the other apps rely on common.nsh at minimum they will need the > > installer/windows/Makefile.in changes and I'd like Sunbird to no longer rely on > > the CloseApp macro prior to this being checked in so they don't break. As a > > side note, they are currently using trunk on comm-central and don't have a > > 1.9.1 of the toolkit. > > This can be done in another bug, right? It can be done in this bug or another bug. For bugs like this which will require that they are updated at the same time I typically just keep it in the same bug but either way is fine.

(In reply to comment #64) > It can be done in this bug or another bug. For bugs like this which will > require that they are updated at the same time I typically just keep it in the > same bug but either way is fine. So, let's keep everything here, then.

(In reply to comment #62) > As a side note, they are currently using trunk on comm-central and don't have a > 1.9.1 of the toolkit. comm-central pulls releases/mozilla-1.9.1 these days, mozilla-central-based comm-central builds are still a bit out.

I'll request review on this from the Thunderbird reviewers after I have finished reviewing the main patch and verify that all is well.

Comment on attachment 352375 [details] [diff] [review] Patch (v1.2) Hey Ehsan, I just have nits at present and there is no reason to resubmit until I finish the review. I also checked Thunderbird, Seamonkey, and Sunbird tonight and all is looking really good so far. The patch so far looks really good. >diff --git a/browser/installer/windows/Makefile.in b/browser/installer/windows/Makefile.in >--- a/browser/installer/windows/Makefile.in >+++ b/browser/installer/windows/Makefile.in >... >@@ -52,15 +53,23 @@ PP_LOCALIZED_FILES = \ > packages-static \ > $(NULL) > >-INSTALLER_FILES = \ >- app.tag \ >+INSTALLER_FILES_CONV = \ > nsis/installer.nsi \ > nsis/uninstaller.nsi \ > nsis/shared.nsh \ > $(NULL) > >+INSTALLER_FILES = \ >+ app.tag \ >+ $(NULL) >+ >+# Text files stored in UTF-8 encoding nit: comment is not really necessary since this info can be found by looking below. If you'd like to keep the comment please add a similar comment to INSTALLER_FILES_CONV as well as makensis.mk and perhaps change the comment to # UTF-8 encoded files that need to be converted to UTF-16LE for NSIS >+BRANDING_FILES_CONV = \ >+ branding.nsi \ >+ $(NULL) >+ >+# Rest of the branding files nit: comment is not necessary >@@ -87,21 +95,43 @@ installer:: > # included for mar file generation. > uninstaller:: > $(RM) -rf $(CONFIG_DIR) && mkdir $(CONFIG_DIR) >+ for i in $(INSTALLER_FILES_CONV); do \ >+ iconv -f UTF-8 -t UTF-16LE $(srcdir)/$$i | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \ >+ $(CONFIG_DIR)/`basename $$i`; \ nit: the last two lines added should be lined up with the iconv line >+ done > $(INSTALL) $(addprefix $(srcdir)/,$(INSTALLER_FILES)) $(CONFIG_DIR) >+ for i in $(BRANDING_FILES_CONV); do \ >+ iconv -f UTF-8 -t UTF-16LE $(DIST)/branding/$$i | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \ >+ $(CONFIG_DIR)/$$i; \ nit: the last two lines added should be lined up with the iconv line >+ done > $(INSTALL) $(addprefix $(DIST)/branding/,$(BRANDING_FILES)) $(CONFIG_DIR) > $(EXIT_ON_ERROR) \ > for i in $(PP_LOCALIZED_FILES); do \ > $(PERL) $(topsrcdir)/config/preprocessor.pl $(DEFINES) $(ACDEFINES) $(srcdir)/$$i > $(CONFIG_DIR)/$$i; \ > done > $(PERL) $(topsrcdir)/config/preprocessor.pl -Fsubstitution $(DEFINES) $(ACDEFINES) \ >- $(srcdir)/nsis/defines.nsi.in > $(CONFIG_DIR)/defines.nsi >+ $(srcdir)/nsis/defines.nsi.in | iconv -f UTF-8 -t UTF-16LE | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \ >+ $(CONFIG_DIR)/defines.nsi nit: the last two lines added should be lined up with the first > $(PERL) $(topsrcdir)/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl \ > $(topsrcdir) $(call EXPAND_LOCALE_SRCDIR,browser/locales)/installer $(AB_CD) \ >- $(WIN_INSTALLER_CHARSET) $(CONFIG_DIR) >+ $(CONFIG_DIR) > > $(CONFIG_DIR)/setup.exe:: > $(RM) -rf $(CONFIG_DIR) && mkdir $(CONFIG_DIR) >+ for i in $(INSTALLER_FILES_CONV); do \ >+ iconv -f UTF-8 -t UTF-16LE $(srcdir)/$$i | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \ >+ $(CONFIG_DIR)/`basename $$i`; \ nit: the last two lines added should be lined up with the iconv line >+ done > $(INSTALL) $(addprefix $(srcdir)/,$(INSTALLER_FILES)) $(CONFIG_DIR) >+ for i in $(BRANDING_FILES_CONV); do \ >+ iconv -f UTF-8 -t UTF-16LE $(DIST)/branding/$$i | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \ >+ $(CONFIG_DIR)/$$i; \ nit: the last two lines added should be lined up with the iconv line >@@ -110,10 +140,12 @@ uninstaller:: > $(PERL) $(topsrcdir)/toolkit/mozapps/installer/windows/nsis/make-installremoves.pl \ > ../removed-files > $(CONFIG_DIR)/removed-files.log > $(PERL) $(topsrcdir)/config/preprocessor.pl -Fsubstitution $(DEFINES) $(ACDEFINES) \ >- $(srcdir)/nsis/defines.nsi.in > $(CONFIG_DIR)/defines.nsi >+ $(srcdir)/nsis/defines.nsi.in | iconv -f UTF-8 -t UTF-16LE | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \ >+ $(CONFIG_DIR)/defines.nsi nit: the last two lines added should be lined up with the first >diff --git a/toolkit/mozapps/installer/windows/nsis/makensis.mk b/toolkit/mozapps/installer/windows/nsis/makensis.mk >--- a/toolkit/mozapps/installer/windows/nsis/makensis.mk >+++ b/toolkit/mozapps/installer/windows/nsis/makensis.mk >... >@@ -44,24 +45,33 @@ ABS_CONFIG_DIR := $(shell pwd)/$(CONFIG_ > > SFX_MODULE ?= $(error SFX_MODULE is not defined) > >-TOOLKIT_NSIS_FILES = \ >- AppAssocReg.dll \ >+# Text files stored in UTF-8 encoding nit: comment is not really necessary since this info can be found by looking below. >+TOOLKIT_NSIS_FILES_CONV = \ > common.nsh \ > locales.nsi \ >- nsProcess.dll \ > overrides.nsh \ >- ShellLink.dll \ >- UAC.dll \ > version.nsh \ > $(NULL) > >+# Rest of the toolkit files nit: comment is not necessary >+TOOLKIT_NSIS_FILES = \ >+ AppAssocReg.dll \ >+ nsProcess.dll \ >+ ShellLink.dll \ >+ UAC.dll \ >+ $(NULL) >+ > $(CONFIG_DIR)/setup.exe:: >+ for i in $(TOOLKIT_NSIS_FILES_CONV); do \ >+ iconv -f UTF-8 -t UTF-16LE $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/$$i | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > $(CONFIG_DIR)/$$i; \ nit: the last line added should be lined up with the iconv line >@@ -78,9 +88,13 @@ installer:: > # For building the uninstaller during the application build so it can be > # included for mar file generation. > uninstaller:: >+ for i in $(TOOLKIT_NSIS_FILES_CONV); do \ >+ iconv -f UTF-8 -t UTF-16LE $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/$$i | \ >+ cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > $(CONFIG_DIR)/$$i; \ nit: the last line added should be lined up with the iconv line >diff --git a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl >--- a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl >+++ b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl >... >@@ -37,8 +38,7 @@ my $topsrcdir = "$ARGV[0]"; > my $topsrcdir = "$ARGV[0]"; Would you please change this to the following and change $topsrcdir to $mozsrcdir where it is used? Turns out that comm-central isn't setting this to the correct directory and changing this along with the comment should make what should be passed more obvious. my $mozsrcdir = "$ARGV[0]"; # dir that contains the toolkit source Also, could you add the following so any future consumers will fail when that isn't set? > # Read the codepage for the locale and the optional font name, font size, and > # whether the locale is right to left from locales.nsi. > my $inFile = "$mozsrcdir/toolkit/mozapps/installer/windows/nsis/locales.nsi"; > open(locales, "<$inFile"); >+if (!-e $inFile) { >+ die "Error $inFile does not exist!"; >+} >@@ -103,13 +96,13 @@ while( $line = <infile> ) { > $value =~ s/\s+$//; # trim whitespace from the end of the string > $value =~ s/^"(.*)"$/$1/g; # remove " at the beginning and end of the value > $value =~ s/(")/\$\\$1/g; # prefix " with $\ >- $value =~ s/…/.../g; # replace � (unicode ellipsis) with ... >+ $value =~ s/(\\[rnt])/\$$1/g; # prefix all occurences of \\r, \\n and \\t with $ nit: the previous comment was incorrect. It should be as follows here and other comments prefix all occurences of \r, \n and \t with $ >@@ -135,14 +128,14 @@ while( $line = <infile> ) { > $value =~ s/(")/\$\\$1/g; # prefix " with $\ > $value =~ s/(\\n)/\\r$1/g; # insert \\r before each occurence of \\n > $value =~ s/(\\r)\\r/$1/g; # replace all ocurrences of \\r\\r with \\r >- $value =~ s/…/.../g; # replace � (unicode ellipsis) with ... >+ $value =~ s/(\\[rnt])/\$$1/g; # prefix all occurences of \\r, \\n and \\t with $ nit: same here. Please fix the other two comments >@@ -163,14 +156,13 @@ while( $line = <infile> ) { > $string =~ s/"/\$\\"/g; # replace " with $\" > $string =~ s/(\\n)/\\r$1/g; # insert \\r before each occurence of \\n > $string =~ s/(\\r)\\r/$1/g; # replace all ocurrences of \\r\\r with \\r >- $string =~ s/(\\[rn])/\$$1/g; # prefix all occurences of \\r and \\n with $ >- $string =~ s/…/.../g; # replace � (unicode ellipsis) with ... >+ $string =~ s/(\\[rnt])/\$$1/g; # prefix all occurences of \\r, \\n and \\t with $ nit: same here. Please fix the other two comments >@@ -178,9 +170,10 @@ sub cpConvert > { > my $srcFile = $_[0]; > my $targetFile = $_[1]; >- my $targetCodepage = $_[2]; >- print "iconv -f UTF-8 -t $targetCodepage $configDir/$srcFile > $configDir/$targetFile\n"; >- system("iconv -f UTF-8 -t $targetCodepage $configDir/$srcFile > $configDir/$targetFile") && >+ my $targetCodepage = "UTF-16LE"; >+ my $prependBOM = "cat $topsrcdir/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin -"; >+ print "iconv -f UTF-8 -t $targetCodepage $configDir/$srcFile | $prependBOM > $configDir/$targetFile\n"; >+ system("iconv -f UTF-8 -t $targetCodepage $configDir/$srcFile | $prependBOM > $configDir/$targetFile") && nit: might as well just use UTF-16LE directly instead of $targetCodepage

In common.nsh the install.log should be written as UTF-16LE. To do this replace all instances of FileWrite $fhInstallLog with FileWriteUTF16LE $fhInstallLog and add the following to the StartInstallLog macro > FileOpen $fhInstallLog "$INSTDIR\install.log" w >+ FileWriteWord $fhInstallLog "65279" >

Here is the updated patch with nits from comment 68, and also the file write encoding mentioned in comment 69. Correct me if I'm wrong, but I think we need FileWriteUTF16LE instead of FileWrite everywhere, since the strings are all Unicode.

(In reply to comment #70) > Created an attachment (id=352614) [details] > Patch (v1.3) > > Here is the updated patch with nits from comment 68, and also the file write > encoding mentioned in comment 69. > > Correct me if I'm wrong, but I think we need FileWriteUTF16LE instead of > FileWrite everywhere, since the strings are all Unicode. The only other file I am currently concerned about is the uninstall.log and we likely could get away with ANSI in it but I currently would prefer UTF-16LE. If it is encoded in UTF-16LE then it will need to be converted during install and software update. If not, then I am slightly concerned with paths to shortcuts containing multiple charsets. Still thinking on this one...

Any updates?

As the patch stands the uninstall.log ends up in a bad state. I am currently going over the options available along with the behavior of the TextCompare macro and should have a plan on how to address this in the next day.

After testing and going through what it will take to make the uninstall.log UTF-16LE I think we should leave it as ANSI for now so we can get this into the tree safely for 3.1. I'm going over the patch and should have the review finished tonight so no need to resubmit yet.

I was finally able to get a working UTF-16LE uninstall.log file. I'll file a bug depending on this and submit a patch for that part.

Comment on attachment 352614 [details] [diff] [review] Patch (v1.3) Go ahead and leave all of the writes to the uninstall.log as they were and I'll take care of that part in bug 470182. I'll most likely put the patches for Seamonkey and Thunderbird in that bug and update Sunbird in a new bug due to the number of changes it needs. >diff --git a/browser/installer/windows/Makefile.in b/browser/installer/windows/Makefile.in >--- a/browser/installer/windows/Makefile.in >+++ b/browser/installer/windows/Makefile.in >... >diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh >--- a/toolkit/mozapps/installer/windows/nsis/common.nsh >+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh >... >@@ -1376,7 +1456,7 @@ > * i - int (includes char, byte, short, handles, pointers and so on) > * t - text, string (LPCSTR, pointer to first character) > * * - pointer specifier -> the proc needs the pointer to type, affects next > * char (parameter) [ex: '*i' - pointer to int] nit: please update the comments above to reflect the change > * see the NSIS documentation for additional information. > */ >-!define RegCreateKey "Advapi32::RegCreateKeyA(i, t, *i) i" >+!define RegCreateKey "Advapi32::RegCreateKeyW(i, w, *i) i" > >@@ -2504,7 +2584,7 @@ > StrCmp $R6 "\" +1 +2 > StrCpy $R9 "$R8" -1 > >- System::Call 'kernel32::GetLongPathNameA(t r18, t .r17, i 1024)i .r16' >+ System::Call 'kernel32::GetLongPathNameW(w r18, w .r17, i 1024)i .r16' > StrCmp "$R7" "" +4 +1 ; Empty string when GetLongPathNameA is not present. nit: please change GetLongPathNameA to GetLongPathNameW in the comments. >@@ -5471,7 +5552,7 @@ > * Adds a section divider to the human readable log. > */ > !macro WriteLogSeparator >- FileWrite $fhInstallLog "$\r$\n----------------------------------------\ >+ FileWriteUTF16LE $fhInstallLog "$\r$\n----------------------------------------\ > ---------------------------------------$\r$\n" nit: please line up the indentation as follows >- FileWrite $fhInstallLog "$\r$\n----------------------------------------\ >+ FileWriteUTF16LE $fhInstallLog "$\r$\n----------------------------------------\ >- ---------------------------------------$\r$\n" >+ ---------------------------------------$\r$\n" >diff --git a/toolkit/mozapps/installer/windows/nsis/makensis.mk b/toolkit/mozapps/installer/windows/nsis/makensis.mk >--- a/toolkit/mozapps/installer/windows/nsis/makensis.mk >+++ b/toolkit/mozapps/installer/windows/nsis/makensis.mk >... >@@ -44,24 +45,32 @@ ABS_CONFIG_DIR := $(shell pwd)/$(CONFIG_ >... >+TOOLKIT_NSIS_FILES = \ >+ AppAssocReg.dll \ >+ nsProcess.dll \ Please delete nsProcess.dll and remove it here >diff --git a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl >--- a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl >+++ b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl >... >@@ -135,14 +131,14 @@ while( $line = <infile> ) { > $value =~ s/(")/\$\\$1/g; # prefix " with $\ > $value =~ s/(\\n)/\\r$1/g; # insert \\r before each occurence of \\n > $value =~ s/(\\r)\\r/$1/g; # replace all ocurrences of \\r\\r with \\r nit: if you don't mind please fix the comments on the above two lines as well and any others that may exist r=me but please resubmit an updated patch. The source files for the plugins will also need to be checked into other-licenses/nsis/. I think it would be best to put them in a sub-directory named Contrib since NSIS does this as well. I will also need to review those changes as well. Thanks again

(In reply to comment #76) > >diff --git a/toolkit/mozapps/installer/windows/nsis/makensis.mk b/toolkit/mozapps/installer/windows/nsis/makensis.mk > >--- a/toolkit/mozapps/installer/windows/nsis/makensis.mk > >+++ b/toolkit/mozapps/installer/windows/nsis/makensis.mk > >... > >@@ -44,24 +45,32 @@ ABS_CONFIG_DIR := $(shell pwd)/$(CONFIG_ > >... > >+TOOLKIT_NSIS_FILES = \ > >+ AppAssocReg.dll \ > >+ nsProcess.dll \ > Please delete nsProcess.dll and remove it here On second thought let's keep this around for now including adding the source to the tree

Here is the updated patch, with the plugin sources added to other-licenses/nsis.

Just and update... I'm going to finish up the patch for bug 470182 before reviewing the NSIS plugin code.

Comment on attachment 353725 [details] [diff] [review] Patch (v1.4) Hey Ehsan, (In reply to comment #76) > (From update of attachment 352614 [details] [diff] [review]) > Go ahead and leave all of the writes to the uninstall.log as they were and I'll > take care of that part in bug 470182. I'll most likely put the patches for > Seamonkey and Thunderbird in that bug and update Sunbird in a new bug due to > the number of changes it needs. Please remove the changes where the uninstall.log is written to as UTF-16LE

(In reply to comment #80) > Please remove the changes where the uninstall.log is written to as UTF-16LE Hopefully I caught all of them.

Ehsan, Just letting you know I haven't forgotten about this... the patch for bug 470182 is a tad more involved than I first thought and I'm going to finish it up before reviewing the latest patch. I'm also going to make a diff between the original plugin source to the modified plugin source in order to limit the review to your code changes.

(In reply to comment #82) > Just letting you know I haven't forgotten about this... the patch for bug > 470182 is a tad more involved than I first thought and I'm going to finish it > up before reviewing the latest patch. I'm also going to make a diff between the > original plugin source to the modified plugin source in order to limit the > review to your code changes. Great, thanks! BTW, since you have managed the other related bugs in order to prepare everything in all products, and you know the correct order of the things to land in order to minimize breakage across different products, feel free to land the patch for me if it gets r+. :-)

Any updates? The string freeze is fast approaching...

I got pulled into a couple of other bugs and haven't had time to finish this. There aren't any strings that need to be changed though so the string freeze shouldn't affect this.

This is really close and I am requesting blocking so we can get this for Beta 3

Not blocking per beltzner, but auto-approved for b3 once the patch is finished/reviewed.

yeah, a191=beltzner a priori!

To comment on bug 399153 comment 21: Please remove charset.mk on the 1.9.1 branch, too? It's nothing that's going to touch our l10n release systems, and it's going to make life easier for localizations coming in from scratch to 3.1. Thanks.

Will do Axel, I'll likely end up being the one checking this in due to the other bugs that need to have their patches checked in at the same time.

(In reply to comment #90) > Will do Axel, I'll likely end up being the one checking this in due to the > other bugs that need to have their patches checked in at the same time. Yes, please do that. I appreciate your help.

I want to split out the patch in attachment #354312 [details] [diff] [review] for easier review. I changed / fixed a couple of the comments and fixed the incorrect indenting due to my mistaken nit in the locales Makefile.in and makensis.mk.

One major problem which I just realized concerns about the InstallOptions module. All WriteINIStr calls create ANSI ini files, which are no good. This causes the installer custom pages to be forced to use ANSI strings. The results will be less than perfect for Persian (Persian characters get mapped to Arabic Windows charset; some get dropped, some replaced), but may be totally unacceptable to other languages. Can you think of a better solution? I tried calling WritePrivateProfileStringW to no avail. Another option which I can think of would be to create DLLs containing the custom dialog templates, instead of using ini files. One way to do this would be to create resource scripts on the fly during the build process, and then compiling them to create a DLL containing the dialog templates, and then using that DLL inside the installer scripts. Can you think of an easier and better solution?

Hey Ehsan, could you try removing the following from installer.nsi !system 'echo ; > options.ini' !system 'echo ; > shortcuts.ini' !system 'echo ; > summary.ini' and ReserveFile options.ini ReserveFile shortcuts.ini ReserveFile summary.ini and !insertmacro MUI_INSTALLOPTIONS_EXTRACT "options.ini" !insertmacro MUI_INSTALLOPTIONS_EXTRACT "shortcuts.ini" !insertmacro MUI_INSTALLOPTIONS_EXTRACT "summary.ini" and test it out? I suspect it is due to the call to WriteINIStr finding the existing file without a BOM and writing the strings as ANSI. If that doesn't work (which would be surprising to me) I suspect that we could use the bom file to create these files. This code was initially added due to problems I had when first creating the installer and I believe that the problem I encountered has since been fixed.

Nah, that didn't work. I didn't try creating those files with the bom file, because I didn't know where the files should be created? Can you please test that? I'm gonna get some sleep right now, so let me know if you need me to do something, and I'll take care of it tomorrow morning. BTW, why don't we create the INI files inside the build scripts, and do it using WriteINIStr? MUI's ioSpecial.ini is created beforehand, and it works a treat.

Strange that WriteINIStr doesn't do the right thing here as it does elsewhere. We can create those files any other way... I like to keep the file creation in the installer script so less files are touched to create new ones or remove ones that are removed. I suspect just using the bom file will fix this and will check it out.

Looks like the method I suggested works.

(In reply to comment #95) >... > BTW, why don't we create the INI files inside the build scripts, and do it > using WriteINIStr? MUI's ioSpecial.ini is created beforehand, and it works a > treat. I suspect you meant in the NSIS scripts? This also works and is cleaner IMO FileOpen $0 "$PLUGINSDIR\options.ini" w FileWriteWord $0 "65279" FileClose $0

One thing that is rather strange is that when writing to the shortcuts log using WriteINIStr it was created as UTF-8... just to be safe I am going to use this same method when it is created.

WriteINIStr uses WritePrivateProfileString() in kernel32.lib. If you look at the documentation for this function it states: "If the file was created using Unicode characters, the function writes Unicode characters to the file. Otherwise, the function writes ANSI characters." So the suggestion above should work.

The screenshot that you attached was the Arabic locale, which has its own code page, and it's not possible to tell if the problem has been solved in that locale or not. Can you attach your patch so that I could apply it and test it with fa?

OK, here is a patch which I tested and works great. This should be applied on top of my other patch in this bug. I used the method that you suggested.

Comment on attachment 356301 [details] [diff] [review] Unicode .ini files patch >diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi >--- a/browser/installer/windows/nsis/installer.nsi >+++ b/browser/installer/windows/nsis/installer.nsi >@@ -757,19 +757,19 @@ FunctionEnd > # Initialization Functions Please remove the echo and ReserveFile statements. > Function .onInit > StrCpy $LANGUAGE 0 > ${SetBrandNameVars} "$EXEDIR\localized\distribution\setup.ini" > > ${InstallOnInitCommon} "$(WARN_MIN_SUPPORTED_OS_MSG)" > >- !insertmacro MUI_INSTALLOPTIONS_EXTRACT "options.ini" >- !insertmacro MUI_INSTALLOPTIONS_EXTRACT "shortcuts.ini" >- !insertmacro MUI_INSTALLOPTIONS_EXTRACT "summary.ini" >+ !insertmacro InitInstallOptionsFile "options.ini" >+ !insertmacro InitInstallOptionsFile "shortcuts.ini" >+ !insertmacro InitInstallOptionsFile "summary.ini" > > ; Setup the options.ini file for the Custom Options Page > WriteINIStr "$PLUGINSDIR\options.ini" "Settings" NumFields "6" > > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Text "$(OPTIONS_SUMMARY)" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Right "-1" >diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh >--- a/toolkit/mozapps/installer/windows/nsis/common.nsh >+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh >@@ -399,16 +399,18 @@ > > ################################################################################ > # Macros for creating Install Options ini files > # > # DEPRECATED - all ini creation code should be added to the application's > # installer and uninstaller files. > > !macro createComponentsINI >+ !insertmacro InitInstallOptionsFile "components.ini" >+ > WriteINIStr "$PLUGINSDIR\components.ini" "Settings" NumFields "5" > > WriteINIStr "$PLUGINSDIR\components.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 1" Text "$(OPTIONAL_COMPONENTS_LABEL)" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 1" Right "-1" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 1" Top "5" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 1" Bottom "15" >@@ -430,16 +432,18 @@ > WriteINIStr "$PLUGINSDIR\components.ini" "Field 3" Left "30" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 3" Right "-1" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 3" Top "32" > WriteINIStr "$PLUGINSDIR\components.ini" "Field 3" Bottom "52" > ${EndIf} > !macroend > > !macro createShortcutsINI >+ !insertmacro InitInstallOptionsFile "shortcuts.ini" >+ > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Settings" NumFields "4" > > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 1" Text "$(CREATE_ICONS_DESC)" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 1" Right "-1" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 1" Top "5" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 1" Bottom "15" >@@ -466,16 +470,18 @@ > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 4" Left "15" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 4" Right "-1" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 4" Top "60" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 4" Bottom "70" > WriteINIStr "$PLUGINSDIR\shortcuts.ini" "Field 4" State "1" > !macroend > > !macro createBasicCustomOptionsINI >+ !insertmacro InitInstallOptionsFile "options.ini" >+ > WriteINIStr "$PLUGINSDIR\options.ini" "Settings" NumFields "5" > > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Text "$(OPTIONS_SUMMARY)" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Right "-1" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Top "0" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Bottom "10" >@@ -565,16 +571,18 @@ > WriteINIStr "$PLUGINSDIR\options.ini" "Field 7" Text "$(OPTION_CUSTOM_DESC)" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 7" Left "30" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 7" Right "-1" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 7" Top "97" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 7" Bottom "117" > !macroend > > !macro createBasicCustomSetAsDefaultOptionsINI >+ !insertmacro InitInstallOptionsFile "options.ini" >+ > WriteINIStr "$PLUGINSDIR\options.ini" "Settings" NumFields "6" > > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Text "$(OPTIONS_SUMMARY)" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Right "-1" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Top "0" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 1" Bottom "10" >@@ -615,16 +623,18 @@ > WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Left "0" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Right "-1" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Top "124" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" Bottom "145" > WriteINIStr "$PLUGINSDIR\options.ini" "Field 6" State "1" > !macroend > > !macro createSummaryINI >+ !insertmacro InitInstallOptionsFile "summary.ini" >+ > WriteINIStr "$PLUGINSDIR\summary.ini" "Settings" NumFields "3" > > WriteINIStr "$PLUGINSDIR\summary.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\summary.ini" "Field 1" Text "$(SUMMARY_INSTALLED_TO)" > WriteINIStr "$PLUGINSDIR\summary.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\summary.ini" "Field 1" Right "-1" > WriteINIStr "$PLUGINSDIR\summary.ini" "Field 1" Top "5" > WriteINIStr "$PLUGINSDIR\summary.ini" "Field 1" Bottom "15" >@@ -676,16 +686,18 @@ > WriteINIStr "$PLUGINSDIR\summary.ini" "Field $0" Top "35" > WriteINIStr "$PLUGINSDIR\summary.ini" "Field $0" Bottom "45" > > WriteINIStr "$PLUGINSDIR\summary.ini" "Settings" NumFields "$0" > ${EndIf} > !macroend > > !macro un.createUnConfirmINI >+ !insertmacro InitInstallOptionsFile "unconfirm.ini" >+ > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Settings" NumFields "5" > > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Field 1" Type "label" > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Field 1" Text "$(UN_CONFIRM_UNINSTALLED_FROM)" > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Field 1" Left "0" > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Field 1" Right "-1" > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Field 1" Top "5" > WriteINIStr "$PLUGINSDIR\unconfirm.ini" "Field 1" Bottom "15" >@@ -4738,16 +4750,18 @@ > !insertmacro GetParameters > !insertmacro UnloadUAC > !insertmacro UpdateUninstallLog > > !verbose push > !verbose ${_MOZFUNC_VERBOSE} > !define UninstallOnInitCommon "!insertmacro UninstallOnInitCommonCall" > >+ !insertmacro InitInstallOptionsFile "unconfirm.ini" >+ > Function UninstallOnInitCommon > ; Prevents breaking Thunderbird All of these macros are deprecated so please don't change them... no other apps use them and I have a patch waiting on review to remove them. >@@ -5557,8 +5571,15 @@ > /** > * Adds a section divider to the human readable log. > */ > !macro WriteLogSeparator > FileWriteUTF16LE $fhInstallLog "$\r$\n----------------------------------------\ > ---------------------------------------$\r$\n" > !macroend > !define WriteLogSeparator "!insertmacro WriteLogSeparator" >+ >+!macro InitInstallOptionsFile FILE >+ FileOpen $0 "$PLUGINSDIR\${FILE}" w >+ FileWriteWord $0 "65279" >+ FileClose $0 >+ !insertmacro INSTALLOPTIONS_WRITE "\${FILE}" "Settings" "RTL" "$(^RTL)" In my testing this isn't necessary since it is auto added by the MUI code based on the languages nlf file. I'd like to see this again before r+ing it. Thanks

Also rename InitInstallOptionsFile to InitUTF16LEFile since I am going to want to use this for the shortcuts and we might as well use it for the install log as well

(In reply to comment #104) > Also rename InitInstallOptionsFile to InitUTF16LEFile since I am going to want > to use this for the shortcuts and we might as well use it for the install log > as well InitUTF16LEFile or something else that is generic and states exactly what it does

(In reply to comment #103) > Please remove the echo and ReserveFile statements. Done. > All of these macros are deprecated so please don't change them... no other apps > use them and I have a patch waiting on review to remove them. Well, UninstallOnInitCommon is actually being used, but anyway I moved the change there to uninstall.nsi. > In my testing this isn't necessary since it is auto added by the MUI code based > on the languages nlf file. The MUI code which does this is MUI_INSTALLOPTIONS_EXTRACT which we no longer use. I just tested it, and without that line, the subdialog containing the custom controls turns up LTR. So, actually this line *is* required. (In reply to comment #104) > Also rename InitInstallOptionsFile to InitUTF16LEFile since I am going to want > to use this for the shortcuts and we might as well use it for the install log > as well Done.

(In reply to comment #106) >... > The MUI code which does this is MUI_INSTALLOPTIONS_EXTRACT which we no longer > use. I just tested it, and without that line, the subdialog containing the > custom controls turns up LTR. So, actually this line *is* required. I was still using it in the hack I threw together so this doesn't surprise. Thanks for looking into it. Thanks for moving the call to uninstall.nsi... I prefer to leave it up to the app to decide whether to display that page or not. I'll review the patch over the weekend and it should land by no later than monday.

Comment on attachment 356302 [details] [diff] [review] Unicode .ini files patch (v2) Quick nit... the param to InitUTF16LEFile should be the full file path so it can be used by other callers such as the ones to create the install log file and the shortcuts log file.

(In reply to comment #108) > (From update of attachment 356302 [details] [diff] [review]) > Quick nit... the param to InitUTF16LEFile should be the full file path so it > can be used by other callers such as the ones to create the install log file > and the shortcuts log file. Done.

(In reply to comment #107) > I'll review the patch over the weekend and it should land by no later than > monday. Cool, thanks for all your help. I'll try to blog about this before then, and also post to the l10n mailing list in order to give the localizers a little heads-up.

Hey Ehsan, thanks for writing the blog about it. Could you add that this is also applicable to Thunderbird 3.0, SeaMonkey 2.0, and Sunbird 1.0 since trunk is a little vague?

Comment on attachment 356310 [details] [diff] [review] Unicode .ini files patch (v3) I finally have time to sit down and go over this properly... >diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh >--- a/toolkit/mozapps/installer/windows/nsis/common.nsh >+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh >@@ -5557,8 +5557,15 @@ > /** > * Adds a section divider to the human readable log. > */ > !macro WriteLogSeparator > FileWriteUTF16LE $fhInstallLog "$\r$\n----------------------------------------\ > ---------------------------------------$\r$\n" > !macroend > !define WriteLogSeparator "!insertmacro WriteLogSeparator" >+ >+!macro InitUTF16LEFile FILE >+ FileOpen $0 "${FILE}" w >+ FileWriteWord $0 "65279" >+ FileClose $0 >+ WriteIniStr "${FILE}" "Settings" "RTL" "$(^RTL)" >+!macroend Sorry about changing my mind on this but after looking at the creation of the install log it appears the only other caller of this would be for the shortcuts log. Since it can't have the call to >+ WriteIniStr "${FILE}" "Settings" "RTL" "$(^RTL)" I think it is cleaner to go with what you had originally of InitInstallOptionsFile. Please use _FILE for the param for consistency with the other macros and go back to just using the file name. Please add a comment above the macro explaining what it does along the lines of the following /** * Creates an InstallOptions file with a UTF-16LE BOM and adds the RTL value * to the Settings section. * * @param _PATH_TO_FILE * The full path to the file to be created. */

Also, put the new InitInstallOptionsFile macro in the "# User interface callback helper defines and macros" section of common.nsh

(In reply to comment #112) >... > Please add a comment above the macro explaining what it does along the lines of > the following > /** > * Creates an InstallOptions file with a UTF-16LE BOM and adds the RTL value > * to the Settings section. > * > * @param _PATH_TO_FILE > * The full path to the file to be created. > */ Should be something along the lines of > * @param _FILE > * The name of the file to be created in $PLUGINSDIR.

btw: did you test the uninstaller? It looks to me like unconfirm.ini should be created at the end of un.onInit

In uac.h I see you've changed -#if !defined(NTDDI_VISTA) || defined(BUILD_OLDSDK) +#if 0 Why? It compiles fine for me fine with the Vista SDK installed

I removed the extraneous UAC.sln and UAC.vcproj

I made it so it will compile with and without UNICODE defined

Not going to bother adding the Include directory with the nsProcess.nsh or the Example directory as well as the examples from the UAC Plugin since they aren't pertinent to compiling the plugin with the changes made.

(In reply to comment #111) > Hey Ehsan, thanks for writing the blog about it. Could you add that this is > also applicable to Thunderbird 3.0, SeaMonkey 2.0, and Sunbird 1.0 since trunk > is a little vague? Sure, done.

Fixed all the comments. Also, you were right about the uninstaller. I corrected it, and tested it to make sure that it works correctly.

(In reply to comment #116) > In uac.h I see you've changed > -#if !defined(NTDDI_VISTA) || defined(BUILD_OLDSDK) > +#if 0 > > Why? It compiles fine for me fine with the Vista SDK installed I don't have Vista SDK installed, but I have a rather odd combination of SDKs installed (including a couple of SDK's coming with VC, and another separate installation!). So I'm pretty confident that my setup is not typical, and quite frankly I'm not sure what its implications are, so if it compiles fine with the Vista SDK for you, then let it be. :-)

Comment on attachment 356393 [details] [diff] [review] Unicode .ini files patch (v4) Yay! Looks good and thanks!

I went with minimal changes to the files so we can update them easily if needed

I've removed setup-processes since it isn't used anymore and I'll remove the Processes.dll as well.

There were no changes to the ExDLL source

I've got all of the patches queued and I've built / tested all of the apps using all of the patches... everything looks good. With the tree being closed I suspect the earliest I will push the changes is Monday evening Pacific time.

Thanks Rob. Henrik will be verifying there are no broken artifacts to installer and updater on windows for en-us.

Pushed everything but the Plugin source (will do that later) to mozilla-central http://hg.mozilla.org/mozilla-central/rev/efc8e32bfe59 I'm going to wait for one cycle of green before marking fixed and landing on mozilla-1.9.1

(In reply to comment #134) > Pushed everything but the Plugin source (will do that later) to mozilla-central > http://hg.mozilla.org/mozilla-central/rev/efc8e32bfe59 btw: the nsProcess.dll changes caused the plugin to crash so I fixed it before checking in... in the rush to get it working and verify all of the plugins I checked the changes to the nsis scripts under bug 470182... sorry about that.

Pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d1520b4e2aa6 I forgot to pick up the change to nsProcess.dll and pushed it immediately after the previous push http://hg.mozilla.org/releases/mozilla-1.9.1/rev/54becfa2f7b1 Still going to leave this open until I test an hourly installer.

I tested with an hourly trunk Shredder build including going through the steps to test the plugins and everything so far looks good so resolving -> fixed.

Also tested with an hourly Minefield build... everything looks good. Thanks Ehsan!

Eduardo raised an interesting question in bug 457797, whether we want this for 3.0.x even. Given that we're going to see fx 3.0.x builds for a bit, it makes me wonder.

I stop wondering and wandering, we'll not backport this.