[webkit-reviews] review requested: [Bug 9214] Images using QT plugin do not display correctly : [Attachment 22060] Patch v2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 2 22:25:11 PDT 2008

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has asked  for review:
Bug 9214: Images using QT plugin do not display correctly

Attachment 22060: Patch v2

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
(In reply to comment #10)
> (From update of attachment 21875 [edit])
> 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
> we recognize it's an image type? I guess not, just wondering.

The QuickTime plug-in will still be used when isImageType() returns false,
since we fall back to using a RenderPartObject in that case.

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

This feels like premature optimization, and I'd prefer not to add two more
state variables (one to track when a cached value is invalidated by the change
of an attribute, as well the cached value itself) and complicate the class
unless it's proven to be needed.

> +	       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
> 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
> like KURL.h, rather than having the inline code here.

This was fixed in an earlier commit:  r34929 (see Comment #14).

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


New to this patch is HTMLPlugInImageElement, which is a new class that
HTMLObjectElement and HTMLEmbedElement extend in order to share code and member

More information about the webkit-reviews mailing list