648997 - Implement BlobBuilder

Status: RESOLVED FIXED

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

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: First go

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 525520 [details] [diff] [review]
First go

This is horribly bitrotted after the slice changes.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Unbitrotted, with a test for leaking.

Comment 5

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

Comment on attachment 531214 [details] [diff] [review]
Patch

Removing this from my queue for now based on discussions with Kyle

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Updated patch per our email conversation.  It's much cleaner now.

Comment 8

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

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Full patch

Comment 11

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

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: hg export patch

Comment 13

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

Checked in

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

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

Relanded.  There was nothing wrong with this.

http://hg.mozilla.org/mozilla-central/rev/6672e37207b8

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

Follow landed on m-c

http://hg.mozilla.org/mozilla-central/rev/395f32fc8a33

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And followup landed on aurora

http://hg.mozilla.org/releases/mozilla-aurora/rev/1127822a7385

Comment 19

[Eric Shepherd [:sheppy]]

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.