771617 - [OS.File] readAll

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, enhancement)

Description

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

Implement a simple method that can read a whole file.

Comment 1

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

Let's WONTFIX this for the moment.

Comment 2

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

Turns out that I need it for proper testing, so opening back.

Comment 3

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

Attachment: Refactoring to better share code between implementations of OS.File

Firstly, refactoring code.

Comment 4

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

Attachment: readAll/writeAll

Comment 5

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

Attachment: Companion testsuite

Comment 6

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

I should add that the idea now is not to read a whole file but to repeat |read|/|write| as many times as necessary until we are certain that all the bytes that could be read have been read.

While |read| and |readAll| generally do the exact same thing on a disk, experience shows that this is not always the case with TryServer. Not sure why, maybe an exotic file system, faulty disks, NFS, or something such.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 647160 [details] [diff] [review]
Refactoring to better share code between implementations of OS.File

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

Looks like everything got moved around to the correct places.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 647161 [details] [diff] [review]
readAll/writeAll

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

Full marks for the implementation.  I think the names and the interface could use some work, though.

For instance, look at your test code.  I don't really want to have to repeat all that stuff every time I want to read a file.  I just want to:

let f = OS.File.open(...);
let contents = OS.File.read(f);

Done.  Doesn't matter how big the file is.  Or maybe I only wanted the first identifying bytes in the file:

let contents = OS.File.read(f, 4);

Reading into a previously allocated buffer should be a separate |readInto| function to avoid overloading |read| too much.

OK, so this conflicts with current OS.File usage.  And we want to leave open the possibility of people who really really want to deal with all the system-level details.

Off-the-top-of-my-head proposal:

- Rename current OS.File.{read,write} to OS.File.sys{read,write}.  Makes it more clear these functions have system level behavior.
- read and write should, by default, loop calling sys{read,write} as appropriate to read/write requested sizes.
- read should do intelligent things by allocating ArrayBuffers when appropriate.
- readInto should introspect passed ArrayBuffers and array-typed things; explicit sizes required for pointers, optional for ArrayBuffers etc.

WDYT?  (I realize this is not quite parallel with the string encoding stuff elsewhere.  We should sort that out.)

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 647162 [details] [diff] [review]
Companion testsuite

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

No problems with this code, but the API may be revised somewhat.  It may be appropriate to add more tests based on the revised API, in which case I'd like to see this again.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +180,5 @@
>    compare_files("test_read_write_file", src_file_name, tmp_file_name);
>    OS.File.remove(tmp_file_name);
>  }
>  
> +function test_readall_writeall_file()

Looks like you forgot to add a call to this somewhere.

Comment 10

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

(In reply to Nathan Froyd (:froydnj) from comment #8)
> Comment on attachment 647161 [details] [diff] [review]
> readAll/writeAll
> 
> Review of attachment 647161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Full marks for the implementation.  I think the names and the interface
> could use some work, though.
> 
> For instance, look at your test code.  I don't really want to have to repeat
> all that stuff every time I want to read a file.  I just want to:
> 
> let f = OS.File.open(...);
> let contents = OS.File.read(f);
> 
> Done.  Doesn't matter how big the file is.  Or maybe I only wanted the first
> identifying bytes in the file:
> 
> let contents = OS.File.read(f, 4);
> 
> Reading into a previously allocated buffer should be a separate |readInto|
> function to avoid overloading |read| too much.
> 
> OK, so this conflicts with current OS.File usage.  And we want to leave open
> the possibility of people who really really want to deal with all the
> system-level details.

Well, this kind of system-level API was essentially how Taras wanted OS.File to work, so I have adapted :)

> 
> Off-the-top-of-my-head proposal:
> 
> - Rename current OS.File.{read,write} to OS.File.sys{read,write}.  Makes it
> more clear these functions have system level behavior.
> - read and write should, by default, loop calling sys{read,write} as
> appropriate to read/write requested sizes.
> - read should do intelligent things by allocating ArrayBuffers when
> appropriate.
> - readInto should introspect passed ArrayBuffers and array-typed things;
> explicit sizes required for pointers, optional for ArrayBuffers etc.
> 
> WDYT?  (I realize this is not quite parallel with the string encoding stuff
> elsewhere.  We should sort that out.)

