Closed Bug 752756 Opened 9 years ago Closed 9 years ago

Extend nsINIParser/nsIINIParser/nsIINIParserWriter to read/write UTF-16LE INI files with BOM


(Core :: XPCOM, defect)

Windows 7
Not set





(Reporter: TimAbraldes, Assigned: TimAbraldes)




(1 file, 5 obsolete files)

UTF-16LE is the preferred flavor of Unicode file encoding on Windows.  Other tools like NSIS cannot read/write Unicode INI files unless they are UTF-16LE with a BOM.  This causes problems for us when we attempt to natively install/uninstall webapps (see bug 751177).

At least on Windows, we should support reading/writing INI files that are encoded as UTF-16LE.  Additionally, we should probably support BOMs for UTF-8 and for UTF-16LE.
I've tested this manually, but it seems likely that automated tests are desired for this patch.  I'll look into writing those now.

A couple of things are worth noting:
  Although nsIINIParserWriter can now write INI files in any unicode encoding that nsIConverterOutputStream supports, nsINIParser can only read UTF-8 and, on Windows, UTF-16LE encoded INI files.
  nsINIParser is used from nsBrowserApp and from the webapprt stub, which means that it cannot use XPCOM because XPCOM might not have been initialized by the time nsINIParser is used.

Benjamin (or whoever Benjamin delegates to) - how do you feel about this approach?
Assignee: nobody → tabraldes
Attachment #624473 - Flags: feedback?(benjamin)
Comment on attachment 624473 [details] [diff] [review]
Patch v1 - Parse UTF-16LE BOM in nsINIParser on Windows. Add charset parameter to nsIINIParserWrite::writeFile()

I don't think there's any reason to support arbitrary encodings in the writer, just UTF8 and UTF16. And document that UTF16 is one of those windows-only things that shouldn't be used in cross-platform code.
Implemented feedback; this patch allows only UTF-16LE and UTF-8 INI files to be written by nsIINIParserWriter::writeFile().  Also, the previous patch was broken due to a finish()/close() mismatch.  Files now get written correctly.

Still to do:
  Track down memory leak caused by this patch
  Add unit tests
  Do we need a class that implements both nsIConverterOutputStream and nsISafeOutputStream?
Attachment #624473 - Attachment is obsolete: true
Attachment #624473 - Flags: feedback?(benjamin)
In terms of the API, I don't like that you are accepting an arbitrary charset string (especially undocumented!). I think it would be better to just use a flag, i.e.

interface nsIINIParserWriter ... {
   * Windows and the NSIS installer code sometimes expect INI files to be in
   * UTF-16 encoding. On Windows only, this flag to writeFile can be used to
   * change the encoding from its default UTF-8.
  const unsigned long WRITE_UTF16 = 0x1;

  void writeFile([optional] in nsILocalFile aINIFile,
                 [optional] in unsigned long aFlags);

And then in the impl, instead of getting a converter each time, just have __utf8Converter and __utf16Converter.
Implemented bsmedberg's suggestion (`writeFile` takes a flag specifying whether to write UTF-16LE or not, rather than taking an arbitrary string).

Added unit tests.

Benjamin: Please let me know what you think! (or delegate)
Attachment #624622 - Attachment is obsolete: true
Attachment #625830 - Flags: review?(benjamin)
Fixing bustage from previous patch.  I looked through mxr and couldn't find any references to `nsINIParser::writeFile()` other than the one causing the Linux and Android bustage.  If there are more they'll get flushed out in this try run:
Attachment #625830 - Attachment is obsolete: true
Attachment #625830 - Flags: review?(benjamin)
Attachment #625909 - Flags: review?(benjamin)
Comment on attachment 625909 [details] [diff] [review]
Patch v4 - Hopefully fix Linux and Android bustage

It would probably be good to enforce that we only write UTF16 on Windows, either with a build-time ifdef or a runtime `'nsIWindowsRegKey' in Components.interfaces` check
Attachment #625909 - Flags: review?(benjamin) → review+
Carrying forward r+

Pushed to try along with the patch for bug 751177:

jsmith will do a quick verification that this patch works and doesn't break INI processing:
  1) Install Ed's app on Windows and Mac
  2) Verify that Ed's app launches on Windows and Mac
  3) Uninstall Ed's app

If all goes well I'll land this tonight
Attachment #625909 - Attachment is obsolete: true
Attachment #626132 - Flags: review+
Attachment #626132 - Attachment is obsolete: true
Attachment #626346 - Flags: review+
Target Milestone: --- → mozilla15
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.