Last Comment Bug 747876 - Efficient JS File API: synchronous front-end
: Efficient JS File API: synchronous front-end
Status: RESOLVED FIXED
[dev-doc-at https://developer.mozilla...
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 707679 707681
Blocks: OS.File 753768 762867 764436
  Show dependency treegraph
 
Reported: 2012-04-23 03:49 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-10-07 08:28 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Front-end glue (860 bytes, patch)
2012-04-26 08:31 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Windows front-end (15.69 KB, patch)
2012-04-26 08:31 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Unix front-end (19.43 KB, patch)
2012-04-26 08:32 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Companion testsuite (11.54 KB, patch)
2012-04-26 08:32 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Companion testsuite (11.67 KB, patch)
2012-04-27 09:06 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Unix front-end (21.40 KB, patch)
2012-04-27 09:08 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Windows front-end (18.03 KB, patch)
2012-04-27 09:10 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: feedback-
Details | Diff | Splinter Review
Unix front-end (31.99 KB, patch)
2012-04-30 04:08 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Windows front-end (19.26 KB, patch)
2012-05-11 08:41 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Additional libc constants, for BSD copy (1.37 KB, patch)
2012-05-11 08:42 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Unix front-end (25.37 KB, patch)
2012-05-11 08:43 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Companion testsuite (11.88 KB, patch)
2012-05-11 08:43 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Companion testsuite (12.94 KB, patch)
2012-05-15 06:16 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Unix front-end (25.15 KB, patch)
2012-05-15 06:16 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Windows front-end (18.97 KB, patch)
2012-05-15 06:17 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Windows front-end (18.85 KB, patch)
2012-05-16 01:45 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Windows front-end (17.40 KB, patch)
2012-05-16 12:43 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Splinter Review
Companion testsuite (10.27 KB, patch)
2012-05-18 06:56 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Windows front-end (22.95 KB, patch)
2012-05-18 06:58 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Companion testsuite (10.26 KB, patch)
2012-05-18 08:44 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Front-end glue (1.61 KB, patch)
2012-05-18 08:45 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Additional windows constants, for error-checking (1.14 KB, patch)
2012-05-18 08:45 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Unix front-end (24.44 KB, patch)
2012-05-18 08:46 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Companion testsuite (10.34 KB, patch)
2012-05-23 08:06 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Additional libc constants, for BSD copy (1.42 KB, patch)
2012-05-23 08:07 PDT, David Teller [:Yoric] (please use "needinfo")
khuey: review+
Details | Diff | Splinter Review
Windows front-end (22.99 KB, patch)
2012-05-23 08:07 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Splinter Review
Unix front-end (24.72 KB, patch)
2012-05-23 08:08 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Additional windows constants, for error-checking (1.19 KB, patch)
2012-05-23 08:08 PDT, David Teller [:Yoric] (please use "needinfo")
khuey: review+
Details | Diff | Splinter Review
Front-end glue (866 bytes, patch)
2012-05-23 08:09 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
doug.turner: review+
Details | Diff | Splinter Review
Windows front-end, continued (5.03 KB, patch)
2012-05-24 03:38 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Windows front-end, part 3 (4.84 KB, patch)
2012-05-30 02:50 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Splinter Review
Unix front-end, part 2 (9.25 KB, patch)
2012-05-30 02:54 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Splinter Review
Scope-based resource control (3.67 KB, patch)
2012-05-30 02:55 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Splinter Review
Unix front-end (24.52 KB, patch)
2012-05-31 07:30 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Shared code (2.87 KB, patch)
2012-05-31 07:35 PDT, David Teller [:Yoric] (please use "needinfo")
doug.turner: review+
Details | Diff | Splinter Review
Windows front-end (21.87 KB, patch)
2012-05-31 07:39 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Windows front-end (21.88 KB, patch)
2012-06-07 08:12 PDT, David Teller [:Yoric] (please use "needinfo")
doug.turner: review+
Details | Diff | Splinter Review
Unix front-end (24.56 KB, patch)
2012-06-07 08:14 PDT, David Teller [:Yoric] (please use "needinfo")
doug.turner: review+
Details | Diff | Splinter Review
Shared code (2.88 KB, patch)
2012-06-12 23:39 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Front-end glue (880 bytes, patch)
2012-06-12 23:39 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Additional windows constants, for error-checking (1.20 KB, patch)
2012-06-12 23:39 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Windows front-end (21.88 KB, patch)
2012-06-12 23:40 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Additional libc constants, for BSD copy (1.42 KB, patch)
2012-06-12 23:40 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Unix front-end (24.57 KB, patch)
2012-06-12 23:41 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Companion testsuite (11.29 KB, patch)
2012-06-12 23:57 PDT, David Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Splinter Review
Companion testsuite (11.18 KB, patch)
2012-06-15 02:29 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Shared code (3.78 KB, patch)
2012-06-15 02:30 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Front-end glue (880 bytes, patch)
2012-06-15 02:32 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Unix front-end (24.57 KB, patch)
2012-06-15 02:38 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Windows front-end (21.88 KB, patch)
2012-06-15 02:40 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Only include copyfile.h on mac (822 bytes, patch)
2012-06-17 05:51 PDT, Landry Breuil (:gaston)
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-04-23 03:49:49 PDT
Implement a cross-platform synchronous API on top of the Unix back-end (bug 707679) and the Windows back-end (bug 707681). This API should work on only on chrome workers.

Followup bug 729057 will deal with turning this synchronous API for workers in an asynchronous API for the main thread.
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-04-26 08:31:00 PDT
Created attachment 618658 [details] [diff] [review]
Front-end glue
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-04-26 08:31:54 PDT
Created attachment 618660 [details] [diff] [review]
Windows front-end
Comment 3 David Teller [:Yoric] (please use "needinfo") 2012-04-26 08:32:22 PDT
Created attachment 618661 [details] [diff] [review]
Unix front-end
Comment 4 David Teller [:Yoric] (please use "needinfo") 2012-04-26 08:32:51 PDT
Created attachment 618662 [details] [diff] [review]
Companion testsuite
Comment 5 (dormant account) 2012-04-26 13:39:13 PDT
Comment on attachment 618660 [details] [diff] [review]
Windows front-end

A couple things.
+       write: function write(buffer, nbytes) {
+         this._ensureOpen();
+         WinFile.WriteFile(this._fd, buffer, nbytes, gBytesWrittenPtr);
+         return gBytesWritten.value;
+       },
Why no error handling. Is WinFile.WriteFile throwing an exception via some declareFFI-created-wrapper? As I said before, I'm against changing behavior of wrapped functions in any way(beyond cdatafinalizer safety valve).

Why is there worker/main-thread communication in declareFFI?
Comment 6 (dormant account) 2012-04-26 13:47:08 PDT
Comment on attachment 618660 [details] [diff] [review]
Windows front-end

One of the reasons I asked to start with minimal functionality is because adding more stuff just increases your chances making a mistake
+     File.truncate = function truncate(path, options) {
+       return generic_open(path, false, Const.TRUNCATE_EXISTING, options);
+     };
^ wrong. Above is completely busted in presence of links(yes windows has those tool) and is much less efficient.
The correct way to do this is to seek + SetFileSize ala http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.cpp#69

Please take out of all of APIs that are no strictly required from the higher level api(harder to make mistakes when doing mere wrappers).
Comment 7 (dormant account) 2012-04-26 13:50:12 PDT
Comment on attachment 618661 [details] [diff] [review]
Unix front-end

+       setPositionFromEnd: function setPositionFromEnd(pos) {
+         this._ensureOpen();
+         return UnixFile.lseek(this._fd, pos, Const.SEEK_END);
+       },
+

I do not see utility in above & other setPosition*. it does nothing beyond obscuring the SEEK_END seek

r-ing because this code needs to be rewritten to take out exception handling from low level functions
Comment 8 (dormant account) 2012-04-26 13:52:34 PDT
Comment on attachment 618662 [details] [diff] [review]
Companion testsuite

+  try {
+    test_init();
+    test_open_existing_file();
+    test_open_non_existing_file();
+    test_pump_existing_file();
+    test_copy_existing_file();
+    test_move_file();
+    test_append_to_file();
+    test_truncate_file();
+  } catch (x) {
+    log("Catching error: " + x);
+    log("Stack: " + x.stack);
+    ok(false, x.toString() + "\n" + x.stack);
+  }

Why not just let things fail?
Comment 9 David Teller [:Yoric] (please use "needinfo") 2012-04-27 00:37:34 PDT
(In reply to Taras Glek (:taras) from comment #5)
> Comment on attachment 618660 [details] [diff] [review]
> Windows front-end
> 
> A couple things.
> +       write: function write(buffer, nbytes) {
> +         this._ensureOpen();
> +         WinFile.WriteFile(this._fd, buffer, nbytes, gBytesWrittenPtr);
> +         return gBytesWritten.value;
> +       },
> Why no error handling. Is WinFile.WriteFile throwing an exception via some
> declareFFI-created-wrapper? As I said before, I'm against changing behavior
> of wrapped functions in any way(beyond cdatafinalizer safety valve).

Indeed, I have incorporated error-checking in the Type passed to declareFFI. |Type.zero_or_nothing| informs |declareFFI| that 0 is an error and that the function returns an unspecified value otherwise.

> Why is there worker/main-thread communication in declareFFI?

Er, what?
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-04-27 00:48:26 PDT
(In reply to Taras Glek (:taras) from comment #6)
> Comment on attachment 618660 [details] [diff] [review]
> Windows front-end
> 
> One of the reasons I asked to start with minimal functionality is because
> adding more stuff just increases your chances making a mistake
> +     File.truncate = function truncate(path, options) {
> +       return generic_open(path, false, Const.TRUNCATE_EXISTING, options);
> +     };
> ^ wrong. Above is completely busted in presence of links(yes windows has
> those tool) and is much less efficient.

Gasp. So much for the Windows API documentation.

> The correct way to do this is to seek + SetFileSize ala
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.cpp#69

So three system calls instead of one is faster? Ok, I will fix that.

> Please take out of all of APIs that are no strictly required from the higher
> level api(harder to make mistakes when doing mere wrappers).

Well, as I mentioned yesterday, I still have no scenario and no clear idea which APIs are "strictly required". The two bugs mentioned by Dietrich when I asked for his use cases seem to require only "read whole file asynchronously as String" and "copy file asynchronously", so if you want, I can just throw away my synchronous front-end and just provide these two functions.

(In reply to Taras Glek (:taras) from comment #7)
> I do not see utility in above & other setPosition*. it does nothing beyond
> obscuring the SEEK_END seek

Well, the idea is simply to have the same function name for Windows and Unix, which is the whole point of this synchronous front-end. Using libc constants SEEK_END et al. on a API that also works under Windows would be rather bad style.

> r-ing because this code needs to be rewritten to take out exception handling
> from low level functions

(In reply to Taras Glek (:taras) from comment #8)
> Comment on attachment 618662 [details] [diff] [review]
> Companion testsuite
[...] 
> Why not just let things fail?

The idea is to get better stack reporting. I do not remember whether this is actually required, though.
Comment 11 Doug Turner (:dougt) 2012-04-27 09:05:57 PDT
> ^ wrong. Above is completely busted in presence of links(yes windows has
> those tool) and is much less efficient.

We continue to care about links?  I did work in the nsifile stuff to support shortcuts - but was under the impression it was used very little (if not at all)
Comment 12 Doug Turner (:dougt) 2012-04-27 09:06:14 PDT
I mean shortcuts...
Comment 13 David Teller [:Yoric] (please use "needinfo") 2012-04-27 09:06:34 PDT
Created attachment 619065 [details] [diff] [review]
Companion testsuite
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-04-27 09:08:17 PDT
Created attachment 619067 [details] [diff] [review]
Unix front-end

Shifted the exceptions from the back-end to the front-end, slightly changed the shape of the File object.
Comment 15 David Teller [:Yoric] (please use "needinfo") 2012-04-27 09:10:30 PDT
Created attachment 619068 [details] [diff] [review]
Windows front-end

Exceptions moved from back-end to front-end, truncate function rewritten as asked. This code has not been tested yet, though.
Comment 16 (dormant account) 2012-04-27 20:59:30 PDT
(In reply to Doug Turner (:dougt) from comment #12)
> I mean shortcuts...

i meant windows hardlinks :)
Comment 17 (dormant account) 2012-04-27 21:02:03 PDT
Comment on attachment 618658 [details] [diff] [review]
Front-end glue

I'm leave this kind of stuff to dougt
Comment 18 (dormant account) 2012-04-27 21:11:15 PDT
Comment on attachment 619067 [details] [diff] [review]
Unix front-end

+       close: function close() {
+         if (this._fd) {
+           let fd = this._fd;
+           this._fd = null;
+           delete this.fd;
+           Object.defineProperty(this, "fd", {get: File.prototype._nofd});
+           return this._closeResult = WinFile.close(fd);
+         }

WinFile, really?

I think rather than having setPositionFromEnd..etc, provide a single .seek and take an argument for START, CURRENT, END or whatever you want to call them.

for code like 
+         let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR);
+         if (result == -1) {
+           throw new File.Error();
+         }
+         return result;

instead of cut'n'paste do
return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where checkError would do the error checking boilerplate

I'm not sure what Unix-style open is, your array of opens is more confusing than helpful. 

As I said before:instead of implementing everything, only implement functions as needed. 

Too much stuff here
Comment 19 David Teller [:Yoric] (please use "needinfo") 2012-04-28 05:04:28 PDT
(In reply to Taras Glek (:taras) from comment #18)
> Comment on attachment 619067 [details] [diff] [review]
> Unix front-end
> 
> +       close: function close() {
> +         if (this._fd) {
> +           let fd = this._fd;
> +           this._fd = null;
> +           delete this.fd;
> +           Object.defineProperty(this, "fd", {get: File.prototype._nofd});
> +           return this._closeResult = WinFile.close(fd);
> +         }
> 
> WinFile, really?

Oops. Forgot to |qref| before uploading.

> I think rather than having setPositionFromEnd..etc, provide a single .seek
> and take an argument for START, CURRENT, END or whatever you want to call
> them.

Ok. I marginally prefer my solution, as it avoids having to add yet another set of constants, but that's not big deal.

So, do you want me to put the constants in |OS.File|? In a subobject |OS.File.Constants|?


> for code like 
> +         let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR);
> +         if (result == -1) {
> +           throw new File.Error();
> +         }
> +         return result;
> 
> instead of cut'n'paste do
> return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where
> checkError would do the error checking boilerplate

ok

> I'm not sure what Unix-style open is, your array of opens is more confusing
> than helpful. 

Well, that was the result of a design discussion with Dietrich, Paul O'Shannessy and Drew Willcoxon on the topic of OS.File API a few months ago, plus a more recent discussion with Jason Orendorff on the topic of OS-specific APIs.

One of the main ideas is to avoid "one size doesn't quite fit anybody" APIs such as the file opening API of nspr/xpcom that require Unix-specific options even on Windows. Rather, we offer a set of APIs that cover the most common cases that can be made portable without performance penalty (that's "open", "truncate", "truncateC", ...), plus OS-specific APIs (prefixed by "unix" or "win"), to handle the myriad of cases in which no portable solution can be envisioned.

Requires more documentation, of course.

> As I said before:instead of implementing everything, only implement
> functions as needed. 
> 
> Too much stuff here

I fully agree with the general idea. As I mentioned, for the moment, I cannot do this until I have a clear idea of what is needed, so I am experimenting with nice API design, in the hope of obtaining something that I can show to potential users and get some feedback. It was my understanding that this was also something that you wanted.

As I said before, if you could point me towards what is needed, that would be great. As I mentioned, for the moment, the only scenarios I have seen that do not involve a full API, both provided by Dietrich, are:
1/ read a full file to a string;
2/ copy a file.

As I told you, scenario 1 is not really possible in the current state of js-ctypes, but I plan to work on patches to make it possible (bug 552551).

So, if you prefer, as I wrote above, I can throw away all that code for the moment (or move it to another bug) and just provide file copy.

What do you think, Taras?
Comment 20 David Teller [:Yoric] (please use "needinfo") 2012-04-30 04:08:44 PDT
Created attachment 619520 [details] [diff] [review]
Unix front-end

Attaching a new version.

ChangeLog:
- factored the error-handling mechanism;
- File.prototype.close now raises an error if |close| returns -1;
- plenty more documentation and clarifications;
- this time, the qref has been done :)
- added several missing error checks;
- now, all implementations of |copy| and |move| implement option |noOverwrite|.

Now, Taras, as I mentioned, if you can just tell me which features you want in priority, I will be able to divide the patch in smaller chunks adding only these features.
Comment 21 (dormant account) 2012-05-02 15:19:41 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> (In reply to Taras Glek (:taras) from comment #18)
> > Comment on attachment 619067 [details] [diff] [review]
> > Unix front-end
> > 
> > +       close: function close() {
> > +         if (this._fd) {
> > +           let fd = this._fd;
> > +           this._fd = null;
> > +           delete this.fd;
> > +           Object.defineProperty(this, "fd", {get: File.prototype._nofd});
> > +           return this._closeResult = WinFile.close(fd);
> > +         }
> > 
> > WinFile, really?
> 
> Oops. Forgot to |qref| before uploading.
> 
> > I think rather than having setPositionFromEnd..etc, provide a single .seek
> > and take an argument for START, CURRENT, END or whatever you want to call
> > them.
> 
> Ok. I marginally prefer my solution, as it avoids having to add yet another
> set of constants, but that's not big deal.
> 
> So, do you want me to put the constants in |OS.File|? In a subobject
> |OS.File.Constants|?
> 
> 
> > for code like 
> > +         let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR);
> > +         if (result == -1) {
> > +           throw new File.Error();
> > +         }
> > +         return result;
> > 
> > instead of cut'n'paste do
> > return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where
> > checkError would do the error checking boilerplate
> 
> ok

I still see explicit if(!...) thow Error code in new patches.
> 
> > I'm not sure what Unix-style open is, your array of opens is more confusing
> > than helpful. 
> 
> Well, that was the result of a design discussion with Dietrich, Paul
> O'Shannessy and Drew Willcoxon on the topic of OS.File API a few months ago,
> plus a more recent discussion with Jason Orendorff on the topic of
> OS-specific APIs.
> 
> One of the main ideas is to avoid "one size doesn't quite fit anybody" APIs
> such as the file opening API of nspr/xpcom that require Unix-specific
> options even on Windows. Rather, we offer a set of APIs that cover the most
> common cases that can be made portable without performance penalty (that's
> "open", "truncate", "truncateC", ...), plus OS-specific APIs (prefixed by
> "unix" or "win"), to handle the myriad of cases in which no portable
> solution can be envisioned.
> 
> Requires more documentation, of course.

I think this way insanity lies. I think we should do what common apis(python, fopen) do and specify file mode as a string. permissions can be an optional argument.

I will strongly r- code with a bunch of similar looking open/create/etc functions. Having lots of really similar, but slightly different functions will increase the maintenance burden.


> 
> > As I said before:instead of implementing everything, only implement
> > functions as needed. 
> > 
> > Too much stuff here
> 
> I fully agree with the general idea. As I mentioned, for the moment, I
> cannot do this until I have a clear idea of what is needed, so I am
> experimenting with nice API design, in the hope of obtaining something that
> I can show to potential users and get some feedback. It was my understanding
> that this was also something that you wanted.
> 
> As I said before, if you could point me towards what is needed, that would
> be great. As I mentioned, for the moment, the only scenarios I have seen
> that do not involve a full API, both provided by Dietrich, are:
> 1/ read a full file to a string;
> 2/ copy a file.
> 
> As I told you, scenario 1 is not really possible in the current state of
> js-ctypes, but I plan to work on patches to make it possible (bug 552551).
> 
> So, if you prefer, as I wrote above, I can throw away all that code for the
> moment (or move it to another bug) and just provide file copy.
> 
> What do you think, Taras?

I don't understand why you can't do 1 atm.
Comment 22 (dormant account) 2012-05-02 15:23:24 PDT
Comment on attachment 619068 [details] [diff] [review]
Windows front-end

as i mentioned, too many variants of same function, error handling needs to be factored out.
Comment 23 (dormant account) 2012-05-02 15:28:16 PDT
Comment on attachment 619520 [details] [diff] [review]
Unix front-end

catch_negative -> throw_on_negative

As I mentioned elsewhere seek/read/open/etc functions need to be unified. It's it's up to you where you put the seek, etc constants.

If something is unclear, please chat with me before posting a revised patch.
Comment 24 David Teller [:Yoric] (please use "needinfo") 2012-05-02 15:53:05 PDT
(In reply to Taras Glek (:taras) from comment #21)
> > What do you think, Taras?
> 
> I don't understand why you can't do 1 atm.

Let's discuss this on irc. Essentially, two reasons:
- no support for encodings other than utf-8;
- no support for improperly encoded strings (i.e. typically what you get from one |read| operation).
Comment 25 David Teller [:Yoric] (please use "needinfo") 2012-05-03 02:36:03 PDT
(In reply to Taras Glek (:taras) from comment #23)
> Comment on attachment 619520 [details] [diff] [review]
> Unix front-end
> 
> catch_negative -> throw_on_negative

You are right, that's clearer.

> I think this way insanity lies. I think we should do what common apis(python,
> fopen) do and specify file mode as a string. permissions can be an optional
> argument.

[...]

Permissions are not an issue. Well, besides the fact that they are very much not portable between Unix and Windows, which I solve with the |options| object.

> As I mentioned elsewhere seek/read/open/etc functions need to be unified.
> It's it's up to you where you put the seek, etc constants.

[...]

> I will strongly r- code with a bunch of similar looking open/create/etc 
> functions. Having lots of really similar, but slightly different functions will
> increase the maintenance burden.

Ok. We are trading a little robustness in the API for a little robustness in the implementation, but this remains reasonable.


> If something is unclear, please chat with me before posting a revised patch.

Just to clarify: I posted the revised patch just to have a more readable base for discussion.
Comment 26 David Teller [:Yoric] (please use "needinfo") 2012-05-11 08:41:52 PDT
Created attachment 623157 [details] [diff] [review]
Windows front-end
Comment 27 David Teller [:Yoric] (please use "needinfo") 2012-05-11 08:42:33 PDT
Created attachment 623158 [details] [diff] [review]
Additional libc constants, for BSD copy
Comment 28 David Teller [:Yoric] (please use "needinfo") 2012-05-11 08:43:34 PDT
Created attachment 623159 [details] [diff] [review]
Unix front-end
Comment 29 David Teller [:Yoric] (please use "needinfo") 2012-05-11 08:43:57 PDT
Created attachment 623160 [details] [diff] [review]
Companion testsuite
Comment 30 David Teller [:Yoric] (please use "needinfo") 2012-05-15 06:16:25 PDT
Created attachment 624013 [details] [diff] [review]
Companion testsuite
Comment 31 David Teller [:Yoric] (please use "needinfo") 2012-05-15 06:16:55 PDT
Created attachment 624014 [details] [diff] [review]
Unix front-end
Comment 32 David Teller [:Yoric] (please use "needinfo") 2012-05-15 06:17:31 PDT
Created attachment 624015 [details] [diff] [review]
Windows front-end
Comment 33 (dormant account) 2012-05-15 23:43:15 PDT
Comment on attachment 624015 [details] [diff] [review]
Windows front-end

+         if (options && options.from) {
That's wrong when from is 0. .hasOwnProperty('from') or typeof options.from =='int'

+           throw_on_zero(
+             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
+           );
+           this.setPosition(pos);
+         } else {
+           throw_on_zero(
+             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
+           );

This is a weird api...read() should always set the file position after what was read in.

same with writeI)...especially write
write has same problems

I'm still not sure what the point of winOpen is.

+         case undefined:
+         case 'r':

add // fall through
when falling through a case on purpose

there is no default handler when you pass in a mode that cant be parsed..it should throw an exception. 

I also doubt that passing modes as a string is the right thing to do(even if i suggested it). This is not C, you can pass an array of [Const.* modes] and skip the parsing step.

+           WinFile.chdir(path); //FIXME: Not implemented ????

GetCurrentDirectory loop seems crazy, should expose MAX_PATH and use that + 1 for file length

Also, allocating on with a +1 buffer increase is crazy inefficient.


I don't see the point of the pump function...even if i were convinced that we need pumping I would say that buffer allocation should be the responsibility of the user(allows more buffer reuse and feature less fragile state)
Comment 34 (dormant account) 2012-05-15 23:43:51 PDT
Comment on attachment 624014 [details] [diff] [review]
Unix front-end

Lets get one platform r+ed and then we can r+ the other...I prefer to review windows to start with.
Comment 35 (dormant account) 2012-05-15 23:45:46 PDT
Comment on attachment 624013 [details] [diff] [review]
Companion testsuite

This is useful for reference, but i don't need to review this until we have r+ed the api.
Comment 36 David Teller [:Yoric] (please use "needinfo") 2012-05-16 00:56:25 PDT
(In reply to Taras Glek (:taras) from comment #33)
> Comment on attachment 624015 [details] [diff] [review]
> Windows front-end
> 
> +         if (options && options.from) {
> That's wrong when from is 0. .hasOwnProperty('from') or typeof options.from
> =='int'

Right.

> +           throw_on_zero(
> +             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
> +           );
> +           this.setPosition(pos);
> +         } else {
> +           throw_on_zero(
> +             WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null)
> +           );
> 
> This is a weird api...read() should always set the file position after what
> was read in.
>
> 
> same with writeI)...especially write
> write has same problems

I was attempting to provide |pread|/|pwrite|-like functionality. If this is useless, I can drop that.

> I'm still not sure what the point of winOpen is.

Well, the idea is to let users access a lower-level API, by providing a function that can be used exactly as the Windows API.

Typical use would look like

  let file;
  if (OS.Win) {
    file = OS.Win.winOpen(/*my exact args, I know what I am doing*/)
  } else if (OS.Unix) {
    file = OS.Unix.unixOpen(/*my exact args, I know what I am doing*/)
  }
  // ...

Now, since I have added options winShare, winSecurity, etc., we can decide that this feature is redundant.

> 
> +         case undefined:
> +         case 'r':
> 
> add // fall through
> when falling through a case on purpose
> 
> there is no default handler when you pass in a mode that cant be parsed..it
> should throw an exception.

Good catch. In the Unix version, it throws a |TypeError|, but I obviously forgot to put this in the Windows version.

> I also doubt that passing modes as a string is the right thing to do(even if
> i suggested it). This is not C, you can pass an array of [Const.* modes] and
> skip the parsing step.

Mmmhh... Ok. Not the actual |Const| modes (these wouldn't be portable), but I can do something. Note that this will end up much more verbose:

  OS.File.open("foo", [OS.File.open.APPEND, OS.File.open.RW, OS.File.open.CREATE])

or something such. This will not really simplify the parsing step (we still need argument validation), but it might be easier on users.

We might also, more or less equivalently, introduce something along the lines of

  OS.File.open("foo", {append: true, rw: true, create: true})

A little easier on the eyes, but it is also easier to introduce a typo.

> 
> +           WinFile.chdir(path); //FIXME: Not implemented ????

Thanks.

> 
> GetCurrentDirectory loop seems crazy, should expose MAX_PATH and use that +
> 1 for file length

Unfortunately, under Windows, MAX_PATH is *not* the maximal length for a path. The maximal length for a path is "approximately" (sic) 32768 (doc is fuzzy, but it seems that this can be 32768 + the name of the drive or share), while the value of MAX_PATH, iirc, is 4095. I do not think that we want to allocate 32kb each time we request a path, so we have to try and be smarter.

> Also, allocating on with a +1 buffer increase is crazy inefficient.

Good thing I am not doing it, then :)
I am allocating with the value returned by |GetCurrentDirectory|, which is the required size of the buffer.

Rewriting the algorithm to make this clearer.

> I don't see the point of the pump function...even if i were convinced that
> we need pumping I would say that buffer allocation should be the
> responsibility of the user(allows more buffer reuse and feature less fragile
> state)

In that case, let's remove it. The Linux version needs it to implement copy (using splice), but we can probably do without on other platforms.
Comment 37 David Teller [:Yoric] (please use "needinfo") 2012-05-16 01:45:41 PDT
Created attachment 624323 [details] [diff] [review]
Windows front-end

Attaching an updated patch. Removed winOpen and pump, fixed option.from in read/write, added some documentation on read/write, changed handling of modes in open, clarified get current directory.
Comment 38 (dormant account) 2012-05-16 10:29:02 PDT
Comment on attachment 624323 [details] [diff] [review]
Windows front-end

+
+     let gBytesRead = new ctypes.int32_t(-1);
+     let gBytesReadPtr = gBytesRead.address();
+     let gBytesWritten = new ctypes.int32_t(-1);
+     let gBytesWrittenPtr = gBytesWritten.address();

these should be 'private' with _ prefix, right? I don't see a reason to expose these.
> 
> I was attempting to provide |pread|/|pwrite|-like functionality. If this is
> useless, I can drop that.
> 

drop that

> > I'm still not sure what the point of winOpen is.
> 
> Well, the idea is to let users access a lower-level API, by providing a
> function that can be used exactly as the Windows API.
> 

Usually you let apps use a native open..and provide functionality to import the fd..we can add that later when we actually need it


> We might also, more or less equivalently, introduce something along the
> lines of
> 
>   OS.File.open("foo", {append: true, rw: true, create: true})
> 
> A little easier on the eyes, but it is also easier to introduce a typo.
> 

Lets do that, lets also add validation such that if a bonus/unexpected options argument is passed, we throw an exception. We should do the options validation in a followup

pump didn't get removed
Comment 39 David Teller [:Yoric] (please use "needinfo") 2012-05-16 12:41:04 PDT
(In reply to Taras Glek (:taras) from comment #38)
> Comment on attachment 624323 [details] [diff] [review]
> Windows front-end
> 
> +
> +     let gBytesRead = new ctypes.int32_t(-1);
> +     let gBytesReadPtr = gBytesRead.address();
> +     let gBytesWritten = new ctypes.int32_t(-1);
> +     let gBytesWrittenPtr = gBytesWritten.address();
> 
> these should be 'private' with _ prefix, right? I don't see a reason to
> expose these.

They are actually private, just as closure-local variables, rather than as per-file fields. I am taking advantage of the fact that two concurrent read/write cannot take place concurrently to save (a little) memory and (a little) time.

> > I was attempting to provide |pread|/|pwrite|-like functionality. If this is
> > useless, I can drop that.
> > 
> 
> drop that

Done.

> > > I'm still not sure what the point of winOpen is.
> > 
> > Well, the idea is to let users access a lower-level API, by providing a
> > function that can be used exactly as the Windows API.
> > 
> 
> Usually you let apps use a native open..and provide functionality to import
> the fd..we can add that later when we actually need it

Done.

> > We might also, more or less equivalently, introduce something along the
> > lines of
> > 
> >   OS.File.open("foo", {append: true, rw: true, create: true})
> > 
> > A little easier on the eyes, but it is also easier to introduce a typo.
> > 
> 
> Lets do that, lets also add validation such that if a bonus/unexpected
> options argument is passed, we throw an exception. We should do the options
> validation in a followup

Let me attach a version that uses an object, with some option validation.

> pump didn't get removed

Oops. Done.
Comment 40 David Teller [:Yoric] (please use "needinfo") 2012-05-16 12:43:51 PDT
Created attachment 624495 [details] [diff] [review]
Windows front-end

Thanks for the quick review. Here is a new (untested) version.
Comment 41 (dormant account) 2012-05-16 21:50:19 PDT
Comment on attachment 624495 [details] [diff] [review]
Windows front-end

This seems ok
Comment 42 David Teller [:Yoric] (please use "needinfo") 2012-05-18 06:56:52 PDT
Created attachment 625091 [details] [diff] [review]
Companion testsuite
Comment 43 David Teller [:Yoric] (please use "needinfo") 2012-05-18 06:58:22 PDT
Created attachment 625092 [details] [diff] [review]
Windows front-end
Comment 44 David Teller [:Yoric] (please use "needinfo") 2012-05-18 07:01:41 PDT
I have updated the Windows and Unix front-ends to put them at the same level of features. Along the way, I have found a few issues in the Windows front-end, so if you want a re-review, here is the update:
- more documentation;
- removed some debugging code;
- added two |because*| getters to determine error reasons;
- a few |let| => |const|;
- added a mode flag |existing| for |open|, to open the file only if it exists;
- reworked the code that handles mode for |open| into something a little less ad-hoc;
- handled "truncate an existing file", as per your earlier recommendations.
Comment 45 David Teller [:Yoric] (please use "needinfo") 2012-05-18 08:44:42 PDT
Created attachment 625124 [details] [diff] [review]
Companion testsuite
Comment 46 David Teller [:Yoric] (please use "needinfo") 2012-05-18 08:45:07 PDT
Created attachment 625125 [details] [diff] [review]
Front-end glue
Comment 47 David Teller [:Yoric] (please use "needinfo") 2012-05-18 08:45:36 PDT
Created attachment 625126 [details] [diff] [review]
Additional windows constants, for error-checking
Comment 48 David Teller [:Yoric] (please use "needinfo") 2012-05-18 08:46:20 PDT
Created attachment 625127 [details] [diff] [review]
Unix front-end
Comment 49 David Teller [:Yoric] (please use "needinfo") 2012-05-23 08:06:33 PDT
Created attachment 626446 [details] [diff] [review]
Companion testsuite
Comment 50 David Teller [:Yoric] (please use "needinfo") 2012-05-23 08:07:07 PDT
Created attachment 626448 [details] [diff] [review]
Additional libc constants, for BSD copy
Comment 51 David Teller [:Yoric] (please use "needinfo") 2012-05-23 08:07:57 PDT
Created attachment 626449 [details] [diff] [review]
Windows front-end
Comment 52 David Teller [:Yoric] (please use "needinfo") 2012-05-23 08:08:29 PDT
Created attachment 626450 [details] [diff] [review]
Unix front-end
Comment 53 David Teller [:Yoric] (please use "needinfo") 2012-05-23 08:08:54 PDT
Created attachment 626451 [details] [diff] [review]
Additional windows constants, for error-checking
Comment 54 David Teller [:Yoric] (please use "needinfo") 2012-05-23 08:09:41 PDT
Created attachment 626452 [details] [diff] [review]
Front-end glue
Comment 55 (dormant account) 2012-05-23 15:11:33 PDT
Comment on attachment 626449 [details] [diff] [review]
Windows front-end

+           case "existing":
+             existing = true;
+           default:


missing break

What's with because in the property names? Seems like they would be just as good without, no?
Comment 56 (dormant account) 2012-05-23 15:22:22 PDT
Comment on attachment 626450 [details] [diff] [review]
Unix front-end

+           case "existing":
+             existing = true;
+           default:
+             throw new TypeError("Mode " + key + " not understood");
+           }
+         }

Same bug as in windows frontend. This leads me to believe that you should factor out the open argument parsing and share it.(post a followup patch to the windows code so dont have to rereview the whole thing)

I image it would take mode and return an object with members for 
+         let read = false;
+         let write = false;
+         let trunc = false;
+         let create = false;
+         let existing = false;

is the pump fallback useful on any platform in current code?

splice can fail even if it is available:
  EINVAL Target file system doesn't support splicing; target file is opened in append mode; neither of the descriptors refers to a pipe; or offset given for nonseekable device.


We need to work on filesystems too lazy to implement splice(fallback on sendfile?)

Also I'm ok with landing code that only works within a single (decent) filesystem and them applying fallbacks as a followup(ie for move, copy, splice)
Comment 57 David Teller [:Yoric] (please use "needinfo") 2012-05-24 01:38:35 PDT
(In reply to Taras Glek (:taras) from comment #55)
> Comment on attachment 626449 [details] [diff] [review]
> Windows front-end
> 
> +           case "existing":
> +             existing = true;
> +           default:
> 
> 
> missing break

Thanks.

> What's with because in the property names? Seems like they would be just as
> good without, no?

Sorry, I don't understand the question.
Comment 58 David Teller [:Yoric] (please use "needinfo") 2012-05-24 01:49:39 PDT
(In reply to Taras Glek (:taras) from comment #56)
> Comment on attachment 626450 [details] [diff] [review]
> Unix front-end
> 
> +           case "existing":
> +             existing = true;
> +           default:
> +             throw new TypeError("Mode " + key + " not understood");
> +           }
> +         }
> 
> Same bug as in windows frontend. This leads me to believe that you should
> factor out the open argument parsing and share it.(post a followup patch to
> the windows code so dont have to rereview the whole thing)
>
> 
> I image it would take mode and return an object with members for 
> +         let read = false;
> +         let write = false;
> +         let trunc = false;
> +         let create = false;
> +         let existing = false;
>

Ok, I'll find a way to factor this out.
 
> is the pump fallback useful on any platform in current code?

I do not think it is necessary for any tier one platform. I included this because I am pretty sure that there are some Unices that implement neither |splice| nor |copyfile|, and I wanted to have a fallback.

> splice can fail even if it is available:
>   EINVAL Target file system doesn't support splicing; target file is opened
> in append mode; neither of the descriptors refers to a pipe; or offset given
> for nonseekable device.
> 
> 
> We need to work on filesystems too lazy to implement splice(fallback on
> sendfile?)

Should work on Linux, but I'm not 100% confident that it will work on other OSes. For instance, on BSD, I believe that |sendfile| is only for sockets.

> Also I'm ok with landing code that only works within a single (decent)
> filesystem and them applying fallbacks as a followup(ie for move, copy,
> splice)

Ok.
Comment 59 David Teller [:Yoric] (please use "needinfo") 2012-05-24 03:38:25 PDT
Created attachment 626749 [details] [diff] [review]
Windows front-end, continued

Is this what you had in mind?
Comment 60 David Teller [:Yoric] (please use "needinfo") 2012-05-24 03:38:53 PDT
Comment on attachment 626449 [details] [diff] [review]
Windows front-end

I'll ask dougt to review this once we have a final version.
Comment 61 (dormant account) 2012-05-29 08:46:49 PDT
Comment on attachment 626749 [details] [diff] [review]
Windows front-end, continued

I would like to get rid of discrete variables in this.
+       let read = false;
+       let write = false;
+       let trunc = false;
+       let create = false;
+       let existing = false;
->
let ret = {
read:false, write:false...
}
+         case "truncate":
+           trunc = true;
+           write = true;
->
ret.trunk -> true
...
return ret
  let {
+           read, write, trunc, create, existing
+         } = OS.Shared._aux.normalizeOpenMode(options);

let mode = OS.Shared._aux.normalizeOpenMode(options);

but otherwise looks good
Comment 62 David Teller [:Yoric] (please use "needinfo") 2012-05-30 02:50:06 PDT
Created attachment 628283 [details] [diff] [review]
Windows front-end, part 3

Here we go. I will fold the patches a little differently once I have your approval, Taras.
Comment 63 David Teller [:Yoric] (please use "needinfo") 2012-05-30 02:54:49 PDT
Created attachment 628285 [details] [diff] [review]
Unix front-end, part 2

Applying requested changes to Unix front-end, too. Again, I will do some re-folding somewhere along the way.
Comment 64 David Teller [:Yoric] (please use "needinfo") 2012-05-30 02:55:45 PDT
Created attachment 628286 [details] [diff] [review]
Scope-based resource control

This function proved useful both for part 2 of the Unix front-end and for the ongoing work on thumbnails.
Comment 65 (dormant account) 2012-05-30 10:59:02 PDT
Comment on attachment 628286 [details] [diff] [review]
Scope-based resource control

10:56 <@taras> and i think instead of having magic cleanup function
10:56 <@taras> and passing resources
10:56 <@taras> you should just pass in 2 functions
10:56 <@taras> cleanup, action
10:57 <@taras> otherwise too much magic is happening in cleanup(), afaik
10:57 < Yoric> Well, I have the feeling that the common scenario is either:
10:57 < Yoric> - closing one file;
10:57 < Yoric> - or closing many files.
10:57 <@taras> so you make a function called try_finally
10:57 <@taras> and try_finally_fd
10:57 <@taras> for the common scenario
10:57 < Yoric> ok
10:57 <@taras> and try_finally_fsls for the other one
10:58 <@taras> err
10:58 <@taras> try_finally_fdls

it's not obvious what using does from name. I propose try_finally
Comment 66 David Teller [:Yoric] (please use "needinfo") 2012-05-31 07:30:28 PDT
Created attachment 628727 [details] [diff] [review]
Unix front-end

Unix front-end, consolidated in one patch.
Comment 67 David Teller [:Yoric] (please use "needinfo") 2012-05-31 07:35:39 PDT
Created attachment 628730 [details] [diff] [review]
Shared code

Shared code, extracted from its patch.
Comment 68 David Teller [:Yoric] (please use "needinfo") 2012-05-31 07:39:30 PDT
Created attachment 628734 [details] [diff] [review]
Windows front-end

Windows front-end, consolidated in one patch.
Comment 69 David Teller [:Yoric] (please use "needinfo") 2012-05-31 07:40:35 PDT
Comment on attachment 626452 [details] [diff] [review]
Front-end glue

And a simpler review to rest a little bit after the complex stuff :)
Comment 70 Doug Turner (:dougt) 2012-06-04 14:22:25 PDT
Comment on attachment 628734 [details] [diff] [review]
Windows front-end

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

on mac and unix, paths use '/' as the file node seperator.  on windows, it is '\'.  Suppose I need to use this API, and say -- want to delete a file that exists at $HOME/doomtext.  How do we do that in an cross platform way?

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +42,5 @@
> +     // and we use the following global mutable values:
> +     let gBytesRead = new ctypes.int32_t(-1);
> +     let gBytesReadPtr = gBytesRead.address();
> +     let gBytesWritten = new ctypes.int32_t(-1);
> +     let gBytesWrittenPtr = gBytesWritten.address();

these are no more global than WinFile declared above, right?  These are basically instance member variables.  You probably should drop the 'g' prefix.  Maybe use 'm' instead?

@@ +56,5 @@
> +      */
> +     let File = function File(fd) {
> +       this._fd = fd;
> +     };
> +     File.prototype = {

add newline whitespace before File.prototype.

@@ +69,5 @@
> +       // Placeholder getter, used to replace |get fd| once
> +       // the file is closed.
> +       _nofd: function nofd(operation) {
> +         operation = operation ||
> +             this._nofd.caller.name ||

This is different than the unix one.  You added this._nofd.caller.name here.  Which is correct?

@@ +88,5 @@
> +       close: function close() {
> +         if (this._fd) {
> +           let fd = this._fd;
> +           this._fd = null;
> +           delete this.fd;

why the explict delete here?

@@ +118,5 @@
> +        * the end of the file has been reached.
> +        * @throws {OS.File.Error} In case of I/O error.
> +        */
> +       read: function read(buffer, nbytes, options) {
> +         // |gBytesReadPtr| is a pointer to |gBytesRead|.

useless comment.

@@ +175,5 @@
> +         // In this implementation,
> +         // OS.File.POS_START == OS.Constants.Win.FILE_BEGIN
> +         // OS.File.POS_CURRENT == OS.Constants.Win.FILE_CURRENT
> +         // OS.File.POS_END == OS.Constants.Win.FILE_END
> +         whence = (whence == undefined)?Const.FILE_BEGIN:whence;

spaces between operators

@@ +322,5 @@
> +
> +       let share = options.winShare || DEFAULT_SHARE;
> +       let security = options.winSecurity || null;
> +       let flags = options.winFlags || DEFAULT_FLAGS;
> +       let template = options.winTemplate?options.winTemplate._fd:null;

same - add spaces around ? and :
Comment 71 Doug Turner (:dougt) 2012-06-04 14:25:12 PDT
Comment on attachment 628727 [details] [diff] [review]
Unix front-end

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

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +397,5 @@
> +       };
> +     } else {
> +       // If the OS does not implement file copying for us, we need to
> +       // implement it ourselves. For this purpose, we need to define
> +       // a pumping function.

What platform would this code run on?  What platforms do not have copyfile(3)?
Comment 72 David Teller [:Yoric] (please use "needinfo") 2012-06-07 08:12:55 PDT
Created attachment 630983 [details] [diff] [review]
Windows front-end
Comment 73 David Teller [:Yoric] (please use "needinfo") 2012-06-07 08:14:32 PDT
Created attachment 630986 [details] [diff] [review]
Unix front-end

Fixing a few typoes.
Comment 74 Doug Turner (:dougt) 2012-06-11 09:15:18 PDT
Comment on attachment 630983 [details] [diff] [review]
Windows front-end

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

r with those changes.

::: toolkit/components/osfile/Makefile.in
@@ +26,5 @@
>  	$(NSINSTALL) $(srcdir)/osfile_shared.jsm $(FINAL_TARGET)/modules/osfile
>  	$(NSINSTALL) $(srcdir)/osfile_unix_back.jsm $(FINAL_TARGET)/modules/osfile
>  	$(NSINSTALL) $(srcdir)/osfile_unix_front.jsm $(FINAL_TARGET)/modules/osfile
>  	$(NSINSTALL) $(srcdir)/osfile_win_back.jsm $(FINAL_TARGET)/modules/osfile
> +	$(NSINSTALL) $(srcdir)/osfile_win_front.jsm $(FINAL_TARGET)/modules/osfile
\ No newline at end of file

\ No newline at end of file

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +333,5 @@
> +                 ||(!("winAccess" in options) && "winDisposition" in options)) {
> +         throw new TypeError("OS.File.open requires either both options " +
> +           "winAccess and winDisposition or neither");
> +       } else {
> +         mode = OS.Shared._aux.normalizeOpenMode(mode);

Declare 'mode' above with the rest.
Comment 75 Doug Turner (:dougt) 2012-06-11 09:18:26 PDT
Comment on attachment 630986 [details] [diff] [review]
Unix front-end

I am unsure where UnixFile.copyfile isn't going to be defined.  Why do we need this code?
Comment 76 David Teller [:Yoric] (please use "needinfo") 2012-06-11 10:09:36 PDT
(In reply to Doug Turner (:dougt) from comment #75)
> Comment on attachment 630986 [details] [diff] [review]
> Unix front-end
> 
> I am unsure where UnixFile.copyfile isn't going to be defined.  Why do we
> need this code?

UnixFile.copyfile is only defined under BSD/MacOS X. For all other platforms, we need to reimplement it.
Comment 77 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:24:13 PDT
(In reply to Doug Turner (:dougt) from comment #74)
> Comment on attachment 630983 [details] [diff] [review]
> Windows front-end
> 
> Review of attachment 630983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r with those changes.
> 
> ::: toolkit/components/osfile/Makefile.in
> 
> \ No newline at end of file

Done.

> 
> ::: toolkit/components/osfile/osfile_win_front.jsm
> @@ +333,5 @@
> > +                 ||(!("winAccess" in options) && "winDisposition" in options)) {
> > +         throw new TypeError("OS.File.open requires either both options " +
> > +           "winAccess and winDisposition or neither");
> > +       } else {
> > +         mode = OS.Shared._aux.normalizeOpenMode(mode);
> 
> Declare 'mode' above with the rest.

Are you sure? I am just normalizing an argument. Declaring it might make code a little messier.
Comment 78 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:39:05 PDT
Created attachment 632570 [details] [diff] [review]
Shared code
Comment 79 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:39:29 PDT
Created attachment 632571 [details] [diff] [review]
Front-end glue
Comment 80 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:39:53 PDT
Created attachment 632572 [details] [diff] [review]
Additional windows constants, for error-checking
Comment 81 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:40:19 PDT
Created attachment 632573 [details] [diff] [review]
Windows front-end
Comment 82 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:40:47 PDT
Created attachment 632574 [details] [diff] [review]
Additional libc constants, for BSD copy
Comment 83 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:41:17 PDT
Created attachment 632575 [details] [diff] [review]
Unix front-end
Comment 84 David Teller [:Yoric] (please use "needinfo") 2012-06-12 23:57:39 PDT
Created attachment 632580 [details] [diff] [review]
Companion testsuite
Comment 85 David Teller [:Yoric] (please use "needinfo") 2012-06-14 01:26:16 PDT
Ahaha, everything is reviewed.
I am tracking down an intermittent orange. I will land this once I have found it.
Comment 86 David Teller [:Yoric] (please use "needinfo") 2012-06-15 02:29:43 PDT
Created attachment 633441 [details] [diff] [review]
Companion testsuite
Comment 87 David Teller [:Yoric] (please use "needinfo") 2012-06-15 02:30:24 PDT
Created attachment 633442 [details] [diff] [review]
Shared code
Comment 88 David Teller [:Yoric] (please use "needinfo") 2012-06-15 02:32:03 PDT
Created attachment 633443 [details] [diff] [review]
Front-end glue
Comment 89 David Teller [:Yoric] (please use "needinfo") 2012-06-15 02:38:56 PDT
Created attachment 633444 [details] [diff] [review]
Unix front-end
Comment 90 David Teller [:Yoric] (please use "needinfo") 2012-06-15 02:40:27 PDT
Created attachment 633445 [details] [diff] [review]
Windows front-end
Comment 91 Tim Taubert [:ttaubert] 2012-06-15 08:21:16 PDT
Will land.
Comment 94 Landry Breuil (:gaston) 2012-06-17 01:53:28 PDT
re https://hg.mozilla.org/mozilla-central/rev/9e4b5966a2e8, i'm afraid the copyfile API is OSX only. There's no copyfile.h on OpenBSD, and after a bit of googling there doesnt seem to be one on FreeBSD either.

I'm pretty sure it should be included only in the XP_MACOSX case..
Comment 95 Landry Breuil (:gaston) 2012-06-17 02:13:02 PDT
from http://www.manpagez.com/man/3/copyfile/

HISTORY
     The copyfile() API was introduced in Mac OS X 10.5.
Comment 96 Landry Breuil (:gaston) 2012-06-17 05:51:08 PDT
Created attachment 633892 [details] [diff] [review]
Only include copyfile.h on mac

Only include copyfile.h on mac, tests passes fine here now :

/usr/obj/m-c/ $TEST_PATH=toolkit/components/osfile/tests/mochi/ gmake mochitest-chrome
...
201 INFO Passed: 195
202 INFO Failed: 0
...
Comment 97 David Teller [:Yoric] (please use "needinfo") 2012-06-17 07:54:13 PDT
Thanks for the quick patch, gaston.
Comment 98 Landry Breuil (:gaston) 2012-06-17 08:53:16 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/b9981690ede0 (and indeed, i've put the wrong bug # in the commit message... DOH!)
Comment 99 Ed Morley [:emorley] 2012-06-18 07:37:39 PDT
https://hg.mozilla.org/mozilla-central/rev/b9981690ede0

Landry, I don't suppose you could put a note in your calendar to add a reference pointing to here in bug 767876 once it exists? Would just help hg blame :-)
Comment 100 Landry Breuil (:gaston) 2012-06-26 05:38:34 PDT
(In reply to Ed Morley [:edmorley] from comment #99)
> https://hg.mozilla.org/mozilla-central/rev/b9981690ede0
> 
> Landry, I don't suppose you could put a note in your calendar to add a
> reference pointing to here in bug 767876 once it exists? Would just help hg
> blame :-)

I'd have if i could.. but the now-existing bug 767876 is 'access denied' for everyone apparently :(

Note You need to log in before you can comment on or make changes to this bug.