I agree with the general idea (I believe that getting a final API would require some preliminary experimentation, so I will not comment on this exact API). However, this does not fit too well with the requirements I had when I started working on OS.File. I believe that this would be more appropriate in another module based on OS.File, perhaps OS.Streams, or OS.FileUtils or something such.

Comment 11

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

Attachment: Companion testsuite

Here is the test file, correctly merged this time. Just one line of difference.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #10)
> > OK, so this conflicts with current OS.File usage.  And we want to leave open
> > the possibility of people who really really want to deal with all the
> > system-level details.
> 
> Well, this kind of system-level API was essentially how Taras wanted OS.File
> to work, so I have adapted :)

Hm, I think that was a misguided idea.  See below.

> I agree with the general idea (I believe that getting a final API would
> require some preliminary experimentation, so I will not comment on this
> exact API). However, this does not fit too well with the requirements I had
> when I started working on OS.File. I believe that this would be more
> appropriate in another module based on OS.File, perhaps OS.Streams, or
> OS.FileUtils or something such.

I don't think putting the low-level details in OS.File and high-level details someplace else is a winning strategy.  My watching-from-the-fringes perception is that OS.File is meant to replace all the suckiness that is doing XPCOM file IO from Javascript.  If OS.File's API is still sucky, I don't think people are going to go digging for OS.RealIO or whatever.  If nothing else, I don't think people would look favorably upon it, since using OS.File would just be exchanging one form of suckiness for another.

And please, don't put the actually usable stuff in OS.FileUtils; if people have to use a *Utils module *all the time* to get their work done, it means that the original module wasn't designed to do useful stuff in the first place.

Now, if you wanted to have OS.SysFile to hold all the pointer-bashing details that OS.File currently has or was envisioned to have, and build OS.File with a nice, usable JS-esque API on top of OS.SysFile, I think that'd be reasonable.

All that being said, I don't want to hold up your work while you redesign OS.File on my review whim.

How about this as a plan: we try to reshape readAll and friends to have a bit more "high-level" feel.  We also open a bug for moving low-level parts from OS.File into someplace else so that OS.File's eventual API is higher level.  That way we/you can experiment with the API, but we don't forget that there's some cleanup to do before OS.File is a pristine jewel of file I/O goodness.  Sound good?

Comment 13

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

(In reply to Nathan Froyd (:froydnj) from comment #12)
>
> Now, if you wanted to have OS.SysFile to hold all the pointer-bashing
> details that OS.File currently has or was envisioned to have, and build
> OS.File with a nice, usable JS-esque API on top of OS.SysFile, I think
> that'd be reasonable.
> 
> All that being said, I don't want to hold up your work while you redesign
> OS.File on my review whim.
> 
> How about this as a plan: we try to reshape readAll and friends to have a
> bit more "high-level" feel.  We also open a bug for moving low-level parts
> from OS.File into someplace else so that OS.File's eventual API is higher
> level.  That way we/you can experiment with the API, but we don't forget
> that there's some cleanup to do before OS.File is a pristine jewel of file
> I/O goodness.  Sound good?

Effectively, I believe that we will eventually want a higher level API on top of the current OS.File. Now, there are several questions that remain open:

1. what should go into this higher level API?
2. should we have one low-level module and one high-level module?
3. should the high-level module be called OS.File?
4. what about experiments?

My personal take is the following:

1. I would like to avoid prematurely going too high level and producing an API that misses its target. Consequently, I intend to first provide all required primitives, then see how OS.File is used and finally base the high level API on actual scenarios. Going too high-level too fast would cut considerably our ability to evolve the API.

2. We can definitely have two modules. I suspect that we will want this eventually. However, I believe that have not reached that stage. For ideas on how to integrate read-and-allocate in the current design of OS.File, see below.

3. This needs to be decided quickly, as a few developers have already filed bugs depending on OS.File. If we keep OS.File with its current content, we could call the future high-level API OS.FileIO, or OS.File.IO, or something such.

4. Toying with future ideas? I like that. Maybe OS.File.Future or OS.Future.File?

Regardless, do not hesitate to open a bug :)


