Closed Bug 786196 Opened 8 years ago Closed 8 years ago

[OS.File] Implement flushing

Categories

(Toolkit :: OS.File, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Luqman)

References

Details

(Whiteboard: [mentor=Yoric][lang=js][lang=c])

Attachments

(1 file, 5 obsolete files)

We need an operation |OS.File.prototype.flush|.
This operation should be implemented with:
- |fsync| under Unix;
- |FlushFileBuffers| under Windows.

Getting in the code of OS.File might be tricky, but implementation should be easy, so marking this as a mentored bug.
Attached patch [OS.File] Implement flushing (obsolete) — Splinter Review
Comment on attachment 656116 [details] [diff] [review]
[OS.File] Implement flushing

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

This looks good, thanks for the patch.
Attachment #656116 - Flags: review+
It might be useful to add a trivial testsuite that ensures that |foo.flush()| does not cause an error, where |foo| is an open file.
Attached patch [OS.File] Implement flushing (obsolete) — Splinter Review
Added simple testcase.
Attachment #656116 - Attachment is obsolete: true
Comment on attachment 656166 [details] [diff] [review]
[OS.File] Implement flushing

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

Looks good. However, I am not sure whether flushing works on files opened for reading. Just in case it doesn't, could you adapt the test suite to files opened for writing?
Attachment #656166 - Flags: feedback+
Attached patch [OS.File] Implement flushing (obsolete) — Splinter Review
Attachment #656166 - Attachment is obsolete: true
Comment on attachment 656171 [details] [diff] [review]
[OS.File] Implement flushing

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

Er... please don't overwrite important files during the test :)
Attachment #656171 - Flags: feedback+
Attached patch [OS.File] Implement flushing (obsolete) — Splinter Review
No more overwriting important files
Attachment #656171 - Attachment is obsolete: true
Comment on attachment 656188 [details] [diff] [review]
[OS.File] Implement flushing

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

Looks good. Could you just call the file test_flush.tmp (no directory component) to match other files created by the test? No need to re-ask me for a review for this.
Attachment #656188 - Flags: review+
Attached patch [OS.File] Implement flushing (obsolete) — Splinter Review
Attachment #656188 - Attachment is obsolete: true
Assignee: nobody → laden
Status: NEW → ASSIGNED
Attachment #656191 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e53491a235ca
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.