Closed Bug 943422 Opened 6 years ago Closed 6 years ago

Implement FileUtils.openAtomicFileOutputStream()

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: Yoric, Assigned: lpy)

References

(Blocks 1 open bug)

Details

(Keywords: main-thread-io, Whiteboard: [Async][mentor=Yoric][lang=js])

Attachments

(1 file, 4 obsolete files)

At the moment, FileUtils offers a method openSafeFileOutputStream() which opens a safe file output stream. Bug 928321 introduces a faster variant, the atomic file output stream. We should:
- create a method FileUtils.openAtomicFileOutputStream() that opens an atomic output stream;
- add comments in FileUtils.jsm explaining that openAtomicFileOutputStream() is generally better;
- update the documentation.
Hi David, I take this bug. Should I wait until Bug 928321 checkin?
Assignee: nobody → pylaurent1314
(In reply to Peiyong Lin[:lpy] from comment #1)
> Hi David, I take this bug. Should I wait until Bug 928321 checkin?

Yes, please.
Attached patch bug943422-v1.patch (obsolete) — Splinter Review
Implement |FileUtils.openAtomicFileOutputStream()|
Attachment #8344194 - Flags: review?(dteller)
Comment on attachment 8344194 [details] [diff] [review]
bug943422-v1.patch

Review of attachment 8344194 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
I'm not setting r+ yet, because I'd like a test in toolkit/modules/tests/xpcshell/test_FileUtils.js
Attachment #8344194 - Flags: review?(dteller)
Attached patch bug943422-V2.patch (obsolete) — Splinter Review
Add openAtomicFileOutputStream in toolkit/modules/tests/xpcshell/test_FileUtils.js

By the way, I saw |closeSafeFileOutputStream| in here http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/FileUtils.jsm#125, should I implement |closeAtomicFileOutputStream|?

Thanks!
Attachment #8344194 - Attachment is obsolete: true
Attachment #8346504 - Flags: review?(dteller)
Attached patch bug943422-V3.patch (obsolete) — Splinter Review
Update again. :)  Add |closeAtomicFileOutputStream|, tests in toolkit/modules/tests/xpcshell
Attachment #8346504 - Attachment is obsolete: true
Attachment #8346504 - Flags: review?(dteller)
Attachment #8346529 - Flags: review?(dteller)
Comment on attachment 8346529 [details] [diff] [review]
bug943422-V3.patch

Review of attachment 8346529 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/xpcshell/test_FileUtils.js
@@ +128,5 @@
>  
>    run_next_test();
>  });
>  
> +add_test(function test_openAtomicFileOutputStream_defaultFlags() {

A bit too much copy&paste for my taste. Could you factor the code from openSafeFileOutputStream and openAtomicFileOutputStream tests, since they differ only from two function calls?
Attachment #8346529 - Flags: review?(dteller) → feedback+
Attached patch bug943422-V4.patch (obsolete) — Splinter Review
Thank you :)
I also do this for |test_openFileOutputStream*|.
Attachment #8346529 - Attachment is obsolete: true
Attachment #8346622 - Flags: review?(dteller)
Comment on attachment 8346622 [details] [diff] [review]
bug943422-V4.patch

Review of attachment 8346622 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with a few minor changes.
Have you run this through Try yet?

::: toolkit/modules/tests/xpcshell/test_FileUtils.js
@@ +90,5 @@
>  
>    run_next_test();
>  });
>  
> +let openFileOutputStream_defaultFlags = function (aArg, aFileName) {

"aArg" is not a very useful name, "aKind" would be nicer.

@@ +97,5 @@
> +  if (aArg == "atomic") {
> +    fos = FileUtils.openAtomicFileOutputStream(file);
> +  } else if (aArg == "safe") {
> +    fos = FileUtils.openSafeFileOutputStream(file);
> +  } else {

Just to be on the safe side, can you ensure that the string is always one of the accepted strings ("atomic", "safe", "")?

@@ +128,2 @@
>  
> +let openFileOutputStream_modeFlags = function(aArg, aFileName) {

Same comments below.
Attachment #8346622 - Flags: review?(dteller) → review+
use |do_check_true| to ensure aKind to be one of |("atomic", "safe", "")|
Attachment #8346622 - Attachment is obsolete: true
Attachment #8348008 - Flags: review+
I set the checkin flag, will update document after patch land. Thank you Yoric :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/935b34e51fed
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.