Bug 55800 - WK2 needs printing support on Windows
: WK2 needs printing support on Windows
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit2
: 528+ (Nightly build)
: PC Windows 7
: P2 Normal
Assigned To: Jon Honeycutt
: InRadar, PlatformOnly
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-04 15:57 PST by Jon Honeycutt
Modified: 2011-03-04 17:22 PST (History)
1 user (show)

See Also:


Attachments
Patch (11.61 KB, patch)
2011-03-04 16:47 PST, Jon Honeycutt
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Honeycutt 2011-03-04 15:57:44 PST
We need printing support for WebKit2 on Windows.

<rdar://problem/8903808>
Comment 1 Jon Honeycutt 2011-03-04 16:47:52 PST
Created attachment 84830 [details]
Patch
Comment 2 WebKit Review Bot 2011-03-04 16:49:39 PST
Attachment 84830 [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/UIProcess/API/C/WKPage.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Darin Adler 2011-03-04 16:53:24 PST
Comment on attachment 84830 [details]
Patch

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.
Comment 4 Jon Honeycutt 2011-03-04 17:22:22 PST
(In reply to comment #3)
> (From update of attachment 84830 [details])
> 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.

Moved the braces onto their own lines.

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

OK, made these unconditional in the header.

> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1992
> >  // FIXME: Find a better place for Mac specific code.
> 
> FIXME doesn’t make much sense any more.

Removed the FIXME.

Thanks for the review! Landed in r80398.