Added support for seek() and truncate() #6

Merged
merged 6 commits into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
Contributor

piranna commented Oct 21, 2012

Added support for seek() and truncate() methods on FileWriter and also improved write() support according to the File API specification based on code from FileWriter polyfill (https://github.com/piranna/ShareIt/blob/master/js/webp2p/polyfills/FileWriter.js) at ShareIt! project (also developed by myself). Since I started the polyfill using your implementation as basis, I think it's fair to return my improved and fixed version... :-)

Owner

ebidel commented Jan 3, 2013

Haven't looked closely at the code yet, but can you use Google's JS style guide for spacing, brackets, etc.
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml.

Contributor

piranna commented Jan 3, 2013

Fixed :-) I'm too much used to the BSD and Pep-8 styles... :-P

shacharz commented Apr 9, 2013

I'd like to see this pull request accepted, is it inefficient or anything?

Owner

ebidel commented Apr 9, 2013

Thanks for this. Sorry about taking to long to review. LGTM

ebidel merged commit 12f5208 into ebidel:master Apr 9, 2013

Owner

ebidel commented Apr 9, 2013

This isn't passing the tests. Can you fire them up in FF (start a web serve in the root dir and hit http://localhost/tests/)?

Contributor

piranna commented Apr 9, 2013

Don't worry, to be honest I forgot about it X-D I'll check it now.

Contributor

piranna commented Apr 9, 2013

I've started it and got the next error:

Uncaught ReferenceError: module is not defined tests.js:6
(anonymous function) tests.js:6

The line is:

module('window methods', {

Seems qUnit is not available, I've added it by hand and now it's working. I'm going to look on the bug.

Contributor

piranna commented Apr 9, 2013

I can't be able to fix it, the bug is happening when you create an empty FileEntry, so it don't have a blob object. I've tried to fix it using a mock file:

var blob_ = fileEntry.file_ ? fileEntry.file_.blob_ : {size: 0};

but now test doesn't work, it only show the first one as running. Any idea?

Owner

ebidel commented Apr 9, 2013

That's one change I've made ad well. Currently having other issues with this business:

// Calc the head and tail fragments
var head = blob_.slice(0, position_);
var tail = blob_.slice(position_ + data.length);

I'm looking into it, don't sweat it.

Contributor

piranna commented Apr 9, 2013

Yeah, I did it to be able to do a 'data' write in the middle of a file at current 'position_' just like the specification (and POSIX, by the way) say.

Ok, I'll keep me code here if you need that I review something :-)

Owner

ebidel commented Apr 9, 2013

Got the tests working. You had a data.length instead of data.size that tripped me up for far too long :(

Here's what write() looks like now:

this.write = function(data) {
if (!data) {
throw Error('Expected blob argument to write.');
}

// Call onwritestart if it was defined.
if (this.onwritestart) {
  this.onwritestart();
}

// TODO: not handling onprogress, onwrite, onabort. Throw an error if
// they're defined.

if (!blob_) {
  blob_ = new Blob([data], {type: data.type});
} else {
  // Calc the head and tail fragments
  var head = blob_.slice(0, position_);
  var tail = blob_.slice(position_ + data.size);

  // Calc the padding
  var padding = position_ - head.size;
  if (padding < 0) {
    padding = 0;
  }
  // Do the "write". In fact, a full overwrite of the Blob.
  // TODO: figure out if data.type should overwrite the exist blob's type.
  blob_ = new Blob([head, new Uint8Array(padding), data, tail],
                   {type: blob_.type});
}

// Set the blob we're writing on this file entry so we can recall it later.
fileEntry.file_.blob_ = blob_;

idb_.put(fileEntry, function(entry) {
  // Add size of data written to writer.position.
  position_ += data.size;

  if (this.onwriteend) {
    this.onwriteend();
  }
}.bind(this), this.onerror);

};

I'm going to add tests for seek() and truncate().

Contributor

piranna commented Apr 9, 2013

Don't know why I was using .length instead of .size, maybe they are equivalent for other objects? Code looks good anyway, thank you for your time looking it :-)

Owner

ebidel commented Apr 9, 2013

Blob doesn't have a .length but ArrayBuffer's do and the FileWriter interface does. It's easy to get things mixed up!!

Contributor

piranna commented Apr 9, 2013

Maybe that's the reason, in my original code (ShareIt!) I was using ArrayBuffers...

Owner

ebidel commented Apr 9, 2013

Everything should be right as mud now. Give v0.0.3 a test and see if it breaks anything.
Thanks again!

e92d595

Contributor

piranna commented Apr 10, 2013

Tests are being passed on the latest version, good job! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment