User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-US) AppleWebKit/534.2 (KHTML, like Gecko) Chrome/6.0.447.0 Safari/534.2 Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b2pre) Gecko/20100629 Minefield/4.0b2pre Implement the Blob interface for the File API as defined in the HTML5 specification: http://www.w3.org/TR/FileAPI/#blob Implementing Blob means that files can be read in chunks, rather than reading the entire file into memory to access it. More information here: http://www.filosophy.org/2010/06/a-state-of-limbo-the-html5-file-api-filereader-and-blobs/ Reproducible: Always Steps to Reproduce: 1. Navigate to page http://rapilabs.com/test_slicing.html 2. Choose a text file using the file field 3. An alert should display the first 1024 characters of the file Actual Results: Error console shows: Error: file.slice is not a function Source File: http://rapilabs.com/test_slicing.html Line: 7 Expected Results: An alert displaying the first 1024 characters of the file Chrome 6 implements the Blob interface and displays the expected results.

Not a jseng issue. Jonas, is this on our 2.0 list?

One other note... Chrome 6 doesn't exist yet. There are Chrome 6 development builds, much like there are Firefox 4 development builds like the one you were testing.

The only tricky part here was implementing the nsIInputStream which streams part of a file. There might be more graceful ways of doing that, but this felt the safest and most cross-platform. The bulk of the patch is really search'n'replacing s/File/Blob/ in a bunch of code since we now generally deal with Blobs rather than Files. They are for most intents and purposes the same though, the only difference is that Files have a name. There is also a minor somewhat unrelated tweak in the patch too. I made nsDOMMemoryFile::GetMozFullPathInternal return an empty string. This shouldn't have an effect at all really since the only place where we call that function is from nsHTMLInputElement, but that (currently) always deals with "real" files. So the only real changes here are in nsDOMFile.cpp/h and in nsFileStreams.cpp/h.

Also, the test (but not the actual code) relies on the fix for bug 599300, so marking a dependency.

This fixes a build error on gcc and adds even more tests. I'm running into some image/png encoding issues on windows resulting in getting less data there than ideally needed for the test. For some reason the encoded png image on windows is about a quarter of the size on mac. This doesn't affect the functionality, but does somewhat affect the tests there. I've pushed updated tests to try to see if they help.

Attaching this for posterity. Originally I used this approach when implementing .slice. I do think it makes more sense, but I don't see much reason for landing it now as it doesn't fix any bugs. Basically the idea is that nsDOMFile and nsDOMMemoryFile has enough in common that it results in less and cleaner code to simply merge the two classes. I do think we'll want to do something like this eventually, as to allow flushing memory-backed files to disk if they are large and remains in memory for a long time. So we'll need to allow morphing a memory-backed file into a disk-backed file. That will obviously need more changes though.

