Closed Bug 664783 Opened 13 years ago Closed 13 years ago

Implement FileAPI: FileReaderSync

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: wchen, Assigned: wchen)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

Patch for FileReaderSync rev 4
121.63 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Assignee: nobody → wchen
http://dev.w3.org/2006/webapi/FileAPI/#FileReaderSync

As Mike Smith likes to quote, "/TR stands for Trash".
Initial patch for implementation of FileReaderSync. Comments are appreciated.
Attachment #539919 - Flags: review?(bent.mozilla)
This won't work in the general case ... if the nsIInputStream is non-blocking you're going to lose.  It also kinda sucks that we're going to block a thread on doing IO (even if it's not the main thread) but I don't think that's avoidable.
The whole point of FileReaderSync is to block while reading (hence the 'Sync' part of the name), so indeed it's unavoidable.

I think it's fine to assume that the inputstream returned from the blob is blocking. At worst, make readAsX throw an exception if the stream is non-blocking.
(In reply to comment #4)
> The whole point of FileReaderSync is to block while reading (hence the
> 'Sync' part of the name), so indeed it's unavoidable.

Well the point of FileReaderSync is to block the script, that doesn't ipso facto mean blocking the thread ... in our implementation the distinction is irrelevant though. :-/

> I think it's fine to assume that the inputstream returned from the blob is
> blocking. At worst, make readAsX throw an exception if the stream is
> non-blocking.

Yeah this we can live with, but we should ensure that we bail noticeably.

Another thing I noticed, your read as data URL stuff should use my shiny new "Encode an input stream to base 64" API (http://mxr.mozilla.org/mozilla-central/source/xpcom/io/Base64.h?force=1).
Comment on attachment 539919 [details] [diff] [review]
Patch for implementation of FileReaderSync

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

This looks really good overall! You've got some general problems with our string class usage (they're ridiculously confusing, no worries!) and a few other problems with the structured clone stuff but nothing too big. I pointed out  abunch of style things too just so that you can get used to the way we do this.

::: content/base/src/nsDOMFile.cpp
@@ +139,5 @@
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  MimeDetectorRunnable(nsIFile* aFile, nsAString& aType)

Hm. It looks like you don't need this second parameter at all since you're going to call GetContentType() in the end.

@@ +151,5 @@
> +    nsCOMPtr<nsIMIMEService> mimeService =
> +      do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCAutoString mimeType;

Just use nsCString here. Using an auto string means that you'll copy the result into stack space rather than just using a refcounted heap pointer.

@@ +169,5 @@
> +    return mContentType;
> +  }
> +
> +private:
> +  nsAutoString mContentType;

nsString is what you want here.

@@ +257,5 @@
>  nsDOMFile::GetType(nsAString &aType)
>  {
>    if (mContentType.IsEmpty() && mFile && mIsFullFile) {
> +    nsCOMPtr<nsIThread> mainThread;
> +    nsresult rv = NS_GetMainThread(getter_AddRefs(mainThread));

This is unnecessary, see below.

@@ +263,5 @@
>  
> +    // Create a runnable to detect the mime type on the main thread
> +    // because detection is not thread safe.
> +    nsRefPtr<MimeDetectorRunnable> runnable =
> +      new MimeDetectorRunnable(mFile, aType);

You don't want to do this if you're already on the main thread. Use NS_IsMainThread() to tell.

@@ +268,2 @@
>  
> +    rv = mainThread->Dispatch(runnable, NS_DISPATCH_SYNC);

Just use NS_DispatchToMainThread() here.

::: dom/workers/Events.cpp
@@ +42,5 @@
>  #include "jsapi.h"
>  #include "jscntxt.h"
>  
> +#include "nsAutoPtr.h"
> +#include "nsIDOMFile.h"

Hopefully you can remove these when you move the structured clone callbacks, see below.

@@ +47,5 @@
>  #include "nsTraceRefcnt.h"
>  
> +#include "Exceptions.h"
> +#include "File.h"
> +#include "Worker.h"

These too.

@@ +508,5 @@
>    }
>  
> +  static JSObject*
> +  ReadStructuredClone(JSContext* aCx, JSStructuredCloneReader* aReader,
> +                      uint32 aTag, uint32 aData, void* aClosure)

All this needs to live in WorkerPrivate.cpp, in the first WorkerStructuredCloneCallbacks.

@@ +529,5 @@
> +        if (!jsFile) {
> +          return NULL;
> +        }
> +
> +        return jsFile;

Just return jsFile, no need to null-check and return NULL :)

@@ +571,5 @@
>        }
>  
> +      // Release reference to objects that were AddRef'd for
> +      // cloning into worker.
> +      event->mClonedObjects.Clear();

You still want to do this even if something failed above. You can make a nsTArray on the stack, swap elements from mClonedObjects, then it will auto-clear when it goes out of scope.

::: dom/workers/Exceptions.cpp
@@ +376,5 @@
> +  JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, Finalize,
> +  JSCLASS_NO_OPTIONAL_MEMBERS
> +};
> +
> +

Nit: remove extra newline here

