[webkit-reviews] review granted: [Bug 101382] [WK2] Print preview should vend images to the UIProcess instead of PDFs : [Attachment 172874] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 101382: [WK2] Print preview should vend images to the UIProcess instead of
PDFs
https://bugs.webkit.org/show_bug.cgi?id=101382

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172874&action=review


Looks great!

You might want to additionally run this by Sam or Anders.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:218
> +    // If the user has already changed print setup, then this response is
obsolete. And this callback is not in response to the latest request,

I think that there is something wrong with grammar of this moved comment.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:474
> +    HashMap<WebCore::IntRect, RefPtr<WebImage> >::iterator
pagePreviewIterator = _pagePreviews.find(rect);

WebImage looks like an API type that shouldn't be used here.

> Source/WebKit2/UIProcess/API/mac/WKPrintingView.mm:517
> +    const IntRect& imageBounds = imageData->bitmap()->bounds();

Does this const reference buy us any performance over a plain IntRect (not that
this code is performance critical anyway)?

> Source/WebKit2/UIProcess/GenericCallback.h:237
> +    : CallbackBase(context)
> +    , m_callback(callback)

Indentation.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3198
> +	       drawRectToPDFFromPDFDocument(graphicsContext->platformContext(),
pdfDocument.get(), printInfo, rect);

Please rename this function.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3203
> +	       image = scaledSnapshotWithOptions(rect, 1.0,
SnapshotOptionsShareable | SnapshotOptionsExcludeSelectionHighlighting);

Style nit: 1, not 1.0.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3209
> +    if (!image)
> +	   return;

UI process needs its response back!

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3212
> +    image->bitmap()->createHandle(handle);

This creates a read-write handle. I wonder why.