Closed Bug 644680 Opened 13 years ago Closed 13 years ago

FileUtils.openSafeFileOutputStream() should init stream with DEFER_OPEN

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: philikon, Assigned: philikon)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 6 obsolete files)

Right now it does this:

  fos.init(file, modeFlags, this.PERMS_FILE, 0);

It should do this:

  fos.init(file, modeFlags, this.PERMS_FILE, fos.DEFER_OPEN);

:)
Assignee: nobody → philipp
Shocked to find FileUtils.jsm was without tests, so I wrote some before doing the actual patch. There's one TODO comment in there that is addressed in the Part 1 patch.
Attachment #526169 - Flags: review?(sdwilsh)
Attached patch Part 1 (v1): use DEFER_OPEN (obsolete) — Splinter Review
Use DEFER_OPEN. Like a G6.
Attachment #526170 - Flags: review?(sdwilsh)
Comment on attachment 526169 [details] [diff] [review]
Part 0 (v1): Tests for FileUtils.jsm

I should point out that this patch in its current form requires bug 648367. If that is somehow rejected, it's easy enough to copy'n'paste run_next_test from other places. But really I'd rather not. So bonus points for pestering Ted to get me an r+ sooner :)
Blocks: 610832
Rebased on top of bug 648367 (use add_test instead of gTests.push)
Attachment #526169 - Attachment is obsolete: true
Attachment #526169 - Flags: review?(sdwilsh)
Attachment #526359 - Flags: review?(sdwilsh)
Attached patch Part 1 (v1.1): use DEFER_OPEN (obsolete) — Splinter Review
Rebase on top of Part 0 v1.1, no changes to the patch itself.
Attachment #526170 - Attachment is obsolete: true
Attachment #526170 - Flags: review?(sdwilsh)
Attachment #526365 - Flags: review?(sdwilsh)
Comment on attachment 526359 [details] [diff] [review]
Part 0 (v1.1): Tests for FileUtils.jsm

