Last Comment Bug 541594 - extend nsIINIParser to allow writing INI files
: extend nsIINIParser to allow writing INI files
Status: RESOLVED FIXED
[fixed-lorentz]
: dev-doc-complete, fixed-seamonkey2.0.4
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a2
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on: 548692
Blocks: 540532
  Show dependency treegraph
 
Reported: 2010-01-22 18:23 PST by Justin Dolske [:Dolske]
Modified: 2014-12-09 15:47 PST (History)
8 users (show)
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4-fixed


Attachments
Patch v.1 (9.36 KB, patch)
2010-01-22 18:23 PST, Justin Dolske [:Dolske]
benjamin: review-
Details | Diff | Splinter Review
Patch v.2 (21.02 KB, patch)
2010-01-28 20:37 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.3 (21.02 KB, patch)
2010-01-28 20:39 PST, Justin Dolske [:Dolske]
benjamin: review+
Details | Diff | Splinter Review
Patch on top to avoid duplicating parsing logic (15.18 KB, patch)
2010-01-29 09:28 PST, Benjamin Smedberg [:bsmedberg]
dolske: review+
Details | Diff | Splinter Review
Patch v.4 (24.78 KB, patch)
2010-02-02 22:32 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
(Dv1-SM) Update SeaMonkey packaging (1.72 KB, patch)
2010-02-10 09:47 PST, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Dv1a-SM) Update SeaMonkey packaging [Checkin: Comment 17] (1.74 KB, patch)
2010-02-10 09:51 PST, Serge Gautherie (:sgautherie)
bugspam.Callek: review+
Details | Diff | Splinter Review
(Ev1-SM-191) Support SeaMonkey downgrading [Checkin: Comment 18] (947 bytes, patch)
2010-02-10 09:55 PST, Serge Gautherie (:sgautherie)
kairo: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2010-01-22 18:23:02 PST
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.
Comment 1 Benjamin Smedberg [:bsmedberg] 2010-01-28 07:45:05 PST
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 [:bsmedberg] 2010-01-28 07:48:33 PST
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] 2010-01-28 20:37:35 PST
Created attachment 424183 [details] [diff] [review]
Patch v.2

Switched to using a simpler, all-JS implementation.
Comment 4 Justin Dolske [:Dolske] 2010-01-28 20:39:15 PST
Created attachment 424184 [details] [diff] [review]
Patch v.3

Doh, forgot to copy the most recent patch.
Comment 5 Benjamin Smedberg [:bsmedberg] 2010-01-29 09:28:13 PST
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.
Comment 6 Justin Dolske [:Dolske] 2010-02-01 15:40:16 PST
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 [:bsmedberg] 2010-02-02 14:52:22 PST
No, that's preserved, see the 0 == this._iniFile.fileSize check in _readFile.
Comment 8 Benjamin Smedberg [:bsmedberg] 2010-02-02 14:53:03 PST
Comment on attachment 424184 [details] [diff] [review]
Patch v.3

r=me with my changes on top
Comment 9 Justin Dolske [:Dolske] 2010-02-02 22:32:34 PST
Created attachment 424934 [details] [diff] [review]
Patch v.4

Merged with bsmedberg's changes.
Comment 10 Justin Dolske [:Dolske] 2010-02-09 17:14:42 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/a435d40af2da
Comment 11 Justin Dolske [:Dolske] 2010-02-10 02:03:49 PST
Also pushed http://hg.mozilla.org/mozilla-central/rev/4ba8ccb0cadc

(add nsINIProcessor.js to the package manifest, *sigh*)
Comment 12 Mark Banner (:standard8) 2010-02-10 03:59:42 PST
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) 2010-02-10 09:47:10 PST
Created attachment 426265 [details] [diff] [review]
(Dv1-SM) Update SeaMonkey packaging
Comment 14 Serge Gautherie (:sgautherie) 2010-02-10 09:51:11 PST
Created attachment 426267 [details] [diff] [review]
(Dv1a-SM) Update SeaMonkey packaging
[Checkin: Comment 17]

Dv1-SM, correctly sorted.
Comment 15 Serge Gautherie (:sgautherie) 2010-02-10 09:55:08 PST
Created attachment 426269 [details] [diff] [review]
(Ev1-SM-191) Support SeaMonkey downgrading
[Checkin: Comment 18]

Looking for "approval-seamonkey2.0.4=?" actually.
Comment 16 Robert Kaiser 2010-02-10 14:59:22 PST
(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) 2010-02-10 19:10:08 PST
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) 2010-02-10 19:10:55 PST
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 [:bsmedberg] 2010-03-24 08:28:32 PDT
http://hg.mozilla.org/projects/firefox-lorentz/rev/87645240ad71
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-06 16:10:49 PDT
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 [:bsmedberg] 2010-04-07 06:11:46 PDT
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
Comment 23 Eric Shepherd [:sheppy] 2014-12-09 14:29:59 PST
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 2014-12-09 15:15:11 PST
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);

Note You need to log in before you can comment on or make changes to this bug.