Summary: | Images using QuickTime plugin do not display correctly | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Augusto <caugusto> | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, ddkilzer, ian, mrowe, rberg | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 417.x | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
URL: | 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 | ||||||||
Attachments: |
|
Description
Carlos Augusto
2006-06-01 12:46:32 PDT
The bug here is that <embed>s are not making images like <object>s do. Based on Dave's comment, this is a bug. 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. 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. 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 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.)
(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? Dave, does this result in the QT plugin not being used at all in the linked URL, or am I misunderstanding the change? 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
Comment on attachment 21875 [details]
Patch v1
Clearing darin's r+ to revise patch.
(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). Cleanup that was originally part of Attachment #21875 [details] committed as r34895: http://trac.webkit.org/changeset/34895 (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 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. 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
$ 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 (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. Windows build fix: http://trac.webkit.org/changeset/35609 Other platform build fixes: http://trac.webkit.org/changeset/35610 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 (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. 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. 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 |