Bug 13344

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:

Version:

523.x (Safari 3)

Hardware:

Mac

OS:

OS X 10.4

Attachments:

Description	Flags
Patch for this issue and issue # 10015 regarding adobe plugin	none
New patch for this issue and issue #10015 regarding plugins	none
New patch for this issue and issue #10015 regarding plugins	hyatt: review-
New patch for this issue and issue #10015 regarding plugins	mjs: review-

Madhu M

Reported 2007-04-12 15:15:47 PDT

In OBJECT tag if the type is given as application/pdf the pdf file is not getting opened using adobe plug-in.(http://bugs.webkit.org/show_bug.cgi?id=10015). If the type is having some other value like "Application/PDF" or some invalid value, the pdf is getting opened in a new browser window using adobe plug-in.It is working fine if type attribute is not given in the object tag.But a wrong value for type will cause the pdf file to open in a new window.

Attachments
Patch for this issue and issue # 10015 regarding adobe plugin (84.45 KB, patch) 2007-04-12 17:57 PDT, Madhu M	no flags	Details Formatted Diff Diff
New patch for this issue and issue #10015 regarding plugins (110.56 KB, patch) 2007-04-13 16:39 PDT, Madhu M	no flags	Details Formatted Diff Diff
New patch for this issue and issue #10015 regarding plugins (110.20 KB, patch) 2007-04-16 15:05 PDT, Madhu M	hyatt: review-	Details Formatted Diff Diff
New patch for this issue and issue #10015 regarding plugins (111.47 KB, patch) 2007-05-07 17:54 PDT, Madhu M	mjs: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Madhu M

Comment 1 2007-04-12 17:57:19 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.

Mark Rowe (bdash)

Comment 2 2007-04-12 21:16:29 PDT

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.

Madhu M

Comment 3 2007-04-13 16:39:33 PDT

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

Darin Adler

Comment 4 2007-04-13 21:05:54 PDT

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.

Madhu M

Comment 5 2007-04-16 15:05:11 PDT

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

Darin Adler

Comment 6 2007-04-16 21:48:03 PDT

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?

Dave Hyatt

Comment 7 2007-04-18 14:08:29 PDT

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?

Madhu M

Comment 8 2007-04-24 11:48:41 PDT

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

Madhu M

Comment 9 2007-05-07 17:54:23 PDT

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 ?

Darin Adler

Comment 10 2007-05-11 09:04:36 PDT

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?

Darin Adler

Comment 11 2007-05-11 09:06:10 PDT

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

Maciej Stachowiak

Comment 12 2007-05-17 17:36:37 PDT

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

Maciej Stachowiak

Comment 13 2007-05-29 01:48:53 PDT

Comment on attachment 14407 [details] New patch for this issue and issue #10015 regarding plugins r- for me and darin's comments

Eric Seidel (no email)

Comment 14 2007-10-01 09:52:28 PDT

I think we managed to scare off our poor contributer. :(

Alexey Proskuryakov

Comment 15 2022-07-01 11:35:28 PDT

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.

Note You need to log in before you can comment on or make changes to this bug.