507805 - Support for asynchronous file data access

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Matin Movassate]

Attachment: Patch for async file access v1

Manipulating local file data (pictures, videos, documents, etc.) can prove to be one of the more time-consuming tasks when interacting with a web page. The soon-to-be-defunct method for obtaining file contents blocks the remainder of the script while the requested data gets synchronously fetched. Execution within a script could only continue once the file was opened and all of its data was completely retrieved.

Per the HTML5 File API draft, however, calls to access file data now occur asynchronously. Thus, subsequent steps within a script are no longer blocked by file reading methods, as execution proceeds immediately after a call is dispatched to retrieve file data. Once the entirety of the file's content has been read and properly encoded, the data gets passed into a user-specified file-handler callback function. If an exception were raised during file reading, and the user passed in an optional error-handler callback, then a new FileError containing the relevant status code gets passed into the error handler.

An example illustrating the difference in usage:
   
  //Old method
  try {
    var fileTextUTF16 = textFile.getAsTextOld("UTF-16");
    validateText(fileTextUTF16);
    var fileTextUTF8 = textFile.getAsTextOld();
    outputText(fileTextUTF8);
    var picData = picFile.getAsDataURLOld();
    handleURI(picData);
  } catch (FileException err) { handleError(err); }

  ...

  //New method
  textFile.getAsTextNew(validateText, "UTF-16", handleError);
  textFile.getAsTextNew(outputText);
  picFile.getAsDataURI(handleURI, handleError);

  ...

  function validateText(fileAsText) {
     if (isProperJapanese(fileAsText))
       alert("Hai!");
  }

  function outputText(fileAsText) {
      outputToPage("Your essay in UTF-8: " + fileAsText);
  }

  function handleURI(fileAsDataURI) {
     if (fileAsDataURI.length < 1000 && IsGifImage(fileAsDataURI))
       displayGif(fileAsDataURI);
  }

  function handleError(error) {
    output("Error retrieving file data: " + error.errorCode);
  }

The asynchronous functions provide the ability to consume file data as either text (encoded in a user-specified format) or as a data URI. In addition, a cancelPendingReads() function is provided in order to terminate any already-established yet incomplete calls for retrieving file content.

