Bug 75232 - REGRESSION (WebKit2): Printing a subframe containing a PDF prints the on-screen view instead of the entire PDF document
Summary: REGRESSION (WebKit2): Printing a subframe containing a PDF prints the on-scre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-12-26 17:33 PST by mitz
Modified: 2012-01-02 12:10 PST (History)
3 users (show)

See Also:


Attachments
Proposed fix (34.73 KB, patch)
2011-12-26 17:36 PST, mitz
mitz: review-
Details | Formatted Diff | Diff
Proposed fix (30.65 KB, patch)
2011-12-29 12:22 PST, mitz
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-12-26 17:33:19 PST
<rdar://problem/8750356>
Comment 1 mitz 2011-12-26 17:36:33 PST
Created attachment 120561 [details]
Proposed fix
Comment 2 WebKit Review Bot 2011-12-26 17:41:19 PST
Attachment 120561 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2651:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexey Proskuryakov 2011-12-26 19:03:42 PST
Comment on attachment 120561 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=120561&action=review

> Source/WebKit2/Shared/PrintInfo.h:29
> +#include <WebCore/FloatRect.h>

You could include FloatSize.h.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2734
> +        // FIXME: If the frame cannot be printed (e.g. if it contains an encrypted PDF that disallows
> +        // printing), this function should return nil.

Is this something you plan to address very soon? Otherwise, a bug number would be helpful.

What's the symptom here - will a single blank page be printed for non-printable documents?

> Source/WebKit2/WebProcess/Plugins/PluginView.h:72
> +    RetainPtr<CGPDFDocumentRef> pdfDocumentForPrinting() const { return m_plugin->pdfDocumentForPrinting(); }

I do not understand why we have to talk to WebCore here. Is this for <embed type="application/pdf">? But these end up in BuiltinPDFViews, too.

Eventually, there will be no CGPDFDocument parsing in WebProcess, so adding such plumbing across framework boundary is against the long term plan.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2658
> +    CGRect cropBox = CGPDFPageGetBoxRect(page, kCGPDFCropBox);
> +    cropBox = CGRectIntersection(cropBox, CGPDFPageGetBoxRect(page, kCGPDFMediaBox));

This is suspicious - many PDFs don't have a crop box. See e.g. code in BuiltInPDFView::calculateSizes.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2668
> +    if (widthDifference || heightDifference)
> +        CGContextTranslateCTM(context, widthDifference / 2, heightDifference / 2);

At least on screen, PDFs really dislike non-integer translation (see drawing code in BuiltinPDFView).
Comment 4 Alexey Proskuryakov 2011-12-26 19:04:40 PST
This patch adds some Windows build trouble.
Comment 5 mitz 2011-12-26 19:14:16 PST
Comment on attachment 120561 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=120561&action=review

Thanks for the review! I am going to work on this some more.

>> Source/WebKit2/Shared/PrintInfo.h:29
>> +#include <WebCore/FloatRect.h>
> 
> You could include FloatSize.h.

Indeed. At some point I was using a FloatRect here (which is really why I made the conversion).

>> Source/WebKit2/UIProcess/API/mac/WKView.mm:2734
>> +        // printing), this function should return nil.
> 
> Is this something you plan to address very soon? Otherwise, a bug number would be helpful.
> 
> What's the symptom here - will a single blank page be printed for non-printable documents?

I am not planning to address this immediately, since the UI process doesn’t know whether the frame contents are printable, and the symptom is quite benign (an single empty page). For what it’s worth, I think it’s strange that non-printable PDFs have been signaled only through a nil NSPrintOperation and not by disabling the print command.

>> Source/WebKit2/WebProcess/Plugins/PluginView.h:72
>> +    RetainPtr<CGPDFDocumentRef> pdfDocumentForPrinting() const { return m_plugin->pdfDocumentForPrinting(); }
> 
> I do not understand why we have to talk to WebCore here. Is this for <embed type="application/pdf">? But these end up in BuiltinPDFViews, too.
> 
> Eventually, there will be no CGPDFDocument parsing in WebProcess, so adding such plumbing across framework boundary is against the long term plan.

m_plugin is a WebKit::Plugin, not a WebCore object.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2658
>> +    cropBox = CGRectIntersection(cropBox, CGPDFPageGetBoxRect(page, kCGPDFMediaBox));
> 
> This is suspicious - many PDFs don't have a crop box. See e.g. code in BuiltInPDFView::calculateSizes.

Interesting. I was trying to follow what PDFView does but I didn’t test thoroughly for compatibility.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2668
>> +        CGContextTranslateCTM(context, widthDifference / 2, heightDifference / 2);
> 
> At least on screen, PDFs really dislike non-integer translation (see drawing code in BuiltinPDFView).

Also interesting!
Comment 6 Alexey Proskuryakov 2011-12-26 22:50:26 PST
> m_plugin is a WebKit::Plugin, not a WebCore object.

I probably added the comment to a wrong place then. There is some new interaction with WebCore that I don't understand, as you needed to export a function from WebCore.

> Also interesting!

That was tracked as bug 70072.
Comment 7 mitz 2011-12-26 23:05:47 PST
(In reply to comment #6)
> > m_plugin is a WebKit::Plugin, not a WebCore object.
> 
> I probably added the comment to a wrong place then. There is some new interaction with WebCore that I don't understand, as you needed to export a function from WebCore.

I am using PluginDocument::pluginWidget. I am not sure how else to get from a WebFrame to its (possible) BuiltInPDFView.
Comment 8 Alexey Proskuryakov 2011-12-26 23:20:55 PST
Indeed.
Comment 9 mitz 2011-12-29 12:22:27 PST
Created attachment 120764 [details]
Proposed fix

- Still haven’t looked into the Windows build issue
- Omitted the PrintInfo change which wasn’t closely related
- Added rounding when centering
- Added an empty-check in isDisplayingPDFDocument()
- Added an empty-crop-box check
Comment 10 mitz 2011-12-29 12:23:51 PST
- Forgot to fix style error
Comment 11 WebKit Review Bot 2011-12-29 12:24:57 PST
Attachment 120764 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2658:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 mitz 2011-12-31 12:31:04 PST
Fixed in <http://trac.webkit.org/r103858>.

The Windows build has been broken for a while, so whether this broke it further I don’t know.
Comment 13 Alexey Proskuryakov 2011-12-31 12:58:55 PST
I clicked on the red bubble for the first patch, and it was clear that it broke the build further.
Comment 14 Chris Petersen 2012-01-02 11:21:50 PST
I would like to do some testing in this area but noticed a couple of things. With Webkit NB 103874, a inline PDF that I have previously loaded doesn't seem to load if page has been cached.  Known issue already ?
Comment 15 Alexey Proskuryakov 2012-01-02 12:10:37 PST
I haven't seen that reported yet.