744907 - Remove BlobBuilder

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We decided to go with a constructor for Blob instead.

Comment 1

[Simon Pieters [:zcorpan]]

ETA on this? (I see the constructor is supported now.)

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 3

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

Do we need the BlobBuilder "wrapper class" in the XHR code? Can't we use BlobSet directly instead? Seems like a simpler mental model to have fewer wrapper classes.

However, more importantly, I think we need to warn about this being deprecated for at least a release or two before we remove it. This was an API which gained attention pretty quickly, so I'm worried there are sites using it out there.

Comment 4

[Masatoshi Kimura [:emk]]

We shouldn't remove MozBlobBuilder right now because of bug 752402.

Comment 5

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

I forgot that we already warn about MozBlobBuilder usage. So I think we should just sit tight for a release or two before we remove it.

Comment 6

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

Comment on attachment 621439 [details] [diff] [review]
Patch v1

Removing review request while we're waiting for a couple of release trains to pass (and possibly for google to ship the constructor)

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 621439 [details] [diff] [review]
Patch v1

> --- a/content/base/src/nsDOMBlobBuilder.cpp
> +++ b/content/base/src/nsDOMBlobBuilder.cpp
> --- a/content/base/src/nsDOMBlobBuilder.h
> +++ b/content/base/src/nsDOMBlobBuilder.h
Let's rename these files to nsDOMMultipartFile.cpp/.h (after a few releases).

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 621439 [details] [diff] [review]
Patch v1

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

How about after the next merge?

Comment 9

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

Comment on attachment 621439 [details] [diff] [review]
Patch v1

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

r=me with that fixed.

::: content/base/src/nsXMLHttpRequest.h
@@ +655,5 @@
> +    nsresult AppendVoidPtr(const void* aData, PRUint32 aLength)
> +    { return mBlobSet.AppendVoidPtr(aData, aLength); }
> +
> +    already_AddRefed<nsIDOMBlob>
> +    GetBlobInternal(const nsACString& aContentType)

Just move this function to BlobSet and get rid of the BlobBuilder class.

Comment 10

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

Next merge happened, so I think this can be checked in.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1.1

This bitrotted quite a bit; would you mind doing a quick pass over it again?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

With review comment addressed.

Comment 13

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

Comment on attachment 662143 [details] [diff] [review]
Patch v2

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

Yay! Thanks!

Comment 14

[Masatoshi Kimura [:emk]]

>@@ -2262,19 +2258,18 @@ nsXMLHttpRequest::OnStopRequest(nsIReque
>     if (mDOMFile) {
>       mResponseBlob = mDOMFile;
>       mDOMFile = nullptr;
>     } else {
>       // Smaller files may be written in cache map instead of separate files.
Conflicts with bug 791001 :(

Comment 15

[Masatoshi Kimura [:emk]]

>--- a/content/base/src/nsDOMBlobBuilder.cpp
>+++ b/content/base/src/nsDOMBlobBuilder.cpp
Isn't the file renamed?

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Masatoshi Kimura [:emk] from comment #15)
> >--- a/content/base/src/nsDOMBlobBuilder.cpp
> >+++ b/content/base/src/nsDOMBlobBuilder.cpp
> Isn't the file renamed?

I'd be happy to, but couldn't really think of a good name for the two classes left in the file.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/758e3d456e0b