Bug 13344 - pdf file is getting opened in a new window using adobe plugin
Summary: pdf file is getting opened in a new window using adobe plugin
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-04-12 15:15 PDT by Madhu M
Modified: 2010-06-10 15:11 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Madhu M 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.
Comment 1 Madhu M 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.
Comment 2 Mark Rowe (bdash) 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.
Comment 3 Madhu M 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
Comment 4 Darin Adler 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.
Comment 5 Madhu M 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
Comment 6 Darin Adler 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?
Comment 7 Dave Hyatt 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?
Comment 8 Madhu M 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 





Comment 9 Madhu M 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 ?
Comment 10 Darin Adler 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?
Comment 11 Darin Adler 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?
Comment 12 Maciej Stachowiak 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.
Comment 13 Maciej Stachowiak 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
Comment 14 Eric Seidel (no email) 2007-10-01 09:52:28 PDT
I think we managed to scare off our poor contributer. :(