As the implementation currently stands, the asynchronous versions of GetAsText() and GetAsDataURI(), along with some of their helper methods, have temporary names so as to avoid any namespace collision with their synchronous counterparts.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 392077 [details] [diff] [review]
Patch for async file access v1

 interface nsIDOMFile : nsISupports
 {
   readonly attribute DOMString fileName;
   readonly attribute unsigned long long fileSize;
 
   DOMString getAsText(in DOMString encoding);
                                   // raises(FileException) on retrieval
   DOMString getAsDataURL();
                                   // raises(FileException) on retrieval
   DOMString getAsBinary();
                                   // raises(FileException) on retrieval
+  void getAsTextNew(in nsIDOMFileAsText callback, [optional] in DOMString encoding,
+					          [optional] in nsIDOMFileErrorCallback errorCallback);

I think we need to figure out how to deal with this temporary naming etc, I'm not sure checking this in side by side with the sync methods is the best way, but if it is, should we name this mozGetAsText() or somesuch so people who use this at least know it's mozilla specific? Jonas, what do you think about this?

-NS_IMPL_ADDREF(nsDOMFile)
-NS_IMPL_RELEASE(nsDOMFile)
+NS_IMPL_THREADSAFE_ADDREF(nsDOMFile)
+NS_IMPL_THREADSAFE_RELEASE(nsDOMFile)

We should figure out if this is really needed, and if it is we should comment as to why...

- In nsDOMFile::GetFileContent():

+  //pop loader onto array of stream loaders, so it can be cancelled within CancelPendingRequests
+  if(!mStreamLoaders.AppendObject(loader))
+    return NS_ERROR_FAILURE;

If we cancel pending requests, don't we want to cancel the channels rather than the stream loaders? I'm not convinced that canceling the stream loader actually ends up canceling the underlying channel, and even if it does I'd think canceling as far down as we can would be the best thing to do...

Also, this needs to be done as late as possible, after the file has been successfully opened (i.e. after AsyncOpen() returns w/o an error) to avoid forgetting the stream listener, or channel per the above comment, in this array.

+  switch (rv) {
+    case NS_ERROR_FILE_NOT_FOUND:
+    {
+      error = new nsDOMFileError(8);
+      NS_ENSURE_TRUE(error, NS_ERROR_OUT_OF_MEMORY);
+
+      aFileErrorCallback->HandleEvent(error);
+      return NS_OK;
+    }
+    case NS_ERROR_FILE_ACCESS_DENIED:
+    {
+      error = new nsDOMFileError(18);
+      NS_ENSURE_TRUE(error, NS_ERROR_OUT_OF_MEMORY);
+
+      aFileErrorCallback->HandleEvent(error);
+      return NS_OK;
+    }
+    case NS_ERROR_DOM_FILE_NOT_READABLE_ERR:
+    {
+      error = new nsDOMFileError(24);
+      NS_ENSURE_TRUE(error, NS_ERROR_OUT_OF_MEMORY);
+
+      aFileErrorCallback->HandleEvent(error);
+      return NS_OK;
+    }
+  }
+
+  return NS_ERROR_FAILURE;

All those cases in the switch statement are identical except for the error code. Maybe just set the error code into a local variable in the switch, move the code to create the nsDOMFileError and call the callback after the switch, and move the return NS_ERROR_FAILURE into a default case in the switch statement?

- In nsDOMFile::OnStreamComplete():

We need to do *something* here if aStatus indicates that an error happened after the file was opened and AsyncOpen() returned.

+  if (format == FILE_AS_TEXT) {
+    rv = GetAsTextInternal(fileAsTextCallback, charset, fileErrorCallback,
+                            reinterpret_cast<const char*>(aString), aStringLen);
+  }
+  else if (format == FILE_AS_DATAURI) {
+    rv = GetAsDataURIInternal(fileAsTextCallback, fileErrorCallback,
+                            reinterpret_cast<const char*>(aString), aStringLen);
+  }
+  else {
+    rv = NS_ERROR_FAILURE;

Can we really ever get here? We should probably at least assert that we don't...

- In nsDOMFile::GetAsTextInternal():

+  //Necessary to wrap the data string in an input stream (I believe), so we can initialize
+  // a converter stream with it

Add a space after // on the first line here. In fact, do that for all comments in this patch (or not) to stay consistent with surrounding code...

- In nsDOMFile::GetAsDataURIInternal():

+  char readBuf[4096];
+  PRUint32 leftOver = 0;
+  PRUint32 numRead;
+  PRUint32 totalRead = 0;;
+  do {
+    numRead = sizeof(readBuf) - leftOver;
+    PRUint32 amtRemaining = aDataLen - totalRead;
+    if (numRead > amtRemaining)
+      numRead = amtRemaining;
+
+    memcpy(readBuf + leftOver, aFileData + totalRead, numRead);
+    totalRead += numRead;
+
+    PRUint32 numEncode = numRead + leftOver;
+    leftOver = 0;
+
+    if (numEncode == 0) break;
+
+    // unless this is the end of the file, encode in multiples of 3
+    if (numRead > 0) {
+      leftOver = numEncode % 3;
+      numEncode -= leftOver;
+    }
+
+    // out buffer should be at least 4/3rds the read buf, plus a terminator
+    char *base64 = PL_Base64Encode(readBuf, numEncode, nsnull);
+    AppendASCIItoUTF16(base64, result);
+    PR_Free(base64);

Why memcpy() from aFileData into readBuf above, and have PL_Base64Encode() allocate? It would seem to me that you could do this more efficiently by passing aFileData + offset directly into PL_Base64Encode(), and you could pass in readBuf as the destination argument to PL_Base64Encode() and do the encoding without any extra allocations (except for appending to the result). Also, pass the length to AppendASCIItoUTF16() so it doesn't need to do a strlen() each time through this loop.

http://mxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/include/plbase64.h#46 explains the math for calculating buffer sizes n' all that good stuff.

+    if (leftOver) {
+      memmove(readBuf, readBuf + numEncode, leftOver);
+    }

If you do the above, this memmove should be avoidable as well.

- In nsDOMFile::GuessCharsetNew():

We have the bulk of this code elsewhere already don't we? Could we share code here?

Also, we should have tests that test that the loop in GetAsDataURIInternal() works right, i.e. have a test that deals with more than 4k of data and test that the result is correct. Also test with sizes where size % 3 equals 0, 1, and 2.

Other than that I think this is looking pretty good, but I do want to have one more look through this, so r- until then.

Comment 2

[Matin Movassate]

Attachment: Patch for async file access v2

Thanks for the feedback Johnny!

Improvements:
1. No reason for THREADSAFE AddRef/Release implementations. Removed. :-)
2. Stored open channels instead of active streamLoaders, and made sure to add channels to mOpenChannels *after* AsyncOpen returns.
3. Removed redundant cases in fileError switch statement.
4. Added an assertion to ensure we're fetching file contents in a legal format.
5. GetAsDataURIInternal() does away with the intermediary buffer and encodes directly to the string result.
6. New helper ConvertStreamNew now avoids the unnecessary InputStream wrapper and converts directly to the string result.
7. Added tests for determining correct GetAsDataURIInternal() performance on differing file sizes (specifically 3 different file sizes, one with length % 3 == 0, another with length % 3 == 1, and another with length % 3 == 2)

Still up for grabs:
1. Not sure what we should be doing about the older getAsText and getAsDataURL functions. I'll just leave them in until I hear definitive word regarding their status.

Comment 3

[Matin Movassate]

Attachment: Patch for async file access v3

Updated file API according to most recent version of spec: http://dev.w3.org/2006/webapi/FileUpload/publish/FileAPI.xhtml

One important difference, however: splicing is not yet supported. And because it's not yet implemented, there's no reason for there to be a distinction between the FileData and File classes.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

The patch may have a security bug.
The callbacks are implemented in JS and those are called asynchronously, so
JS stack may have some random context on top of it.
See a similar case, Bug 452762.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(IMO, the spec should use DOM events, not callbacks, so that support for progress events could be added later.)

Comment 6

[Matin Movassate]

Attachment: Patch for async file access v4

Fixed a minor bug; now media-types for undetectable files are null, as specified in the spec.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Please be careful with what I mentioned in comment #4.
As far as I see, nothing pushes the right JS context to the stack when
asynchronous callbacks are called.

Comment 8

[Matin Movassate]

Attachment: Patch for event-based async access, v1

Entirely new API for accessing file data! So we're no longer using function callbacks, but instead an event-based method for communicating file content and read progress. Check out Jonas' spec proposal here for specific details regarding this new functionality. (When considering FileRequest semantics, think XMLHttpRequest.) :

http://lists.w3.org/Archives/Public/public-webapps/2009JulSep/0565.html

I'm still uncertain as to the specific behavior of FileRequest's read-only attributes:
-|response|. On an error or an abort, should we set |response| to null? The empty string? The contents we've accumulated thus far? Right now, I set |response| to null.
-|readyState|. Should our FileRequest object be in the INITIAL stage up to and including dispatch of loadstart? Or just strictly before? (Meaning readyState == LOADING when loadstart gets dispatched, which is the behavior I currently have.) Also, on an abort or an error, should readyState's value be INITIAL or DONE? My guess is the former.
-|status|. What range of values can this hold? My implementation currently treats this attribute equivalently to the error codes of the previous callback-based API, meaning status == nsIDOMFileError::SUCCESS on successful reads, status == nsIDOMFileError:CANCEL_ERR on aborts, status == nsIDOMFileError::NOT_FOUND_ERR/NOT_READABLE_ERR/SECURITY_ERR on errors, etc.

Hopefully, most of this uncertainty will be resolved once the spec gets formally updated.

For test_fileapi.html, I essentially converted the tests I already had for the callback methods to accomodate event-based methods. Thus, the test is in no way a comprehensive assessment of the new API. In particular, progress events - almost the entire motivation for this redesign - aren't yet tested (though they're implemented). This should change soon. :-)

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 400227 [details] [diff] [review]
Patch for event-based async access, v1