For the case of read-and-allocate, I believe that this is trivial, we just need to introduce:
- ArrayBuffer OS.File.readFull(path, optional ArrayBuffer, options);
- OS.File.writeFull(path, ArrayBuffer | String, options).

Comment 14

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

> All that being said, I don't want to hold up your work while you redesign OS.File on my review whim.

Don't forget to r+ your f+, then :)

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #13)
> 1. I would like to avoid prematurely going too high level and producing an
> API that misses its target. Consequently, I intend to first provide all
> required primitives, then see how OS.File is used and finally base the high
> level API on actual scenarios. Going too high-level too fast would cut
> considerably our ability to evolve the API.

But we already have a good first-order approximation of what a pleasant file access API looks like.  It's called Python.  We don't need to be aggressively reinventing the wheel here.  See also below.

> 3. This needs to be decided quickly, as a few developers have already filed
> bugs depending on OS.File. If we keep OS.File with its current content, we
> could call the future high-level API OS.FileIO, or OS.File.IO, or something
> such.

Having OS.File.read* and (OS.FileIO.read* or OS.File.IO.read*) would be bad.  Why are there two separate "families" of read functions?  Why do we have an IO module but also have IO functions in OS.File--shouldn't those functions be in the IO.module?  etc.  (I realize you can make the same criticism of my OS.File and OS.SysFile proposal; I think the argument in that case is that one is clearly the system-level API and one is not.  Maybe you feel differently.  Or maybe the low-level primitives just shouldn't be exposed at all!)

> For the case of read-and-allocate, I believe that this is trivial, we just
> need to introduce:
> - ArrayBuffer OS.File.readFull(path, optional ArrayBuffer, options);
> - OS.File.writeFull(path, ArrayBuffer | String, options).

Sure, it's trivial to write the functions.  Permit me to indulge in Socratic dialogue.

"Why are these functions called readAll and writeAll?"

"Those functions make sure that they read or write the entire portion of the buffer specified by the call." (NB: subject to EOF conditions in read, obviously.)

"Oh, OK."

"You look puzzled."

"Shouldn't that be the default behavior?"

"The low-level system IO functions don't guarantee that the entire portion of the buffer be read or written in a single call.  So our wrappers around those functions don't make that guarantee either.  And then we have wrappers around those wrappers to provide the behavior that everybody actually wants."

"I see.  So why not call these, um, second-level wrappers read and write then?"

"Those names are taken up by the wrappers around the low-level system IO functions."

"Do I ever want to use those wrapper functions?"

"No, there's all sorts of failure conditions associated with those functions.  And they have unpleasant calling conventions; you need to mess with pointers and things.  So don't use them."

"But you still expose them in the API?"

"Yes."

"And the functions that are associated with the names you'd obviously want to use are actually the functions that you never want to use?"

"Yes."

"Why did the API get designed this way?"

"Because we evolved the API starting with low-level functions and then added higher-level functions as we needed them, rather than thinking about to write a nice API from JavaScript in the first place.  Of course, some of the good names got taken up by low-level functions or higher-level functions that actually turned out to be the wrong thing, so we kept having to come up with new variations on read and write."

"I see."

"It's OK; we have documentation that clearly states which variants are supposed to be used, so developers won't make mistakes."

"Developers read documentation?"

"Well, we can at least tell them that they should have read the documentation when they screw up."

"So those variations just have suffixes that describe the behavior you always wanted anyway, but developers are now forced to type and read repeatedly, even though they effectively have zero semantic content?  And we have an API to document that's twice as big as it should be because we evolved it rather than designing it?"

"When you put it that way...yes."

"..."

FIN

I exaggerate, perhaps.  But not too much.

I agree that you don't want to multiply functions unnecessarily.  Thou Shalt Not Write A Function Beforest Thou Hast A Concrete Use-Case and all that.  But like I said above, we already know (roughly) how to write a good file access API.  We can be a little more efficient in JavaScript because we have mutable strings, but the basic ideas can be borrowed freely from Python.  Or Perl.  Or Ruby.  Or wherever.  As long as it's not C.

Comment 16

[Nathan Froyd [:froydnj]]

I think the tension here is whether OS.File is simply a wrapper around C library stuff, or whether it's meant to be a convenient file API.  My understanding is that it was conceived as the former with some vague "XPCOM file IO sucks, this will be much better" handwaving towards the latter direction; this patch (and, I assume, followon patches) is trying to nudge it in the latter direction.

If we are going to go in the latter direction, then, I think we might as well make some effort to design a convenient API, rather than "this function looks useful, let's add it!"

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #14)
> > All that being said, I don't want to hold up your work while you redesign OS.File on my review whim.
> 
> Don't forget to r+ your f+, then :)

:)  At the very least, I think you need to:

- make the calling interface for readAll and writeAll identical to those of read and write;
- rename readAll to read and writeAll to write.  I am ambivalent about whether the current system-level read and write are exported under other names; I think _read and _write would be appropriate if they are.

That way people get the behavior they expect from read and write.  Not quite as convenient as Python's read(), perhaps, but closer.

I would be more comfortable r+'ing that than the current version.  Still not totally comfortable, but more comfortable.

Comment 17

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

There is effectively a tension.

Let me start with a little context about OS.File.

The reason for which OS.File was commissioned in the first place (by Taras) was not to make something more convenient than nsIFile but to make something more I/O-efficient. As it turns out, one of the two reason for which nsIFile is inefficient in the first place is that it went high-level too fast, and in the wrong direction. The stated goal was effectively to make a low-level wrapper on top of Posix/WinAPI, and only once that low-level wrapper was complete to start working on something higher-level.

Now, as it turns out, I am doing my best to also make it as convenient as possible. But since this is not the stated objective, I do not have full latitude on this.

Comment 18

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

Now, second part of my reply, on the specific topic of read/readAll/readFull, vs write/writeAll/writeFull (or whatever).

Yes, renaming sounds good, I have just opened bug 780483 about this topic.

I believe that we should go the following way:
- in this bug, introduce readAll/writeAll (hopefully renamed to read/write);
- in a followup bug, introduce OS.File.read/write, a variant that opens, reads, closes, initially with an array argument;
- in a followup bug, make both the length and the array argument of all these functions/methods optional and have all these functions/methods return an ArrayBuffer if necessary.

What do you think of this plan?

btw, last time I checked, JavaScript did not have mutable strings :)

Comment 19

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

Well, by Taras fiat, no renaming for the moment.
I suspect that we will end up with a higher-level File API as part of Jetpack or something such.

Comment 20

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

Attachment: readAll/writeAll, v2

Introducing a new version of readAll/writeAll.

Major changes:
- readAll can now auto-allocate its buffer if no buffer is specified;
- since every option to readAll is optional, well, they all go in an |options| object;
- to keep some homogeneity between allocate and do-not-allocate, the return value now returns both the buffer and the number of bytes read;
- adapted to the new offsetBy.

Comment 21

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

Attachment: Companion testsuite, v2

Considerably expanded the test suite to gp with the implementation.

Comment 22

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

Attachment: readAll/writeAll, v2

Same one, without debugging code.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 651772 [details] [diff] [review]
readAll/writeAll, v2

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

Please make this have a readAll that allocates, and a readInto that doesn't.  I think this makes the API better and eliminates corner cases from the already large amount of checking that you do.  (You can specify 'offset' for the allocating case, for instance.  While it's not obviously wrong, it is a foot-bb-gun, at least.)

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +95,5 @@
> +    if ("offset" in options) {
> +      offset = options.offset;
> +    }
> +
> +    // Determine |pointer| and |buffer|

You should work on refactoring this logic so that readAll and writeAll can use the same logic.  You miss the null check in writeAll, for instance.  Bonus points for refactoring such that the offset checking and the '// Sanity check' logic is shared too.

@@ +109,5 @@
> +      } else if ("byteLength" in options.buffer) {
> +        buffer = options.buffer;
> +        pointer = exports.OS.Shared.Type.uint8_t.out_ptr.implementation(buffer);
> +      } else {
> +        throw new TyepError("Expecting a C pointer or an ArrayBuffer");

"TypeError".  Somebody doesn't run the testsuite and/or doesn't make it complete enough. :)

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 651771 [details] [diff] [review]
Companion testsuite, v2

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

