769191 - [OS.File] read and write should accept a default length

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

Common scenario:

let a = new ArrayBuffer(21);
let file = OS.File.open(...)
file.read(a, a.byteLength);

If no length is given, we should use the length of the array buffer.

Comment 1

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

Attachment: Default arguments of OS.File.prototype.read/write

Comment 2

[(dormant account)]

Comment on attachment 637423 [details] [diff] [review]
Default arguments of OS.File.prototype.read/write

bugzilla is failing so I can't see the patch, but this makes sense

Comment 3

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

Doing this differently.

Comment 4

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

Attachment: Default arguments for AbstractFile.prototype.{readTo, write}

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 656581 [details] [diff] [review]
Default arguments for AbstractFile.prototype.{readTo, write}

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

Somewhere in worker_test_osfile_front.js it would be good to add a test for C pointer without options.bytes, for both readTo and write, to check that the error is thrown correctly.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +166,5 @@
>        throw new TypeError("Expecting a non-null pointer");
>      }
>      ptr = exports.OS.Shared.Type.uint8_t.out_ptr.cast(candidate);
> +    if (bytes == null) {
> +      throw new TypeError("This function expects either an ArrayBuffer or a C pointer and a number of bytes.");

I think this error message should indicate that |bytes| is required with a C pointer and shouldn't talk about ArrayBuffers; this is the pointer case, after all.

Comment 6

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

Attachment: Default arguments for AbstractFile.prototype.{readTo, write}

Added default arguments and changed error message.

Comment 7

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

Hum, I meant "added *test* for default arguments".

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(Remarkably) Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47

https://hg.mozilla.org/integration/mozilla-inbound/rev/a88c590a2c0b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a88c590a2c0b