Comment on attachment 481558 [details] [diff] [review] Patch v1.1 Looks pretty good, the only thing that jumps out at me are some overflow issues. Also, while I think it's reasonable for me to review the File API parts of this, the nsPartialFileInputStream stuff should get sr. >diff --git a/content/base/public/nsDOMFile.h b/content/base/public/nsDOMFile.h > class nsDOMFile : public nsIDOMFile, > public nsIXHRSendable, > public nsICharsetDetectionObserver > { > public: > NS_DECL_ISUPPORTS >+ NS_DECL_NSIDOMBLOB > NS_DECL_NSIDOMFILE > NS_DECL_NSIXHRSENDABLE > > nsDOMFile(nsIFile *aFile, const nsAString& aContentType) > : mFile(aFile), >- mContentType(aContentType) >+ mContentType(aContentType), >+ mIsFile(true) mIsFile needs a better name. Perhaps mIsCompleteFile? or mIsSubFile? (and reverse the polarity on everything) > {} > > nsDOMFile(nsIFile *aFile) >- : mFile(aFile) >+ : mFile(aFile), >+ mIsFile(true) > {} > >+ nsDOMFile(const nsDOMFile* aOther, PRUint64 aStart, PRUint64 aLength, >+ const nsAString& aContentType) >+ : mFile(aOther->mFile), >+ mStart(aOther->mIsFile ? aStart : >+ (aOther->mStart + aStart)), Drop the mIsFile check here, if it is a file aOther->mStart will be 0. >+ mLength(aLength), >+ mContentType(aContentType), >+ mIsFile(false) >+ { >+ NS_ASSERTION(mFile, "must have file"); I think a zero sized slice should accept a null mFile here, so that we don't hold the file reference longer than necessary. >+ // Ensure non-null mContentType >+ mContentType.SetIsVoid(PR_FALSE); >+ } >+ > virtual ~nsDOMFile() {} > > // from nsICharsetDetectionObserver > NS_IMETHOD Notify(const char *aCharset, nsDetectionConfident aConf); > >-private: >+protected: > nsCOMPtr<nsIFile> mFile; >+ >+ // start and length in >+ PRUint64 mStart; >+ PRUint64 mLength; >+ > nsString mContentType; >+ >+ bool mIsFile; >+ >+ // Used during charset detection > nsCString mCharset; >- > nsresult GuessCharset(nsIInputStream *aStream, > nsACString &aCharset); > nsresult ConvertStream(nsIInputStream *aStream, const char *aCharset, > nsAString &aResult); > }; > > class nsDOMMemoryFile : public nsDOMFile > { > public: > nsDOMMemoryFile(void *aMemoryBuffer, > PRUint64 aLength, > const nsAString& aName, > const nsAString& aContentType) > : nsDOMFile(nsnull, aContentType), >- mInternalData(aMemoryBuffer), mLength(aLength), mName(aName) >- { } >+ mDataOwner(new DataOwner()), >+ mName(aName) >+ { >+ mDataOwner->mData = aMemoryBuffer; > >- ~nsDOMMemoryFile(); >+ mStart = 0; >+ mLength = aLength; >+ } >+ >+ nsDOMMemoryFile(const nsDOMMemoryFile* aOther, PRUint64 aStart, >+ PRUint64 aLength, const nsAString& aContentType) >+ : nsDOMFile(nsnull, aContentType), >+ mDataOwner(aOther->mDataOwner) >+ { >+ NS_ASSERTION(mDataOwner && mDataOwner->mData, "must have data"); Ideally we'd accept a null mDataOwner for a zero byte slice so that we can free the memory as soon as possible. >+ >+ mIsFile = false; >+ mStart = aOther->mStart + aStart; >+ mLength = aLength; >+ >+ // Ensure non-null mContentType >+ mContentType.SetIsVoid(PR_FALSE); >+ } > > NS_IMETHOD GetName(nsAString&); > NS_IMETHOD GetSize(PRUint64*); > NS_IMETHOD GetInternalStream(nsIInputStream**); >- NS_IMETHOD GetMozFullPath(nsAString&); > NS_IMETHOD GetMozFullPathInternal(nsAString&); >+ NS_IMETHOD Slice(PRUint64 aStart, PRUint64 aLength, >+ const nsAString& aContentType, nsIDOMBlob **aBlob); > > protected: >- void* mInternalData; >- PRUint64 mLength; >+ class DataOwner { >+ public: >+ NS_INLINE_DECL_REFCOUNTING(DataOwner); >+ ~DataOwner() { >+ PR_Free(mData); >+ } >+ void* mData; >+ }; Why not make DataOwner have a constructor that takes the pointer? Seems cleaner to me. >+ >+ // Used when backed by a memory store >+ nsRefPtr<DataOwner> mDataOwner; >+ > nsString mName; > }; > > class nsDOMFileList : public nsIDOMFileList > { > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIDOMFILELIST >@@ -158,17 +216,17 @@ public: > nsDOMFileError(PRUint16 aErrorCode) : mCode(aErrorCode) {} > > private: > PRUint16 mCode; > }; > > class NS_STACK_CLASS nsDOMFileInternalUrlHolder { > public: >- nsDOMFileInternalUrlHolder(nsIDOMFile* aFile, nsIPrincipal* aPrincipal >+ nsDOMFileInternalUrlHolder(nsIDOMBlob* aFile, nsIPrincipal* aPrincipal > MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM); > ~nsDOMFileInternalUrlHolder(); > nsAutoString mUrl; > private: > MOZILLA_DECL_USE_GUARD_OBJECT_NOTIFIER > }; > > #endif >diff --git a/content/base/public/nsIDOMFile.idl b/content/base/public/nsIDOMFile.idl >+interface nsIDOMBlob; This forward declaration is unnecessary. >diff --git a/content/base/src/nsDOMFile.cpp b/content/base/src/nsDOMFile.cpp > NS_IMETHODIMP >+nsDOMFile::Slice(PRUint64 aStart, PRUint64 aLength, >+ const nsAString& aContentType, nsIDOMBlob **aBlob) >+{ >+ *aBlob = nsnull; >+ >+ // Trunkate aLength and aStart so that we stay within this file. Truncate >+ PRUint64 thisLength; >+ nsresult rv = GetSize(&thisLength); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (aStart > thisLength) { >+ aStart = aLength = 0; >+ } >+ else if (aStart + aLength > thisLength) { >+ aLength = thisLength - aStart; >+ } var file = GetFileFromSomewhere(); // Get a 20 byte file from somewhere var blob = file.slice(10, 10, "text/plain"); var blob2 = blob.slice(2, 2^64-1, "text/plain"); The clamping is going to overflow and we're going to create some sort of nonsense blob. >+ >+ // Create the new file >+ NS_ADDREF(*aBlob = new nsDOMFile(this, aStart, aLength, aContentType)); >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP > nsDOMFile::GetInternalStream(nsIInputStream **aStream) > { >- return NS_NewLocalFileInputStream(aStream, mFile, -1, -1, >- nsIFileInputStream::CLOSE_ON_EOF | >- nsIFileInputStream::REOPEN_ON_REWIND); >+ return mIsFile ? >+ NS_NewLocalFileInputStream(aStream, mFile, -1, -1, >+ nsIFileInputStream::CLOSE_ON_EOF | >+ nsIFileInputStream::REOPEN_ON_REWIND) : >+ NS_NewPartialLocalFileInputStream(aStream, mFile, mStart, mLength, >+ -1, -1, >+ nsIFileInputStream::CLOSE_ON_EOF | >+ nsIFileInputStream::REOPEN_ON_REWIND); > } > > NS_IMETHODIMP > nsDOMFile::GetInternalUrl(nsIPrincipal* aPrincipal, nsAString& aURL) > { > NS_ENSURE_STATE(aPrincipal); > > nsresult rv; >@@ -476,57 +515,69 @@ nsDOMFile::ConvertStream(nsIInputStream > aResult.Append(result); > rv = unicharStream->ReadString(8192, result, &numChars); > } > > return rv; > } > > // nsDOMMemoryFile Implementation >-nsDOMMemoryFile::~nsDOMMemoryFile() >-{ >- PR_Free(mInternalData); >-} >- > NS_IMETHODIMP > nsDOMMemoryFile::GetName(nsAString &aFileName) > { >+ NS_ASSERTION(mIsFile, "Should only be called on files"); > aFileName = mName; > return NS_OK; > } > > NS_IMETHODIMP > nsDOMMemoryFile::GetSize(PRUint64 *aFileSize) > { > *aFileSize = mLength; > return NS_OK; > } > > NS_IMETHODIMP >+nsDOMMemoryFile::Slice(PRUint64 aStart, PRUint64 aLength, >+ const nsAString& aContentType, nsIDOMBlob **aBlob) >+{ >+ *aBlob = nsnull; >+ >+ // Trunkate aLength and aStart so that we stay within this file. Truncate >+ if (aStart > mLength) { >+ aStart = aLength = 0; >+ } >+ else if (aStart + aLength > mLength) { >+ aLength = mLength - aStart; >+ } Same overflow problem. Please factor the clamping out into a separate function so that we don't duplicate this logic. >+ >+ // Create the new file >+ NS_ADDREF(*aBlob = new nsDOMMemoryFile(this, aStart, aLength, aContentType)); >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP > nsDOMMemoryFile::GetInternalStream(nsIInputStream **aStream) > { > if (mLength > PR_INT32_MAX) > return NS_ERROR_FAILURE; > >- PRInt32 l = mLength; >- >- return NS_NewByteInputStream(aStream, (const char*)mInternalData, l); >-} >- >-NS_IMETHODIMP >-nsDOMMemoryFile::GetMozFullPath(nsAString &aFileName) >-{ >- aFileName.Truncate(); >- return NS_OK; >+ return NS_NewByteInputStream(aStream, >+ static_cast<const char*>(mDataOwner->mData) + >+ mStart, >+ (PRInt32)mLength); > } > > NS_IMETHODIMP > nsDOMMemoryFile::GetMozFullPathInternal(nsAString &aFilename) > { >- return GetName(aFilename); >+ NS_ASSERTION(mIsFile, "Should only be called on files"); >+ aFilename.Truncate(); >+ return NS_OK; > } > > // nsDOMFileList implementation > > DOMCI_DATA(FileList, nsDOMFileList) > > NS_INTERFACE_MAP_BEGIN(nsDOMFileList) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMFileList) >@@ -568,17 +619,17 @@ NS_IMPL_RELEASE(nsDOMFileError) > > NS_IMETHODIMP > nsDOMFileError::GetCode(PRUint16* aCode) > { > *aCode = mCode; > return NS_OK; > } > >-nsDOMFileInternalUrlHolder::nsDOMFileInternalUrlHolder(nsIDOMFile* aFile, >+nsDOMFileInternalUrlHolder::nsDOMFileInternalUrlHolder(nsIDOMBlob* aFile, > nsIPrincipal* aPrincipal > MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) { > MOZILLA_GUARD_OBJECT_NOTIFIER_INIT; > aFile->GetInternalUrl(aPrincipal, mUrl); > } > > nsDOMFileInternalUrlHolder::~nsDOMFileInternalUrlHolder() { > if (!mUrl.IsEmpty()) { >diff --git a/content/base/test/test_fileapi_slice.html b/content/base/test/test_fileapi_slice.html >new file mode 100644 >--- /dev/null >+++ b/content/base/test/test_fileapi_slice.html >@@ -0,0 +1,395 @@ >+<!DOCTYPE HTML> >+<html> >+<head> >+<!-- >+https://bugzilla.mozilla.org/show_bug.cgi?id=414796 I assume you just copied this bug number from somewhere else, because it's not this bug ;-) My only real comment on the test is that I'd like to see testing of the clamping and of the zero size slices, as those seem like the hairer parts of the spec. It's possible that there are tests for that already that I just missed. >diff --git a/dom/base/nsDOMClassInfoID.h b/dom/base/nsDOMClassInfoID.h >+#define NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(_class, condition) \ >+ if ((condition) && \ >+ (aIID.Equals(NS_GET_IID(nsIClassInfo)) || \ >+ aIID.Equals(NS_GET_IID(nsXPCClassInfo)))) { \ >+ foundInterface = NS_GetDOMClassInfoInstance(eDOMClassInfo_##_class##_id); \ >+ if (!foundInterface) { \ >+ *aInstancePtr = nsnull; \ >+ return NS_ERROR_OUT_OF_MEMORY; \ >+ } \ >+ } else >+ Remove the existing define at http://mxr.mozilla.org/mozilla-central/ident?i=NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL while you're at it. >diff --git a/netwerk/base/public/nsIFileStreams.idl b/netwerk/base/public/nsIFileStreams.idl >+ * @param behaviorFlags flags specifying various behaviors of the class >+ * (see enumerations in the class) Nit: enumerations in nsIFileInputStream. >diff --git a/netwerk/base/src/nsFileStreams.cpp b/netwerk/base/src/nsFileStreams.cpp Nothing really jumped out at me here, but it would nice to have an xpcshell test for this interface. r=me with these comments addressed.

