as I understand from my reading of [1], we don't support the 3rd argument jpeg quality for toBlob whereas we do for toDataURL. The spec [2] says that we should support it in both functions. We badly needs this in Bug 889765 for Firefox OS... My understanding is that it's not so much work, we only need to parse the incoming arguments in the same way that we do for data URL. [1] https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLCanvasElement.cpp#579 [2] http://www.w3.org/TR/html5/embedded-content-0.html#dom-canvas-toblob Feel free to resolve WFM if I understood wrongly. Asking for leo because bug 889765 is leo+.

How badly is this needed? Is there a workaround? Adding another function argument at this point brings some risk.

for leo we could try to resize the image a little more, but I'd really want to use the quality instead. I'd like to use it when generating the thumbnails in bug 889902 as well, which would probably save us some memory. And you know we need this :)

Here is a WIP patch. code compile in the content/html directory but I haven't tested it yet and my full build has not finished yet.

load this file with and without this patch and you'll see the difference. Especially look at the "quality = 0" line.

the first one had a bug. qualities are < 1 :)

asking feedback because my reftests don't work yet

Comment on attachment 774515 [details] [diff] [review] wip patch v2 reworking the reftests, I had some explanation from :padenot.

reftests are passing, but they crashing when running only the new reftests. The don't crash when running with others...

try is https://tbpl.mozilla.org/?tree=Try&rev=f2138e09e6b8

Comment on attachment 774672 [details] [diff] [review] patch v1 Review of attachment 774672 [details] [diff] [review]: ----------------------------------------------------------------- Blake, can you sr the idl/webidl changes, since we're changing the web-accessible API? ::: content/html/content/reftests/toblob-todataurl/images/LICENSE @@ +1,1 @@ > +this file comes from http://en.wikipedia.org/wiki/File:Example.png I don't think you can do this. For one thing, it's not actually a license! Better to just create your own, or copy something that we already have eg in image/test/reftest. ::: content/html/content/src/HTMLCanvasElement.cpp @@ +475,5 @@ > +HTMLCanvasElement::ParseParams(JSContext* aCx, > + const nsAString& aType, > + const JS::Value& aEncoderOptions, > + nsAString& aParams, > + bool& usingCustomParseOptions) { { on new line @@ +484,5 @@ > + double quality = aEncoderOptions.toNumber(); > + // Quality must be between 0.0 and 1.0, inclusive > + if (quality >= 0.0 && quality <= 1.0) { > + aParams.AppendLiteral("quality="); > + aParams.AppendInt(NS_lround(quality * 100.0)); Oh man. I realize you only moved this code, but it's pretty crappy that we're doing this with string manipulation. I'd have much rather we fixed Imagelib to not need this stuff. ::: dom/interfaces/html/nsIDOMHTMLCanvasElement.idl @@ +74,3 @@ > void toBlob(in nsIFileCallback callback, > + [optional] in DOMString type, > + [optional] in jsval params); I think you need to rev the nsIDOMHTMLCanvasElement uuid.

addressed comments from Joe. Thanks for the quick review ! I simplified how the reftests are done, now all html files share the same js files which makes it easier to edit. I did my own png file as well ! new try is https://tbpl.mozilla.org/?tree=Try&rev=d9938306af81

will add a new patch with r= sr= once everything is cleared.

