Last Comment Bug 648997 - Implement BlobBuilder
: Implement BlobBuilder
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla6
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
http://dev.w3.org/2009/dap/file-syste...
Depends on: 655909
Blocks: 557540
  Show dependency treegraph
 
Reported: 2011-04-11 08:01 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-06-23 14:40 PDT (History)
4 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
First go (42.63 KB, patch)
2011-04-12 14:39 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (50.41 KB, patch)
2011-05-09 17:29 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (60.08 KB, patch)
2011-05-11 17:57 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (46.67 KB, patch)
2011-05-14 22:08 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jonas: review-
Details | Diff | Splinter Review
Interdiff (17.04 KB, patch)
2011-05-19 12:12 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (45.20 KB, patch)
2011-05-19 12:14 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jonas: review+
Details | Diff | Splinter Review
hg export patch (46.52 KB, patch)
2011-05-20 14:24 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Rename nsIDOMBlobBuilder to nsIDOMMozBlobBuilder (2.20 KB, patch)
2011-05-24 13:01 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
khuey: review+
jst: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-11 08:01:46 PDT

    
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-12 14:39:52 PDT
Created attachment 525520 [details] [diff] [review]
First go
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-12 15:58:32 PDT
Todo to myself:

- This needs to participate in cycle collection (a JS expando ref from a Blob to a BlobBuilder that owns the Blob ...)
- Should test GC hazards better
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-22 18:45:27 PDT
Comment on attachment 525520 [details] [diff] [review]
First go

This is horribly bitrotted after the slice changes.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-09 17:29:00 PDT
Created attachment 531214 [details] [diff] [review]
Patch

Unbitrotted, with a test for leaking.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-11 13:21:25 PDT
Comment on attachment 531214 [details] [diff] [review]
Patch

Removing this from my queue for now based on discussions with Kyle
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-11 17:57:25 PDT
Created attachment 531819 [details] [diff] [review]
Patch

Jonas and I discussed this in the office today and came to the conclusion that it was suboptimal to throw away data at the end, and instead we should grab the information we need from the blobs.

This does that through the GetInternalRepresentation function.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-14 22:08:43 PDT
Created attachment 532493 [details] [diff] [review]
Patch

Updated patch per our email conversation.  It's much cleaner now.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-18 16:58:22 PDT
Comment on attachment 532493 [details] [diff] [review]
Patch

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

Would like to see a new patch for some of this stuff. Please include an interdiff (mq makes it easy).

::: content/base/public/nsDOMFile.h
@@ +61,5 @@
> +
> +nsresult NS_NewBlobBuilder(nsISupports* *aSupports);
> +void ParseSize(PRInt64 aSize, PRInt64& aStart, PRInt64& aEnd);
> +
> +class DataOwner {

It doesn't seem like you need to move DataOwner or add the mLength member any more, right?

@@ -59,3 @@
>  
>  class nsDOMFile : public nsIDOMFile,
> -                  public nsIDOMBlob_MOZILLA_2_0_BRANCH,

Thanks for cleaning this up!

However, there's no need to keep the old unprefixed-nonscriptable slice method any more, so just nuke it.

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +51,5 @@
> +#define PR_INT64_MIN (-PR_INT64_MAX - 1)
> +
> +using namespace mozilla;
> +
> +class nsDOMMultipartBlob : public nsDOMFile

Hmm.. is there a reason to inherit nsDOMFile other than to get classinfo to work?

@@ +71,5 @@
> +                      const nsAString& aContentType, PRUint8 optional_argc,
> +                      nsIDOMBlob **aBlob);
> +
> +protected:
> +  InfallibleTArray<nsCOMPtr<nsIDOMBlob> > mBlobs;

I would just use nsTArray and rely on the fact that it by default is infallible. We're unlikely to ever change (or if we do, we'll have to audit all nsTArray usage anyway). We're IMHO more likely to get rid of the InfallibleTArray typedef.

@@ +132,5 @@
> +    rv = stream->AppendStream(scratchStream);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +
> +  NS_ABORT_IF_FALSE(mStart == 0, "Er, what?");

Why this? Isn't mStart unused by nsDOMMultipartBlob?

@@ +158,5 @@
> +
> +  ParseSize((PRInt64)thisLength, aStart, aEnd);
> +
> +  // If we clamped to nothing we create an empty blob
> +  InfallibleTArray<nsCOMPtr<nsIDOMBlob> > blobs;

Nit: Instead of creating this array, it might be slightly simpler to just create a multipart blob directly and give it a AppendBlob method (which could also keep a running tab on the total length). Up to you though.

