Implement FileReader.readAsArrayBuffer()

RESOLVED FIXED in mozilla7

Status

()

enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: pcwalton, Assigned: hippopotamus.gong)

Tracking

({dev-doc-complete})

unspecified
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-chrome][parity-safari])

Attachments

(1 attachment, 17 obsolete attachments)

21.76 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Reporter

Description

9 years ago
The FileReader interface contains a readAsArrayBuffer() function:

https://developer.mozilla.org/en/DOM/FileReader#readAsArrayBuffer%28%29

It'd be really nice to have this; data URLs are a pain.
Even better would be to implement something based on Dave Hermans binary-data harmony proposal. Not sure if we want to do both (in which case I shouldn't hijack this bug), or if we want to just do one (in which case this bug is probably the place to discuss which we should do).
Duplicate of this bug: 635274

Comment 3

8 years ago
I've received feedback from colleagues that they would like to see this method implemented in Firefox.

Given our recent meeting about the evolution of the typed array spec, it seems it and the binary data proposal will coexist for some time, so it would be great if support for readAsArrayBuffer could be added to Gecko.
Assignee

Updated

8 years ago
Assignee: nobody → sgong
Assignee

Comment 4

8 years ago
Attachment #533697 - Flags: review?(jonas)
Comment on attachment 533697 [details] [diff] [review]
The implementation of readAsArrayBuffer

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

::: content/base/src/nsDOMFileReader.cpp
@@ +116,5 @@
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>  
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsDOMFileReader)
> +  if(tmp->mResultArrayBuffer) {
> +    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(tmp->mResultArrayBuffer, "mResultArrayBuffer")

Nit: This line exceeds 80 chars.

@@ +174,5 @@
>      mReadyState(nsIDOMFileReader::EMPTY),
>      mProgressEventWasDelayed(PR_FALSE),
>      mTimerIsActive(PR_FALSE),
> +    mReadTotal(0), mReadTransferred(0),
> +    mResultArrayBufferRooted(false)

The order of initialization here is wrong, mResultArrayBufferRooted should be set after mResultArrayBuffer, or change the header to match.

@@ +279,5 @@
> +  if (mDataFormat == FILE_AS_ARRAYBUFFER) {
> +    if (mReadyState == nsIDOMFileReader::DONE) {
> +      *aResult = mResultArrayBuffer ?
> +                 OBJECT_TO_JSVAL(mResultArrayBuffer) :
> +                 JSVAL_NULL;

if readyState is DONE then you must have an array buffer, so no need to check it here.

@@ +508,5 @@
>  
>    nsresult rv = NS_OK;
>    switch (mDataFormat) {
> +    case FILE_AS_ARRAYBUFFER:
> +      // Intentionally fall-through to FILE_AS_BINARY.

This no longer makes sense (didn't reuse the FILE_AS_BINARY implementation). Just replace that with a break line.

@@ +550,5 @@
>    //Implicit abort to clear any other activity going on
>    Abort();
>    mError = nsnull;
>    SetDOMStringToNull(mResult);
> +  mResultArrayBuffer = nsnull;

Abort() will set this to null, so this line is unnecessary.

@@ +590,5 @@
> +  if (mDataFormat == FILE_AS_ARRAYBUFFER) {
> +    RootResultArrayBuffer();
> +    mResultArrayBuffer = js_CreateArrayBuffer(aCx, mReadTotal);
> +    if (!mResultArrayBuffer) {
> +      return NS_ERROR_FAILURE;

I would issue a warning here so that if this ever fails we can notice in the console.

::: content/base/src/nsDOMFileReader.h
@@ +79,5 @@
>  
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    NS_DECL_NSIDOMFILEREADER
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(nsDOMFileReader, nsXHREventTarget)

Nit: Exceeds 80 chars.

@@ +120,5 @@
>      FILE_AS_TEXT,
>      FILE_AS_DATAURL
>    };
>  
> +  nsresult ReadFileContent(JSContext* aCx, nsIDOMBlob *aFile, const nsAString &aCharset, eDataFormat aDataFormat); 

Nit: Exceeds 80 chars.

::: content/base/test/test_fileapi.html
@@ +28,5 @@
>  } catch (e) {
>    ok(true, "Threw on an unprivileged attempt to construct a File");
>  }
>  
> +const minFileSize = 2000;

I would not make this change, see below.

@@ +374,5 @@
> +    
> +    u8v = new Uint8Array(event.target.result);
> +    for (var i = 0; i < expectedLength; i++) {
> +      is(u8v[i], expectedResult.charCodeAt(i), "array buffer char code");
> +    }

Here, instead of calling is() inside the for loop, just do something like this:

  var ok = true;
  for (var i = 0; i < expectedLength; i++) {
    if (u8v[i] != expectedResult.charCodeAt(i)) {
      ok = false;
    }
  }
  is(ok, true, "array buffer char code");
Assignee

Comment 6

8 years ago
Attachment #533770 - Flags: review?(bent.mozilla)
Comment on attachment 533770 [details] [diff] [review]
Changes according to bent's comment

Looks good to me, over to sicking.
Attachment #533770 - Flags: review?(jonas)
Attachment #533770 - Flags: review?(bent.mozilla)
Attachment #533770 - Flags: review+
Assignee

Comment 8

8 years ago
Assignee

Updated

8 years ago
Attachment #533828 - Flags: review?(jonas)
Comment on attachment 533828 [details] [diff] [review]
Smashed patch of the previous 2

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

Just small fixes, but would still like to see a final patch.

::: content/base/src/nsDOMFileReader.cpp
@@ +120,5 @@
> +                                               "mResultArrayBuffer")
> +  }
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +

Just one blank line is enough.

@@ +589,5 @@
> +    mResultArrayBuffer = js_CreateArrayBuffer(aCx, mReadTotal);
> +    if (!mResultArrayBuffer) {
> +      NS_WARNING("Failed to create JS array buffer");
> +      return NS_ERROR_FAILURE;
> +    }

Probably safer to call the RootResultArrayBuffer() function only if the js_CreateArrayBuffer call succeeds.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +290,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsXHREventTarget)
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXHREventTarget,
>                                                    nsDOMEventTargetWrapperCache)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

This seems to be part of another patch. Please revert all the XHR changes.

::: content/base/test/test_fileapi.html
@@ +138,5 @@
>  
> +// Test get result without reading
> +r = new FileReader();
> +is(r.readyState, FileReader.EMPTY,
> +  "readyState in test reader get result without reading");

Why this addition?

@@ +378,5 @@
> +      if (u8v[i] != expectedResult.charCodeAt(i)) {
> +        match = false;
> +      } 
> +    }
> +    is(match, true, "array buffer char code");

