Closed
Bug 644680
Opened 13 years ago
Closed 13 years ago
FileUtils.openSafeFileOutputStream() should init stream with DEFER_OPEN
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: philikon, Assigned: philikon)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 6 obsolete files)
7.86 KB,
patch
|
Details | Diff | Splinter Review | |
3.49 KB,
patch
|
robert.strong.bugs
:
superreview+
|
Details | Diff | Splinter Review |
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 | ||
Updated•13 years ago
|
Assignee: nobody → philipp
Assignee | ||
Comment 1•13 years ago
|
||
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)
Assignee | ||
Comment 2•13 years ago
|
||
Use DEFER_OPEN. Like a G6.
Attachment #526170 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 3•13 years ago
|
||
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 :)
Assignee | ||
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
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 6•13 years ago
|
||
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)
Comment 7•13 years ago
|
||
(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?)
Assignee | ||
Comment 8•13 years ago
|
||
(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.
Comment 9•13 years ago
|
||
(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...
Assignee | ||
Comment 10•13 years ago
|
||
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 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
(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 13•13 years ago
|
||
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+
Assignee | ||
Comment 14•13 years ago
|
||
Address sdwilsh's review comment: document what test_openSafeFileOutputStream_modeFlags tests.
Attachment #526359 -
Attachment is obsolete: true
Assignee | ||
Comment 15•13 years ago
|
||
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)
Assignee | ||
Comment 16•13 years ago
|
||
(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
Assignee | ||
Comment 17•13 years ago
|
||
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)
Assignee | ||
Comment 18•13 years ago
|
||
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)
Assignee | ||
Comment 19•13 years ago
|
||
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)
Comment 20•13 years ago
|
||
(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 21•13 years ago
|
||
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)
Assignee | ||
Comment 22•13 years ago
|
||
(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.
Comment 23•13 years ago
|
||
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
Comment 24•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #526402 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 25•13 years ago
|
||
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
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 26•13 years ago
|
||
Documentation updated: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFileOutputStream And mentioned on Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 27•13 years ago
|
||
(In reply to comment #26) > https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFileOutputStream This isn't the documentation page for FileUtils.openSafeFileOutputStream. I think you want to update https://developer.mozilla.org/en/JavaScript_code_modules/FileUtils.jsm#openSafeFileOutputStream%28%29
Keywords: dev-doc-complete → dev-doc-needed
Comment 28•13 years ago
|
||
I did update that page, I just pasted the wrong URL. :)
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 29•13 years ago
|
||
(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.
Description
•