(In reply to comment #7) > Comment on attachment 481558 [details] [diff] [review] > Patch v1.1 > > Looks pretty good, the only thing that jumps out at me are some overflow > issues. Also, while I think it's reasonable for me to review the File API > parts of this, the nsPartialFileInputStream stuff should get sr. Definitely. Didn't want to ask for sr until I had r. > >diff --git a/content/base/public/nsDOMFile.h b/content/base/public/nsDOMFile.h > > class nsDOMFile : public nsIDOMFile, > > public nsIXHRSendable, > > public nsICharsetDetectionObserver > > { > > public: > > NS_DECL_ISUPPORTS > >+ NS_DECL_NSIDOMBLOB > > NS_DECL_NSIDOMFILE > > NS_DECL_NSIXHRSENDABLE > > > > nsDOMFile(nsIFile *aFile, const nsAString& aContentType) > > : mFile(aFile), > >- mContentType(aContentType) > >+ mContentType(aContentType), > >+ mIsFile(true) > mIsFile needs a better name. Perhaps mIsCompleteFile? or mIsSubFile? (and > reverse the polarity on everything) How about mIsFullFile? > > {} > > > > nsDOMFile(nsIFile *aFile) > >- : mFile(aFile) > >+ : mFile(aFile), > >+ mIsFile(true) > > {} > > > >+ nsDOMFile(const nsDOMFile* aOther, PRUint64 aStart, PRUint64 aLength, > >+ const nsAString& aContentType) > >+ : mFile(aOther->mFile), > >+ mStart(aOther->mIsFile ? aStart : > >+ (aOther->mStart + aStart)), > Drop the mIsFile check here, if it is a file aOther->mStart will be 0. Actually no, currently mStart is uninitialized for nsDOMFile with mIsFile=true. I did ponder changing that, but it felt weird to have mStart initialized and used, but mLength not. > >+ mLength(aLength), > >+ mContentType(aContentType), > >+ mIsFile(false) > >+ { > >+ NS_ASSERTION(mFile, "must have file"); > I think a zero sized slice should accept a null mFile here, so that we don't > hold the file reference longer than necessary. I think a zero-sized slice is not worth optimizing for. I don't see any real-world reasons for anyone to do so. > >+ class DataOwner { > >+ public: > >+ NS_INLINE_DECL_REFCOUNTING(DataOwner); > >+ ~DataOwner() { > >+ PR_Free(mData); > >+ } > >+ void* mData; > >+ }; > > Why not make DataOwner have a constructor that takes the pointer? Seems > cleaner to me. Good idea, will do. > >diff --git a/content/base/public/nsIDOMFile.idl b/content/base/public/nsIDOMFile.idl > >+interface nsIDOMBlob; > > This forward declaration is unnecessary. actually, the IDL compiler tripped if I didn't forward declare. I think that it didn't like parsing the slice() function declaration, which returns an nsIDOMBlob, before the nsIDOMBlob interface was fully parsed. > >diff --git a/content/base/src/nsDOMFile.cpp b/content/base/src/nsDOMFile.cpp > > NS_IMETHODIMP > >+nsDOMFile::Slice(PRUint64 aStart, PRUint64 aLength, > >+ const nsAString& aContentType, nsIDOMBlob **aBlob) > >+{ > >+ *aBlob = nsnull; > >+ > >+ // Trunkate aLength and aStart so that we stay within this file. > Truncate > >+ PRUint64 thisLength; > >+ nsresult rv = GetSize(&thisLength); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ if (aStart > thisLength) { > >+ aStart = aLength = 0; > >+ } > >+ else if (aStart + aLength > thisLength) { > >+ aLength = thisLength - aStart; > >+ } > > var file = GetFileFromSomewhere(); // Get a 20 byte file from somewhere > var blob = file.slice(10, 10, "text/plain"); > var blob2 = blob.slice(2, 2^64-1, "text/plain"); > > The clamping is going to overflow and we're going to create some sort of > nonsense blob. Good catch! Will add clamping somewhere. > My only real comment on the test is that I'd like to see testing of the > clamping and of the zero size slices, as those seem like the hairer parts of > the spec. It's possible that there are tests for that already that I just > missed. That is tested already. Though I realized that i only test zero-sized slices that are produced due to clamping. Will add a test for a zero-sized slice produced through a explicit zero-length. > Remove the existing define at > http://mxr.mozilla.org/mozilla-central/ident?i=NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL > while you're at it. Will do. > >diff --git a/netwerk/base/src/nsFileStreams.cpp b/netwerk/base/src/nsFileStreams.cpp > Nothing really jumped out at me here, but it would nice to have an xpcshell > test for this interface. Yeah, this occurred to me too just recently. Will do.

Is there a reason the spec allows zero sized slices at all?

Better return a zero-sized slice than to throw. Or, javascript String.substr allows it. Consider for example if someone does file upload by slicing the file in 10000 byte slices. If they have some rounding errors or off-by-one errors in the math, they might end up with a zero-sized chunk at the end. If we return a zero-sized slice then things will likely work just fine. If we throw it might fail to upload anything.

OK, fair enough. The rest of your comments sound good to me.

Comment on attachment 481558 [details] [diff] [review] Patch v1.1 +++ b/netwerk/base/public/nsIFileStreams.idl Can you briefly document somewhere in this file what will happen if offset > filesize, or offset+length>filesize? I'm fine if you specify that the behaviour is undefined, but I think it would be good to document whether that's an OK thing to do or not. +++ b/netwerk/base/src/nsFileStreams.cpp +nsPartialFileInputStream::Read(char* aBuf, PRUint32 aCount, PRUint32* aResult) +{ + PRUint32 readsize = TruncateSize(aCount); + if (readsize == 0 && mBehaviorFlags & CLOSE_ON_EOF) { + Close(); + *aResult = 0; + return NS_OK; Hmm, so this means that calling Read() with a zero count will close the file. That sounds wrong. I'm not sure that this code should be before Read at all, why not add an mPosition==mStart+mLength check after the nsFileInputStream::Read call? + if (offset < (PRInt64)mStart || offset > (PRInt64)(mStart + mLength)) { + return NS_ERROR_FAILURE; INVALID_ARGUMENT perhaps?

(In reply to comment #12) > Hmm, so this means that calling Read() with a zero count will close the file. > That sounds wrong. I'm not sure that this code should be before Read at all, > why not add an mPosition==mStart+mLength check after the > nsFileInputStream::Read call? I retract this comment. The existing Read() behaves the same way, so this is fine.

Addresses review comments. Still lacking the xpcshell test. I've got it mostly working but the problem is that our *current* file streams are very broken so I'm having to use a different approach when testing.

Comment on attachment 482704 [details] [diff] [review] Patch v1.2 thanks, sr=biesi on the netwerk/ part

Now with all tests (there's a lot of them) and ready for approval. Forwarding the reviews from previous patches. The only thing different in this patch is the addition of the xpcshell test which tests the new partial-file-stream.

Comment on attachment 482777 [details] [diff] [review] Patch v1.3 a=beltzner, and if it sticks on m-c, it can go on the b7 relbranch

Checked in! http://hg.mozilla.org/mozilla-central/rev/1b3947ed93c6 http://hg.mozilla.org/mozilla-central/rev/df4d2a82abf5 Also checked in to b7 branch http://hg.mozilla.org/mozilla-central/rev/104e35d09064 Appears to have stuck without problems.

So for docs, looks like we finally actually implement the Blob interface, with File then implementing that interface?

Documented: https://developer.mozilla.org/en/DOM/Blob And updated: https://developer.mozilla.org/en/DOM/File https://developer.mozilla.org/en/DOM/FileReader