Created attachment 159922 [details] Patch

Comment on attachment 159922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159922&action=review > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:137 > + // we now know that we have a valid <img> element as the argument, we can attach the image to it. uppercase? > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:155 > + copyPixels(image, width, height, imageData->data()->data()); copyPixelsInto ? > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:171 > + JSStringRelease(str); > + return value; I still prefer a newline before return > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:178 > + QString stringValue = QString::fromLatin1("[Qt Native Pixmap %1,%2]").arg(size.width()).arg(size.height()); [Qt Native Pixmap ? why is that a good description? toString of an images normally doesnt give size does it? > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:207 > + if (hint == qMetaTypeId<QPixmap>()) > + return QVariant::fromValue<QPixmap>(toPixmap(*originalVariant)); > > - if (!imageElement) > - goto returnEmptyVariant; > + if (hint == qMetaTypeId<QImage>()) > + return QVariant::fromValue<QImage>(toImage(*originalVariant)); You had the opposite conversion in a separate method earlier

Comment on attachment 159922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159922&action=review > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:109 > +static JSValueRef getPixmapWidth(JSContextRef context, JSObjectRef object, JSStringRef propertyName, JSValueRef* exception) No need for "propertyName" and "exception". > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:143 > + toJS(exec, global, imageElement->document()); Ignoring the return value of the toJS() call seems weird here. Does this have a wanted side-effect? Or maybe this is just a typo / leftover from the past? > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:196 > + JSObject* jsObject; > + JSHTMLImageElement* elementJSWrapper; > + HTMLImageElement* imageElement; > + CachedImage* cachedImage; > + Image* image; > + QImage* nativeImage; Why declare them here instead of in first use definition? > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:199 > goto returnEmptyVariant; Just an idea: what about "return emptyVariantForHint(hint)" and the code after "returnEmptyVariant" label becomes a function?

Comment on attachment 159922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159922&action=review > Source/WebCore/ChangeLog:11 > + The conversion uses a simple JSClassRef based binding and only a few > + uses of private JSC API for the HTML element DOM bindings remain. I'll try to be a bit more elaborate here in the next patch. >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:109 >> +static JSValueRef getPixmapWidth(JSContextRef context, JSObjectRef object, JSStringRef propertyName, JSValueRef* exception) > > No need for "propertyName" and "exception". The parameters are required as part of the JSObjectGetPropertyCallback signature, but I've removed their names now from both getters. >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:137 >> + // we now know that we have a valid <img> element as the argument, we can attach the image to it. > > uppercase? Done >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:143 >> + toJS(exec, global, imageElement->document()); > > Ignoring the return value of the toJS() call seems weird here. Does this have a wanted side-effect? Or maybe this is just a typo / leftover from the past? That does seem rather bogus. I'll remove it :) >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:155 >> + copyPixels(image, width, height, imageData->data()->data()); > > copyPixelsInto ? Good idea :) Done. >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:171 >> + return value; > > I still prefer a newline before return Done. And also used a JSRetainPtr for the string (before Caio sees it ;) >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:178 >> + QString stringValue = QString::fromLatin1("[Qt Native Pixmap %1,%2]").arg(size.width()).arg(size.height()); > > [Qt Native Pixmap ? why is that a good description? toString of an images normally doesnt give size does it? This is almost exclusively used for debug output, so I think it's a convenient output format for debugging. >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:196 >> + QImage* nativeImage; > > Why declare them here instead of in first use definition? Well spotted! >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:199 >> goto returnEmptyVariant; > > Just an idea: what about "return emptyVariantForHint(hint)" and the code after "returnEmptyVariant" label becomes a function? Hmm. Tempting. I'll give it a shot. With tail call optimization it might not be so bad, but I kind of liked the goto :) >> Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:207 >> + return QVariant::fromValue<QImage>(toImage(*originalVariant)); > > You had the opposite conversion in a separate method earlier I don't quite understand :). What do you mean?

Comment on attachment 159922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159922&action=review > Source/WebCore/bridge/qt/qt_pixmapruntime.cpp:186 > + return JSObjectMake(context, getClassRef(), new QVariant(value)); As it turns out, this is also a leak, because we're missing a finalize callback that deletes the variant.

Created attachment 161183 [details] Patch

LGTM.

Comment on attachment 161183 [details] Patch to me too

Committed r127440: <http://trac.webkit.org/changeset/127440>