[webkit-reviews] review denied: [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]

mitz at webkit.org has denied  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 mitz at webkit.org
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!