Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Description • 13 years ago

We should do this and get it standardized.

Not sure whether or not we need to keep mozGetAsFile.  I think we can afford to just drop it completely.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 13 years ago

So, the comment in the spec has basically

  void toBlob(FileCallback cb, optional DOMString type, any... args);

I'm not sure if it's useful to change the name of the API now, as we'll need to change it at least once more. Waiting until Hixie fleshes this out and implement the spec then seems more logical to me.

	David Teller [:Yoric] - still alive but not very active
	Comment 2 • 12 years ago

I am currently developing a patch using |mozGetAsFile| for bug 753768 but I believe that it would work just as well with |toBlob|.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 3 • 12 years ago

Gaia is going to want this to go fast.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 4 • 12 years ago

This doesn't do off-main-thread encoding, but it does dispatch the response off the event loop to mimic the async behavior.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 12 years ago

Comment on attachment 671562 [details] [diff] [review]
Patch

>+// XXXkhuey this should be async, but we're lazy.
You mean off-main-thread?


>+NS_IMETHODIMP
>+nsHTMLCanvasElement::ToBlob(nsIFileCallback* aCallback,
>+                            const nsAString& aType,
>+                            nsIVariant* aParams,
>+                            uint8_t optional_argc)
aParamName


Before using aType, it should be lowercased.


>+{
>+  // do a trust check if this is a write-only canvas
>+  if ((mWriteOnly) &&
No need for () around mWriteOnly

>+      !nsContentUtils::IsCallerTrustedForRead()) {
>+    return NS_ERROR_DOM_SECURITY_ERR;
>+  }
>+  void* imgData = nullptr;
>+  rv = NS_ReadInputStreamToBuffer(stream, &imgData, (uint32_t)imgSize);
I'd prefer C++ cast

>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // The DOMFile takes ownership of the buffer
>+  nsRefPtr<nsDOMMemoryFile> blob =
>+    new nsDOMMemoryFile(imgData, (uint32_t)imgSize, type);
ditto

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 6 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/227e87a7c628

	Doug Turner (:dougt)
	Comment 7 • 12 years ago

this is going to be needed to work around many of the the OOM in gallery, video, and music.  +'ing.

	Justin Lebar (not reading bugmail)
	Comment 8 • 12 years ago

(In reply to Doug Turner (:dougt) from comment #7)
> this is going to be needed to work around many of the the OOM in gallery,
> video, and music.  +'ing.

Can you elaborate, or point to bugs?

base-64-encoded URIs are 2.7x larger than blobs, but I don't see how using this would actually solve an underlying problem; if we OOM with data URIs, we'd OOM with this, just after showing something like 3x more images.

	Doug Turner (:dougt)
	Comment 9 • 12 years ago

we can use mozGetAsFile.  leaving just as a nom.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 10 • 12 years ago

Please don't use mozGetAsFile.  We want to remove that eventually.

	Ed Morley [:emorley]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/227e87a7c628
https://hg.mozilla.org/mozilla-central/rev/30395df97c29

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 12 • 12 years ago

Comment on attachment 671562 [details] [diff] [review]
Patch

Gaia will want this to be able to use memory more efficiently

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Certain Gaia apps may be written with less supported APIs or may crash after running out of memory more often.
Testing completed (on m-c, etc.): On m-c, has automated tests.
Risk to taking this patch (and alternatives if risky): Low.  This is just an async wrapper around an existing synchronous codepath.
String or UUID changes made by this patch: This changes the IID for nsIDOMHTMLCanvasElement.  I could avoid that if strictly necessary, but I don't think it matters.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbfef8616b63 (I qfolded the 2 patches into 1)

	David Flanagan [:djf]
	Comment 14 • 12 years ago

This is great!  For the record, Gaia only uses data urls when for transferring images through activities because the actvities API doesn't support blobs yet.  Everywhere else we use mozGetAsFile.  So toBlob() probably won't help with memory at all, unless mozGetAsFile is really inefficient in some way that I don't know about.

I was hopeful that the fact that is async would improve the speed of thumbnail and album art metadata parsing for the Gallery and Music apps and also help the screenshot code in the window manager app.  But give Kyle's comment #4 above, I'm not sure this will help us with speed either.  Could you elaborate, Kyle?

If there is a speed improvement, then I think we should convert the metadata parsing code in Gallery and Music, and the screenshot code in the window manager to use toBlob(), but leave other mozGetAsFile() calls as is for v1.

For v2, we'll obviously want to convert everything to use the standard method instead of the moz-prefixed one.

cc'ing timdream because he owns the window manager screenshot stuff and should know about this.

	Markus Stange [:mstange]
	Comment 15 • 12 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> This doesn't do off-main-thread encoding

Is there a bug for implementing off-main-thread encoding?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 16 • 12 years ago

No.

	Markus Stange [:mstange]
	Comment 17 • 12 years ago

Filed bug 817700.

	Florian Scholz (Open Web Docs)
	Comment 18 • 10 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/19
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement.toBlob