[webkit-reviews] review granted: [Bug 75232] REGRESSION (WebKit2): Printing a subframe containing a PDF prints the on-screen view instead of the entire PDF document : [Attachment 120561] Proposed fix

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted mitz at webkit.org's request for
review:
Bug 75232: REGRESSION (WebKit2): Printing a subframe containing a PDF prints
the on-screen view instead of the entire PDF document
https://bugs.webkit.org/show_bug.cgi?id=75232

Attachment 120561: Proposed fix
https://bugs.webkit.org/attachment.cgi?id=120561&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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).