extend nsIINIParser to allow writing INI files

RESOLVED FIXED in mozilla1.9.3a2



8 years ago
3 years ago


(Reporter: Dolske, Assigned: Dolske)


({dev-doc-complete, fixed-seamonkey2.0.4})

dev-doc-complete, fixed-seamonkey2.0.4
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 .4-fixed)


(Whiteboard: [fixed-lorentz])


(3 attachments, 5 obsolete attachments)



8 years ago
Created attachment 423114 [details] [diff] [review]
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.
Attachment #423114 - Flags: review?(benjamin)
Assignee: nobody → dolske

Comment 1

8 years ago
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.
Attachment #423114 - Flags: review?(benjamin) → review-

Comment 2

8 years ago
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.
Flags: in-testsuite?

Comment 3

8 years ago
Created attachment 424183 [details] [diff] [review]
Patch v.2

Switched to using a simpler, all-JS implementation.
Attachment #423114 - Attachment is obsolete: true
Attachment #424183 - Flags: review?(benjamin)

Comment 4

8 years ago
Created attachment 424184 [details] [diff] [review]
Patch v.3

Doh, forgot to copy the most recent patch.
Attachment #424183 - Attachment is obsolete: true
Attachment #424184 - Flags: review?(benjamin)
Attachment #424183 - Flags: review?(benjamin)

Comment 5

8 years ago
Created attachment 424266 [details] [diff] [review]
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.
Attachment #424266 - Flags: review?(dolske)

Comment 6

8 years ago
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.
Attachment #424266 - Flags: review?(dolske) → review+

Comment 7

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

Comment 8

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

r=me with my changes on top
Attachment #424184 - Flags: review?(benjamin) → review+

Comment 9

8 years ago
Created attachment 424934 [details] [diff] [review]
Patch v.4

Merged with bsmedberg's changes.
Attachment #424184 - Attachment is obsolete: true
Attachment #424266 - Attachment is obsolete: true

Comment 10

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/a435d40af2da
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2


8 years ago
Flags: in-testsuite? → in-testsuite+

Comment 11

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

(add nsINIProcessor.js to the package manifest, *sigh*)
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:


Hopefully this will fix the tests.
Created attachment 426265 [details] [diff] [review]
(Dv1-SM) Update SeaMonkey packaging
Attachment #426265 - Flags: review?(bugspam.Callek)
Created attachment 426267 [details] [diff] [review]
(Dv1a-SM) Update SeaMonkey packaging
[Checkin: Comment 17]

Dv1-SM, correctly sorted.
Attachment #426265 - Attachment is obsolete: true
Attachment #426267 - Flags: review?(bugspam.Callek)
Attachment #426265 - Flags: review?(bugspam.Callek)
Created attachment 426269 [details] [diff] [review]
(Ev1-SM-191) Support SeaMonkey downgrading
[Checkin: Comment 18]

Looking for "approval-seamonkey2.0.4=?" actually.
Attachment #426269 - Flags: review?(kairo)


8 years ago
Attachment #426269 - Flags: review?(kairo) → review+
Attachment #426267 - Flags: review?(bugspam.Callek) → review+

Comment 16

8 years ago
(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 on attachment 426267 [details] [diff] [review]
(Dv1a-SM) Update SeaMonkey packaging
[Checkin: Comment 17]

Attachment #426267 - Attachment description: (Dv1a-SM) Update SeaMonkey packaging → (Dv1a-SM) Update SeaMonkey packaging [Checkin: Comment 17]
Comment on attachment 426269 [details] [diff] [review]
(Ev1-SM-191) Support SeaMonkey downgrading
[Checkin: Comment 18]

Attachment #426269 - Attachment description: (Ev1-SM-191) Support SeaMonkey downgrading → (Ev1-SM-191) Support SeaMonkey downgrading [Checkin: Comment 18]
Keywords: fixed-seamonkey2.0.4
Depends on: 548692

Comment 19

8 years ago
Whiteboard: [fixed-lorentz]

Comment 20

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

Comment 22

8 years ago
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
status1.9.2: --- → .4-fixed


3 years ago
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 24

3 years ago
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);
Keywords: dev-doc-complete → dev-doc-needed

Comment 25

3 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.