[Webkit-unassigned] [Bug 9214] Images using QT plugin do not display correctly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=9214


ddkilzer at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #21875|0                           |1
        is obsolete|                            |
  Attachment #22060|                            |review?
               Flag|                            |




------- Comment #15 from ddkilzer at webkit.org  2008-07-02 22:25 PDT -------
Created an attachment (id=22060)
 --> (https://bugs.webkit.org/attachment.cgi?id=22060&action=view)
Patch v2

(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 though
> 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 line
> would read better.

Fixed.

> +        // 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.

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.

Fixed.

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

Fixed.

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


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.