Closed Bug 541594 Opened 11 years ago Closed 11 years ago

extend nsIINIParser to allow writing INI files

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: dev-doc-complete, fixed-seamonkey2.0.4, Whiteboard: [fixed-lorentz])

Attachments

(3 files, 5 obsolete files)

Attached patch Patch v.1 (obsolete) — Splinter Review
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 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-
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?
Attached patch Patch v.2 (obsolete) — Splinter Review
Switched to using a simpler, all-JS implementation.
Attachment #423114 - Attachment is obsolete: true
Attachment #424183 - Flags: review?(benjamin)
Attached patch Patch v.3 (obsolete) — Splinter Review
Doh, forgot to copy the most recent patch.
Attachment #424183 - Attachment is obsolete: true
Attachment #424184 - Flags: review?(benjamin)
Attachment #424183 - Flags: review?(benjamin)
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 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+
No, that's preserved, see the 0 == this._iniFile.fileSize check in _readFile.
Comment on attachment 424184 [details] [diff] [review]
Patch v.3

r=me with my changes on top
Attachment #424184 - Flags: review?(benjamin) → review+
Attached patch Patch v.4Splinter Review
Merged with bsmedberg's changes.
Attachment #424184 - Attachment is obsolete: true
Attachment #424266 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/a435d40af2da
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
Flags: in-testsuite? → in-testsuite+
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:

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

Hopefully this will fix the tests.
Attachment #426265 - Flags: review?(bugspam.Callek)
Dv1-SM, correctly sorted.
Attachment #426265 - Attachment is obsolete: true
Attachment #426267 - Flags: review?(bugspam.Callek)
Attachment #426265 - Flags: review?(bugspam.Callek)
Looking for "approval-seamonkey2.0.4=?" actually.
Attachment #426269 - Flags: review?(kairo)
Attachment #426269 - Flags: review?(kairo) → review+
Attachment #426267 - Flags: review?(bugspam.Callek) → review+
(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]


http://hg.mozilla.org/comm-central/rev/b11105b4c247
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]


http://hg.mozilla.org/releases/comm-1.9.1/rev/c4df47c1ec03
Attachment #426269 - Attachment description: (Ev1-SM-191) Support SeaMonkey downgrading → (Ev1-SM-191) Support SeaMonkey downgrading [Checkin: Comment 18]
Depends on: 548692
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
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.
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);
You need to log in before you can comment on or make changes to this bug.