@@ +165,5 @@
> +  PRUint64 finalLength = length;
> +
> +  NS_ABORT_IF_FALSE(aStart + length <= thisLength, "Er, what?");
> +
> +  if (aStart || length) {

Why the aStart check here?

@@ +168,5 @@
> +
> +  if (aStart || length) {
> +    // Prune the list of instructions if we can
> +    PRUint32 i;
> +    PRUint32 len = mBlobs.Length();

nsTArray.Length() is really fast. No need for the temporary IMHO.

@@ +169,5 @@
> +  if (aStart || length) {
> +    // Prune the list of instructions if we can
> +    PRUint32 i;
> +    PRUint32 len = mBlobs.Length();
> +    for (i = 0; i < len; i++) {

If you add |&& aStart| to the loop condition, you'd optimize the case when the slice happens to fall right between two blobs to not call slice. Actually, wouldn't it work if you simply made |aStart| be the only loop condition?

@@ +186,5 @@
> +                                           getter_AddRefs(firstBlob));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        blobs.AppendElement(firstBlob);
> +        length -= upperBound;

Another optimization: If length is 0 here, you could simply return firstBlob as result of the function. No need to use a multipart blob at all.

@@ +190,5 @@
> +        length -= upperBound;
> +        i++;
> +        break;
> +      }
> +      aStart -= l;

Nit: I'm not a big fan of modifying arguments unless it's done in really trivial way early on (like supplying default). It just makes following the code simpler if you can rely on that invariant. I'd just introduce a |skipStart| variable which is initialized to aStart instead. Up to you though.

@@ +241,5 @@
> +  nsresult AppendString(JSString* aString, JSContext* aCx);
> +  nsresult AppendBlob(nsIDOMBlob* aBlob);
> +  nsresult AppendArrayBuffer(js::ArrayBuffer* aBuffer);
> +
> +  bool EnsureBufferSize(PRUint64 aSize)

The current name makes it sound like it's just changing the current, total, buffer capacity. Maybe something like ExpandBufferSize or some such would be better.

@@ +376,5 @@
> +
> +    return AppendString(str, aCx);
> +  }
> +
> +  if (JSVAL_IS_PRIMITIVE(aData)) {

So I think this is wrong per WebIDL. Basically what we should do is that if it isn't any of the types that we know (blob, arraybuffer), we should do the normal JS "convert to string" algorithm. I believe we have some pretty simple JS-API for doing that.

@@ +400,5 @@
> +  js::ArrayBuffer* buffer = js::ArrayBuffer::fromJSObject(obj);
> +  if (buffer)
> +    return AppendArrayBuffer(buffer);
> +
> +  return NS_ERROR_NOT_IMPLEMENTED;

So same thing here. If it isn't an object type that we know, coerce into a string instead.

@@ +403,5 @@
> +
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +nsresult NS_NewBlobBuilder(nsISupports* *aSupports)

Do we really need this? Just use new nsDOMBlobBuilder as needed.

::: content/base/test/fileutils.js
@@ +132,5 @@
> +    SimpleTest.finish();
> +  }
> +}
> +
> +function createFileWithData(fileData) {

Ideally we should create a SpecialPowers function which does this, but since you just moved this we can do it later.

@@ +150,5 @@
> +
> +  return fileList.files[0];
> +}
> +
> +function gc() {

I know you just moved this, but please kill this function in favor of using SpecialPowers.gc();
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-19 12:12:03 PDT
Created attachment 533745 [details] [diff] [review]
Interdiff

(In reply to comment #8)
> Comment on attachment 532493 [details] [diff] [review] [review]
> Patch
> 
> Review of attachment 532493 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Would like to see a new patch for some of this stuff. Please include an
> interdiff (mq makes it easy).
> 
> ::: content/base/public/nsDOMFile.h
> @@ +61,5 @@
> > +
> > +nsresult NS_NewBlobBuilder(nsISupports* *aSupports);
> > +void ParseSize(PRInt64 aSize, PRInt64& aStart, PRInt64& aEnd);
> > +
> > +class DataOwner {
> 
> It doesn't seem like you need to move DataOwner or add the mLength member
> any more, right?

Correct.  Reverted.

> @@ -59,3 @@
> >  
> >  class nsDOMFile : public nsIDOMFile,
> > -                  public nsIDOMBlob_MOZILLA_2_0_BRANCH,
> 
> Thanks for cleaning this up!
> 
> However, there's no need to keep the old unprefixed-nonscriptable slice
> method any more, so just nuke it.

Done.

> ::: content/base/src/nsDOMBlobBuilder.cpp
> @@ +51,5 @@
> > +#define PR_INT64_MIN (-PR_INT64_MAX - 1)
> > +
> > +using namespace mozilla;
> > +
> > +class nsDOMMultipartBlob : public nsDOMFile
> 
> Hmm.. is there a reason to inherit nsDOMFile other than to get classinfo to
> work?

They still share several methods.  Eventually I'd like to split nsDOMFile into an nsDOMBlob that everything can inherit from and an nsDOMFile that adds file specific bits.
> 
> @@ +71,5 @@
> > +                      const nsAString& aContentType, PRUint8 optional_argc,
> > +                      nsIDOMBlob **aBlob);
> > +
> > +protected:
> > +  InfallibleTArray<nsCOMPtr<nsIDOMBlob> > mBlobs;
> 
> I would just use nsTArray and rely on the fact that it by default is
> infallible. We're unlikely to ever change (or if we do, we'll have to audit
> all nsTArray usage anyway). We're IMHO more likely to get rid of the
> InfallibleTArray typedef.

Done.

> @@ +132,5 @@
> > +    rv = stream->AppendStream(scratchStream);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +  }
> > +
> > +  NS_ABORT_IF_FALSE(mStart == 0, "Er, what?");
> 
> Why this? Isn't mStart unused by nsDOMMultipartBlob?

