Closed Bug 744907 Opened 12 years ago Closed 12 years ago

Remove BlobBuilder

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

We decided to go with a constructor for Blob instead.
ETA on this? (I see the constructor is supported now.)
Depends on: 747628
Assignee: nobody → Ms2ger
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #621439 - Flags: review?(jonas)
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.
We shouldn't remove MozBlobBuilder right now because of bug 752402.
Depends on: 752402
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 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)
Attachment #621439 - Flags: review?(jonas)
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 on attachment 621439 [details] [diff] [review]
Patch v1

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

How about after the next merge?
Attachment #621439 - Flags: review?(jonas)
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.
Attachment #621439 - Flags: review?(jonas) → review+
Next merge happened, so I think this can be checked in.
Attached patch Patch v1.1 (obsolete) — Splinter Review
This bitrotted quite a bit; would you mind doing a quick pass over it again?
Attachment #621439 - Attachment is obsolete: true
Attachment #662126 - Flags: review?(jonas)
Attached patch Patch v2Splinter Review
With review comment addressed.
Attachment #662126 - Attachment is obsolete: true
Attachment #662126 - Flags: review?(jonas)
Attachment #662143 - Flags: review?(jonas)
Comment on attachment 662143 [details] [diff] [review]
Patch v2

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

Yay! Thanks!
Attachment #662143 - Flags: review?(jonas) → review+
>@@ -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 :(
>--- a/content/base/src/nsDOMBlobBuilder.cpp
>+++ b/content/base/src/nsDOMBlobBuilder.cpp
Isn't the file renamed?
(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.
https://hg.mozilla.org/mozilla-central/rev/758e3d456e0b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 794010
Depends on: 834619
No longer depends on: 834619
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.