You really should make nsDOMFileRequest inherit nsXHREventTarget. That would give you a lot of the progressevents stuff for free.

And make nsIDOMFileRequest not have all the on* attribute implemented on nsIXMLHttpRequestEventTarget as you'll get them for free through nsXHREventTarget. You'll still have to leave onloadend on nsIDOMFileRequest, but that's ok for now.

We should probably rename nsXHREventTarget to nsProgressEventsTarget in the process.

>diff --git a/content/base/public/nsIDOMFile.idl b/content/base/public/nsIDOMFile.idl
> interface nsIDOMFile : nsISupports
> {
>-  readonly attribute DOMString fileName;
>-  readonly attribute unsigned long long fileSize;
>+  readonly attribute DOMString name;
>+  readonly attribute unsigned long long size;

I think we need to leave the old names for at least one release. However it would be good to add a warning to the console any time someone accesses the deprecated names.

>diff --git a/content/base/public/nsIDOMFileError.idl b/content/base/public/nsIDOMFileError.idl
>+[scriptable, uuid(4BDAFB64-15E2-49C1-A090-4315A7884A56)]
>+interface nsIDOMFileError : nsISupports
>+{
>+  //File error codes
>+  const unsigned short SUCCESS = 0;
>+  const unsigned short NOT_FOUND_ERR = 8;
>+  const unsigned short NOT_READABLE_ERR = 24;
>+  const unsigned short SECURITY_ERR = 18;
>+  const unsigned short CANCEL_ERR = 25;
>+  const unsigned short ENCODING_ERR = 26;
>+
>+  readonly attribute unsigned short errorCode;
>+};

I think this should just be called 'code' in order to follow how HTMLMediaElement does it in the HTML5 spec.

>diff --git a/content/base/public/nsIDOMFileRequest.idl b/content/base/public/nsIDOMFileRequest.idl

>+[scriptable, uuid(074FEC26-7FAB-4E05-9E60-EC49E148F5EF)]
>+interface nsIDOMFileRequest : nsIDOMEventTarget
>+{
>+  void readAsBinaryString(in nsIDOMFile filedata);
>+  void readAsText(in nsIDOMFile filedata, [optional] in DOMString encoding);
>+  void readAsDataURL(in nsIDOMFile file);
>+
>+  void abort();
>+
>+  const unsigned short INITIAL = 0;
>+  const unsigned short LOADING = 1;
>+  const unsigned short DONE = 2;
>+  readonly attribute unsigned short readyState;
>+
>+  readonly attribute DOMString response;
>+  readonly attribute unsigned long status;

Actually, status here should not be an unsigned long, but rather an nsIDOMFileError. This is the pattern HTML5 is using, so I think we should follow that here.


>+NS_IMETHODIMP
>+nsDOMFile::GetInternalFile(nsIFile **aFile)
>+{
>+  NS_IF_ADDREF(*aFile = mFile);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMFile::SetInternalFile(nsIFile *aFile)
>+{
>+  mFile = aFile;
>+  return NS_OK;
>+}

>@@ -143,30 +181,16 @@ nsDOMFile::GetAsText(const nsAString &aC
...
> NS_IMETHODIMP
>-nsDOMFile::GetInternalFile(nsIFile **aFile)
>-{
>-  NS_IF_ADDREF(*aFile = mFile);
>-  return NS_OK;
>-}
>-
>-NS_IMETHODIMP
>-nsDOMFile::SetInternalFile(nsIFile *aFile)
>-{
>-  mFile = aFile;
>-  return NS_OK;
>-}

Why move these?

>diff --git a/content/base/src/nsDOMFileRequest.cpp b/content/base/src/nsDOMFileRequest.cpp
>+nsresult
>+nsDOMFileRequest::HandleEvent(nsIDOMEvent* aEvent)
>+{
>+  return NS_OK;
>+}

Looks like this is coming from nsIDOMLoadListener. I don't see a reason you need to implement that interface.

>+// nsIDOMLoadListener
>+
>+nsresult
>+nsDOMFileRequest::Abort(nsIDOMEvent* aEvent)
>+{
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsDOMFileRequest::BeforeUnload(nsIDOMEvent* aEvent)
>+{
>+  //Clean up state before the page exits
>+  Abort();
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsDOMFileRequest::Load(nsIDOMEvent* aEvent)
>+{
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsDOMFileRequest::Unload(nsIDOMEvent* aEvent)
>+{
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsDOMFileRequest::Error(nsIDOMEvent* aEvent)
>+{
>+  return NS_OK;
>+}

None of these should be needed.

>+NS_IMETHODIMP
>+nsDOMFileRequest::ReadAsBinaryString(nsIDOMFile* aFile)
>+{
>+  nsString aCharset;
>+  nsresult rv = ReadFileContent(aFile, aCharset, FILE_AS_BINARY);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMFileRequest::ReadAsText(nsIDOMFile* aFile,
>+                             const nsAString &aCharset)
>+{
>+  nsresult rv = ReadFileContent(aFile, aCharset, FILE_AS_TEXT);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMFileRequest::ReadAsDataURL(nsIDOMFile* aFile)
>+{
>+  nsString aCharset;
>+  nsresult rv = ReadFileContent(aFile, aCharset, FILE_AS_DATAURL);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return NS_OK;
>+}

For all of these, just end with:

return ReadFileContent(...);

And just do ReadFileContent(aFile, EmptyString(), ...) for the callsites that don't use the charset parameter.

>+NS_IMETHODIMP
>+nsDOMFileRequest::Abort()
>+{
>+  if (mReadyState != nsIDOMFileRequest::LOADING)
>+    return NS_OK;
>+
>+  //Clear progress data
>+  mResponse.Truncate();
>+  mReadCount = 0;
>+  mDataLen = 0;
>+
>+  //Revert status, response and readystate attributes
>+  SetDOMStringToNull(mResponse);

The SetDOMStringToNull call empties the string, so the Truncate() call isn't needed.



>+  mReadyState = nsIDOMFileRequest::INITIAL;
>+  mStatus = nsIDOMFileError::CANCEL_ERR;
>+    
>+  //Non-null channel indicates a read is currently active
>+  if (mChannel) {
>+    nsCOMPtr<nsIRequest> request = do_QueryInterface(mChannel);
>+    NS_ENSURE_TRUE(request, NS_ERROR_FAILURE);
>+
>+    //Cancel request requires an error status
>+    request->Cancel(NS_ERROR_FAILURE);
>+  }

No need for the QI here. nsIChannel inherits nsIRequest, so you can call Cancel on the mChannel directly.

>+  //Clean up memory buffer
>+  if (mFileData) {
>+    free(mFileData);
>+    mFileData = NULL;
>+  }

Use 'nsnull' rather than 'NULL'.
You should use PR_Free rather than 'free'. Same goes for malloc/realloc elsewhere.
(PR_)Free is null-safe, so no need for the if-statement.

>+// nsIProgressEventSink
>+
>+NS_IMETHODIMP
>+nsDOMFileRequest::OnProgress(nsIRequest *aRequest,
>+                             nsISupports *aContext,
>+                             PRUint64 aProgress,
>+                             PRUint64 aProgressMax)
>+{
>+  //Update our progress fields and dispatch the event
>+  mReadTransferred = aProgress;
>+  mReadTotal = aProgressMax;
>+  DispatchProgressEvent(NS_LITERAL_STRING(PROGRESS_STR));
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMFileRequest::OnStatus(nsIRequest *aRequest,
>+                           nsISupports *aContext,
>+                           nsresult aStatus,
>+                           const PRUnichar *aStatusArg)
>+{
>+  return NS_OK;
>+}

Hmm.. actually, I think we don't really need to use a nsIProgressEventSink here. You can simply dispatch "progress" events from OnDataAvailable instead. That should simplify the code.

Additionally, we don't want to dispatch a "progress" event every time we get data, as that could be way too often. Instead we should dispatch it every 50ms at most. See the nsXMLHttpRequest::StartProgressEventTimer function for how to set up a timer to do this.

>+NS_IMETHODIMP
>+nsDOMFileRequest::OnStopRequest(nsIRequest *aRequest,
>+                                nsISupports *aContext,
>+                                nsresult aStatus)
>+{
>+  //If we're here as a result of a call from Abort(),
>+  //simply ignore the request.
>+  if (mReadyState == nsIDOMFileRequest::INITIAL)
>+    return NS_OK;
>+
>+  //FileRequest must be in DONE stage after a load
>+  mReadyState = nsIDOMFileRequest::DONE;
>+
>+  //Set the status field as appropriate
>+  if (NS_FAILED(aStatus))
>+    return DispatchError(aStatus);
>+
>+  mStatus = nsIDOMFileError::SUCCESS;
>+
>+  nsresult rv;
>+  switch (mDataFormat) {
>+    case FILE_AS_BINARY:
>+      AppendASCIItoUTF16(mFileData, mResponse);
>+      break;
>+    case FILE_AS_TEXT:
>+      rv = GetAsText(mCharset, mFileData, mDataLen, mResponse);
>+      break;
>+    case FILE_AS_DATAURL:
>+      rv = GetAsDataURL(mFile, mFileData, mDataLen, mResponse);
>+      break;
>+    default:
>+     return NS_ERROR_FAILURE;
>+  }

Actually, the idea is for FILE_AS_BINARY and FILE_AS_TEXT to continuously update mResponse, so that you can see the data read so far in the .response member during progress events. However since partial data-urls doesn't really make sense it's fine to do just data-urls at the end though.

Ok, need to get to bed. Got as far as nsDOMFileRequest::ReadFileContent

Let me know when you think you'll have time to work on this, and if you need any help. Sorry it took so long to get to this review :(

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> (From update of attachment 400227 [details] [diff] [review])
> You really should make nsDOMFileRequest inherit nsXHREventTarget. That would
> give you a lot of the progressevents stuff for free.
Sounds reasonable.
Though, nsXHREventTarget inherits nsIXMLHttpRequestEventTarget, so that
would need to be changed.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 400227 [details] [diff] [review] [details])
> > You really should make nsDOMFileRequest inherit nsXHREventTarget. That would
> > give you a lot of the progressevents stuff for free.
> Sounds reasonable.
> Though, nsXHREventTarget inherits nsIXMLHttpRequestEventTarget, so that
> would need to be changed.

Indeed, we should probably rename that nsIProgressEventsTarget or some such

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

nsIXMLHttpRequestEventTarget is from the XHR2 draft (where it is called XMLHttpRequestEventTarget), so we shouldn't rename it.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But it's a [NoInterfaceObject] so it's not actually exposed to script. I really think we should stop following interface names defined by specs. Specs don't optimize for few interfaces and it's costing us in implementation complexity and performance to follow them. So in the case when interfaces aren't exposed to script, I see no benefit to following them to the letter.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> But it's a [NoInterfaceObject] so it's not actually exposed to script.
Ah, indeed. My mistake.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 400227 [details] [diff] [review]
Patch for event-based async access, v1

You should probably make nsDOMFileRequest::Abort() clear the mFile member, just to keep things consistent.

There's a feature that's missing, though that should probably wait until a separate patch. Basically we should update the mResponse member as data is coming in, so that you can read the data read so far during progress events. Except when reading as a data-url (partial data-urls doesn't really make sense). Please file a bug on this.


>+nsDOMFileRequest::ReadFileContent(nsIDOMFile* aFile,
>+	      	 	          const nsAString &aCharset,
>+                                  PRUint32 aDataFormat)

Fix whitespace. You have a tab-character in there that's messing things up. Please make sure there's no tab characters anywhere in the patch. Makefiles are the only exception, they're allowed there.

>+{ 
...
>+  //Set our notification callbacks attr. for progress events
>+  rv = mChannel->SetNotificationCallbacks(this);
>+  NS_ENSURE_SUCCESS(rv, rv);

If you remove the nsIProgressEventSink implementation, which I think you can, then I think you can also remove this call as well as the nsIInterfaceRequestor implementation.

>+  rv = mChannel->AsyncOpen(this, nsnull);

Add an NS_ENSURE_SUCCESS(rv, rv) after the call to AsyncOpen.

>+  //FileRequest should be in loading state here
>+  mReadyState = nsIDOMFileRequest::LOADING;
>+  DispatchProgressEvent(NS_LITERAL_STRING(LOADSTART_STR));
>+
>+  if (NS_SUCCEEDED(rv)) {
>+    mStatus = nsIDOMFileError::SUCCESS;
>+    return NS_OK;
>+  }

Set this up at the top of the function instead. This should be the default status.

>+  //Despite error, we should still be in DONE stage
>+  mReadyState = nsIDOMFileRequest::DONE;
>+
>+  //Set the response string to null on an error
>+  SetDOMStringToNull(mResponse);
>+
>+  return DispatchError(rv);

This is a bit unintuitive. It's much easier to read to put error-handling in if-statements, and let the main code-line handle the common successful case. Anyhow, most of what you're doing here you don't actually need to do. If AsyncOpen returns an error we don't need to dispatch an error event, nor set mReadyState to DONE. Just doing an NS_ENSURE_SUCCESS(rv, rv) produces the result we want, that'll throw an exception in javascript. If the call to readAsX fails, we basically don't want any of the members to change (beyond what the call to Abort() changes).

>+nsresult
>+nsDOMFileRequest::DispatchError(nsresult rv)
>+{
>+  //Set the status attribute, and dispatch the error event
>+  switch (rv) {
>+    case NS_ERROR_FILE_NOT_FOUND:
>+      mStatus = nsIDOMFileError::NOT_FOUND_ERR;
>+      break;
>+    case NS_ERROR_FILE_ACCESS_DENIED:
>+      mStatus = nsIDOMFileError::SECURITY_ERR;
>+      break;
>+    case NS_ERROR_DOM_FILE_NOT_READABLE_ERR:
>+      mStatus = nsIDOMFileError::NOT_READABLE_ERR;
>+      break;
>+    default:
>+      return NS_ERROR_FAILURE;

I think the default case should just be to use nsIDOMFileError::NOT_READABLE_ERR. That way we'll always fire an error that is somewhat descriptive. Also make DispatchError return 'void' since it always succeeds.

>+void
>+nsDOMFileRequest::DispatchProgressEvent(const nsAString& aType)
>+{
>+  DispatchProgressEvent(this, aType, mReadComplete, mReadTransferred, mReadTotal);
>+}
>+
>+void
>+nsDOMFileRequest::DispatchProgressEvent(nsPIDOMEventTarget* aTarget,
>+                                        const nsAString& aType,
>+                                        PRBool aLengthComputable,
>+                                        PRUint64 aLoaded, PRUint64 aTotal)

Why have two separate DispatchProgressEvent methods? It seems like everyone is calling the first version anyway so just use that signature and let the implementation use the members directly.

...
>+  aTarget->DispatchDOMEvent(nsnull, event, nsnull, nsnull);

You can actually use nsEventDispatcher::Dispatch here, that's somewhat cleaner

>+nsDOMFileRequest::GetAsText(const nsAString &aCharset,
>+                            const char *aFileData,
>+                            PRUint32 aDataLen,
>+                            nsAString& aResult)
>+{
>+  nsresult rv;
>+  nsCAutoString charsetGuess;
>+  if (!aCharset.IsEmpty()) {
>+    CopyUTF16toUTF8(aCharset, charsetGuess);
>+  } else {
>+    rv = GuessCharset(aFileData, aDataLen, charsetGuess);
>+    NS_ENSURE_SUCCESS(rv, DOMFileResult(rv));
>+  }

No need to call DOMFileResult here. Just do the normal NS_ENSURE_SUCCESS(rv, rv).
That'll also remove the only caller to DOMFileResult, so you can just remove that function.

>+nsDOMFileRequest::ConvertStream(const char *aFileData,
>+                                PRUint32 aDataLen,
>+                                const char *aCharset,
>+                                nsAString &aResult)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsICharsetConverterManager> charsetConverter = 
>+    do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIUnicodeDecoder> unicodeDecoder;
>+  rv = charsetConverter->GetUnicodeDecoder(aCharset, getter_AddRefs(unicodeDecoder));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRInt32 destLength;
>+  rv = unicodeDecoder->GetMaxLength(aFileData, aDataLen, &destLength);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRInt32 srcLength = aDataLen;
>+  aResult.SetLength(destLength); //Make sure we have enough space for the conversion

nit: the grouping here is a bit strange. These two lines of code have nothing to do with each other, but it almost looks like they do until you look in detail. Group the |srcLength = ...| line with the call to Convert below instead.

You also need to make sure that the call to SetLength succeeds, otherwise you'll have a buffer overflow problem. And you should use SetCapacity rather than SetLength. A simple solution is to simply do:

aResult.SetCapacity(destLength);
destLength = aResult.Capacity();

>+nsDOMFileRequest::GuessCharset(const char *aFileData,
>+                               PRUint32 aDataLen,
>+                               nsACString &aCharset)
...
>+  if (detector) {
>+    detector->Init(this);
>+
>+    PRBool done;
>+    PRUint32 numRead; 
>+    PRUint32 totalRead = 0;
>+    do {
>+      char readBuf[4096];
>+      PRUint32 amtRemaining = aDataLen - totalRead;
>+      numRead = (amtRemaining >= sizeof(readBuf) ? sizeof(readBuf) : amtRemaining);
>+      memcpy(readBuf, aFileData, numRead);
>+      totalRead += numRead;
>+
>+      rv = detector->DoIt(readBuf, numRead, &done);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    } while (!done && numRead > 0);

This looks all wrong. You're always reading from the beginning of the aFileData buffer every time. Also, why do the memcpy at all? That way there's no need to do anything in a loop either. Something like this should work:

rv = detector->DoIt(aFileData, aDataLen, &done);
NS_ENSURE_SUCCESS(rv, rv);

rv = detector->Done();
NS_ENSURE_SUCCESS(rv, rv);



...
>+  if (aCharset.IsEmpty()) {
>+    // no charset detected, default to the system charset
>+    nsCOMPtr<nsIPlatformCharset> platformCharset =
>+      do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+      rv = platformCharset->GetCharset(kPlatformCharsetSel_PlainTextInFile,
>+                                       aCharset);
>+    }
>+  }
>+
>+  if (aCharset.IsEmpty()) {
>+    // no sniffed or default charset, try UTF-8
>+    aCharset.AssignLiteral("UTF-8");
>+  }

This isn't working, is it? aCharset never gets updated during the call to the charsetdetector, only mCharset is being updated. I'd say simply remove the aCharset argument and use mCharset everywhere in this function.


>diff --git a/content/base/src/nsDOMFileRequest.h b/content/base/src/nsDOMFileRequest.h

>+#include "nsISupportsUtils.h"      
>+#include "nsString.h"              
>+#include "nsIDOMLoadListener.h"    
>+#include "nsIDOMDocument.h"        
>+#include "nsIURI.h"                
>+#include "nsIHttpChannel.h"        
>+#include "nsIDocument.h"           
>+#include "nsIStreamListener.h"     
>+#include "nsWeakReference.h"       
>+#include "jsapi.h"                 
>+#include "nsIScriptContext.h"      
>+#include "nsIInterfaceRequestor.h" 
>+#include "nsIHttpHeaderVisitor.h"  
>+#include "nsIProgressEventSink.h"  
>+#include "nsCOMArray.h"            
>+#include "nsJSUtils.h"             
>+#include "nsTArray.h"              
>+#include "nsIJSNativeInitializer.h"
>+#include "nsIDOMLSProgressEvent.h" 
>+#include "nsClassHashtable.h"      
>+#include "nsHashKeys.h"            
>+#include "prclist.h"               
>+#include "prtime.h"                
>+#include "nsIDOMNSEvent.h"         
>+#include "nsITimer.h"              
>+#include "nsIPrivateDOMEvent.h"    
>+#include "nsDOMProgressEvent.h"    
>+#include "nsDOMEventTargetHelper.h"
>+#include "nsICharsetDetector.h"
>+#include "nsICharsetDetectionObserver.h"
>+
>+#include "nsIDOMFile.h"
>+#include "nsIDOMFileRequest.h"
>+#include "nsIDOMFileInternal.h"
>+#include "nsIDOMFileList.h"
>+#include "nsIDOMFileError.h"
>+#include "nsIInputStream.h"
>+#include "nsCOMPtr.h"
>+#include "nsIStreamLoader.h"
>+#include "nsIChannel.h"

That's a lot of #includes! Are all of these really needed?


Still need to review the tests, but other than that the patch looks great! Thanks for doing this, especially for fixing up the last few things from school!

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

To follow up on the conversation we had on irc regarding how to do errorhandling when the file doesn't exist.

So it really is a bug that AsyncOpen throws an error if the file doesn't exist. What it should do is let AsyncOpen successfully finish, but then call OnStartRequest with an errorcode.

What you can do to work around the bug is this:

1. Add a member like |nsCOMPtr<nsRunnable<nsDOMFileRequest> > mAsyncOpenRunner;|
   to nsDOMFileRequest
2. In readAsX, don't actually call AsyncOpen, just set up the channel and store
   it in a member. Instead start a runnable to call some function, say
   'OpenChannel', asynchronously. See [1] for an example how.
3. In OpenChannel, call mChannel->AsyncOpen, if that fails, manually call
   OnStartRequest/OnStopRequest with the error code from AsyncOpen in order
   to get the right events fired.
4. In Abort, you need to handle the case when the runnable has been scheduled
   but not started.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2792

Comment 17

[Matin Movassate]

Attachment: Patch for event-based async access, v2

New version of the patch, with every change mentioned above either integrated or discussed with Jonas and ultimately discarded. :-) Still got a few bugs to file though: 1) Shouldn't be relying on the return value of the call to asyncOpen(), 2) GetAsText() should update the mResponse member to reflect all the file text that has come through , 3) Need to ensure that the code works with large files, which currently may not be the case since some variables used to represent size are only 32 bits long.

Comment 18

[Matin Movassate]

Attachment: Patch for event-based async access, v3

Latest version of the patch! Unnecessary #includes and comments removed, error-handling w.r.t. AsyncOpen() tidied up, mReadyState == DONE for events fired from an abort.

Comment 19

[Matin Movassate]

Attachment: Patch for event-based async access, v4

Removed even more superfluous headers, and added an explicit finish to test_fileapi.html so as to ensure that asynchronously-dispatched test cases would complete.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/mozilla-central/rev/a972e284e6ca

Checked in to trunk! Yay!

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/491684dbfd7f

Comment 22

[Eric Shepherd [:sheppy]]

Could someone that knows this API please add comments to the IDL? Aren't IDL files required to have comments before being committed into the tree?

Comment 23

[Arun Ranganathan]

Bug 524744 makes reference to http://dev.w3.org/2006/webapi/FileAPI/ which may result in interface and attribute name changes.

Comment 24

[Timon Van Overveldt]

Hi,

I just started expirmenting with the new File capabilities in Firefox 3.6b3.

I started using File.getAsBinary() together with XMLHttpRequest.sendAsBinary() to send a multipart HTTP request (uploading a file).
Everything worked great.

Now today I noticed that the File documentation page was updated, showing the deprecation of the File.getAsX() methods and the addition of the FileReader API.
I also noticed that the FileReader API is named FileRequest in Firefox 3.6b3.

So I set out with the FileReader/FileRequest documentation to try and update my current code.
After doing this, I noticed that the output of FileRequest.getAsBinaryString() was different than the output I previously got with the File.getAsBinary() method.
This resulted in the following error when trying to send a JPEG file (firebug shows it to originate from the line where the XMLHttpRequest.sendAsBinary() call is made)
 -- String contains an invalid character" code: "5 --

I also verified my observations, directly comparing the result of File.getAsBinary() and FileRequest.getAsBinaryString():
- The resulting string is the same length for both methods.
- However, checking the equality of both results with the "==" operator in JS, the result was "false" (not equal)

Is this a bug, or am I missing something?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, also, in 3.6 you don't have to read the file contents into a string and use xhr.sendAsBinary. You can simply do:

xhr = new XMLHttpRequest();
xhr.open("POST", ...);
xhr.send(file);

Of course, that means that you can't send the file as part of a multipart request. That's covered by bug 528524. Hopefully that'll get implemented in Firefox 3.7

Comment 26

[Arun Ranganathan]

> 
> I started using File.getAsBinary() together with XMLHttpRequest.sendAsBinary()
> to send a multipart HTTP request (uploading a file).
> Everything worked great.
> 
> Now today I noticed that the File documentation page was updated, showing the
> deprecation of the File.getAsX() methods and the addition of the FileReader
> API.
> I also noticed that the FileReader API is named FileRequest in Firefox 3.6b3.


The name change FileRequest --> FileReader is already present in subsequent releases.

> 
> So I set out with the FileReader/FileRequest documentation to try and update my
> current code.
> After doing this, I noticed that the output of FileRequest.getAsBinaryString()
> was different than the output I previously got with the File.getAsBinary()
> method.

You probably mean FileReader.readAsBinaryString(_ )...

> This resulted in the following error when trying to send a JPEG file (firebug
> shows it to originate from the line where the XMLHttpRequest.sendAsBinary()
> call is made)
>  -- String contains an invalid character" code: "5 --
> 
> I also verified my observations, directly comparing the result of
> File.getAsBinary() and FileRequest.getAsBinaryString():
> - The resulting string is the same length for both methods.
> - However, checking the equality of both results with the "==" operator in JS,
> the result was "false" (not equal)
> 
> Is this a bug, or am I missing something?


This is likely to be bug 530220 which has been fixed.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oops, I posted my reply to comment 24 to the wrong bug. Here it is:

I think you might be seeing bug 530220. It is fixed in 3.6 beta 4 which is due
to be released soon. In that release FileReader is also correctly named.

If you don't want to wait for 3.6beta4 to be released, you can download a
nightly release from the same branch from here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.2/

Thanks a ton for experimenting with this! Definitely let us know if you see any
more problems, or if the above builds doesn't fix the problems you're seeing.

Comment 28

[Henri Sivonen (:hsivonen)]

Why was a dependency on character encoding guessing introduced here?

It seems like a bad idea to expose the "universal" detector to the Web, since it's not really universal. It's actually rather arbitrary in what it supports (probably dependent on the interests of people on Netscape's i18n team).

Is there any chance we could remove the dependency on the encoding detector in the file API?

Comment 29

[Henri Sivonen (:hsivonen)]

http://www.w3.org/TR/FileAPI/#encoding-determination seems to default to UTF-8 and not to guessing.

Comment 30

[Henri Sivonen (:hsivonen)]

Filed bug 848842.