[webkit-reviews] review granted: [Bug 55800] WK2 needs printing support on Windows : [Attachment 84830] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jon Honeycutt
<jhoneycutt at apple.com>'s request for review:
Bug 55800: WK2 needs printing support on Windows
https://bugs.webkit.org/show_bug.cgi?id=55800

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

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

> Source/WebKit2/UIProcess/API/C/WKPage.cpp:506
> +    ComputedPagesContext(WKPageComputePagesForPrintingFunction callback,
void* context)
> +	   : callback(callback)
> +	   , context(context) { }

A little strange to break this up into multiple lines like this. I would put
the braces on their own lines, or put it all on one line.

> Source/WebKit2/UIProcess/API/C/WKPagePrivate.h:53
> +#if PLATFORM(MAC) || PLATFORM(WIN)

Putting this kind of #if into a header isn’t right. The macros used for this
can’t easily be used by clients outside WebKit that need to include headers.
Instead, we normally have to use separate headers for platform-specific things
in the API.

For now, you could just have these definitions in the header be unconditional.
The extra declarations would do no harm in the short term for platforms where
nothing is implemented.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1992
>  // FIXME: Find a better place for Mac specific code.

FIXME doesn’t make much sense any more.