Status

()

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

({dev-doc-needed})

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

We decided to go with a constructor for Blob instead.

Comment 1

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

Updated

7 years ago
Depends on: 747628
Assignee

Updated

7 years ago
Assignee: nobody → Ms2ger
Assignee

Comment 2

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

Comment 8

7 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.
Posted 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)
Posted 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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

7 years ago
Depends on: 794010

Updated

6 years ago
Depends on: 834619

Updated

6 years ago
No longer depends on: 834619
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.