[OS.File] |file.write(buffer, {bytes: bytes}| does not work anymore.

RESOLVED FIXED in mozilla27

Status

()

Toolkit
OS.File
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nmaier, Assigned: nmaier)

Tracking

({regression})

Trunk
mozilla27
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Bug 865387 introduced the following change, causing |options.byteLength| to be always overriden by |buffer.byteLength|:

-    if (isTypedArray(buffer) && (!options || !("bytes" in options))) {
-      // Preserve the reference to |outExecutionDuration| option if it is
-      // passed.
+    if (isTypedArray(buffer)) {
+      // Preserve reference to option |outExecutionDuration|, if it is passed.
       options = clone(options, ["outExecutionDuration"]);
       options.bytes = buffer.byteLength;
     }

Bug 865387 comment 33.
(Assignee)

Comment 1

5 years ago
Created attachment 816891 [details] [diff] [review]
OS.File API must preverse |bytes| options to .readTo() and .write().

Alrighty, re-added the check, also re-added the "options is nullish" check and added a simple xpc-shell test.
Attachment #816891 - Flags: review?(dteller)
Comment on attachment 816891 [details] [diff] [review]
OS.File API must preverse |bytes| options to .readTo() and .write().

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

r=me, but I believe that we should keep our files smaller :)

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_bytes.js
@@ +20,5 @@
> +    try {
> +      // 1. Test write, by supplying {bytes:} options smaller than the actual
> +      // buffer.
> +      yield file.write(new Uint8Array(1<<11), {bytes: 1<<10});
> +      do_check_eq((yield file.stat()).size, 1<<10);

Do you really need your files to be that large?
I know that it's going to do no harm under Unix, but I have no such confidence in Windows.
Attachment #816891 - Flags: review?(dteller) → review+
(Assignee)

Comment 3

5 years ago
1024 bytes is considered large these days?
(Assignee)

Comment 4

5 years ago
Created attachment 817174 [details] [diff] [review]
OS.File API must preverse |bytes| options to .readTo() and .write(). r=yoric

- function*() ;)
- Replaced 1<<11 with 2048 and so on, to make it more clear what the sizes are.
Assignee: nobody → maierman
Attachment #816891 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
For some reason, I had seen 1 << 20. My bad.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ef3ac303c242
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 7

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> https://hg.mozilla.org/integration/fx-team/rev/ef3ac303c242

Sorry, this will cause failures on linux. Was too eager with the checkin-needed after receiving try greens for android, OSX and Windows...
https://hg.mozilla.org/mozilla-central/rev/ef3ac303c242
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.