Closed Bug 912457 Opened 6 years ago Closed 6 years ago

[OS.File] OS.File doesn't expose flush on file object from chrome

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ochameau, Assigned: nmaier)

Details

(Keywords: dev-doc-complete, Whiteboard: [Async][mentor=Yoric][lang=js])

Attachments

(1 file, 1 obsolete file)

The documentation talks about a flush method on file objects, but it isn't being exposed, at least when using OS.File from chrome code:
  https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#OS.File.prototype.flush
The change is actually rather simple:
- in osfile_async_worker.jsm, add a method File_prototype_flush that calls File.prototype.flush, much like e.g File_prototype_setPosition calls File.prototype.setPosition);
- in osfile_async_front.jsm, add a method File.prototype.flush that sends message File_prototype_flush, much like e.g. File.prototype.setPosition sends message File_prototype_setPosition.
Whiteboard: [mentor=Yoric][lang=js]
Hi, this could be a first bug to start, can it be assigned to me?
Have fun :)
If you have any questions, don't hesitate to come and chat on #introduction: http://client00.chat.mibbit.com/?server=irc.mozilla.org&channel=%23introduction&nick=tomiberes
Assignee: nobody → tomiberes
Hi Tomiberes, how are things doing on this bug? Do you need any assistance?
Flags: needinfo?(tomiberes)
In the absence of news, de-assigning.
Assignee: tomiberes → nobody
Flags: needinfo?(tomiberes)
Summary: OS.File doesn't expose flush on file object from chrome → [OS.File] OS.File doesn't expose flush on file object from chrome
Whiteboard: [mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js]
Note that this patch is on top of some other patches (bug 928239, bug 924858, bug 924916) in my queue and will fail to apply the xpcshell.ini hunk without the other patches.
Assignee: nobody → maierman
Status: NEW → ASSIGNED
Attachment #818844 - Flags: review?(dteller)
In that case, could you add dependencies?
Comment on attachment 818844 [details] [diff] [review]
Expose async |flush()| in OS.File.

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

Looks good, thanks.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +498,5 @@
> +  },
> +
> +  /**
> +   * Flushes the file's buffers and causes all buffered data
> +   * to be written.

Could you add one line to mention that this operation is generally quite expensive, serves essentially to protect against hardware/kernel/power failures taking place immediately after the write and should be reserved for valuable files?
Attachment #818844 - Flags: review?(dteller) → review+
How about:
Disk flushes are very expensive and therefore should be used carefully, sparingly and only in scenarios where it is vital that data survives system crashes. Even though the function will be executed off the main-thread, it might still affect the overall performance of any running application.
Unbitrotted. Carrying over r=yoric.
Part of this green try: https://tbpl.mozilla.org/?tree=Try&rev=62ec046931e3
Attachment #823021 - Flags: review+
Attachment #818844 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/57f39d5c71fa
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/57f39d5c71fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Async][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla27
Updated article to state |.flush()| is supported in the async interface since Gecko 27:
https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread
You need to log in before you can comment on or make changes to this bug.