Remove BlobBuilder

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

({dev-doc-needed})

Trunk
mozilla18
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
We decided to go with a constructor for Blob instead.

Comment 1

5 years ago
ETA on this? (I see the constructor is supported now.)
(Assignee)

Updated

5 years ago
Depends on: 747628
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Assignee: nobody → Ms2ger
(Assignee)

Comment 2

5 years ago
Created attachment 621439 [details] [diff] [review]
Patch v1
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).
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 662126 [details] [diff] [review]
Patch v1.1

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)
(Assignee)

Comment 12

5 years ago
Created attachment 662143 [details] [diff] [review]
Patch v2

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?
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/mozilla-central/rev/758e3d456e0b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Depends on: 794010

Updated

5 years ago
Depends on: 834619

Updated

5 years ago
No longer depends on: 834619
You need to log in before you can comment on or make changes to this bug.