I didn't look at this very closely, but I note that you missed tests for error conditions. :)  Since these tests will undergo some revisions for readAll vs. readInto, I'm going to cancel the review for now.

Comment 25

[Nathan Froyd [:froydnj]]

Oh, I'm also dubious about the exactAllocate option.  Just do the stat()'ing necessary and use ArrayBuffer.slice if you really want to.

Comment 26

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

(In reply to Nathan Froyd (:froydnj) from comment #23)
> Comment on attachment 651772 [details] [diff] [review]
> readAll/writeAll, v2
> 
> Review of attachment 651772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make this have a readAll that allocates, and a readInto that doesn't.
> I think this makes the API better and eliminates corner cases from the
> already large amount of checking that you do.  (You can specify 'offset' for
> the allocating case, for instance.  While it's not obviously wrong, it is a
> foot-bb-gun, at least.)

Good point.

> 
> ::: toolkit/components/osfile/osfile_shared_front.jsm
> @@ +95,5 @@
> > +    if ("offset" in options) {
> > +      offset = options.offset;
> > +    }
> > +
> > +    // Determine |pointer| and |buffer|
> 
> You should work on refactoring this logic so that readAll and writeAll can
> use the same logic.  You miss the null check in writeAll, for instance. 
> Bonus points for refactoring such that the offset checking and the '//
> Sanity check' logic is shared too.

I'll see what I can do.

> 
> @@ +109,5 @@
> > +      } else if ("byteLength" in options.buffer) {
> > +        buffer = options.buffer;
> > +        pointer = exports.OS.Shared.Type.uint8_t.out_ptr.implementation(buffer);
> > +      } else {
> > +        throw new TyepError("Expecting a C pointer or an ArrayBuffer");
> 
> "TypeError".  Somebody doesn't run the testsuite and/or doesn't make it
> complete enough. :)

Oops :)
I admit that my testsuite did not cover type errors.

Comment 27

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

