Last Comment Bug 744907 - Remove BlobBuilder
: Remove BlobBuilder
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on: 747628 752402 794010
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-12 13:00 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-01-25 02:55 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (28.81 KB, patch)
2012-05-06 11:19 PDT, :Ms2ger (⌚ UTC+1/+2)
jonas: review+
Details | Diff | Splinter Review
Patch v1.1 (31.60 KB, patch)
2012-09-18 04:53 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2 (36.55 KB, patch)
2012-09-18 07:09 PDT, :Ms2ger (⌚ UTC+1/+2)
jonas: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-04-12 13:00:47 PDT
We decided to go with a constructor for Blob instead.
Comment 1 Simon Pieters 2012-04-16 07:51:22 PDT
ETA on this? (I see the constructor is supported now.)
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-05-06 11:19:22 PDT
Created attachment 621439 [details] [diff] [review]
Patch v1
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-06 18:32:58 PDT
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] 2012-05-06 21:52:26 PDT
We shouldn't remove MozBlobBuilder right now because of bug 752402.
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2012-05-07 10:26:35 PDT
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) PTO Until July 5th 2012-05-14 13:45:38 PDT
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] 2012-05-14 16:09:14 PDT
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 (⌚ UTC+1/+2) 2012-08-04 03:31:33 PDT
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) PTO Until July 5th 2012-08-10 13:21:18 PDT
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) PTO Until July 5th 2012-09-17 22:45:02 PDT
Next merge happened, so I think this can be checked in.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-09-18 04:53:07 PDT
Created attachment 662126 [details] [diff] [review]
Patch v1.1

This bitrotted quite a bit; would you mind doing a quick pass over it again?
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-09-18 07:09:15 PDT
Created attachment 662143 [details] [diff] [review]
Patch v2

With review comment addressed.
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2012-09-18 13:25:10 PDT
Comment on attachment 662143 [details] [diff] [review]
Patch v2

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

Yay! Thanks!
Comment 14 Masatoshi Kimura [:emk] 2012-09-18 15:47:51 PDT
>@@ -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] 2012-09-18 23:11:24 PDT
>--- a/content/base/src/nsDOMBlobBuilder.cpp
>+++ b/content/base/src/nsDOMBlobBuilder.cpp
Isn't the file renamed?
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-09-19 00:50:56 PDT
(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 (⌚ UTC+1/+2) 2012-09-20 04:28:53 PDT
https://hg.mozilla.org/mozilla-central/rev/758e3d456e0b

Note You need to log in before you can comment on or make changes to this bug.