Removed.  mStart isn't used, but this assert is just "randomly thrown in".

> @@ +158,5 @@
> > +
> > +  ParseSize((PRInt64)thisLength, aStart, aEnd);
> > +
> > +  // If we clamped to nothing we create an empty blob
> > +  InfallibleTArray<nsCOMPtr<nsIDOMBlob> > blobs;
> 
> Nit: Instead of creating this array, it might be slightly simpler to just
> create a multipart blob directly and give it a AppendBlob method (which
> could also keep a running tab on the total length). Up to you though.

This way avoids creating a blob at all if GetBlob is never called, so I think I prefer this.

> @@ +165,5 @@
> > +  PRUint64 finalLength = length;
> > +
> > +  NS_ABORT_IF_FALSE(aStart + length <= thisLength, "Er, what?");
> > +
> > +  if (aStart || length) {
> 
> Why the aStart check here?

No reason.  Removed.

> @@ +168,5 @@
> > +
> > +  if (aStart || length) {
> > +    // Prune the list of instructions if we can
> > +    PRUint32 i;
> > +    PRUint32 len = mBlobs.Length();
> 
> nsTArray.Length() is really fast. No need for the temporary IMHO.

Fixed, in three spots.

> @@ +169,5 @@
> > +  if (aStart || length) {
> > +    // Prune the list of instructions if we can
> > +    PRUint32 i;
> > +    PRUint32 len = mBlobs.Length();
> > +    for (i = 0; i < len; i++) {
> 
> If you add |&& aStart| to the loop condition, you'd optimize the case when
> the slice happens to fall right between two blobs to not call slice.
> Actually, wouldn't it work if you simply made |aStart| be the only loop
> condition?

Done.  I *think* it would work if aStart is the only condition, but having both doesn't hurt.

> @@ +186,5 @@
> > +                                           getter_AddRefs(firstBlob));
> > +        NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +        blobs.AppendElement(firstBlob);
> > +        length -= upperBound;
> 
> Another optimization: If length is 0 here, you could simply return firstBlob
> as result of the function. No need to use a multipart blob at all.

Done.

> @@ +190,5 @@
> > +        length -= upperBound;
> > +        i++;
> > +        break;
> > +      }
> > +      aStart -= l;
> 
> Nit: I'm not a big fan of modifying arguments unless it's done in really
> trivial way early on (like supplying default). It just makes following the
> code simpler if you can rely on that invariant. I'd just introduce a
> |skipStart| variable which is initialized to aStart instead. Up to you
> though.

Fixed.

> @@ +241,5 @@
> > +  nsresult AppendString(JSString* aString, JSContext* aCx);
> > +  nsresult AppendBlob(nsIDOMBlob* aBlob);
> > +  nsresult AppendArrayBuffer(js::ArrayBuffer* aBuffer);
> > +
> > +  bool EnsureBufferSize(PRUint64 aSize)
> 
> The current name makes it sound like it's just changing the current, total,
> buffer capacity. Maybe something like ExpandBufferSize or some such would be
> better.

Done.

> @@ +376,5 @@
> > +
> > +    return AppendString(str, aCx);
> > +  }
> > +
> > +  if (JSVAL_IS_PRIMITIVE(aData)) {
> 
> So I think this is wrong per WebIDL. Basically what we should do is that if
> it isn't any of the types that we know (blob, arraybuffer), we should do the
> normal JS "convert to string" algorithm. I believe we have some pretty
> simple JS-API for doing that.

Fixed, with a test for integers.  I didn't want to test DOM objects in case their toString results change in the future.

> @@ +400,5 @@
> > +  js::ArrayBuffer* buffer = js::ArrayBuffer::fromJSObject(obj);
> > +  if (buffer)
> > +    return AppendArrayBuffer(buffer);
> > +
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> 
> So same thing here. If it isn't an object type that we know, coerce into a
> string instead.

Done.

> @@ +403,5 @@
> > +
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> > +}
> > +
> > +nsresult NS_NewBlobBuilder(nsISupports* *aSupports)
> 
> Do we really need this? Just use new nsDOMBlobBuilder as needed.

Yes, the class info stuff needs to get at a constructor with this signature.
 
> ::: content/base/test/fileutils.js
> @@ +132,5 @@
> > +    SimpleTest.finish();
> > +  }
> > +}
> > +
> > +function createFileWithData(fileData) {
> 
> Ideally we should create a SpecialPowers function which does this, but since
> you just moved this we can do it later.

Followup fodder.

> @@ +150,5 @@
> > +
> > +  return fileList.files[0];
> > +}
> > +
> > +function gc() {
> 
> I know you just moved this, but please kill this function in favor of using
> SpecialPowers.gc();

Done.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-19 12:14:10 PDT
Created attachment 533746 [details] [diff] [review]
Patch

Full patch
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-19 13:55:45 PDT
Comment on attachment 533746 [details] [diff] [review]
Patch

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

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +78,5 @@
> +NS_IMETHODIMP
> +nsDOMMultipartBlob::GetSize(PRUint64* aLength)
> +{
> +  nsresult rv;
> +  NS_ENSURE_ARG(aLength);

Remove this check. If someone passes in null here they deserve to crash. (This is what we generally do for XPCOM getters)

@@ +162,5 @@
> +  PRUint64 skipStart = aStart;
> +
> +  NS_ABORT_IF_FALSE(aStart + length <= thisLength, "Er, what?");
> +
> +  if (length) {

Is it really worth optimizing this case? Seems like the code would work if you just always executed the code inside this |if|.

@@ +165,5 @@
> +
> +  if (length) {
> +    // Prune the list of instructions if we can
> +    PRUint32 i;
> +    for (i = 0; aStart && i < mBlobs.Length(); i++) {

that should be skipStart instead of aStart, right?

@@ +166,5 @@
> +  if (length) {
> +    // Prune the list of instructions if we can
> +    PRUint32 i;
> +    for (i = 0; aStart && i < mBlobs.Length(); i++) {
> +      nsIDOMBlob* blob = mBlobs.ElementAt(i).get();

mBlobs[i];

@@ +197,5 @@
> +    }
> +
> +    // Now append enough blobs until we're done
> +    if (length != 0) {
> +      for (; i < mBlobs.Length(); i++) {

Combine these to |for(; length && i < mBlobs.Length(); i++)| to reduce indentation.

@@ +211,5 @@
> +                                             getter_AddRefs(lastBlob));
> +          NS_ENSURE_SUCCESS(rv, rv);
> +
> +          blobs.AppendElement(lastBlob);
> +          i++;

Why the i++?

@@ +215,5 @@
> +          i++;
> +          break;
> +        }
> +
> +        blobs.AppendElement(blob);

If you do the combine of the |if| and |for| above, you can simply do

else {
  blobs.AppendElement(blob);
}

and remove the break.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-20 14:24:59 PDT
Created attachment 534111 [details] [diff] [review]
hg export patch
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2011-05-20 14:53:13 PDT
Checked in

http://hg.mozilla.org/mozilla-central/rev/b8404a1d3153
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-20 17:02:45 PDT
Backed out http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=0569f7995cd0

There were conflicts and a botched merge and everything went to hell.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-21 17:55:35 PDT
Relanded.  There was nothing wrong with this.

http://hg.mozilla.org/mozilla-central/rev/6672e37207b8
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-24 13:01:55 PDT
Created attachment 534867 [details] [diff] [review]
Rename nsIDOMBlobBuilder to nsIDOMMozBlobBuilder

This renames this interface to quiet an assertion in nsDOMClassInfo.cpp.  It has rs=sicking over irc.

This should have 0 risk.  We should take it on aurora to fix the assertion.
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-24 14:04:10 PDT
Follow landed on m-c

http://hg.mozilla.org/mozilla-central/rev/395f32fc8a33
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-03 09:39:57 PDT
And followup landed on aurora

http://hg.mozilla.org/releases/mozilla-aurora/rev/1127822a7385
Comment 19 Eric Shepherd [:sheppy] 2011-06-13 13:31:40 PDT
Documentation:

https://developer.mozilla.org/en/DOM/BlobBuilder

And linked to from:

https://developer.mozilla.org/en/Gecko_DOM_Reference#DOM.c2.a0Files
https://developer.mozilla.org/en/DOM/Blob

And Firefox 6 for developers.

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