[webkit-reviews] review granted: [Bug 9214] Images using QT plugin do not display correctly : [Attachment 21875] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 9214: Images using QT plugin do not display correctly
https://bugs.webkit.org/show_bug.cgi?id=9214

Attachment 21875: Patch v1
https://bugs.webkit.org/attachment.cgi?id=21875&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Does this prevent images from ever using the QuickTime plug-in? Are there any
cases where we'd want to let the QuickTime plug-in handle an image, even though
we recognize it's an image type? I guess not, just wondering.

I'm a little concerned that isImageType does a non-trivial amount of work and
we're calling it in multiple places. Should we be caching the boolean result?

+	     RenderImage* imageObj = static_cast<RenderImage*>(renderer());
+	     imageObj->setCachedImage(m_imageLoader->image());

I'm not too fond of that local variable name. Maybe doing this all on one line
would read better.

+	 // Extract the MIME type from the data URL.

I'd like to see a function to extract a MIME type from a data URL put in a file
like KURL.h, rather than having the inline code here.

+	 KURL completedURL(frame->loader()->completeURL(m_url));

I think this would read better with an "=" rather than using construction
syntax.

+    else if (m_innerNonSharedNode->hasTagName(embedTag))
+	 urlString =
static_cast<Element*>(m_innerNonSharedNode.get())->getAttribute(srcAttr);

Can this code be changed to use the imageSourceAttributeName function for
better factoring?

r=me, as-is but please consider my suggestions too