(In reply to John Hammink from comment #1) > How badly is this needed? Is there a workaround? Adding another function > argument at this point brings some risk. Nothing in Gaia uses this yet afaik, so imho there is no risk here. Joe, do you think it would be a good idea to add a reftest without a quality param too ?

Yes, sure!

Comment on attachment 774710 [details] [diff] [review] patch v2 >try: -b do -p linux,linux64,macosx64,win32,android,android-armv6,ics_armv7a_gecko -u reftest,reftest-no-accel,crashtest,mochitests -t none I don't think this is the patch you wanted it to be :)

(In reply to Blake Kaplan (:mrbkap) from comment #16) > Comment on attachment 774710 [details] [diff] [review] > patch v2 > > >try: -b do -p linux,linux64,macosx64,win32,android,android-armv6,ics_armv7a_gecko -u reftest,reftest-no-accel,crashtest,mochitests -t none > > I don't think this is the patch you wanted it to be :) indeed ;) patch v3 coming in some minutes

added reftests for: * q=.92 * q=undefined * no quality param carrying r=joedrew thanks ! new try : https://tbpl.mozilla.org/?tree=Try&rev=af9aa10d2e02

Because b2g18 doesn't have webidl, I made an adhoc patch for it. same reftests are working :) I didn't updated the rev on the idl because strangely the idl already haves the necessary parameters on this branch. try is https://tbpl.mozilla.org/?tree=Try&rev=7e8fda814820

my previous try had some leftover patches, here is the "real" b2g18 try: https://tbpl.mozilla.org/?tree=Try&rev=fe71b198ba1e

Dale was ok with finishing this. Dale, I'd really like to have this in b2g18 :)

Comment on attachment 775136 [details] [diff] [review] b2g18 patch v1 Review of attachment 775136 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me. No meaningful differences from the m-c difference, right?

er, m-c patch.

Comment on attachment 775123 [details] [diff] [review] patch v3 Review of attachment 775123 [details] [diff] [review]: ----------------------------------------------------------------- sr=mrbkap with the reference changed into a pointer and the nits addressed. ::: content/html/content/src/HTMLCanvasElement.cpp @@ +475,5 @@ > +HTMLCanvasElement::ParseParams(JSContext* aCx, > + const nsAString& aType, > + const JS::Value& aEncoderOptions, > + nsAString& aParams, > + bool& usingCustomParseOptions) Gecko style is to use a pointer instead of a reference for out params like this. @@ +477,5 @@ > + const JS::Value& aEncoderOptions, > + nsAString& aParams, > + bool& usingCustomParseOptions) > +{ > + Nit: extra blank line here. @@ +573,5 @@ > HTMLCanvasElement::ToBlob(nsIFileCallback* aCallback, > + const nsAString& aType, > + const JS::Value& aEncoderOptions, > + JSContext* aCx > + ) Nit: the ) should go next to the x on the same line as aCx.

Unclear if this is really needed ? Can you please help suggesting the lowest risk option here between leo+ this bug or doing the workaround as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=889765#c18

Triage - leo+ as required by 889765, will speak to schung with regards to comment 25 and alter nomination if the status changes.

Patch with nits addressed, Carrying r=joedrew, sr=mrbkap Did another try run just to be sure nothing broke with the changes https://tbpl.mozilla.org/?tree=Try&rev=b323a4a68efa

Merged to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab86551b26

Patch for b2g18 with nits addressed, Carrying r=joedrew, sr=mrbkap

Merged to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/b50d763029d2

Bugs aren't marked fixed until the inbound landing has merged to mozilla-central. Also, this is failing reftests. Did you test locally/on try? :-) https://tbpl.mozilla.org/php/getParsedLog.php?id=25427445&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25427332&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=25427314&tree=Mozilla-Inbound Backed out: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/76ecd043c64b

Just seen the try run in comment 27 - there were no reftests requested there. Please also wait until the inbound landing is green before landing on b2g18 if possible. Thank you :-)

ah apologies, I did another try run https://tbpl.mozilla.org/?tree=Try&rev=b323a4a68efa Which looked good, will figure out what went wrong there

b2g18 backout: https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f6f4bc1a621 (In reply to Dale Harvey (:daleharvey) from comment #33) > ah apologies, I did another try run > > https://tbpl.mozilla.org/?tree=Try&rev=b323a4a68efa > > Which looked good, will figure out what went wrong there There are no reftests requested :-) (try: -b o -p all -u mochitests -t none) http://trychooser.pub.build.mozilla.org/ might help?

ugh yeh I lost the binaries when applying the patch, will reup a new one to try, sorry for the churn

(In reply to Dale Harvey (:daleharvey) from comment #28) > Merged to inbound: > https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab86551b26 1) B2G landings should landing on birch, not inbound. (In reply to Dale Harvey (:daleharvey) from comment #30) > Merged to b2g18: > https://hg.mozilla.org/releases/mozilla-b2g18/rev/b50d763029d2 2) Was there any reason this had to land on b2g18 prior to making it over to m-c, which is our normal procedure? In fact, that procedure is in place for exactly the reasons that played out here - avoiding multiple-branch bustage & backouts.

No reasons, it was my mistake, I thought 1. DOM and non b2g specific bugs landed on inbound and 2. that inbound and b2g could happen in parallel. Wont happen again (was my first push) apologies

New patch with binaries, carrying review, pushed to try with reftests enabled ttps://tbpl.mozilla.org/?tree=Try&rev=c26f0706e493

Try pass is green, My account is broken (filed a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=896447) so flagging checkin needed, will reupload b2g18 patch.

Account got fixed, merged to birch https://hg.mozilla.org/projects/birch/rev/1bbc7f3de64c

Updated b2g18 patch (with test files included), carrying review try doesnt work with b2g18 but the patch builds locally and is very close to the central patch. Will leave for sheriffs to uplift so I dont get it wrong again :)

we had to back this out https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4bf993a58a because causing reftest failures on android like https://tbpl.mozilla.org/php/getParsedLog.php?id=25598464&tree=Mozilla-Inbound for the try run, the Try run was performed, but requested "reftest" which doesn't catch android, since they are reftest-N (where N is 1-4), so i guess this was the reason that this reftest failures didn't show up/

Birch also doesn't run android reftests (other than jsreftests), which is why the failures didn't show up before the merge from birch -> m-c.

Added fuzzy-if statements for android reftests, error was due to read-back data being different than what we wrote (all other android reftest do similiar) Pushed to try, single fail was a typo https://tbpl.mozilla.org/?tree=Try&rev=228690e2135b (other fail was an unrelated randomorange)

b2g18 patch with above changes

I added a note in : https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement and https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25