[webkit-reviews] review denied: [Bug 52595] Make basic printing work in WebKit2 : [Attachment 79214] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 52595: Make basic printing work in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=52595

Attachment 79214: proposed patch
https://bugs.webkit.org/attachment.cgi?id=79214&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79214&action=review

> Source/WebKit2/Shared/PrintInfo.cpp:42
> +bool PrintInfo::decode(CoreIPC::ArgumentDecoder* decoder, PrintInfo& t)
> +{
> +    return decoder->decode(CoreIPC::Out(t.pageSetupScaleFactor,
t.availablePaperWidth, t.availablePaperHeight));
> +}

t, eh?

I would have used the word “info”.

> Source/WebKit2/Shared/PrintInfo.h:46
> +#ifdef __OBJC__
> +    PrintInfo(NSPrintInfo *);
> +#endif

It’s possibly a bit dangerous to have no constructors at all when compiling in
a non-ObjC header file. If I remember correctly, not having any explicit
constructors causes some compiler-generated constructors to be created that are
not if there are any explicitly declared ones. But I could be remembering
wrong. To avoid this you could simply put class NSPrintInfo into the else case
of the #ifdef __OBJC___ above and allow this to be unconditional. That’s a
style we’ve used elsewhere in WebKit in the past.

Another approach would be to do the initialization in some other way without
using a constructor. Offer a helper function that makes a PrintInfo from an
NSPrintInfo that you call explicitly.

> Source/WebKit2/Shared/PrintInfo.h:50
> +    float pageSetupScaleFactor;
> +    float availablePaperWidth;
> +    float availablePaperHeight;

I think our usual unwritten style rule is that if we use public data members
without m_ prefixes, I use “struct”. Making this struct instead of class would
follow that convention.

> Source/WebKit2/Shared/mac/PrintInfoMac.mm:35
> +PrintInfo::PrintInfo(NSPrintInfo *printInfo)
> +    : pageSetupScaleFactor([[[printInfo dictionary]
objectForKey:NSPrintScalingFactor] floatValue])
> +    , availablePaperWidth([printInfo paperSize].width - [printInfo
leftMargin] - [printInfo rightMargin])
> +    , availablePaperHeight([printInfo paperSize].height - [printInfo
topMargin] - [printInfo bottomMargin])
> +{
> +}

Because floating point is involved, this will misbehave when printInfo is nil.

Because ObjC stays silent about such things, this will not crash when printInfo
is nil.

Accordingly, I suggest asserting it is not nil to avoid silent strange
behavior.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:115
> +    , m_inPrintingMode(false)

I like m_isInPrintingMode slightly better, although just below this there is
another boolean that does not follow the “proxy <xxx>” style.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2337
> +    if (!m_inPrintingMode) {

I like early return better.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2345
> +    if (m_inPrintingMode) {

I like early return better.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2357
> +#if PLATFORM(MAC)

Seems a shame to have this all inside #if. Can we compile more of this on
non-Mac just to keep things a bit more tidy? What’s the downside of doing that?


> Source/WebKit2/UIProcess/WebPageProxy.h:349
> +    void drawRectToPDF(WebFrameProxy*, const WebCore::IntRect&,
Vector<uint8_t>& pdfData);

Mitz told me that it’s OK to use a Vector as a return result in cases like this
and it won’t copy the data. Not sure he’s right though.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:169
> +NSString * const PrintedFrameKey = @"WebKitPrintedFrameKey";

Is this another case where const gives internal linkage without using the
static keyword explicitly? Or should you add static here?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1333
> +static void setFrameBeingPrinted(NSPrintOperation *printOperation,
WebFrameProxy* frame)
> +{
> +    [[[printOperation printInfo] dictionary] setObject:[WebFrameWrapper
webFrameWrapperWithFrame:frame] forKey:PrintedFrameKey];
> +}
> +
> +
> +static WebFrameProxy* frameBeingPrinted()
> +{
> +    return [[[[[NSPrintOperation currentOperation] printInfo] dictionary]
objectForKey:PrintedFrameKey] webFrame];
> +}
> +
> +
>  - (NSPrintOperation *)printOperationWithPrintInfo:(NSPrintInfo *)printInfo
forFrame:(WKFrameRef)frameRef

Extra blank lines here. One should be sufficient.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1358
> +    LOG(View, "knowsPageRange:\n");

I think the \n should be removed here. I guess this was a printf during your
development?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1363
> +    if (frame->isMainFrame() && _data->_pdfViewController)
> +	   return [super knowsPageRange:range];

Is the isMainFrame test needed here? Is it helpful?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1374
> +    if ([NSGraphicsContext currentContextDrawingToScreen]) {

I believe this will return NO for some non-printing cases, such as capturing
into a buffer using cacheDisplayInRect:toBitmapImageRep:. Ideally it would be
better to work in those cases.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1406
> +    CGPDFPageRef pdfPage = CGPDFDocumentGetPage(pdfDocument, 1);
> +    if (!pdfPage) {
> +	   LOG_ERROR("Printing data doesn't have page 1");
> +	   return;
> +    }

Returning here will leak the pdfDocument.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1429
> +// FIXME 3491344: This is an AppKit-internal method that we need to override
in order
> +// to get our shrink-to-fit to work with a custom pagination scheme. We can
do this better
> +// if AppKit makes it SPI/API.
> +- (CGFloat)_provideTotalScaleFactorForPrintOperation:(NSPrintOperation
*)printOperation 
> +{
> +    return _data->_totalScaleFactorForPrinting;
> +}
> +
> +

Another extra blank line here.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:369
> +    m_printContext.clear();

Anders wants you to write this:

    m_printContext = nullptr;

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1693
> +	   m_printContext = new PrintContext(coreFrame);

Please call adoptPtr here. An explicit call to adoptPtr is not required yet,
but eventually it will be.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1700
> +    m_printContext.clear();

Anders wants you to write this:

    m_printContext = nullptr;

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1719
> +    // If we're asked to print, we should actually print something.
> +    if (resultPageRects.isEmpty())
> +	   resultPageRects.append(IntRect(0, 0, 1, 1));

The comment is slightly oblique. I’d like a more-direct comment that states
that a 1x1 pixel rectangle will be a single blank page and that’s what we want.


> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1736
> +    CFMutableDataRef pdfPageData = CFDataCreateMutable(0, 0);

This leaks. I’d like to see smart pointers.

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:136
> +    DrawRectToPDF(uint64_t frameID, WebCore::IntRect rect) ->
(Vector<uint8_t> pdfData)

Will it be more efficient to do this with DataReference?