| Summary: | pdf file is getting opened in a new window using adobe plugin | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Madhu M <madhu.mukund> |
| Component: | Plug-ins | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED WONTFIX | ||
| Severity: | Normal | CC: | ap, darin, mrowe |
| Priority: | P2 | ||
| Version: | 523.x (Safari 3) | ||
| Hardware: | Mac | ||
| OS: | OS X 10.4 | ||
| Attachments: | |||
|
Description
Madhu M
2007-04-12 15:15:47 PDT
Created attachment 14023 [details] Patch for this issue and issue # 10015 regarding adobe plugin This patch contains fix for this issue and issue #10015. http://bugs.webkit.org/show_bug.cgi?id=10015 . #10015 - pdf file is not opening in adobe plug in if the type is given as application/pdf. Can somebody please review these issues and the patch submitted. These issues are very critical for us. Ouch. It seems SVN isn't smart enough to know that your PDF is a binary file so it's included the raw contents in the patch rather than wrapping it nicely in base64. You'll need to do 'svn propset svn:mime-type application/octet-stream path/to/file.pdf' then recreate the patch before we can land the change. Created attachment 14032 [details]
New patch for this issue and issue #10015 regarding plugins
Thanks Mark Rowe. I have made the changes as suggested by you and submitting the patch again. Could you please review this and give comments. The layout tests show 100% success after applying my changes.
Regards
Madhu M
Comment on attachment 14032 [details] New patch for this issue and issue #10015 regarding plugins Here are some nitpicks: Need spaces after commas in function calls. > </HTML> > \ No newline at end of file Should have a newline at the end of the file. > +<HTML> > <HEAD> > <H3> The PDF file should be opened in the same window of the browser using adobe plug-in</H3> > <OBJECT id="PDFObject" height="500" width="100%" type="Application/pdf" data="testqueue.pdf"></OBJECT> > </HEAD> > </HTML> There are no pluses, which makes it look like this entire file is one big line -- I think there's a CR/LF problem here. +2007-04-13 Madhu <madhu.mukund@wipro.com> + + Reviewed by NOBODY (OOPS!). + + * plugins/simple_pdf_test-expected.txt: Added. Change log entry doesn't say what this test is about. Look at other LayoutTests/ChangeLog entries to see what we usually do. > + if (!equalIgnoringCase(name,"type")) { > + if (embed || !uniqueParamNames.contains(name.impl())) { I think this change needs a comment. It's entirely unclear why the name "type" is a special case. The change log does explain things but a comment in the code is warranted in this case. The change log makes it sound like this change is "for the Adobe plug-in", but it's not really specific to any Adobe requirement. It would be better to word it in a way that makes it more clear. Created attachment 14054 [details]
New patch for this issue and issue #10015 regarding plugins
Thanks Darin.
I have incorporated your comments and submitting the patch again.
Could you please review the same and give the comments ?
Regards
Madhu M
I'm not sure I understand why the changes in the patch are correct. Should we really be leaving out the "type" parameter? Do other browsers leave it out? Comment on attachment 14054 [details]
New patch for this issue and issue #10015 regarding plugins
I don't understand the RenderPartObject patch. Why would the type be ignored?
The value of 'type' attribute is validated and it is set to the 'serviceType' parameter while making the request for the object. Please refer the following function call. bool success = frame->loader()->requestObject(this, url, AtomicString(o->name()), serviceType, paramNames, paramValues). If the type attribute is having wrong value, it is getting validated and the correct value is being set to 'serviceType' from the data attribute. So the parameter serviceType is ensured to have a correct value. But after this, if the 'paramNames' include type and a wrong value in 'paramValues', the plugin is getting opened in a new window. The serviceType seems to be getting overridden by this value. To solve this there are two ways. 1. Remove the type from the paramNames and its value from the paramValues, once type is set to 'serviceType'. (I have followed this in my patch). 2. Set the correct value of type in 'paramValues' from 'serviceType' So, if it is not OK to follow (1), can I go ahead with (2) and submit the patch. Regards Madhu Created attachment 14407 [details]
New patch for this issue and issue #10015 regarding plugins
I am submitting a new patch for this issue. In this I am following the (2) method in my previous comment. Please see the details below
If the 'type' attribute is having wrong value, it is getting validated and the
correct value is being set to 'serviceType' from the data attribute. So the
parameter serviceType is ensured to have a correct value. But after this, if
the 'paramNames' include type and a wrong value in 'paramValues', the plugin is
getting opened in a new window. The serviceType seems to be getting overridden
by this value.
To solve this there are two ways.
1. Remove the type from the paramNames and its value from the paramValues, once
type is set to 'serviceType'. (I have followed this in my previous patch).
2. Set the correct value of type in 'paramValues' from 'serviceType'.(This method is followed in this patch).
Could you please review this and give comments ?
Comment on attachment 14407 [details] New patch for this issue and issue #10015 regarding plugins + // http://bugs.webkit.org/show_bug.cgi?id=10015 + // If the type attribute is given as application/pdf in OBJECT tag, + // it should use the Adobe plugin to open the pdf file instead of opening it as an image. + if (!m_serviceType.isEmpty() && equalIgnoringCase(m_serviceType, "application/pdf")) + return false; + Given the comment, the code seems to assume that there's always a plug-in installed. Is that right? Have you tested what impact this change has when there is not a plug-in installed? + if (equalIgnoringCase(name, "type") && !serviceType.isEmpty() ) There's an extra space before the trailing parenthesis. Do these new layout tests require that people install the Adobe plug-in? Or do they work without the plug-in? (In reply to comment #9) > If the 'type' attribute is having wrong value, it is getting validated and the > correct value is being set to 'serviceType' from the data attribute. So the > parameter serviceType is ensured to have a correct value. But after this, if > the 'paramNames' include type and a wrong value in 'paramValues', the plugin is > getting opened in a new window. The serviceType seems to be getting overridden > by this value. > > To solve this there are two ways. > > 1. Remove the type from the paramNames and its value from the paramValues, once > type is set to 'serviceType'. (I have followed this in my previous patch). > > 2. Set the correct value of type in 'paramValues' from 'serviceType'.(This > method is followed in this patch). What do other browsers do in this circumstance? + if (!m_serviceType.isEmpty() && equalIgnoringCase(m_serviceType, "application/pdf")) I don't think HTMLObjectElement is the right place to have a special case for PDF - it should be in the - (ObjectElementType)determineObjectFromMIMEType:(NSString*)MIMEType URL:(NSURL*)URL method on WebFrameBridge, since that is what does the determination of whether to treat something as an image, plugin or frame. We also want to make sure our normal inline PDF viewing works right when the Adobe Acrobat plugin is not installed. Comment on attachment 14407 [details]
New patch for this issue and issue #10015 regarding plugins
r- for me and darin's comments
I think we managed to scare off our poor contributer. :( Mass closing plug-in bugs, as plug-in support has been removed from WebKit. Please comment and/or reopen if this still affects WebKit in some way. |