Bug 75232 – REGRESSION (WebKit2): Printing a subframe containing a PDF prints the on-screen view instead of the entire PDF document

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

<rdar://problem/8750356>

Created attachment 120561 [details]
Proposed fix

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

This patch adds some Windows build trouble.

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!

[Alexey Proskuryakov]

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

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

[Alexey Proskuryakov]

Indeed.

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

- Forgot to fix style error

[WebKit Review Bot]

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.

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.

[Alexey Proskuryakov]

I clicked on the red bubble for the first patch, and it was clear that it broke the build further.

[Chris Petersen]

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 ?

[Alexey Proskuryakov]

I haven't seen that reported yet.