This is an API change that is going to need sr.
Attachment #526359 - Flags: superreview?(robert.bugzilla)
(In reply to comment #6)
> This is an API change that is going to need sr.
(although I think you actually want those changed rolled into the other part of this patch, right?)
(In reply to comment #7)
> (In reply to comment #6)
> > This is an API change that is going to need sr.
> (although I think you actually want those changed rolled into the other part of
> this patch, right?)

That patch doesn't change the API at all! It just updates the documentation to reflect reality.
(In reply to comment #8)
> That patch doesn't change the API at all! It just updates the documentation to
> reflect reality.
Er, yeah.  I think I misread the patch somehow...
Comment on attachment 526359 [details] [diff] [review]
Part 0 (v1.1): Tests for FileUtils.jsm

Canceling sr per comment 8
Attachment #526359 - Flags: superreview?(robert.bugzilla)
Comment on attachment 526365 [details] [diff] [review]
Part 1 (v1.1): use DEFER_OPEN

>+++ b/toolkit/mozapps/shared/FileUtils.jsm
>-    fos.init(file, modeFlags, this.PERMS_FILE, 0);
>+    fos.init(file, modeFlags, this.PERMS_FILE, fos.DEFER_OPEN);
This, however, does constitue an API change and should be documented in the jsm *and* get sr.

>+++ b/toolkit/mozapps/shared/test/unit/test_FileUtils.js
>-  //TODO FileUtils.openSafeFileOutputStream() should use DEFER_OPEN in which
>-  // case we want to verify that the file hasn't been created yet and gets
>-  // created once we write to it.
>+  // FileUtils.openSafeFileOutputStream() opens the stream with DEFER_OPEN
>+  // which means the file will not be open until we write to it.
>+  do_check_false(file.exists());
>+
>+  let data = "imagine";
>+  fos.write(data, data.length);
>   do_check_true(file.exists());
You don't actually need to do the writing to see if it exists.  The file output (and safe file output) stream has explicit tests for this in the DEFER_OPEN case.

> add_test(function test_openSafeFileOutputStream_modeFlags() {
I don't even know what this is trying to test.  You either need more comments, or a better test name, or both!

r=sdwilsh with changes addressed.
Attachment #526365 - Flags: superreview?(robert.bugzilla)
Attachment #526365 - Flags: review?(sdwilsh)
Attachment #526365 - Flags: review+
(In reply to comment #11)
> >-    fos.init(file, modeFlags, this.PERMS_FILE, 0);
> >+    fos.init(file, modeFlags, this.PERMS_FILE, fos.DEFER_OPEN);
> This, however, does constitue an API change and should be documented in the jsm
> *and* get sr.

Yup, will add a line to the doc comment.

> >+  let data = "imagine";
> >+  fos.write(data, data.length);
> >   do_check_true(file.exists());
> You don't actually need to do the writing to see if it exists.  The file output
> (and safe file output) stream has explicit tests for this in the DEFER_OPEN
> case.

True. Doesn't hurt, though, does it? :)

> > add_test(function test_openSafeFileOutputStream_modeFlags() {
> I don't even know what this is trying to test.  You either need more comments,
> or a better test name, or both!

It tests that I can pass in my own mode flags. Will add a comment.

Thanks!
Comment on attachment 526359 [details] [diff] [review]
Part 0 (v1.1): Tests for FileUtils.jsm

>+++ b/toolkit/mozapps/shared/test/Makefile.in
You copied and pasted this license block from another file and not from https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-sh
Copyright date is also wrong ;)

Also, can we avoid adding a makefile (which makes build times take longer) by adding an XPCSHELL_TESTS = test/unit in the makefile above this one?

>+++ b/toolkit/mozapps/shared/test/unit/test_FileUtils.js
You need a license block here.  Might I suggest https://www.mozilla.org/MPL/boilerplate-1.1/pd-c ?

>@@ -0,0 +1,130 @@
>+Components.utils.import("resource://gre/modules/FileUtils.jsm");
>+
>+function do_check_throws(resultCode, testFunc) {
>+  try {
>+    testFunc();
>+    do_throw("Expected " + resultCode + " exception!");
>+  } catch(ex) {
>+    do_check_eq(ex.result, Components.results[resultCode]);
>+  }
>+}
Almost all other instances of do_check_throws in the tree look like this (with the exception of the jpake stuff):
https://hg.mozilla.org/mozilla-central/annotate/ec809c159ad2/extensions/cookie/test/unit/head_cookies.js#l32

Can you make it the same and then file a bug to get this method added to head.js please?

>+add_test(function test_getFile() {
We should also add a test for trying to get something that is garbage and the output is what we expect.

>+add_test(function test_getFile_followLinks() {
>+  //TODO We lack the ability to create symlinks from JS.
>+  run_next_test();
>+});
Don't even bother then.

>+add_test(function test_getDir_followLinks() {
>+  //TODO We lack the ability to create symlinks from JS.
>+  run_next_test();
>+});
ditto

>+add_test(function test_openSafeFileOutputStream_modeFlags() {
I really don't understand what this is trying to do here...

>+add_test(function test_closeSafeFileOutputStream() {
>+  let file = FileUtils.getFile("ProfD", ["george"]);
>+  let fos = FileUtils.openSafeFileOutputStream(file);
>+
>+  // We can write data to the stream just fine while it's open.
>+  let data = "here comes the sun";
>+  fos.write(data, data.length);
Can we pretty please not do sync I/O here?  People look at tests for samples on how to do things, and I'd like to make sure we keep it all async.

r=sdwilsh with those changes
Attachment #526359 - Flags: review?(sdwilsh) → review+
Address sdwilsh's review comment: document what test_openSafeFileOutputStream_modeFlags tests.
Attachment #526359 - Attachment is obsolete: true
Attached patch Part 1 (v1.2): use DEFER_OPEN (obsolete) — Splinter Review
Address sdwilsh's review comment: document DEFER_OPEN change in the JSM.
Attachment #526365 - Attachment is obsolete: true
Attachment #526365 - Flags: superreview?(robert.bugzilla)
Attachment #526384 - Flags: superreview?(robert.bugzilla)
(In reply to comment #13)
> >@@ -0,0 +1,130 @@
> >+Components.utils.import("resource://gre/modules/FileUtils.jsm");
> >+
> >+function do_check_throws(resultCode, testFunc) {
> >+  try {
> >+    testFunc();
> >+    do_throw("Expected " + resultCode + " exception!");
> >+  } catch(ex) {
> >+    do_check_eq(ex.result, Components.results[resultCode]);
> >+  }
> >+}
> Almost all other instances of do_check_throws in the tree look like this (with
> the exception of the jpake stuff):
> https://hg.mozilla.org/mozilla-central/annotate/ec809c159ad2/extensions/cookie/test/unit/head_cookies.js#l32
> 
> Can you make it the same and then file a bug to get this method added to
> head.js please?

Filed Bug 650402. Major sigh on that implementation, though. I think callback functions should always be the *last* argument to a function, it just reads better. Also, passing in the actual error name makes debugging easier, but that could be fixed.

> >+add_test(function test_getFile() {
> We should also add a test for trying to get something that is garbage and the
> output is what we expect.

Clarification from IRC: garbage = a key the dirservice doesn't know.

> >+add_test(function test_openSafeFileOutputStream_modeFlags() {
> I really don't understand what this is trying to do here...

Added a comment in v1.2

> >+add_test(function test_closeSafeFileOutputStream() {
> >+  let file = FileUtils.getFile("ProfD", ["george"]);
> >+  let fos = FileUtils.openSafeFileOutputStream(file);
> >+
> >+  // We can write data to the stream just fine while it's open.
> >+  let data = "here comes the sun";
> >+  fos.write(data, data.length);
> Can we pretty please not do sync I/O here?  People look at tests for samples on
> how to do things, and I'd like to make sure we keep it all async.

14:46 < philikon> sdwilsh: so, about avoiding sync i/o
14:46 < philikon> sdwilsh: i want to test FileUtils.closeOutputStream
14:46 < philikon> which means i can't use NetUtil.asyncCopy
14:46 < sdwilsh> oh the async helpers close it don't they...
14:46 < philikon> because that already closes it for me
14:46 < sdwilsh> forget it then
14:46 < philikon> thank you
Incorporate sdwilsh's review comments:
* Get rid of toolkit/mozapps/shared/test/Makefile.in, just inline the xpcshell test registration in toolkit/mozapps/share/Makefile.in
* Use common do_check_throws implementation.
* Added licence header to test file.
* Removed TODO comments for symlink tests
Attachment #526383 - Attachment is obsolete: true
Attachment #526400 - Flags: superreview?(robert.bugzilla)
Comment on attachment 526400 [details] [diff] [review]
Part 0 (v1.3): Tests for FileUtils.jsm

Oops wrong file for sr
Attachment #526400 - Flags: superreview?(robert.bugzilla)
Rebase on top of Part 0 v1.3, no changes to the patch itself.
Attachment #526384 - Attachment is obsolete: true
Attachment #526384 - Flags: superreview?(robert.bugzilla)
Attachment #526402 - Flags: superreview?(robert.bugzilla)
(In reply to comment #16)
> Filed Bug 650402. Major sigh on that implementation, though. I think callback
> functions should always be the *last* argument to a function, it just reads
> better. Also, passing in the actual error name makes debugging easier, but that
> could be fixed.
We can change that syntax if needed.
Comment on attachment 526402 [details] [diff] [review]
Part 1 (v1.3): use DEFER_OPEN

>diff --git a/toolkit/mozapps/shared/FileUtils.jsm b/toolkit/mozapps/shared/FileUtils.jsm
>--- a/toolkit/mozapps/shared/FileUtils.jsm
>+++ b/toolkit/mozapps/shared/FileUtils.jsm
>@@ -105,23 +105,25 @@ var FileUtils = {
> 
>   /**
>    * Opens a safe file output stream for writing.
>    * @param   file
>    *          The file to write to.
>    * @param   modeFlags
>    *          (optional) File open flags. Can be undefined.
>    * @returns nsIFileOutputStream to write to.
>+   * @note The stream is initialized with the DEFER_OPEN behavior flag.
>+   *       See nsIFileOutputStream.
nit: I don't believe that @note is a valid javadoc comment tag and the note should just be moved to the description.

>    */
>   openSafeFileOutputStream: function FileUtils_openSafeFileOutputStream(file, modeFlags) {
>     var fos = Cc["@mozilla.org/network/safe-file-output-stream;1"].
>               createInstance(Ci.nsIFileOutputStream);
>     if (modeFlags === undefined)
>       modeFlags = this.MODE_WRONLY | this.MODE_CREATE | this.MODE_TRUNCATE;
>-    fos.init(file, modeFlags, this.PERMS_FILE, 0);
>+    fos.init(file, modeFlags, this.PERMS_FILE, fos.DEFER_OPEN);
Are there cases where a consumer wouldn't want DEFER_OPEN? If so, it could just be an optional param that defaults to DEFER_OPEN. I'm going to leave it to sdwilsh whether this would be appropriate.

Requesting review from sdwilsh for the above.
Attachment #526402 - Flags: superreview?(robert.bugzilla)
Attachment #526402 - Flags: superreview+
Attachment #526402 - Flags: review?(sdwilsh)
(In reply to comment #21)
> nit: I don't believe that @note is a valid javadoc comment tag and the note
> should just be moved to the description.

I get a ton of hits for @note when I grep for it in mozilla-central. I can take it out, of course, if you prefer or if for some reason it shouldn't be used.
No biggy... it is just a nit and it wouldn't be the first time a comment tag crept into the code that isn't per the javadoc style guide
(In reply to comment #21)
> Are there cases where a consumer wouldn't want DEFER_OPEN? If so, it could just
> be an optional param that defaults to DEFER_OPEN. I'm going to leave it to
> sdwilsh whether this would be appropriate.
Maybe some legacy consumer depends on it creating the file (which is why we didn't make it the default for nsIFileOutputStream), but there is no good reason to depend on that.  We should force it on all new consumers who are more likely to use this API so that they end up not doing disk I/O on the GUI thread.
Attachment #526402 - Flags: review?(sdwilsh)
http://hg.mozilla.org/mozilla-central/rev/e3e1ade248ca
http://hg.mozilla.org/mozilla-central/rev/b3dbcd294764
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFileOutputStream

And mentioned on Firefox 6 for developers.
I did update that page, I just pasted the wrong URL. :)
(In reply to comment #28)
> I did update that page, I just pasted the wrong URL. :)

*cough* I see that now, thanks! Sorry about that :)

(You did say it was Gecko 5 even though it landed for Gecko 6, so I fixed that real quick.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: