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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 2

[tomiberes]

Hi, this could be a first bug to start, can it be assigned to me?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

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

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Hi Tomiberes, how are things doing on this bug? Do you need any assistance?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

In the absence of news, de-assigning.

Comment 6

[Nils Maier [:nmaier]]

Attachment: Expose async |flush()| in OS.File.

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.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

In that case, could you add dependencies?

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 9

[Nils Maier [:nmaier]]

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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Sounds good.

Comment 11

[Nils Maier [:nmaier]]

Attachment: Expose async |flush()| in OS.File. r=yoric

Unbitrotted. Carrying over r=yoric.
Part of this green try: https://tbpl.mozilla.org/?tree=Try&rev=62ec046931e3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/57f39d5c71fa

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/57f39d5c71fa

Comment 14

[Nils Maier [:nmaier]]

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