You can write this as:

is(event.target.result.byteLength, expectedLength, "array buffer size in test " + testName);
var u8v = new Uint8Array(event.target.result);
is(String.fromCharCode.apply(String, u8v), expectedResult, "array buffer contents in test " + testName);
Attachment #533828 - Flags: review?(jonas) → review-
Assignee

Comment 10

8 years ago
The test addition on line 138 is to make sure for a new FileReader, even if we don't read anything, the GetResult() call should return a NULL - should I remove it?
Assignee

Comment 11

8 years ago
Attachment #533858 - Flags: review?(jonas)
Assignee

Comment 12

8 years ago
Attachment #533859 - Flags: review?(jonas)
(In reply to comment #10)
> The test addition on line 138 is to make sure for a new FileReader, even if
> we don't read anything, the GetResult() call should return a NULL - should I
> remove it?

Ah, excellent. However there's no need to add these two lines:

+testHasRun();
+expectedTestCount++;

The purpose of that function and variable is to manage asynchronous tests, but since you can run these tests synchronously they are not needed.
Assignee

Updated

8 years ago
Attachment #533697 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #533770 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #533828 - Attachment is obsolete: true
Assignee

Comment 14

8 years ago
Posted patch Final patch candidate (obsolete) — Splinter Review
Attachment #534015 - Flags: review?
Assignee

Updated

8 years ago
Attachment #534015 - Flags: review? → review?(jonas)
Assignee

Updated

8 years ago
Attachment #533859 - Attachment is obsolete: true
Attachment #533859 - Flags: review?(jonas)
Assignee

Comment 15

8 years ago
Assignee

Updated

8 years ago
Attachment #534105 - Flags: review?(jonas)
Assignee

Comment 17

8 years ago
Attachment #533858 - Attachment is obsolete: true
Attachment #534015 - Attachment is obsolete: true
Attachment #534105 - Attachment is obsolete: true
Attachment #534180 - Flags: review?(jonas)
Attachment #533858 - Flags: review?(jonas)
Attachment #534015 - Flags: review?(jonas)
Attachment #534105 - Flags: review?(jonas)
Assignee

Comment 18

8 years ago
Posted patch Full Patch for Commit (obsolete) — Splinter Review
Attachment #534181 - Flags: review?(jonas)
I pushed this and bug 658683 to tryserver and got one failure and one seemingly random crash:

http://tbpl.mozilla.org/?tree=Try&rev=e2514be69c2d
The crash is definitely not random.  The stack is in nsXMLHttpRequest's CC helper, so there's some sort of problem with the patch for bug 658683.
Assignee

Comment 21

8 years ago
Attachment #534180 - Attachment is obsolete: true
Attachment #534181 - Attachment is obsolete: true
Attachment #534589 - Flags: review?(jonas)
Attachment #534180 - Flags: review?(jonas)
Attachment #534181 - Flags: review?(jonas)
Assignee

Updated

8 years ago
Attachment #534589 - Attachment description: Removed mResultArrayBufferRooted variable. Noted need the patch 658683 being applied first. → Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.
Comment on attachment 534589 [details] [diff] [review]
Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.

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

r=me with this change.

::: content/base/src/nsDOMFileReader.cpp
@@ +178,5 @@
>  nsDOMFileReader::~nsDOMFileReader()
>  {
> +  if (mResultArrayBuffer) {
> +    UnrootResultArrayBuffer();
> +  }

You should do the Unroot call here even if mResultArrayBuffer is null. Consider the case when you first read as an ArrayBuffer, then read as something else, and then the reader is deleted. In that case mResultArrayBuffer is null, but the wrappercache won't unroot since it was never rooting.

Same thing in the unlink call.

And please add a test for this.
Attachment #534589 - Flags: review?(jonas) → review+
Assignee

Comment 23

8 years ago
Attachment #534589 - Attachment is obsolete: true
Attachment #534632 - Flags: review?(jonas)
Assignee

Updated

8 years ago
Attachment #534635 - Flags: review?(jonas)
Attachment #534632 - Attachment is obsolete: true
Attachment #534632 - Flags: review?(jonas)
Comment on attachment 534635 [details] [diff] [review]
Added corresponding test for reading firstly as array buffer then read as binary string

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

r=me with that fixed.

::: content/base/test/test_fileapi.html
@@ +250,5 @@
> +                          testBinaryData.length,
> +                          "to-be-reused reading arrayBuffer")
> +var makeAnotherReadListener5 = function(event) {
> +  r = event.target;
> +  r.removeEventListener("load", makeAnotherReadListener4, false);

That should be makeAnotherReadListener5.
Attachment #534635 - Flags: review?(jonas) → review+
Assignee

Comment 26

8 years ago
Attachment #534635 - Attachment is obsolete: true
Attachment #534644 - Flags: review?(jonas)
Posted patch latest attempt (obsolete) — Splinter Review
This seems to do it.
Attachment #534644 - Attachment is obsolete: true
Attachment #534644 - Flags: review?(jonas)
Pushed by Jonas then backed out because of orange on Windows Mochitest-1.
See:
http://tbpl.mozilla.org/?tree=Firefox&rev=0acfb7c8331b
Assignee

Comment 30

8 years ago
Pushed to tryServer, waiting for results
Attachment #534723 - Attachment is obsolete: true
Attachment #543207 - Flags: review?(jonas)
Comment on attachment 543207 [details] [diff] [review]
With FileReader added to dom_quickstub.qsconf should fix the crash

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

r=me with that! Though please attach a new patch with a better commit comment.

Yay! Very glad we figured this one out, thanks for your help with all the testing!

::: content/base/src/nsDOMFileReader.cpp
@@ +141,5 @@
>  
> +void
> +nsDOMFileReader::RootResultArrayBuffer()
> +{
> +  nsContentUtils::PreserveWrapper(static_cast<nsPIDOMEventTarget*>(this), this);

This won't compile once you update to tip. You should cast to nsIDOMEventTarget rather than nsPIDOMEventTarget.

@@ +167,3 @@
>  {
>    nsLayoutStatics::AddRef();
> +  SetDOMStringToNull(mResult);

Why is this needed?
Attachment #543207 - Flags: review?(jonas) → review+
Assignee

Comment 33

8 years ago
You're right! I did the change but somehow they were lost in the patch. Now the changes according to the new jstypedarray is applied.

New tbpl: http://tbpl.mozilla.org/?tree=Try&rev=24a704daf853
Attachment #543207 - Attachment is obsolete: true
Attachment #543250 - Flags: review?(jonas)
Comment on attachment 543250 [details] [diff] [review]
Updated with the new jstypedarray.h change

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

::: content/base/src/nsDOMFileReader.cpp
@@ +430,5 @@
>                                 &bytesRead);
>      NS_ASSERTION(bytesRead == aCount, "failed to read data");
>    }
> +  else if (mDataFormat == FILE_AS_ARRAYBUFFER) {
> +    JSObject* abuf = js::ArrayBuffer::getArrayBuffer(mResultArrayBuffer);

I don't understand this line. You're converting from a JSObject* to a JSObject*? Are they different objects somehow?

I guess I'd like to get Nikhil to verify that this is required by the new arraybuffer API.

::: content/base/test/test_fileapi.html
@@ +142,5 @@
> +  "readyState in test reader get result without reading");
> +is(r.error, null,
> +  "no error in test reader get result without reading");
> +is(r.result, "",
> +  "result in test reader get result without reading");

Ah, this should indeed be null, so add back the SetDOMStringToNull(mResult) that you had added and change this back to null.
Assignee

Comment 35

8 years ago
Posted patch Final patch candidate (obsolete) — Splinter Review
The tryServer status looks so far so good
Attachment #543250 - Attachment is obsolete: true
Attachment #543303 - Flags: review?(jonas)
Attachment #543250 - Flags: review?(jonas)
Comment on attachment 543303 [details] [diff] [review]
Final patch candidate

Yay!

Please do attach a new patch with a better checkin comment though.
Attachment #543303 - Attachment is patch: true
Attachment #543303 - Flags: review?(jonas) → review+
Assignee

Comment 37

8 years ago
Attachment #543303 - Attachment is obsolete: true
Attachment #543312 - Flags: review?(jonas)
Comment on attachment 543312 [details] [diff] [review]
Now with the correct comment (hopefully:)

(by the way, you don't really need to ask for review when fixing the checkin message :) )
Attachment #543312 - Flags: review?(jonas) → review+

Comment 40

8 years ago
Great thanks a lot Jonas!
http://hg.mozilla.org/mozilla-central/rev/02a734493229
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Updated

8 years ago

Comment 42

8 years ago
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110908135051

I ran the content/base/test/test_fileapi.html test to verify this bug but it failed:
Passed: 210
Failed: 1
Todo: 1
todo | shouldn't throw when opening nonexistent file, should fire error instead
failed | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: SpecialPowers is not defined at file:///D:/test%20cases/tc/test_fileapi.html:407
You need to log in before you can comment on or make changes to this bug.