<khuey> I have part of a patch for it <khuey> Blob is a total disaster area right now though <khuey> we're going to refactor the underlying objects first

fwiw since Blobs are already available on workers (with handwritten JSAPI bindings) I don't expect this will block doing stuff on workers.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2) > fwiw since Blobs are already available on workers (with handwritten JSAPI > bindings) I don't expect this will block doing stuff on workers. Kyle: does this mean that Blob should already be listed here: https://developer.mozilla.org/en-US/docs/Web/Reference/Functions_and%20classes_available_to_workers#DOM-related_classes_available_in_workers ?

Yes. Blobs in workers were implemented in bug 664783.

This is just to share something. This patch is huge it will be bigger and bigger.

We'll also need to fix the blob/file cloning that happens in XPConnect (via Cu.cloneInto), since it looks like we've historically supported that in ObjectWrapper.jsm. Baku, are you aware of this, or should I file a separate blocking bug?

This comment is enough, thanks.

This is the first patch for this porting. The next ones will be about WebIDL.

The patch is green on try: https://tbpl.mozilla.org/?tree=Try&rev=447fb1857e29

Comment on attachment 8446625 [details] [diff] [review] patch 1 - CC on DOMFile Review of attachment 8446625 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/nsDOMFile.h @@ +156,5 @@ > + ~DOMFile() {}; > + > + // The member is the real backend implementation of this DOMFile/DOMBlob. > + // It's thread-safe and not CC-able and it's the only element that is moved > + // between threads. I think we should augment this comment by saying that we should not store any other state in this class. Otherwise this patch will be horribly wrong! What do you think? @@ +162,3 @@ > }; > > // This is the virtual class for any DOMFile backend. It must be nsISupports I gotta sign up to some C++ course... Not sure what a virtual class is. ;-) ::: content/base/src/nsDOMBlobBuilder.cpp @@ +225,5 @@ > JS::Rooted<JSObject*> obj(aCx, &element.toObject()); > nsCOMPtr<nsIDOMBlob> blob = aUnwrapFunc(aCx, obj); > if (blob) { > + nsRefPtr<DOMFileImpl> blobImpl = > + static_cast<DOMFile*>(blob.get())->Impl(); We should really add an AsDOMFile() helper on nsIDOMBlob instead of casting things all over the place like this... Wanna file that follow-up? @@ +280,4 @@ > > #ifdef DEBUG > + // XXX This is only safe so long as all blob implementations in our tree > + // inherit nsDOMFileBase. Please get rid of the comment too. ::: content/base/src/nsDOMFile.cpp @@ +740,5 @@ > mImmutable = !aMutable; > return rv; > } > > +NS_IMPL_ISUPPORTS_INHERITED0(DOMFileImplBase, DOMFileImpl) This is not needed. ::: content/base/src/nsHostObjectProtocolHandler.cpp @@ +494,5 @@ > NS_ASSERTION(info->mPrincipal == principal, "Wrong principal!"); > } > #endif > > + nsRefPtr<DOMFileImpl> blob = static_cast<DOMFileImpl*>(blobImpl.get()); Why is this a strong ref? @@ +582,4 @@ > return NS_ERROR_DOM_BAD_URI; > } > > + nsRefPtr<DOMFileImpl> blob = static_cast<DOMFileImpl*>(blobImpl.get()); Why is this a strong ref? ::: dom/base/URL.cpp @@ +115,5 @@ > const objectURLOptions& aOptions, > nsString& aResult, > ErrorResult& aError) > { > + nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(aBlob); Why is this a strong ref? ::: dom/base/nsGlobalWindow.cpp @@ +7844,5 @@ > + NS_ASSERTION(!data, "Data should be empty"); > + > + nsISupports* supports; > + if (JS_ReadBytes(reader, &supports, sizeof(supports))) { > + nsCOMPtr<nsIDOMBlob> file = new DOMFile(static_cast<DOMFileImpl*>(supports)); Please add a comment explaining what's happening here. @@ +7896,2 @@ > scTag = SCTAG_DOM_BLOB; > + nsRefPtr<DOMFile> file = static_cast<DOMFile*>(blob.get()); Why is this a strong ref? ::: dom/devicestorage/nsDeviceStorage.cpp @@ +2887,5 @@ > ->SendPDeviceStorageRequestConstructor(child, params); > return NS_OK; > } > + > + nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(mBlob.get()); Why is this a strong ref? @@ +2931,5 @@ > ContentChild::GetSingleton() > ->SendPDeviceStorageRequestConstructor(child, params); > return NS_OK; > } > + nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(mBlob.get()); Why is this a strong ref? ::: dom/workers/URL.cpp @@ +856,5 @@ > aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &blobStr); > return; > } > > + nsRefPtr<DOMFile> domBlob = static_cast<DOMFile*>(blob.get()); Why is this a strong ref? ::: dom/workers/WorkerPrivate.cpp @@ +555,5 @@ > + nsRefPtr<DOMFileImpl> fileImpl = > + static_cast<DOMFile*>(file.get())->Impl(); > + > + if (fileImpl->IsCCed()) { > + NS_WARNING("Cycle collected objects are not supported!"); Nit: file objects. @@ +575,5 @@ > + nsRefPtr<DOMFileImpl> blobImpl = > + static_cast<DOMFile*>(blob.get())->Impl(); > + > + if (blobImpl->IsCCed()) { > + NS_WARNING("Cycle collected objects are not supported!"); Nit: blob objects.

This patch is ready to land. I'll upload the others soon.

I'll continue this patch when I'm back from PTO.

This patch ports DOMFile/DOMBlob to WebIDL bindings. It's a big patch I don't know how to split it in independent sub-patches. I would like to receive a review from . hbolley for any js/xpconnect/* changes . smaug for dom/browser-element/* and for nsFrameMessageManager bz, I marked you for a review, but feel free to assign it to somebody else. khuey maybe? Thanks. This patch is the first of 3 and it's only about the porting. In a separated patch I renamed nsDOMFile.h to mozilla/dom/DOMFile.h. Then in another patch I'll get rid of nsDOMBlobBuilder, integrating its methods into DOMFile.

Did some other patch land already so that it is on Aurora already? If so, could we please file a new bug for the new patch - to ease possible regression handling.

(In reply to Olli Pettay [:smaug] from comment #18) > Did some other patch land already so that it is on Aurora already? > If so, could we please file a new bug for the new patch - to ease possible > regression handling. All the other patches I was talking about are not landed yet and they will be rebased on top of this once this is r+.

I completely forgot about the CC patch. I'm closing this bug and I'll fine a new one for the porting.

Comment on attachment 8466256 [details] [diff] [review] patch v1 Moving this patch to bug 1047483