@@ +449,5 @@
>  bool
>  InitClasses(JSContext* aCx, JSObject* aGlobal)
>  {
> +  return !!DOMException::InitClass(aCx, aGlobal) &&
> +         !!FileException::InitClass(aCx, aGlobal);

No need for the !! if you're using &&.

::: dom/workers/File.cpp
@@ +42,5 @@
> +#include "jsapi.h"
> +#include "jsatom.h"
> +#include "jscntxt.h"
> +
> +#include "WorkerInlines.h"

I don't think you use this.

@@ +48,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsIDOMFile.h"
> +#include "nsISupportsUtils.h"
> +#include "nsJSUtils.h"
> +#include "nsStringBuffer.h"

You shouldn't need this.

@@ +89,5 @@
> +
> +protected:
> +  Blob()
> +  {
> +  }

You don't ever expect to instantiate this class, right? Best to leave the constructor/destructor unimplemented to enforce that. (And add a comment to that effect!)

@@ +135,5 @@
> +    }
> +
> +    PRUint64 size;
> +    if (NS_FAILED(blob->GetSize(&size))) {
> +      size = 0;

Hm. Should we throw an exception if this fails?

@@ +153,5 @@
> +    if (!blob) {
> +      return false;
> +    }
> +
> +    nsAutoString type;

nsString.

@@ +155,5 @@
> +    }
> +
> +    nsAutoString type;
> +    if (NS_FAILED(blob->GetType(type))) {
> +      type.Truncate();

Should we throw here?

@@ +179,5 @@
> +      return false;
> +    }
> +
> +    jsdouble start, end = 0;
> +    JSString* jsContentType = JS_NewUCStringCopyZ(aCx, NULL);

You want |JS_GetEmptyString(JS_GetRuntime(aCx))| there.

@@ +196,5 @@
> +    if (NS_FAILED(blob->MozSlice(xpc_qsDoubleToUint64(start),
> +                                 xpc_qsDoubleToUint64(end),
> +                                 contentType, optionalArgc,
> +                                 getter_AddRefs(rtnBlob)))) {
> +      return false;

Remember, any time you return false from one of these js functions you need to have an exception set.

@@ +265,5 @@
> +        nsISupports* priObj = static_cast<nsISupports*>(JS_GetPrivate(aCx, aObj));
> +        nsCOMPtr<nsIDOMFile> file = do_QueryInterface(priObj);
> +        if (file) {
> +          return file;
> +        }

You should just assert |file| here rather than checking the result of QI. After all, you wouldn't have set it as your private obj if it didn't QI to nsIDOMFile.

@@ +278,5 @@
> +    return &sClass;
> +  }
> +
> +protected:
> +  File()

Same comments here about constructor/destructor being left unimplemented.

@@ +323,5 @@
> +    if (!file) {
> +      return false;
> +    }
> +
> +    nsAutoString fullPath;

nsString.

