Bug 9214 – Images using QuickTime plugin do not display correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Carlos Augusto]

Already for a long time I have noticed that when browsing the USPTO.gov site for patents or patent applications, there are problems with displaying pages containing images (tiff files ?).

See the following example, in which the page with text only is correctly rendered:
<http://appft1.uspto.gov/netacgi/nph-Parser?Sect1=PTO2&Sect2=HITOFF&p=1&u=%2Fnetahtml%2FPTO%2Fsearch-bool.html&r=20&f=G&l=50&co1=AND&d=PG01&s1=Apple.AS.&OS=AN/Apple&RS=AN/Apple> 

Now, click on the blue button called "Images" at the top of the page, and on gets this:
<http://aiw1.uspto.gov:80/.aiw?Docid=20060033724&homeurl=http%3A%2F%2Fappft1.uspto.gov%2Fnetacgi%2Fnph-Parser%3FSect1%3DPTO2%2526Sect2%3DHITOFF%2526p%3D1%2526u%3D%25252Fnetahtml%25252FPTO%25252Fsearch-bool.html%2526r%3D20%2526f%3DG%2526l%3D50%2526co1%3DAND%2526d%3DPG01%2526s1%3DApple.AS.%2526OS%3DAN%2FApple%2526RS%3DAN%2FApple&PageNum=&Rtype=&SectionNum=&idkey=4124CCCC30F2>

I have tried with several browsers but they all seem to have the same problem, in which the image seems to be scaled up, and only a fraction of it can be seen.

Since the images are displayed using a QT plugin, I searched for a control panel in which I might be able to change the settings of the QT plugin, but did not find it.

Thank you.
Regards,
Carlos

[Dave Hyatt]

The bug here is that <embed>s are not making images like <object>s do.

[Mark Rowe (bdash)]

Based on Dave's comment, this is a bug.

[Richard Berg]

This bug was handled in Safari Version 2 using a plug in from accordex. Safari Version 3 broke the fix. See http://www.acordex.com/browseProd/VTplugin.html for an explanation.

This has been a long standing bug. Accordex fixed it with a plug in. Since there has been little interest at Apple to fix the underlying bug in QT, at least fix Safari 3 so that the plug in made accordex will function in Safari 3 (as it does in Safari 2) properly.

[Mark Rowe (bdash)]

Richard, you should file a separate bug report about that plugin failing to work correctly with Safari 3.  This bug report is about the original issue.

[Mark Rowe (bdash)]

<rdar://problem/5683413>

[Carlos Augusto]

It has been 2 years since I reported this problem.
Will the fix for this bug in QT have to wait for the new "QuickTime X" ?
In the meanwhile, will there be a fix allowing the Accordex plug-in to work properly ?

Thank you.
Carlos

[David Kilzer (:ddkilzer)]

Created attachment 21875 [details]
Patch v1

Proposed patch.

HTMLEmbedObject::isImageType() was copied directly from HTMLObjectElement.  Should I create a base class for HTMLEmbedElement and HTMLObjectElement, or move common methods to HTMLPlugInElement?    Note that HTMLAppletElement also inherits from HTMLPlugInElement, which wouldn't used some member variables or methods.

(The changes to HTMLObjectElement were to make extracting member variables and methods to a base class easier.)

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Created an attachment (id=21875) [edit]
> Patch v1
> 
> Proposed patch.
> 
> HTMLEmbedObject::isImageType() was copied directly from HTMLObjectElement. 
> Should I create a base class for HTMLEmbedElement and HTMLObjectElement, or
> move common methods to HTMLPlugInElement?    Note that HTMLAppletElement also
> inherits from HTMLPlugInElement, which wouldn't used some member variables or
> methods.
> 
> (The changes to HTMLObjectElement were to make extracting member variables and
> methods to a base class easier.)

- Not sure if code in WebCore/rendering/HitTestResult.cpp is needed.  (Note sure when the objectTag code is used.)

- Is there a better place for the data: URL parsing other than the isImageType() method(s) in HTMLEmbedElement and HTMLObjectElement classes?

[Mark Rowe (bdash)]

Dave, does this result in the QT plugin not being used at all in the linked URL, or am I misunderstanding the change?

[Darin Adler]

Comment on attachment 21875 [details]
Patch v1

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

[David Kilzer (:ddkilzer)]

Comment on attachment 21875 [details]
Patch v1

Clearing darin's r+ to revise patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Dave, does this result in the QT plugin not being used at all in the linked
> URL, or am I misunderstanding the change?

After applying the patch, the QT plugin would only be used when isImageType() returns false (as it would fall back to using a RenderPartObject in that case).

[David Kilzer (:ddkilzer)]

Cleanup that was originally part of Attachment #21875 [details] committed as r34895:

http://trac.webkit.org/changeset/34895

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> (From update of attachment 21875 [details] [edit])
> +        // 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.

Committed as r34929:

http://trac.webkit.org/changeset/34929

[David Kilzer (:ddkilzer)]

Created attachment 22060 [details]
Patch v2

(In reply to comment #10)
> (From update of attachment 21875 [details] [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.

[Darin Adler]

Comment on attachment 22060 [details]
Patch v2

 #if USE(JAVASCRIPTCORE_BINDINGS)
+#include "RenderWidget.h"
 #include "runtime.h"
 #endif

I'd prefer to add this include outside the #if, even if it's not required; my idea would be to minimize #include inside #if when possible.

r=me

[David Kilzer (:ddkilzer)]

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/images/embed-image.html
	A	LayoutTests/fast/images/object-image.html
	A	LayoutTests/platform/mac/fast/images/embed-image-expected.checksum
	A	LayoutTests/platform/mac/fast/images/embed-image-expected.png
	A	LayoutTests/platform/mac/fast/images/embed-image-expected.txt
	A	LayoutTests/platform/mac/fast/images/object-image-expected.checksum
	A	LayoutTests/platform/mac/fast/images/object-image-expected.png
	A	LayoutTests/platform/mac/fast/images/object-image-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/html/HTMLEmbedElement.cpp
	M	WebCore/html/HTMLEmbedElement.h
	M	WebCore/html/HTMLObjectElement.cpp
	M	WebCore/html/HTMLObjectElement.h
	A	WebCore/html/HTMLPlugInImageElement.cpp
	A	WebCore/html/HTMLPlugInImageElement.h
	M	WebCore/rendering/HitTestResult.cpp
Committed r35606

http://trac.webkit.org/changeset/35606

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> (From update of attachment 22060 [details] [edit])
>  #if USE(JAVASCRIPTCORE_BINDINGS)
> +#include "RenderWidget.h"
>  #include "runtime.h"
>  #endif
> 
> I'd prefer to add this include outside the #if, even if it's not required; my
> idea would be to minimize #include inside #if when possible.

I made this change before landing.

[David Kilzer (:ddkilzer)]

Windows build fix:

http://trac.webkit.org/changeset/35609

Other platform build fixes:

http://trac.webkit.org/changeset/35610

[Carlos Augusto]

This problem has reappeared on Safari 4.0.3 on OS X 10.6.1.
Maybe it reappeared earlier and I did not notice it.
See for example the following page:
<http://patft.uspto.gov/netacgi/nph-Parser?Sect1=PTO2&Sect2=HITOFF&p=1&u=%2Fnetahtml%2FPTO%2Fsearch-bool.html&r=7&f=G&l=50&co1=AND&d=PTXT&s1=Apple.ASNM.&OS=AN/Apple&RS=AN/Apple>

When you click on the link at the top to see the images, they do not show up in the respective page(s).

Carlos

[David Kilzer (:ddkilzer)]

(In reply to comment #20)
> This problem has reappeared on Safari 4.0.3 on OS X 10.6.1.
> Maybe it reappeared earlier and I did not notice it.
> See for example the following page:
> <http://patft.uspto.gov/netacgi/nph-Parser?Sect1=PTO2&Sect2=HITOFF&p=1&u=%2Fnetahtml%2FPTO%2Fsearch-bool.html&r=7&f=G&l=50&co1=AND&d=PTXT&s1=Apple.ASNM.&OS=AN/Apple&RS=AN/Apple>
> 
> When you click on the link at the top to see the images, they do not show up in
> the respective page(s).

Let's file a new bug for the issue:  Bug 30181.

[Alexey Proskuryakov]

We'd need a new bug to investigate this - a comment in an already closed bug can easily go unnoticed. 

But in this case, it is known that this is an issue in an underlying Apple library, which is tracked internally as <rdar://problem/7132190>. We don't need to track it as a WebKit bug.

[Carlos Augusto]

Perhaps this is no longer the right place, to call attention to this problem, but the fact of the matter is that this bug, which is not a webkit bug, is still not solved with OS X 10.8 (Safari 6.0).

Needless to say that it is extremely disappointing to realize that 6 years have not been enough to fix this bug in the underlying Apple library, which is tracked internally as <rdar://problem/7132190>.

Although this is not a webkit bug, a workaround has not been provided, and consequently the problem is still UnResolved/UnFixed.

Carlos