HTML5 draft spec says implementation shall support quality parameter in canvas.toDataURL(type, quality, ...) when the type is image/jpeg. WebKit does not support it now.
Created attachment 53127 [details] Proposed patch A proposed patch (including test cases) to the bug.
Attachment 53127 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1694139
Created attachment 53143 [details] Update patch to fix build issue on chromium
Attachment 53143 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1674233
Created attachment 53148 [details] Proposed patch - Fix a typo in the previous patch
Created attachment 53155 [details] Proposed patch - try to satisfy build bot
Comment on attachment 53155 [details] Proposed patch - try to satisfy build bot Generally looks good, but fails on mac: WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMHTMLCanvasElement.mm: In function 'NSString* -[DOMHTMLCanvasElement toDataURL:](DOMHTMLCanvasElement*, objc_selector*, NSString*)': WebKit/WebKitBuild/Release/DerivedSources/WebCore/DOMHTMLCanvasElement.mm:73: error: no matching function for call to 'WebCore::HTMLCanvasElement::toDataURL(NSString*&, WebCore::ExceptionCode&)' WebKit/WebCore/dom/CanvasSurface.h:62: note: candidates are: WebCore::String WebCore::CanvasSurface::toDataURL(const WebCore::String&, double, WebCore::ExceptionCode&)
Oliver, how do you think the ObjC binding should be done here? Is there a concern about backwards compatibility?
Created attachment 53408 [details] Fixed build issue for mac
Maybe this bug gets overlapped by bug 37117?
I don't think so - platforms can do this 37117 their own way.
Comment on attachment 53408 [details] Fixed build issue for mac This looks good and I would like to get this landed. My one suggestion would be to put the test in platform independent location (if possible) and just skip it on platforms where this is not implemented (and file bugs for those platforms to implement the feature). You may also want to test that the quality argument does not effect non-jpep MIME types (if possible). r=me.
(In reply to comment #12) > (From update of attachment 53408 [details]) > This looks good and I would like to get this landed. My one suggestion would > be to put the test in platform independent location (if possible) and just skip > it on platforms where this is not implemented (and file bugs for those > platforms to implement the feature). You may also want to test that the quality > argument does not effect non-jpep MIME types (if possible). > > r=me. One additional thing that occurred to me was that we may want an illegal value for the default value instead of 1.0. For CG, for instance, the default compression for JPEG would not be 1.0, so we would want to know if no quality argument was passed. Does that seem reasonable. I suggest a value of -1.0 as the default.
Should it be committed and the follow up with the additional comments? Seems reasonable... If so then I'll add it to the commit queue.
Yeah, that seems fine.
Comment on attachment 53408 [details] Fixed build issue for mac Rejecting patch 53408 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Sam Weinig', u'--force']" exit_code: 1 Last 500 characters of output: platform/qt/fast/canvas/toDataURL-jpeg-quality-basic.html patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-notnumber-expected.txt patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-notnumber.html patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-outsiderange-expected.txt patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg-quality-outsiderange.html patching file LayoutTests/platform/qt/fast/canvas/toDataURL-jpeg.js Full output: http://webkit-commit-queue.appspot.com/results/1828089
Created attachment 54253 [details] Make the patch upstream
Comment on attachment 54253 [details] Make the patch upstream > +#include <wtf/MathExtras.h> No need for MathExtras.h (see below). > + double quality = 1.0; > + if (args.size() > 1) { > + JSValue v = args.at(1); > + if (v.isNumber()) > + quality = v.toNumber(exec); > + if (quality > 1.0 || quality < 0.0 || !isfinite(quality)) > + quality = 1.0; > + } There is a simpler, cleaner way to write this. double quality = args.at(1).toNumber(exec); if (!(quality >= 0 && quality < 1)) quality = 1; You don't need all those if statements. This correctly handles NAN, infinity, missing arguments, and the like. A NAN will return false from any comparison. > +#include "PlatformString.h" > > #include <wtf/OwnPtr.h> > #include <wtf/Noncopyable.h> > @@ -41,7 +43,6 @@ class FloatSize; > class GraphicsContext; > class ImageBuffer; > class IntPoint; > -class String; There is no need to include the header here in CanvasSurface.h. You can pass a const String& through to another const String& without including the definition of the String class. > - DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type) > + // HTML5 draft spec: DOMString toDataURL(DOMString type, ...); > + [Custom] DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type) > raises(DOMException); I don't understand this comment. You should leave it out unless it has something specific to say. > - if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data())) > + if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data(), quality*100+0.5)) This is not the correct way to scale a [0,1] floating point value to a [0,100] integer value. The correct way is "quality * nextafter(100, 0)".
(In reply to comment #18) > (From update of attachment 54253 [details]) > > +#include <wtf/MathExtras.h> > > No need for MathExtras.h (see below). > > > + double quality = 1.0; > > + if (args.size() > 1) { > > + JSValue v = args.at(1); > > + if (v.isNumber()) > > + quality = v.toNumber(exec); > > + if (quality > 1.0 || quality < 0.0 || !isfinite(quality)) > > + quality = 1.0; > > + } > > There is a simpler, cleaner way to write this. > > double quality = args.at(1).toNumber(exec); > if (!(quality >= 0 && quality < 1)) > quality = 1; > > You don't need all those if statements. This correctly handles NAN, infinity, > missing arguments, and the like. A NAN will return false from any comparison. > HTML 5 draft spec reuires quality must be with number type. So the simpler way can not handle one case in which quality is a numeric like string (e.g. String('0.01'), per the spec, it should be converted to 100% quality) > > +#include "PlatformString.h" > > > > #include <wtf/OwnPtr.h> > > #include <wtf/Noncopyable.h> > > @@ -41,7 +43,6 @@ class FloatSize; > > class GraphicsContext; > > class ImageBuffer; > > class IntPoint; > > -class String; > > There is no need to include the header here in CanvasSurface.h. You can pass a > const String& through to another const String& without including the definition > of the String class. I had thought it need not to include PlatformString.h, but CanvasSurface.cpp doesn't compile by gcc-4.3.3-Ubuntu. I think the reson is that inline function return non-reference type. So If I inlcude PlatformString.h in CavasSurface.cpp before line #include "CanvasSurface.h", it's a bit violation of code style. > > > - DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type) > > + // HTML5 draft spec: DOMString toDataURL(DOMString type, ...); > > + [Custom] DOMString toDataURL(in [ConvertUndefinedOrNullToNullString] DOMString type) > > raises(DOMException); > > I don't understand this comment. You should leave it out unless it has > something specific to say. > OK. Will remove it > > - if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data())) > > + if (!m_data.m_pixmap.save(&buffer, mimeType.substring(sizeof "image").utf8().data(), quality*100+0.5)) > > This is not the correct way to scale a [0,1] floating point value to a [0,100] > integer value. The correct way is "quality * nextafter(100, 0)". quality * nextafter(100, 0) is not bad. But quality * 100 + 0.5 should be faster than the previous one, and the two ways has same precsion. Regard as correctness, if quality = 0.9995, the result is 99 if it adopts the previous way, but I think 100 is more resonable.
Created attachment 54529 [details] Patch -- follow some comments from Dirk Schulze
Comment on attachment 53408 [details] Fixed build issue for mac Cleared Sam Weinig's review+ from obsolete attachment 53408 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 55686 [details] patch 1. Follow some review comments. 2. Describe the purpose of each test cases.
Could this be done without custom bindings? Just take an optional double and then do the equivalent of if (!(0.0 <= quality && quality <= 1.0)) quality = 1.0; in CanvasSurface::toDataURL?
(In reply to comment #23) > Could this be done without custom bindings? > > Just take an optional double and then do the equivalent of > > if (!(0.0 <= quality && quality <= 1.0)) > quality = 1.0; > > in CanvasSurface::toDataURL? It can't. Just as I said in comment #19, we need to check the real type of quality parameter.
Comment on attachment 55686 [details] patch Looks like everything has been addressed and it is well tested. Also appears to be properly ported everywhere. Looks good to me.
Comment on attachment 55686 [details] patch Since this won't apply cleanly anymore, I am going to land this instead of the bot.
Landed in r60458 with some fixes.