Bug 83152

Summary: [GTK] Separate image encoding from dataURL construction
Product: WebKit Reporter: noel gordon <noel.gordon>
Component: CanvasAssignee: noel gordon <noel.gordon>
Status: RESOLVED FIXED    
Severity: Normal CC: mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

noel gordon
Reported 2012-04-04 06:07:41 PDT
[GTK] Separate image encoding from dataURL construction
Attachments
Patch (4.75 KB, patch)
2012-04-04 06:23 PDT, noel gordon 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.69 KB, patch)
2012-04-09 19:59 PDT, noel gordon 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
noel gordon
Comment 1 2012-04-04 06:23:16 PDT
Created attachment 135575 [details] Patch
Martin Robinson
Comment 2 2012-04-05 18:15:04 PDT
Comment on attachment 135575 [details] Patch This seems like a nice refactor, but is there some reason that it will be useful in the future?
Martin Robinson
Comment 4 2012-04-06 08:20:18 PDT
Comment on attachment 135575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135575&action=review > Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:70 > + base64Encode(reinterpret_cast<const char*>(buffer.get()), bufferSize, base64Data); It seems unusual that we need this reinpterpret_cast here. Perhaps you could try removing this before landing?
noel gordon
Comment 5 2012-04-09 19:57:24 PDT
Happy to try removing the reinterpret_cast<> and try landing.
noel gordon
Comment 6 2012-04-09 19:59:49 PDT
Created attachment 136382 [details] Patch for landing
noel gordon
Comment 7 2012-04-09 20:09:29 PDT
Other questions: does the SupportedImageMIMETypeForEncoding registry for GTK contain items that don't begin with "image/"? I'd be surprised, and it seems these lines are redundant, due to the ASSERT at the start of ImageBuffer::toDataURL() ... http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp#L39 39 if (!mimeType.startsWith("image/")) 40 return "data:,";
noel gordon
Comment 8 2012-04-09 20:11:33 PDT
Oh, and is gdk_pixbuf_save_to_buffer() thread-safe? I don't see anything in the docs about that.
WebKit Review Bot
Comment 9 2012-04-10 11:29:28 PDT
Comment on attachment 136382 [details] Patch for landing Clearing flags on attachment: 136382 Committed r113742: <http://trac.webkit.org/changeset/113742>
WebKit Review Bot
Comment 10 2012-04-10 11:29:33 PDT
All reviewed patches have been landed. Closing bug.
noel gordon
Comment 11 2012-04-10 19:59:51 PDT
(In reply to comment #7) > Other questions: does the SupportedImageMIMETypeForEncoding registry for GTK contain items that don't begin with "image/"? I'd be surprised, and it seems these lines are redundant, due to the ASSERT at the start of ImageBuffer::toDataURL() ... > > http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp#L39 > > 39 if (!mimeType.startsWith("image/")) > 40 return "data:,"; Filed bug 83657 about that if you'd like to review.
Note You need to log in before you can comment on or make changes to this bug.