@@ +325,5 @@
> +    }
> +
> +    nsAutoString fullPath;
> +    if (NS_FAILED(file->GetMozFullPath(fullPath))) {
> +      fullPath.Truncate();

Throw?

@@ +346,5 @@
> +    if (!file) {
> +      return false;
> +    }
> +
> +    nsAutoString name;

nsString.

@@ +388,5 @@
> +      nsISupports* priObj = static_cast<nsISupports*>(JS_GetPrivate(aCx, aObj));
> +      nsCOMPtr<nsIDOMBlob> blob = do_QueryInterface(priObj);
> +      if (blob) {
> +        return blob;
> +      }

Just assert here again.

@@ +415,5 @@
> +
> +nsIDOMBlob*
> +GetPrivate(JSContext* aCx, JSObject* aObj)
> +{
> +  return Blob::GetPrivate(aCx, aObj);

Hm... It's feasible that someone could call this with a File object, right? You could still return a nsIDOMBlob from that here whereas you're now going to return null.

::: dom/workers/File.h
@@ +42,5 @@
> +
> +#include "Workers.h"
> +
> +#include "jspubtd.h"
> +#include "nsIDOMFile.h"

No need for this here, just forward declare nsIDOMFile and nsIDOMBlob.

@@ +46,5 @@
> +#include "nsIDOMFile.h"
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +namespace blob {

Hm, let's just have one sub-namespace, 'file'. Then you can have one InitClasses function, CreateBlob/CreateFile.

@@ +55,5 @@
> +JSObject*
> +Create(JSContext* aCx, nsIDOMBlob* aBlob);
> +
> +nsIDOMBlob*
> +GetPrivate(JSContext* aCx, JSObject* aObj);

I think these should be 'GetDOMBlobFromJSObject' and 'GetDOMFileFromJSObject'

::: dom/workers/FileReaderSync.cpp
@@ +58,5 @@
> +
> +namespace {
> +
> +static bool
> +ResultSuccess(JSContext* aCx, nsresult rv)

This should probably be named something like 'EnsureSucceededOrThrow' or 'ThrowIfFailed'

@@ +107,5 @@
> +    return NULL;
> +  }
> +
> +private:
> +  FileReaderSync()

Since your private object lives elsewhere these MOZ_COUNT functions aren't ever going to be hit.

@@ +137,5 @@
> +    return NULL;
> +  }
> +
> +  static nsIDOMBlob*
> +  GetPrivateBlob(JSContext* aCx, JSObject* aObj) {

This doesn't really need to live in FileReaderSync, this could just be an inline function in this file's scope. And maybe rename to GetDOMBlobFromJSObject.

@@ +191,5 @@
> +  {
> +    JSObject* obj = JS_THIS_OBJECT(aCx, aVp);
> +
> +    FileReaderSyncPrivate* fileReader =
> +      GetInstancePrivate(aCx, obj, "readAsArrayBuffer");

You need to null check here.

@@ +201,5 @@
> +
> +    nsIDOMBlob* blob = GetPrivateBlob(aCx, jsBlob);
> +    if (!blob) {
> +      return false;
> +    }

Hm, this pattern (convert args, get nsIDOMBlob*) is repeated for all ReadAs* functions, you could move this into a helper.

@@ +230,5 @@
> +  {
> +    JSObject* obj = JS_THIS_OBJECT(aCx, aVp);
> +
> +    FileReaderSyncPrivate* fileReader =
> +      GetInstancePrivate(aCx, obj, "readAsDataURL");

Null check.

@@ +242,5 @@
> +    if (!blob) {
> +      return false;
> +    }
> +
> +    nsAutoString blobText;

nsString.

@@ +268,5 @@
> +        GetInstancePrivate(aCx, obj, "readAsBinaryString");
> +
> +    JSObject* jsBlob;
> +    if (!JS_ConvertArguments(aCx, aArgc, JS_ARGV(aCx, aVp), "o",
> +                             &jsBlob)) {

Hm, why'd you wrap this?

@@ +277,5 @@
> +    if (!blob) {
> +      return false;
> +    }
> +
> +    nsAutoString blobText;

nsString

@@ +302,5 @@
> +    FileReaderSyncPrivate* fileReader =
> +      GetInstancePrivate(aCx, obj, "readAsText");
> +
> +    JSObject* jsBlob;
> +    JSString* jsEncoding = JS_NewUCStringCopyZ(aCx, NULL);

EmptyString trick here too.

@@ +304,5 @@
> +
> +    JSObject* jsBlob;
> +    JSString* jsEncoding = JS_NewUCStringCopyZ(aCx, NULL);
> +    if (!JS_ConvertArguments(aCx, aArgc, JS_ARGV(aCx, aVp), "o/S",
> +                             &jsBlob, &jsEncoding)) {

Nit: wrapping looks wrong, jsBlob should fit on previous line.

@@ +310,5 @@
> +    }
> +
> +    nsDependentJSString encoding;
> +    if (!encoding.init(aCx, jsEncoding)) {
> +      ThrowFileExceptionForCode(aCx, FILE_NOT_READABLE_ERR);

Hm, I think this is wrong. If init fails it will set the exception for you, but it will be some OOM exception, not FILE_NOT_READABLE. If you need that exception somehow then this is the wrong place for it.

@@ +319,5 @@
> +    if (!blob) {
> +      return false;
> +    }
> +
> +    nsAutoString blobText;

nsString

@@ +343,5 @@
> +  JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, Finalize,
> +  JSCLASS_NO_OPTIONAL_MEMBERS
> +};
> +
> +uint16 FileReaderSync::sFunctionFlags = JSPROP_ENUMERATE;

This won't compile on some of our linux build machines for some reason, just #define it instead at the top of the file like in Events.cpp.

::: dom/workers/FileReaderSyncPrivate.cpp
@@ +76,5 @@
> +}
> +
> +nsresult
> +FileReaderSyncPrivate::ReadAsArrayBuffer(nsIDOMBlob* aBlob, ArrayBuffer* aArrayBuffer)
> +{

Let's assert that this is not called on the main thread. Same with other ReadAs functions since they all block.

@@ +85,5 @@
> +  PRUint32 numRead;
> +  rv = stream->Read((char*) aArrayBuffer->data,
> +                    aArrayBuffer->byteLength, &numRead);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ASSERTION(numRead == aArrayBuffer->byteLength, "failed to read data");

I think you should also assert that stream->Available() is 0.

@@ +93,5 @@
> +
> +nsresult
> +FileReaderSyncPrivate::ReadAsBinaryString(nsIDOMBlob* aBlob, nsAString& aResult)
> +{
> +  aResult.Truncate();

This isn't really necessary, if something fails you'll throw any way.

@@ +103,5 @@
> +  PRUint32 numRead;
> +  do {
> +    char readBuf[4096];
> +    rv = stream->Read(readBuf, sizeof(readBuf), &numRead);
> +    NS_ENSURE_SUCCESS(rv, rv);

Nit: Newline here.

@@ +110,5 @@
> +    if (aResult.Length() - oldLength != numRead) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +  } while (numRead > 0);
> +

Assert stream->Available() is 0 here too.

@@ +116,5 @@
> +}
> +
> +nsresult
> +FileReaderSyncPrivate::ReadAsText(nsIDOMBlob* aBlob,
> +                                  const nsAString& aEncoding, nsAString& aResult)

Nit: wrapping looks wrong here

@@ +118,5 @@
> +nsresult
> +FileReaderSyncPrivate::ReadAsText(nsIDOMBlob* aBlob,
> +                                  const nsAString& aEncoding, nsAString& aResult)
> +{
> +  aResult.Truncate();

Remove.

@@ +124,5 @@
> +  nsCOMPtr<nsIInputStream> stream;
> +  nsresult rv = aBlob->GetInternalStream(getter_AddRefs(stream));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCAutoString charsetGuess;

nsCString.

@@ +125,5 @@
> +  nsresult rv = aBlob->GetInternalStream(getter_AddRefs(stream));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCAutoString charsetGuess;
> +  if (!aEncoding.IsEmpty()) {

Nit: Since you have blocks for both cases please remove the ! here and swap the blocks.

@@ +132,5 @@
> +    rv = GuessCharset(stream, charsetGuess);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(stream);
> +    if (!seekable) return NS_ERROR_FAILURE;

I think you probably want NS_ENSURE_SUCCESS here (includes a warning if it fails). If you really don't want that warning then you should at least make this two lines.

@@ +133,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(stream);
> +    if (!seekable) return NS_ERROR_FAILURE;
> +    rv = seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);

Hm. Add a comment saying that you need to do this because GuessCharset advances the stream.

@@ +137,5 @@
> +    rv = seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  nsCAutoString charset;

nsCString. And move this further down to right before you use it.

@@ +149,5 @@
> +  return ConvertStream(stream, charset.get(), aResult);
> +}
> +
> +nsresult
> +FileReaderSyncPrivate::ReadAsDataURL(nsIDOMBlob* aBlob, nsAString& aResult)

Khuey is right, let's use Base64EncodeInputStream here.

@@ +151,5 @@
> +
> +nsresult
> +FileReaderSyncPrivate::ReadAsDataURL(nsIDOMBlob* aBlob, nsAString& aResult)
> +{
> +  aResult.AssignLiteral("data:");

Hm, you're modifying aResult several times, each time potentially requiring a new malloc. I'd use an nsAutoString to handle all the appends, then assign aResult from that to only copy one time.

@@ +153,5 @@
> +FileReaderSyncPrivate::ReadAsDataURL(nsIDOMBlob* aBlob, nsAString& aResult)
> +{
> +  aResult.AssignLiteral("data:");
> +
> +  nsAutoString contentType;

nsString here.

@@ +156,5 @@
> +
> +  nsAutoString contentType;
> +  aBlob->GetType(contentType);
> +
> +  if (contentType.Length()) {

Nit: contentType.IsEmpty(), reverse the blocks.

@@ +210,5 @@
> +
> +nsresult
> +FileReaderSyncPrivate::ConvertStream(nsIInputStream *aStream,
> +                                   const char *aCharset,
> +                                   nsAString &aResult)

Nit: indent is off here.

@@ +212,5 @@
> +FileReaderSyncPrivate::ConvertStream(nsIInputStream *aStream,
> +                                   const char *aCharset,
> +                                   nsAString &aResult)
> +{
> +  aResult.Truncate();

Unnecessary.

@@ +216,5 @@
> +  aResult.Truncate();
> +
> +  nsCOMPtr<nsIConverterInputStream> converterStream =
> +    do_CreateInstance("@mozilla.org/intl/converter-input-stream;1");
> +  if (!converterStream) return NS_ERROR_FAILURE;

NS_ENSURE_TRUE here. You definitely want a warning if this fails.

@@ +221,5 @@
> +
> +  nsresult rv = converterStream->Init
> +                  (aStream, aCharset,
> +                   8192,
> +                   nsIConverterInputStream::DEFAULT_REPLACEMENT_CHARACTER);

Nit: Please keep the parentheses on the first line, only wrap when you run out of space.

@@ +226,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIUnicharInputStream> unicharStream =
> +    do_QueryInterface(converterStream);
> +  if (!unicharStream) return NS_ERROR_FAILURE;

NS_ENSURE_TRUE here or two lines.

@@ +237,5 @@
> +    aResult.Append(result);
> +    if (aResult.Length() - oldLength != result.Length()) {
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +    rv = unicharStream->ReadString(8192, result, &numChars);

Ew, I don't like this. How about this:

  while (NS_SUCCEEDED(unicharStream->ReadString(8192, result, &numChars)) &&
         numChars) {
    ...
  }

That way you don't duplicate the main function call. Also note that numChars is unsigned, testing != 0 is all you want.

@@ +245,5 @@
> +}
> +
> +nsresult
> +FileReaderSyncPrivate::GuessCharset(nsIInputStream *aStream,
> +                                  nsACString &aCharset)

We definitely need to share this code with nsDOMFileReader.cpp. Let's unify it all in some static function.

::: dom/workers/FileReaderSyncPrivate.h
@@ +40,5 @@
> +#ifndef nsDOMFileReaderSyncPrivate_h
> +#define nsDOMFileReaderSyncPrivate_h
> +
> +#include "jsapi.h"
> +#include "jstypedarray.h"

Try only including jspubtd.h and then forward declaring js::ArrayBuffer.

@@ +43,5 @@
> +#include "jsapi.h"
> +#include "jstypedarray.h"
> +
> +#include "nsICharsetDetectionObserver.h"
> +#include "nsString.h"

nsStringGlue

@@ +44,5 @@
> +#include "jstypedarray.h"
> +
> +#include "nsICharsetDetectionObserver.h"
> +#include "nsString.h"
> +#include "Workers.h"

Make this first.

@@ +52,5 @@
> +
> +BEGIN_WORKERS_NAMESPACE
> +
> +class FileReaderSyncPrivate : public nsICharsetDetectionObserver,
> +                              public PrivatizableBase

PrivatizableBase should come first here.

@@ +68,5 @@
> +                      nsAString& aResult);
> +  nsresult ReadAsDataURL(nsIDOMBlob* aBlob, nsAString& aResult);
> +
> +  // From nsICharsetDetectionObserver
> +  NS_IMETHOD Notify(const char *aCharset, nsDetectionConfident aConf);

use NS_DECL_NSICHARSETDETECTIONOBSERVER

@@ +71,5 @@
> +  // From nsICharsetDetectionObserver
> +  NS_IMETHOD Notify(const char *aCharset, nsDetectionConfident aConf);
> +
> +private:
> +  nsCString mCharset;

Nit: Convention in dom/workers code is to list members first (before the first public section).

::: dom/workers/RuntimeService.cpp
@@ +718,5 @@
> +  mDetectorName = Preferences::GetLocalizedCString("intl.charset.detector");
> +
> +  nsCOMPtr<nsIPlatformCharset> platformCharset =
> +    do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv);
> +  if (NS_SUCCEEDED(rv)) {

You should probably warn if this fails, right? What happens if it does fail? Will we be able to do much without a platform character set?

::: dom/workers/RuntimeService.h
@@ +85,5 @@
>    nsClassHashtable<nsVoidPtrHashKey, WorkerArray> mWindowMap;
>  
>    static PRUint32 sDefaultJSContextOptions;
> +  nsCString mDetectorName;
> +  nsCString mSystemCharset;

Nit: Add these before the static.

@@ +90,3 @@
>  
>  public:
> +

Nit: remove unneeded extra line.

@@ +131,5 @@
>    void
>    ResumeWorkersForWindow(JSContext* aCx, nsPIDOMWindow* aWindow);
>  
> +  const nsACString&
> +  GetDetectorName()

Make this a const method.

@@ +137,5 @@
> +    return mDetectorName;
> +  }
> +
> +  const nsACString&
> +  GetSystemCharset() {

This one too, and put the brace on the following line.

::: dom/workers/Worker.h
@@ +41,5 @@
>  #define mozilla_dom_workers_worker_h__
>  
>  #include "Workers.h"
>  
> +#include "jsapi.h"

Hopefully you can remove this when you move the structured clone stuff to WorkerPrivate.cpp.

@@ +71,5 @@
> +
> +  DOMWORKER_SCTAG_END
> +};
> +
> +} // anonymous namespace

Remove this too.

::: dom/workers/WorkerPrivate.cpp
@@ +69,5 @@
>  #include "Exceptions.h"
>  #include "Principal.h"
>  #include "RuntimeService.h"
>  #include "ScriptLoader.h"
> +#include "Worker.h"

This shouldn't be necessary either.

@@ +160,5 @@
> +
> +    // See if this is a wrapped native.
> +    nsCOMPtr<nsIXPConnectWrappedNative> wrappedNative;
> +    nsContentUtils::XPConnect()->
> +      GetWrappedNativeOfJSObject(aCx, aObj, getter_AddRefs(wrappedNative));

Hm. We can only call this on the main thread. I think you need to move this to the main thread callbacks.
All comments addressed except for the following:

(In reply to comment #6)
> ::: dom/workers/File.cpp
> @@ +415,5 @@
> > +
> > +nsIDOMBlob*
> > +GetPrivate(JSContext* aCx, JSObject* aObj)
> > +{
> > +  return Blob::GetPrivate(aCx, aObj);
> 
> Hm... It's feasible that someone could call this with a File object, right?
> You could still return a nsIDOMBlob from that here whereas you're now going
> to return null.
> 
Blob::GetPrivate checks the class to see if it's a file and if so, it returns the file (which is also a blob).
> ::: dom/workers/FileReaderSync.cpp
> @@ +201,5 @@
> > +
> > +    nsIDOMBlob* blob = GetPrivateBlob(aCx, jsBlob);
> > +    if (!blob) {
> > +      return false;
> > +    }
> 
> Hm, this pattern (convert args, get nsIDOMBlob*) is repeated for all ReadAs*
> functions, you could move this into a helper.
> 
It's not exactly the same for ReadAsText where the function takes an additional argument.
> ::: dom/workers/FileReaderSyncPrivate.cpp
> @@ +85,5 @@
> > +  PRUint32 numRead;
> > +  rv = stream->Read((char*) aArrayBuffer->data,
> > +                    aArrayBuffer->byteLength, &numRead);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  NS_ASSERTION(numRead == aArrayBuffer->byteLength, "failed to read data");
> 
> I think you should also assert that stream->Available() is 0.
> 
You can't call stream->Available on a closed stream. The stream closes when you read to the end.
> @@ +110,5 @@
> > +    if (aResult.Length() - oldLength != numRead) {
> > +      return NS_ERROR_OUT_OF_MEMORY;
> > +    }
> > +  } while (numRead > 0);
> > +
> 
> Assert stream->Available() is 0 here too.
> 
Same as above.
> @@ +116,5 @@
> > +}
> > +
> > +nsresult
> > +FileReaderSyncPrivate::ReadAsText(nsIDOMBlob* aBlob,
> > +                                  const nsAString& aEncoding, nsAString& aResult)
> 
> Nit: wrapping looks wrong here
> 
Second argument doesn't fit on the same line.
> @@ +245,5 @@
> > +}
> > +
> > +nsresult
> > +FileReaderSyncPrivate::GuessCharset(nsIInputStream *aStream,
> > +                                  nsACString &aCharset)
> 
> We definitely need to share this code with nsDOMFileReader.cpp. Let's unify
> it all in some static function.
> 
GuessCharset in FileReaderSyncPrivate takes a stream while nsDOMFileReader takes a pointer to a block of memory. The way in which the method gets the "nsIPlatformCharset" is also a bit different too, in workers it is retrieved ahead of time because it's main thread only while in nsDOMFileReader it gets the platform charset only if the other detection methods fail. I think the only part which is reasonable to unify is the BOM detection code but I'm not sure where that should live.
> ::: dom/workers/FileReaderSyncPrivate.h
> @@ +68,5 @@
> > +                      nsAString& aResult);
> > +  nsresult ReadAsDataURL(nsIDOMBlob* aBlob, nsAString& aResult);
> > +
> > +  // From nsICharsetDetectionObserver
> > +  NS_IMETHOD Notify(const char *aCharset, nsDetectionConfident aConf);
> 
> use NS_DECL_NSICHARSETDETECTIONOBSERVER
> 
Does not exist, it looks like nsICharsetDetectionObserver was implemented manually, not using IDL.
> ::: dom/workers/RuntimeService.cpp
> @@ +718,5 @@
> > +  mDetectorName = Preferences::GetLocalizedCString("intl.charset.detector");
> > +
> > +  nsCOMPtr<nsIPlatformCharset> platformCharset =
> > +    do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &rv);
> > +  if (NS_SUCCEEDED(rv)) {
> 
> You should probably warn if this fails, right? What happens if it does fail?
> Will we be able to do much without a platform character set?
> 
The platform charset is a fallback and even if it fails it will fallback to UTF-8.

Also, the MimeDetectorRunnable went away because we no longer have to detect mime in non-main thread after nsIDOMFiles became immutable.

Thanks for the review Ben!
Attached patch Patch for FileReaderSync rev 2 (obsolete) — — Splinter Review 
revision 2
Attachment #539919 - Attachment is obsolete: true
Attachment #539919 - Flags: review?(bent.mozilla)
Attachment #549032 - Flags: review?(bent.mozilla)
Comment on attachment 549032 [details] [diff] [review]
Patch for FileReaderSync rev 2

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

Looks really great! A few issues remain, let me know if you need help fixing these up. I'd love to get this in by Tuesday and I'll be happy to help.

::: content/base/src/nsDOMFile.cpp
@@ +147,5 @@
>    NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(File, mIsFile)
>    NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(Blob, !mIsFile)
>  NS_INTERFACE_MAP_END
>  
> +// Threadsafe when GetMutable() == PR_TRUE

I think this is backwards.

::: dom/workers/Exceptions.cpp
@@ +367,5 @@
> +
> +
> +JSPropertySpec FileException::sProperties[] = {
> +  { "code", SLOT_code, PROPERTY_FLAGS, GetProperty, NULL },
> +  { "name", SLOT_name, PROPERTY_FLAGS, GetProperty, NULL },

These need 'js_GetterOnlyPropertyStub' for the setter.

::: dom/workers/File.cpp
@@ +52,5 @@
> +#include "xpcprivate.h"
> +#include "xpcquickstubs.h"
> +
> +#define PROPERTY_FLAGS \
> +  JSPROP_ENUMERATE | JSPROP_SHARED | JSPROP_READONLY

We've determined that the proper way to have readonly properties is to use js_GetterOnlyPropertyStub for the setter and to not use the JSPROP_READONLY flag, please remove this one and update the appropriate setters.

@@ +75,5 @@
> +  static JSObject*
> +  InitClass(JSContext* aCx, JSObject* aObj)
> +  {
> +    return JS_InitClass(aCx, aObj, NULL, &sClass, NULL, 0, sProperties,
> +                        sFunctions, NULL, NULL);

Blob is not spec'd with [NoInterfaceObject] so it needs a throwing constructor.

@@ +79,5 @@
> +                        sFunctions, NULL, NULL);
> +  }
> +
> +  static JSObject*
> +  Create(JSContext* aCx, nsIDOMBlob* aBlob) {

Nit: { on next line.

@@ +82,5 @@
> +  static JSObject*
> +  Create(JSContext* aCx, nsIDOMBlob* aBlob) {
> +    JSObject* obj = JS_NewObject(aCx, &sClass, NULL, NULL);
> +    if (obj) {
> +      if (!JS_SetPrivate(aCx, obj, static_cast<nsISupports*>(aBlob))) {

I think we should assert that |static_cast<nsISupports*>(aBlob)| is the same as QI'ing aBlob to nsISupports, like nsCOMPtr does. Maybe something like this:

  NS_ASSERTION(SameCOMIdentity(static_cast<nsISupports*>(aBlob), aBlob), "Bad identity!");

@@ +120,5 @@
> +  {
> +    JS_ASSERT(JS_GET_CLASS(aCx, aObj) == &sClass);
> +
> +    if (JS_GetPrivate(aCx, aObj)) {
> +      nsIDOMBlob* blob = GetPrivate(aCx, aObj);

No need to check the private twice. Once is plenty :)

@@ +180,5 @@
> +    }
> +
> +    PRUint64 size;
> +    if (NS_FAILED(blob->GetSize(&size))) {
> +      return false;

Need to set an exception here.

@@ +183,5 @@
> +    if (NS_FAILED(blob->GetSize(&size))) {
> +      return false;
> +    }
> +
> +    jsdouble start = 0, end = size;

Nit, end = 0, in case someone changes the default of start.

@@ +225,5 @@
> +};
> +
> +JSPropertySpec Blob::sProperties[] = {
> +  { "size", 0, PROPERTY_FLAGS, GetSize, NULL },
> +  { "type", 0, PROPERTY_FLAGS, GetType, NULL },

readonly...

@@ +248,5 @@
> +  static JSObject*
> +  InitClass(JSContext* aCx, JSObject* aObj, JSObject* aParentProto)
> +  {
> +    return JS_InitClass(aCx, aObj, aParentProto, &sClass, NULL, 0, sProperties,
> +                        NULL, NULL, NULL);

File is not spec'd with [NoInterfaceObject] so it needs a throwing constructor.

@@ +255,5 @@
> +  static JSObject*
> +  Create(JSContext* aCx, nsIDOMFile* aFile) {
> +    JSObject* obj = JS_NewObject(aCx, &sClass, NULL, NULL);
> +    if (obj) {
> +      if (!JS_SetPrivate(aCx, obj, static_cast<nsISupports*>(aFile))) {

NS_ASSERTION(SameCOMIdentity(static_cast<nsISupports*>(aFile), aFile), "Bad identity!");

@@ +310,5 @@
> +  {
> +    JS_ASSERT(JS_GET_CLASS(aCx, aObj) == &sClass);
> +
> +    if (JS_GetPrivate(aCx, aObj)) {
> +      nsIDOMFile* file = GetPrivate(aCx, aObj);

One check only. And use GetJSPrivateSafeish.

@@ +324,5 @@
> +      return false;
> +    }
> +
> +    nsString fullPath;
> +    if (NS_FAILED(file->GetMozFullPath(fullPath))) {

This will always return the empty string due to the way that nsDOMFile::GetMozFullPath is implemented. Need to call GetMozFullPathInternal if and only if this is running inside a chrome worker.

@@ +381,5 @@
> +{
> +  if (aObj) {
> +    JSClass* classPtr = JS_GET_CLASS(aCx, aObj);
> +    if (classPtr == &sClass ||
> +        classPtr == File::Class()) {

Nit: this should fit on one line.

@@ +413,5 @@
> +
> +nsIDOMBlob*
> +GetDOMBlobFromJSObject(JSContext* aCx, JSObject* aObj)
> +{
> +  return Blob::GetPrivate(aCx, aObj);

We should JS_ASSERT that this is a blob object here, same for GetDOMFileFromJSObject below.

::: dom/workers/File.h
@@ +65,5 @@
> +
> +nsIDOMFile*
> +GetDOMFileFromJSObject(JSContext* aCx, JSObject* aObj);
> +
> +} // file worker

Nit: // namespace file

::: dom/workers/FileReaderSync.cpp
@@ +63,5 @@
> +
> +namespace {
> +
> +static bool
> +EnsureSucceededOrThrow(JSContext* aCx, nsresult rv)

I think this should be:

  inline bool
  EnsureSucceededOrThrow(JSContext* aCx, nsresult rv)
  {
    if (NS_SUCCEEDED(rv)) {
      return true;
    }

    intN code = rv == NS_ERROR_FILE_NOT_FOUND ?
                FILE_NOT_FOUND_ERR :
                FILE_NOT_READABLE_ERR;
    ThrowFileExceptionForCode(aCx, code);
    return false;
  }

@@ +83,5 @@
> +
> +  return true;
> +}
> +
> +static nsIDOMBlob*

inline, not static.

@@ +84,5 @@
> +  return true;
> +}
> +
> +static nsIDOMBlob*
> +GetDOMBlobFromJSObject(JSContext* aCx, JSObject* aObj) {

Hm, this is duplicated from Blob::GetInstancePrivate... Let's just call that directly? Maybe change file::GetDOMBlobFromJSObject to accept a third boolean argument that specifies whether failure should throw?

@@ +103,5 @@
> +                       "not a Blob.");
> +  return NULL;
> +}
> +
> +

Nit: remove extra line

@@ +137,5 @@
> +  }
> +
> +private:
> +  static FileReaderSyncPrivate*
> +  GetInstancePrivate(JSContext* aCx, JSObject* aObj, const char* aFunctionName) {

Nit: { on next line

@@ +277,5 @@
> +
> +    JSObject* obj = JS_THIS_OBJECT(aCx, aVp);
> +
> +    FileReaderSyncPrivate* fileReader =
> +        GetInstancePrivate(aCx, obj, "readAsBinaryString");

Nit: indent is weird here.

@@ +363,5 @@
> +  JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub, Finalize,
> +  JSCLASS_NO_OPTIONAL_MEMBERS
> +};
> +
> +

Nit: remove extra newline

::: dom/workers/WorkerPrivate.cpp
@@ +160,5 @@
> +        nsCOMPtr<nsIMutable> mutableFile = do_QueryInterface(file);
> +        PRBool isMutable;
> +        NS_ASSERTION(NS_SUCCEEDED(mutableFile->GetMutable(&isMutable)) &&
> +                     !isMutable, "Only immutable file should be passed"
> +                     "to worker");

This should be either a debug-only check or a hard failure... Which?

@@ +214,5 @@
>    }
>  
>    static JSBool
>    Write(JSContext* aCx, JSStructuredCloneWriter* aWriter, JSObject* aObj,
>          void* aClosure)

You've got the main thread case taken care of but I don't see where you handle the non-main thread case. You need another hook for passing a file to a subworker.
(In reply to ben turner [:bent] from comment #11)
> @@ +84,5 @@
> > +  return true;
> > +}
> > +
> > +static nsIDOMBlob*
> > +GetDOMBlobFromJSObject(JSContext* aCx, JSObject* aObj) {
> 
> Hm, this is duplicated from Blob::GetInstancePrivate... Let's just call that
> directly? Maybe change file::GetDOMBlobFromJSObject to accept a third
> boolean argument that specifies whether failure should throw?

Nevermind, ignore me here. I read too quickly, this looks just fine.

> @@ +214,5 @@
> >    }
> >  
> >    static JSBool
> >    Write(JSContext* aCx, JSStructuredCloneWriter* aWriter, JSObject* aObj,
> >          void* aClosure)
> 
> You've got the main thread case taken care of but I don't see where you
> handle the non-main thread case. You need another hook for passing a file to
> a subworker.

Actually, the Read needs to be special on the main thread too. We need to make a main thread DOM File object, not one of our worker kind.
All the comments addressed (thanks Ben) and added test case for posting files to subworker.
Attachment #549030 - Attachment is obsolete: true
Attachment #549035 - Attachment is obsolete: true
Attachment #553247 - Flags: review?(bent.mozilla)
Attachment #553247 - Flags: review?(bent.mozilla)
Attached patch Patch for FileReaderSync rev 3 (obsolete) — — Splinter Review 

        Attachment #549032 -
        Attachment is obsolete: true

        Attachment #549032 -
        Flags: review?(bent.mozilla)

        Attachment #553249 -
        Flags: review?(bent.mozilla)

    
    

    
  
Comment on attachment 553249 [details] [diff] [review]
Patch for FileReaderSync rev 3

rs=me

        Attachment #553249 -
        Flags: review+

    
    

    
  


  
Final patch that William sent me via email.

        Attachment #553247 -
        Attachment is obsolete: true

        Attachment #553249 -
        Attachment is obsolete: true

        Attachment #553249 -
        Flags: review?(bent.mozilla)

        Attachment #553367 -
        Flags: review+

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/1ad8506a40d2

Great work William!
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

    
    

    
  
I understand that this issues doesn't fix that one:
https://bugzilla.mozilla.org/show_bug.cgi?id=667388

Is this correct? If so, how one is supposed to message a file object to the worker for opening it?

    
    

    
  
You need to create a new DOM File object and postMessage that. The most simple way to create a DOM File object is to do the following from chrome code:

myfile = new File(filepath)
or
myfile = new File(anNsIFileObject)

    
    

    
  
Well, it still doesn't work :(
First off, the constructor above doesn't exist in Chrome. It, however, seems to exist in FF (9.0a1 2011-08-29 nightly build), but results in a security error:

Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "..."]

The parameter passed to the constructor comes from a file input (from it's attribute files[0]).

To test:


<html>
    <head>
        <script type="text/javascript">
            window.onload = function()
            {
                var oFileInput = document.getElementById( 'file_path' );
                oFileInput.onchange = function()
				{
					console.log( new File( this.files[0] ) );
				}
            }
        </script>
    </head>
    <body>
        <p>
            <input id="file_path" type="file" />
        </p>
    </body>
</html>

    
    

    
  
"new File" only works from privileged code, otherwise any webpage could read any file on your filesystem. I.e. only addons or firefox code is allowed to do that.

In any case, this bug is about the FileReaderSync inside workers. Your code isn't using workers at all so if you are having problems please file a separate bug.

    
    

    
  
All right, I see you point. I was just wondering how one passes File objects to the workers to be read, and I followed your advice.

You still can't clone File objects to be used in Workers because of this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=667388

So I wonder what's the correct procedure to do it (and to test this issue at all).
Do you have any working example?

    
    

    
  
Discussions about bug 667388, including discussions about workarounds for it, are best had in bug 667388

    
    

    
  
Thanks

    
    

    
  
I've created: https://developer.mozilla.org/en/DOM/FileReaderSync
and I've added to Firefox 8 for devs.
Keywords: dev-doc-needed → dev-doc-complete

    
    

    
  
The docs should mention that the API is only available inside Workers. This is intentional since it enables synchronous IO.

    
    

    
  
Also it would be nice to give some examples in the docs.

    
    

    
  
Thanks for the feedback: I've updated the page with the limitation.

I've also updated the page https://developer.mozilla.org/En/DOM/Worker/Functions_available_to_workers

Jonas: is there a place where I can find a list of i/f only (or not) available to Workers. This last page seems a little bit light.

    
    

    
  
I think that page is the best such documentation. The worker specification itself doesn't specify which APIs are available only in workers, that is the job of the individual specifications. So the FileAPI specification which specifies the FileReaderSync API specifies that it's only available in workers.

    
    

    
  
I fixed up the page to make it a bit more clear/correct.
Blocks: 823484

    
    

    
  
I'm currently updating my documentation about which APIs are available to workers. If I understand this bug correctly, this interface has been available to them since its original implementation here, that is since Firefox 8. Can you confirm/infirm? (Thank you in advance)
Flags: needinfo?(wchen)

    
    

    
  
This is a worker-only API.
Flags: needinfo?(wchen)
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: