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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 23 10:16:53 PDT 2008

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

Attachment 21875: Patch v1

------- 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

+    else if (m_innerNonSharedNode->hasTagName(embedTag))
+	 urlString =

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

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

More information about the webkit-reviews mailing list