Bug 52595 – Make basic printing work in WebKit2

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

[Alexey Proskuryakov]

Still rather early in development, but it works.

[Alexey Proskuryakov]

Created attachment 79214 [details]
proposed patch

[WebKit Review Bot]

Attachment 79214 [details] did not pass style-queue:

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

Source/WebKit2/Shared/PrintInfo.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 79214 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7513165

[Build Bot]

Attachment 79214 [details] did not build on win:
Build output: http://queues.webkit.org/results/7585154

[Darin Adler]

Comment on attachment 79214 [details]
proposed patch

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?

[Anders Carlsson]

Here are my comments:


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

> Source/WebKit2/ChangeLog:25
> +        (WebFrameWrapper): Added a wrapper class for adding a WebFrameFroxy to an NSDictionary.

Typo, Froxy.

> Source/WebKit2/ChangeLog:53
> +        (WebKit::WebPage::updatePreferences): PAss through ShouldPrintBackgrounds.

PAss.

> Source/WebKit2/Shared/PrintInfo.h:32
> +@class NSPrintInfo;

Add 

#else
class NSPrintInfo;

here.

> Source/WebKit2/Shared/PrintInfo.h:44
> +#ifdef __OBJC__

This should be PLATFORM(MAC).

> Source/WebKit2/Shared/PrintInfo.h:45
> +    PrintInfo(NSPrintInfo *);

This constructor should be explicit.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2354
> +    process()->sendSync(Messages::WebPage::ComputePagesForPrinting(frame->frameID(), printInfo), Messages::WebPage::ComputePagesForPrinting::Reply(resultPageRects, resultTotalScaleFactorForPrinting), m_pageID, INT_MAX);

You don't need the INT_MAX here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2361
> +    process()->sendSync(Messages::WebPage::DrawRectToPDF(frame->frameID(), rect), Messages::WebPage::DrawRectToPDF::Reply(pdfData), m_pageID, INT_MAX);

Or here.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:149
> ++ (WebFrameWrapper*)webFrameWrapperWithFrame:(WebFrameProxy*)frame;

This should be initWithFrame to avoid an autorelease.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1394
> +    CGDataProviderRef pdfDataProvider = CGDataProviderCreateWithData(0, pdfData.data(), pdfData.size(), 0);

This can use RetainPtr.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1395
> +    CGPDFDocumentRef pdfDocument = CGPDFDocumentCreateWithProvider(pdfDataProvider);

I think this can use RetainPtr too.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1703
> +void WebPage::computePagesForPrinting(uint64_t frameID, const PrintInfo& printInfo, Vector<WebCore::IntRect>& resultPageRects, double& resultTotalScaleFactorForPrinting)

You can remove WebCore:: here.

[Alexey Proskuryakov]

Created attachment 79225 [details]
proposed patch

>> +    return decoder->decode(CoreIPC::Out(t.pageSetupScaleFactor, t.availablePaperWidth, t.availablePaperHeight));
> > +}
>
> t, eh?

There's a lot of precedent for "t" (all WebEvent code, for example). But I can't see any reason for that, so changed to info.

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

I didn't think that it could be done here, because autogenerated code was making "class" forward declarations. But turns out that there is an override in code generation script.

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

I'd like to think about how the data should be passed on other platforms later. It doesn't seem great to have several seemingly equal drawRectToXXX() methods, of which only one will work for any given platform.

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

I think that it's easier to use an out argument than to worry whether return value optimization will work in each particular case.

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

Yes.

> +    if (frame->isMainFrame() && _data->_pdfViewController)
> +        return [super knowsPageRange:range];
>
> Is the isMainFrame test needed here? Is it helpful?

I'm not entirely sure. Obviously, PDF views don't have subframes, but I wanted to be explicit about what case is being handled here.

> > +    if ([NSGraphicsContext currentContextDrawingToScreen]) {
>
> I believe this will return NO for some non-printing cases

Ugh. I've added a FIXME for now.

> > +    DrawRectToPDF(uint64_t frameID, WebCore::IntRect rect) -> (Vector<uint8_t> pdfData)
> 
> Will it be more efficient to do this with DataReference?

I tried that at first, but DataReference doesn't take ownership of the data, so it can't be used for "out" arguments. Having MallocScribble always on in Xcode debugger saved me this time.

[WebKit Review Bot]

Attachment 79225 [details] did not pass style-queue:

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

Source/WebKit2/Shared/PrintInfo.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Created attachment 79226 [details]
proposed patch

Oh, also adding PrintInfo to other build systems to fix build...

[WebKit Review Bot]

Attachment 79226 [details] did not pass style-queue:

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

Source/WebKit2/Shared/PrintInfo.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 79225 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7528150

[Anders Carlsson]

Comment on attachment 79226 [details]
proposed patch

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

> Source/WebKit2/Shared/PrintInfo.h:45
> +public:

No need for public here.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/75979>.