(In reply to Nathan Froyd (:froydnj) from comment #25)
> Oh, I'm also dubious about the exactAllocate option.  Just do the stat()'ing
> necessary and use ArrayBuffer.slice if you really want to.

Ok, let's do that.

Comment 28

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

Attachment: readAll/readTo/writeFrom, v3

Here is a new version with three functions: readAll/readTo/writeFrom.

Comment 30

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

Attachment: Companion testsuite, v3

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 652350 [details] [diff] [review]
readAll/readTo/writeFrom, v3

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

One last round of changes, I think.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +77,5 @@
> +   * @param {*=} options Optionally, an object that may contain the
> +   * following fields:
> +   * - {number} offset The offset in |buffer| at which to start placing
> +   * data
> +   * - {bool} once If |true|, performs exactly one I/O operation.

Ditch this option, please.  For read and write.

@@ +86,5 @@
> +   */
> +  readTo: function readTo(buffer, bytes, options) {
> +    options = options || noOptions;
> +
> +    // Determine |offset| at which to start writing in |buffer|.

I think you should move the offset handling and the sanity checking into normalizeToPointer.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 652421 [details] [diff] [review]
Companion testsuite, v3

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +174,5 @@
>    while (true) {
>      ok(true, "Position: "+pos);
> +    let chunkSize;
> +    if (prefix != undefined) {
> +      chunkSize = Math.min(4096, prefix - pos);

This doesn't look right.  Let's say prefix is 12, so:

- first time through the loop: chunkSize is 12;
- read 12 bytes;
- pos is now 12;
- second time through the loop: chunkSize is 0.
- er...

So you only compare prefix bytes?  Is that really want you want?

It is entirely possible I'm missing something, but I think you want this calculation to be different (maybe you never call this function with prefix < 4096?).  Perhaps a comment explaining what purpose prefix is supposed to serve would help.

@@ +303,5 @@
> +  dest.close();
> +
> +  compare_files("test_readall_writeall_file (with offset)", src_file_name, tmp_file_name, LEFT);
> +  OS.File.remove(tmp_file_name);
> +

Do you think it's worth doing readTo, C buffer + offset here?

Comment 33

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

(In reply to Nathan Froyd (:froydnj) from comment #32)
> Comment on attachment 652421 [details] [diff] [review]
> Companion testsuite, v3
> 
> Review of attachment 652421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
> @@ +174,5 @@
> >    while (true) {
> >      ok(true, "Position: "+pos);
> > +    let chunkSize;
> > +    if (prefix != undefined) {
> > +      chunkSize = Math.min(4096, prefix - pos);
> 
> This doesn't look right.  Let's say prefix is 12, so:
> 
> - first time through the loop: chunkSize is 12;
> - read 12 bytes;
> - pos is now 12;
> - second time through the loop: chunkSize is 0.
> - er...
> 
> So you only compare prefix bytes?  Is that really want you want?
> 
> It is entirely possible I'm missing something, but I think you want this
> calculation to be different (maybe you never call this function with prefix
> < 4096?).  Perhaps a comment explaining what purpose prefix is supposed to
> serve would help.

Indeed, this is the sole point of prefix.
Ok, ok, I will document it :)

> 
> @@ +303,5 @@
> > +  dest.close();
> > +
> > +  compare_files("test_readall_writeall_file (with offset)", src_file_name, tmp_file_name, LEFT);
> > +  OS.File.remove(tmp_file_name);
> > +
> 
> Do you think it's worth doing readTo, C buffer + offset here?

I can add it.

Comment 34

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

Attachment: Companion testsuite, v4

Comment 35

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

Attachment: Fixing a trivial bug in the Windows implementation

Comment 36

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

Attachment: readAll/readTo/writeFrom, v4

Applied feedback (removed option |once| and corresponding doc, more factoring in normalization), plus a bugfix in |writeFrom| (return value was incorrect).

Comment 37

[Nathan Froyd [:froydnj]]

Comment on attachment 653041 [details] [diff] [review]
readAll/readTo/writeFrom, v4

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

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +148,5 @@
> + *
> + * @return {C pointer} A C pointer of type uint8_t, corresponding to the
> + * start of |candidate| + |offset| bytes.
> + */
> +AbstractFile.normalizeToPointer = function normalizeToPointer(candidate, bytes, offset) {

Thank you!

@@ +151,5 @@
> + */
> +AbstractFile.normalizeToPointer = function normalizeToPointer(candidate, bytes, offset) {
> +  if (!candidate) {
> +    throw new TypeError("Expecting a C pointer or an ArrayBuffer");
> +  }

You don't need this check, you make the same check below.

Comment 38

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

(In reply to Nathan Froyd (:froydnj) from comment #37)
> @@ +151,5 @@
> > + */
> > +AbstractFile.normalizeToPointer = function normalizeToPointer(candidate, bytes, offset) {
> > +  if (!candidate) {
> > +    throw new TypeError("Expecting a C pointer or an ArrayBuffer");
> > +  }
> 
> You don't need this check, you make the same check below.

Well, if I do not check here, I will get a JS error "invalid 'in' operand" which is much less readable. So, I would prefer keeping the check here.

Comment 39

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

Attachment: readAll/readTo/writeFrom, v4

Comment 40

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

Attachment: Fixing a trivial bug in the Windows implementation

Comment 41

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

Attachment: Companion testsuite, v4

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 647160 [details] [diff] [review]
Refactoring to better share code between implementations of OS.File

This is badly bitrotted. Please post an updated patch.

Comment 43

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

Attachment: Refactoring to better share code between implementations of DirectoryIterator

Same one, minus bitrot.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=679ec3757fcf

https://hg.mozilla.org/integration/mozilla-inbound/rev/719af14ab133
https://hg.mozilla.org/integration/mozilla-inbound/rev/e555d4f92c1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6763a02065
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9316111cf6

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/719af14ab133
https://hg.mozilla.org/mozilla-central/rev/e555d4f92c1d
https://hg.mozilla.org/mozilla-central/rev/7b6763a02065
https://hg.mozilla.org/mozilla-central/rev/8a9316111cf6