Bug 1075438 - Removed readTo function from two files and fixed read function to work without readTo.Also removed the test for readTo function from mochi and xpcshell tests. r=Yoric
☠☠ backed out by 366d06412304 ☠ ☠
authorGaurav Rai <gaurav_rai@live.in>
Mon, 27 Oct 2014 06:59:00 -0400
changeset 212660 48099863baecf556a76a607811064e06daa88671
parent 212659 faa391ca3ad7ac5c14ca72191b3d3db1e3862ce6
child 212661 44c6d392bbc850ee299189c17da888bfd8b3fa91
push id9608
push userryanvm@gmail.com
push dateTue, 28 Oct 2014 16:44:21 +0000
treeherderfx-team@44c6d392bbc8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric
bugs1075438
milestone36.0a1
Bug 1075438 - Removed readTo function from two files and fixed read function to work without readTo.Also removed the test for readTo function from mochi and xpcshell tests. r=Yoric
toolkit/components/osfile/modules/osfile_async_front.jsm
toolkit/components/osfile/modules/osfile_shared_front.jsm
toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
toolkit/components/osfile/tests/xpcshell/test_osfile_async_bytes.js
--- a/toolkit/components/osfile/modules/osfile_async_front.jsm
+++ b/toolkit/components/osfile/modules/osfile_async_front.jsm
@@ -614,49 +614,16 @@ File.prototype = {
    */
   stat: function stat() {
     return Scheduler.post("File_prototype_stat", [this._fdmsg], this).then(
       File.Info.fromMsg
     );
   },
 
   /**
-   * Read a number of bytes from the file and into a buffer.
-   *
-   * @param {Typed array | C pointer} buffer This buffer will be
-   * modified by another thread. Using this buffer before the |read|
-   * operation has completed is a BAD IDEA.
-   * @param {JSON} options
-   *
-   * @return {promise}
-   * @resolves {number} The number of bytes effectively read.
-   * @rejects {OS.File.Error}
-   */
-  readTo: function readTo(buffer, options = {}) {
-    // If |buffer| is a typed array and there is no |bytes| options, we
-    // need to extract the |byteLength| now, as it will be lost by
-    // communication.
-    // Options might be a nullish value, so better check for that before using
-    // the |in| operator.
-    if (isTypedArray(buffer) && !(options && "bytes" in options)) {
-      // Preserve reference to option |outExecutionDuration|, if it is passed.
-      options = clone(options, ["outExecutionDuration"]);
-      options.bytes = buffer.byteLength;
-    }
-    // Note: Type.void_t.out_ptr.toMsg ensures that
-    // - the buffer is effectively shared (not neutered) between both
-    //   threads;
-    // - we take care of any |byteOffset|.
-    return Scheduler.post("File_prototype_readTo",
-      [this._fdmsg,
-       Type.void_t.out_ptr.toMsg(buffer),
-       options],
-       buffer/*Ensure that |buffer| is not gc-ed*/);
-  },
-  /**
    * Write bytes from a buffer to this file.
    *
    * Note that, by default, this function may perform several I/O
    * operations to ensure that the buffer is fully written.
    *
    * @param {Typed array | C pointer} buffer The buffer in which the
    * the bytes are stored. The buffer must be large enough to
    * accomodate |bytes| bytes. Using the buffer before the operation
--- a/toolkit/components/osfile/modules/osfile_shared_front.jsm
+++ b/toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ -47,65 +47,47 @@ AbstractFile.prototype = {
     if (this._fd) {
       return this._fd;
     }
     throw OS.File.Error.closed("accessing file", this._path);
   },
   /**
    * Read bytes from this file to a new buffer.
    *
-   * @param {number=} bytes If unspecified, read all the remaining bytes from
-   * this file. If specified, read |bytes| bytes, or less if the file does notclone
-   * contain that many bytes.
+   * @param {number=} maybeBytes (deprecated, please use options.bytes)
    * @param {JSON} options
    * @return {Uint8Array} An array containing the bytes read.
    */
-  read: function read(bytes, options = {}) {
-    options = clone(options);
-    options.bytes = bytes == null ? this.stat().size : bytes;
-    let buffer = new Uint8Array(options.bytes);
-    let size = this.readTo(buffer, options);
-    if (size == options.bytes) {
-      return buffer;
+  read: function read(maybeBytes, options = {}) {
+    if (typeof maybeBytes === "object") {
+    // Caller has skipped `maybeBytes` and provided an options object.
+      options = clone(maybeBytes);
+      maybeBytes = null;
     } else {
-      return buffer.subarray(0, size);
+      options = clone(options || {});
     }
-  },
-
-  /**
-   * Read bytes from this file to an existing buffer.
-   *
-   * Note that, by default, this function may perform several I/O
-   * operations to ensure that the buffer is as full as possible.
-   *
-   * @param {Typed Array | C pointer} buffer The buffer in which to
-   * store the bytes. The buffer must be large enough to
-   * accomodate |bytes| bytes.
-   * @param {*=} options Optionally, an object that may contain the
-   * following fields:
-   * - {number} bytes The number of |bytes| to write from the buffer. If
-   * unspecified, this is |buffer.byteLength|. Note that |bytes| is required
-   * if |buffer| is a C pointer.
-   *
-   * @return {number} The number of bytes actually read, which may be
-   * less than |bytes| if the file did not contain that many bytes left.
-   */
-  readTo: function readTo(buffer, options = {}) {
+    if(!("bytes" in options)) {
+      options.bytes = maybeBytes == null ? this.stat().size : maybeBytes;
+    }
+    let buffer = new Uint8Array(options.bytes);
     let {ptr, bytes} = SharedAll.normalizeToPointer(buffer, options.bytes);
     let pos = 0;
     while (pos < bytes) {
       let chunkSize = this._read(ptr, bytes - pos, options);
       if (chunkSize == 0) {
         break;
       }
       pos += chunkSize;
       ptr = SharedAll.offsetBy(ptr, chunkSize);
     }
-
-    return pos;
+    if (pos == options.bytes) {
+      return buffer;
+    } else {
+      return buffer.subarray(0, pos);
+    }
   },
 
   /**
    * Write bytes from a buffer to this file.
    *
    * Note that, by default, this function may perform several I/O
    * operations to ensure that the buffer is fully written.
    *
--- a/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
+++ b/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ -147,17 +147,16 @@ function toggleDebugTest (pref, consoleL
 }
 
 let test = maketest("Main", function main(test) {
   return Task.spawn(function() {
     SimpleTest.waitForExplicitFinish();
     yield test_stat();
     yield test_debug();
     yield test_info_features_detect();
-    yield test_read_write();
     yield test_position();
     yield test_iter();
     yield test_exists();
     yield test_debug_test();
     info("Test is over");
     SimpleTest.finish();
   });
 });
@@ -216,96 +215,34 @@ let test_info_features_detect = maketest
       } else {
         test.fail("unixGroup is not defined though we are under Unix");
       }
     }
   });
 });
 
 /**
- * Test OS.File.prototype.{read, readTo, write}
- */
-let test_read_write = maketest("read_write", function read_write(test) {
-  return Task.spawn(function() {
-    // Test readTo/write
-    let currentDir = yield OS.File.getCurrentDirectory();
-    let pathSource = OS.Path.join(currentDir, EXISTING_FILE);
-    let pathDest = OS.Path.join(OS.Constants.Path.tmpDir,
-      "osfile async test.tmp");
-
-    let fileSource = yield OS.File.open(pathSource);
-    test.info("Input file opened");
-    let fileDest = yield OS.File.open(pathDest,
-      { truncate: true, read: true, write: true});
-    test.info("Output file opened");
-
-    let stat = yield fileSource.stat();
-    test.info("Input stat worked");
-    let size = stat.size;
-    let array = new Uint8Array(size);
-
-    try {
-      test.info("Now calling readTo");
-      let readLength = yield fileSource.readTo(array);
-      test.info("ReadTo worked");
-      test.is(readLength, size, "ReadTo got all bytes");
-      let writeLength = yield fileDest.write(array);
-      test.info("Write worked");
-      test.is(writeLength, size, "Write wrote all bytes");
-
-      // Test read
-      yield fileSource.setPosition(0);
-      let readAllResult = yield fileSource.read();
-      test.info("ReadAll worked");
-      test.is(readAllResult.length, size, "ReadAll read all bytes");
-      test.is(Array.prototype.join.call(readAllResult),
-              Array.prototype.join.call(array),
-              "ReadAll result is correct");
-    } finally {
-      // Close stuff
-      yield fileSource.close();
-      yield fileDest.close();
-      test.info("Files are closed");
-    }
-
-    stat = yield OS.File.stat(pathDest);
-    test.is(stat.size, size, "Both files have the same size");
-    yield reference_compare_files(pathSource, pathDest, test);
-
-    // Cleanup.
-    OS.File.remove(pathDest);
-  });
-});
-
-
-/**
  * Test file.{getPosition, setPosition}
  */
 let test_position = maketest("position", function position(test) {
   return Task.spawn(function() {
     let file = yield OS.File.open(EXISTING_FILE);
 
     try {
-      let stat = yield file.stat();
-      test.info("Obtained file length");
-
-      let view = new Uint8Array(stat.size);
-      yield file.readTo(view);
+      let view = yield file.read();
       test.info("First batch of content read");
-
       let CHUNK_SIZE = 178;// An arbitrary number of bytes to read from the file
       let pos = yield file.getPosition();
       test.info("Obtained position");
       test.is(pos, view.byteLength, "getPosition returned the end of the file");
       pos = yield file.setPosition(-CHUNK_SIZE, OS.File.POS_END);
       test.info("Changed position");
       test.is(pos, view.byteLength - CHUNK_SIZE, "setPosition returned the correct position");
 
-      let view2 = new Uint8Array(CHUNK_SIZE);
-      yield file.readTo(view2);
+      let view2 = yield file.read();
       test.info("Read the end of the file");
       for (let i = 0; i < CHUNK_SIZE; ++i) {
         if (view2[i] != view[i + view.byteLength - CHUNK_SIZE]) {
           test.is(view2[i], view[i], "setPosition put us in the right position");
         }
       }
     } finally {
       yield file.close();
--- a/toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
+++ b/toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ -22,17 +22,16 @@ self.onmessage = function onmessage_star
   };
   try {
     test_init();
     test_offsetby();
     test_open_existing_file();
     test_open_non_existing_file();
     test_flush_open_file();
     test_copy_existing_file();
-    test_readall_writeall_file();
     test_position();
     test_move_file();
     test_iter_dir();
     test_info();
     test_path();
     test_exists_file();
     test_remove_file();
   } catch (x) {
@@ -179,200 +178,16 @@ function compare_files(test, sourcePath,
     }
   } finally {
     source.close();
     dest.close();
   }
   info(test + ": Comparison complete");
 }
 
-function test_readall_writeall_file()
-{
-  let src_file_name =
-    OS.Path.join("chrome", "toolkit", "components", "osfile", "tests", "mochi",
-                 "worker_test_osfile_front.js");
-  let tmp_file_name =
-    OS.Path.join(OS.Constants.Path.tmpDir, "test_osfile_front.tmp");
-  info("Starting test_readall_writeall_file");
-
-  // read, ArrayBuffer
-
-  let source = OS.File.open(src_file_name);
-  let dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
-  let size = source.stat().size;
-
-  let buf = new Uint8Array(size);
-  let readResult = source.readTo(buf);
-  is(readResult, size, "test_readall_writeall_file: read the right number of bytes");
-
-  dest.write(buf);
-
-  info("test_readall_writeall_file: copy complete (manual allocation)");
-  source.close();
-  dest.close();
-
-  compare_files("test_readall_writeall_file (manual allocation)", src_file_name, tmp_file_name);
-  OS.File.remove(tmp_file_name);
-
-  // read, C buffer
-  source = OS.File.open(src_file_name);
-  dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
-  buf = new ArrayBuffer(size);
-  let ptr = OS.Shared.Type.voidptr_t.implementation(buf);
-  readResult = source.readTo(ptr, {bytes: size});
-  is(readResult, size, "test_readall_writeall_file: read the right number of bytes (C buffer)");
-
-  dest.write(ptr, {bytes: size});
-
-  info("test_readall_writeall_file: copy complete (C buffer)");
-  source.close();
-  dest.close();
-
-  compare_files("test_readall_writeall_file (C buffer)", src_file_name, tmp_file_name);
-  OS.File.remove(tmp_file_name);
-
-  // read/write, C buffer, missing |bytes| option
-  source = OS.File.open(src_file_name);
-  dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
-  let exn = should_throw(function() { source.readTo(ptr); });
-  ok(exn != null && exn instanceof TypeError, "test_readall_writeall_file: read with C pointer and without bytes fails with the correct error");
-  exn = should_throw(function() { dest.write(ptr); });
-  ok(exn != null && exn instanceof TypeError, "test_readall_writeall_file: write with C pointer and without bytes fails with the correct error");
-
-  source.close();
-  dest.close();
-
-  // readTo, ArrayBuffer + offset
-  let OFFSET = 12;
-  let LEFT = size - OFFSET;
-  buf = new ArrayBuffer(size);
-  let offset_view = new Uint8Array(buf, OFFSET);
-  source = OS.File.open(src_file_name);
-  dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
-
-  readResult = source.readTo(offset_view);
-  is(readResult, LEFT, "test_readall_writeall_file: read the right number of bytes (with offset)");
-
-  dest.write(offset_view);
-  is(dest.stat().size, LEFT, "test_readall_writeall_file: wrote the right number of bytes (with offset)");
-
-  info("test_readall_writeall_file: copy complete (with offset)");
-  source.close();
-  dest.close();
-
-  compare_files("test_readall_writeall_file (with offset)", src_file_name, tmp_file_name, LEFT);
-  OS.File.remove(tmp_file_name);
-
-  // read
-  buf = new Uint8Array(size);
-  source = OS.File.open(src_file_name);
-  dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
-
-  readResult = source.read();
-  is(readResult.length, size, "test_readall_writeall_file: read the right number of bytes (auto allocation)");
-
-  dest.write(readResult);
-
-  info("test_readall_writeall_file: copy complete (auto allocation)");
-  source.close();
-  dest.close();
-
-  compare_files("test_readall_writeall_file (auto allocation)", src_file_name, tmp_file_name);
-  OS.File.remove(tmp_file_name);
-
-  // File.readAll
-  readResult = OS.File.read(src_file_name);
-  is(readResult.length, size, "test_readall_writeall_file: read the right number of bytes (OS.File.readAll)");
- 
-  // File.writeAtomic on top of nothing
-  OS.File.writeAtomic(tmp_file_name, readResult,
-    {tmpPath: tmp_file_name + ".tmp"});
-  try {
-    let stat = OS.File.stat(tmp_file_name);
-    info("readAll + writeAtomic created a file");
-    is(stat.size, size, "readAll + writeAtomic created a file of the right size");
-  } catch (x) {
-    ok(false, "readAll + writeAtomic somehow failed");
-    if(x.becauseNoSuchFile) {
-      ok(false, "readAll + writeAtomic did not create file");
-    }
-  }
-  compare_files("test_readall_writeall_file (OS.File.readAll + writeAtomic)",
-                src_file_name, tmp_file_name);
-  exn = null;
-  try {
-    let stat = OS.File.stat(tmp_file_name + ".tmp");
-  } catch (x) {
-    exn = x;
-  }
-  ok(!!exn, "readAll + writeAtomic cleaned up after itself");
-
-  // File.writeAtomic on top of existing file
-  // Remove content and set arbitrary size, to avoid potential false negatives
-  dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
-  dest.setPosition(1234);
-  dest.close();
-
-  OS.File.writeAtomic(tmp_file_name, readResult,
-    {tmpPath: tmp_file_name + ".tmp"});
-  compare_files("test_readall_writeall_file (OS.File.readAll + writeAtomic 2)",
-                src_file_name, tmp_file_name);
-
-  // File.writeAtomic on top of existing file but without overwritten the file
-  exn = null;
-  try {
-    let view = new Uint8Array(readResult.buffer, 10, 200);
-    OS.File.writeAtomic(tmp_file_name, view,
-      { tmpPath: tmp_file_name + ".tmp", noOverwrite: true});
-  } catch (x) {
-    exn = x;
-  }
-  ok(exn && exn instanceof OS.File.Error && exn.becauseExists, "writeAtomic fails if file already exists with noOverwrite option");
-  // Check file was not overwritten.
-  compare_files("test_readall_writeall_file (OS.File.readAll + writeAtomic check file was not overwritten)",
-                src_file_name, tmp_file_name);
-
-  // Ensure that File.writeAtomic fails if no temporary file name is provided
-  // (FIXME: Remove this test as part of bug 793660)
-
-  exn = null;
-  try {
-    OS.File.writeAtomic(tmp_file_name, readResult.buffer,
-      {bytes: readResult.length});
-  } catch (x) {
-    exn = x;
-  }
-  ok(!!exn && exn instanceof TypeError, "writeAtomic fails if tmpPath is not provided");
-
-  // Check that writeAtomic fails when destination path is undefined
-  exn = null;
-  try {
-    let path = undefined;
-    let options = {tmpPath: tmp_file_name};
-    OS.File.writeAtomic(path, readResult.buffer, options);
-  } catch (x) {
-    exn = x;
-  }
-  ok(!!exn && exn instanceof TypeError, "writeAtomic fails if path is undefined");
-
-  // Check that writeAtomic fails when destination path is an empty string
-  exn = null;
-  try {
-    let path = "";
-    let options = {tmpPath: tmp_file_name};
-    OS.File.writeAtomic(path, readResult.buffer, options);
-  } catch (x) {
-    exn = x;
-  }
-  ok(!!exn && exn instanceof TypeError, "writeAtomic fails if path is an empty string");
-
-  // Cleanup.
-  OS.File.remove(tmp_file_name);
-}
-
 /**
  * Test that copying a file using |copy| works.
  */
 function test_copy_existing_file()
 {
   let src_file_name =
     OS.Path.join("chrome", "toolkit", "components", "osfile", "tests", "mochi",
                  "worker_test_osfile_front.js");
--- a/toolkit/components/osfile/tests/xpcshell/test_osfile_async_bytes.js
+++ b/toolkit/components/osfile/tests/xpcshell/test_osfile_async_bytes.js
@@ -4,36 +4,31 @@ Components.utils.import("resource://gre/
 Components.utils.import("resource://gre/modules/Task.jsm");
 
 function run_test() {
   do_test_pending();
   run_next_test();
 }
 
 /**
- * Test to ensure that {bytes:} in options to |readTo| and |write| are correctly
+ * Test to ensure that {bytes:} in options to |write| is correctly
  * preserved.
  */
 add_task(function* test_bytes() {
   let path = OS.Path.join(OS.Constants.Path.tmpDir,
                           "test_osfile_async_bytes.tmp");
   let file = yield OS.File.open(path, {trunc: true, read: true, write: true});
   try {
     try {
       // 1. Test write, by supplying {bytes:} options smaller than the actual
       // buffer.
       yield file.write(new Uint8Array(2048), {bytes: 1024});
       do_check_eq((yield file.stat()).size, 1024);
 
-      // 2. Test same for |readTo|.
-      yield file.setPosition(0, OS.File.POS_START);
-      let read = yield file.readTo(new Uint8Array(1024), {bytes: 512});
-      do_check_eq(read, 512);
-
-      // 3. Test that passing nullish values for |options| still works.
+      // 2. Test that passing nullish values for |options| still works.
       yield file.setPosition(0, OS.File.POS_END);
       yield file.write(new Uint8Array(1024), null);
       yield file.write(new Uint8Array(1024), undefined);
       do_check_eq((yield file.stat()).size, 3072);
     } finally {
       yield file.close();
     }
   } finally {