541594 - extend nsIINIParser to allow writing INI files

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

nsIINIParser can currently read, but not write, INI files.

I wasn't sure about the memory stuff here (malloc/free), is this correct or do I need to pull in the PR_ stuff? That's usually the case, but other stuff here seems to be using the plain versions, and it *seems* to work OK.

Also will add some tests for this.

Comment 1

[Benjamin Smedberg]

Comment on attachment 423114 [details] [diff] [review]
Patch v.1

I think the giant realloc buffer here is not the right solution. I'd be much happier with the following changes:

* Make mSections use a nsCStringHashKey so that the section key isn't dependent on the read buffer
* Make the INIValue destructor virtual
* Add an AllocatedINIValue subclass which has allocation space for its key/value.

In addition, I don't think we should modify nsIINIParser at this point: just create a derived nsIINIWriter interface so we can use the same patch on trunk and branch.

Comment 2

[Benjamin Smedberg]

Want tests, too, and I'm interested to know how you intend to handle section/key/values with embedded '\n' or '\0', keys with embedded '=', or sections with embedded ']'. I think we should probably just reject them up-front with NS_ERROR_ILLEGAL_VALUE, since there isn't an escaping mechanism for INI files.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Switched to using a simpler, all-JS implementation.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Doh, forgot to copy the most recent patch.

Comment 5

[Benjamin Smedberg]

Attachment: Patch on top to avoid duplicating parsing logic

There are some parsing differences here, some of which are minor. Your tests also managed to find two bugs in nsINIParser.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 424266 [details] [diff] [review]
Patch on top to avoid duplicating parsing logic

One change that got lost from the JS version is allowing the creation of a parser for an INI file that exists but is 0 bytes long.

The C++ parser treats this as an error, it should just treat it as a valid input that's empty (same as if it was a file with just whitespace in it).

Similarly for the case of a non-existent file, though I suppose callers could be required to take care of that.

Comment 7

[Benjamin Smedberg]

No, that's preserved, see the 0 == this._iniFile.fileSize check in _readFile.

Comment 8

[Benjamin Smedberg]

Comment on attachment 424184 [details] [diff] [review]
Patch v.3

r=me with my changes on top

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Merged with bsmedberg's changes.

Comment 10

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/a435d40af2da

Comment 11

[Justin Dolske [:Dolske]]

Also pushed http://hg.mozilla.org/mozilla-central/rev/4ba8ccb0cadc

(add nsINIProcessor.js to the package manifest, *sigh*)

Comment 12

[Mark Banner (:standard8)]

We were seeing hangs in the mailnews/ tests. We tracked it down to the nsCRTGlue.cpp change. From irc:

NeilAway: yeah, that change is bogus, it loses the last token if it isn't delimited
Standard9: so http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/tests/unit/data/iniparser07.ini is one of the files
Standard9: from the patch it implies he thinks it shouldn't return as having stuff in the ini file
Standard9: [section1]
Standard9: name1
NeilAway: should use strchr for that, not strtok
• NeilAway notes that the strtok code was sort of wrong before, but in a different way, since it wrote NULL in an edge case whereas all the other versions use the end of the string
NeilAway: unless that's by design, and he really meant to use if (!e || !token)
NeilAway: (in the name1 case, token would be null; in the name1= case token would be key+6)

So we went for the safer fix of backing out the NS_strtok change and changing the if statement:

http://hg.mozilla.org/mozilla-central/rev/75beeb8eed5e

Hopefully this will fix the tests.

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-SM) Update SeaMonkey packaging

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a-SM) Update SeaMonkey packaging [Checkin: Comment 17]

Dv1-SM, correctly sorted.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-SM-191) Support SeaMonkey downgrading [Checkin: Comment 18]

Looking for "approval-seamonkey2.0.4=?" actually.

Comment 16

[Robert Kaiser]

(In reply to comment #15)
> Created an attachment (id=426269) [details]
> (Ev1-SM-191) Support SeaMonkey downgrading
> 
> Looking for "approval-seamonkey2.0.4=?" actually.

Right, take it as that.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 426267 [details] [diff] [review]
(Dv1a-SM) Update SeaMonkey packaging
[Checkin: Comment 17]


http://hg.mozilla.org/comm-central/rev/b11105b4c247

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 426269 [details] [diff] [review]
(Ev1-SM-191) Support SeaMonkey downgrading
[Checkin: Comment 18]


http://hg.mozilla.org/releases/comm-1.9.1/rev/c4df47c1ec03

Comment 19

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/87645240ad71

Comment 20

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/6eae1706dab5
http://hg.mozilla.org/projects/firefox-lorentz/rev/768babe3eb40

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 22

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Comment 23

[Eric Shepherd [:sheppy]]

I've documented nsIINIParserWriter: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIINIParserWriter

I've also updated Firefox 4 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/4

I've also updated Firefox 15 for developers to indicate that writeFile was extended to support the flags parameter: https://developer.mozilla.org/en-US/Firefox/Releases/15

I believe that covers this. If not, please re-open or file a new doc request as needed. Thanks for calling this to our attention.

Comment 24

[Wladimir Palant]

It's rather non-obvious but Patch v.4 introduced XPCOMUtils.IterSimpleEnumerator() and XPCOMUtils.IterStringEnumerator() helpers: http://hg.mozilla.org/mozilla-central/file/d7c76fe69e9a/js/xpconnect/loader/XPCOMUtils.jsm#l260

Usage example for XPCOMUtils.IterSimpleEnumerator():

> var e = XPCOMUtils.IterSimpleEnumerator(dir.directoryEntries, Components.interfaces.nsIFile);
> for (var file in e)
>   console.log(file.path);

Usage example for XPCOMUtils.IterStringEnumerator():

> var e = XPCOMUtils.IterStringEnumerator(iniParser.getSections());
> for (var section in e)
>   console.log(section);

Comment 25

[Wladimir Palant]

Added the two helpers above to https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/XPCOMUtils.jsm and https://developer.mozilla.org/en-US/Firefox/Releases/4#Miscellaneous_changes_to_code_modules. I think that should be it now.