Bug 27240 – Refactor WebFrame::spoolPages to share with Windows Cairo

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

The existing WebFrame::spool will not build under Cairo due to its use of CG-based data types and methods.  This patch moves the platform-specific implementation into a separate method, conditionally compiled for CG and Cairo.  It also abstracts some common functions into separate methods for easier sharing between platforms.

[Brent Fulgham]

Created attachment 32685 [details]
Refactor page spooling

Refactor page spooling code to share with Cairo port.

[Adam Roben (:aroben)]

Comment on attachment 32685 [details]
Refactor page spooling

> +#if PLATFORM(CG)
>  #include <CoreGraphics/CoreGraphics.h>
>  
>  // CG SPI used for printing
> @@ -105,6 +106,9 @@ extern "C" {
>      CGAffineTransform CGContextGetBaseCTM(CGContextRef c); 
>      void CGContextSetBaseCTM(CGContextRef c, CGAffineTransform m); 
>  }
> +#elif PLATFORM(CAIRO)
> +#include <cairo-win32.h>
> +#endif

It's probably best to keep all #includes above all declarations, even though that will require two #if PLATFORM(CG) blocks.

> +void WebFrame::printHeader(void* ctx, COMPtr<IWebUIDelegate> ui, const IntRect& pageRect, float headerHeight)

Why can't ctx be a PlatformGraphicsContext*?

ui should just be an IWebUIDelegate*, to avoid ref-count churn.

The same comments apply to all your new functions.

> +    PlatformGraphicsContext* pctx = (PlatformGraphicsContext*)ctx;

static_cast would be better here (if you don't change the type of ctx to PlatformGraphicsContext).

> +    int x = pageRect.x();
> +    int y = 0;
> +    RECT headerRect = {x, y, x+pageRect.width(), y+(int)headerHeight};
> +    ui->drawHeaderInRect(d->webView, &headerRect, (OLE_HANDLE)(LONG64)pctx);

reinterpret_cast would be better here (I won't comment on other existing uses of C-style casts).

> +#if PLATFORM(CG)
> +void WebFrame::spoolPage (void* ctx, GraphicsContext* spoolCtx, HDC printDC, COMPtr<IWebUIDelegate> ui, float headerHeight, float footerHeight, UINT page, UINT pageCount)

Please remove the space before the opening parenthesis (here and elsewhere in the patch).

Why not have separate WebFrameCG and WebFrameCairo files for the two implementations of spoolPage?

> +    float scale = (float)mediaBox.size().width()/ (float)pageRect.width();
> +    cairo_scale(pctx, -scale, -scale);
> +    cairo_translate(pctx, -pageRect.x(), -pageRect.y()+headerHeight);
> +    cairo_scale(pctx, scale, scale);
> +    cairo_translate(pctx, -pageRect.x(), -pageRect.y()+headerHeight);   // reserves space for header

Seems like we could share all this coordinate space code if we used GraphicsContext a little more. Maybe add a FIXME?

> +    void spoolPage (void* ctx, WebCore::GraphicsContext* spoolCtx, HDC printDC, COMPtr<IWebUIDelegate> ui, float headerHeight, float footerHeight, UINT page, UINT pageCount);
> +    void printHeader(void* ctx, COMPtr<IWebUIDelegate> ui, const WebCore::IntRect& pageRect, float headerHeight);
> +    void printFooter(void* ctx, COMPtr<IWebUIDelegate> ui, const WebCore::IntRect& pageRect, UINT page, UINT pageCount, float headerHeight, float footerHeight);

Please remove the "ui" parameter names from these declarations.

I think drawHeader/drawFooter would be better names, since these functions on their own don't do any printing.

r=me, but please make these changes first (except for maybe the WebFrameCG/WebFrameCairo stuff).

[Brent Fulgham]

(In reply to comment #2)
> (From update of attachment 32685 [details])
> It's probably best to keep all #includes above all declarations, even though
> that will require two #if PLATFORM(CG) blocks.

Done.
 
> > +void WebFrame::printHeader(void* ctx, COMPtr<IWebUIDelegate> ui, const IntRect& pageRect, float headerHeight)
> 
> Why can't ctx be a PlatformGraphicsContext*?

Done.

> ui should just be an IWebUIDelegate*, to avoid ref-count churn.

Done.
 
> > +    ui->drawHeaderInRect(d->webView, &headerRect, (OLE_HANDLE)(LONG64)pctx);
> 
> reinterpret_cast would be better here (I won't comment on other existing uses
> of C-style casts).

Done.
 
> > +#if PLATFORM(CG)
> > +void WebFrame::spoolPage (void* ctx, GraphicsContext* spoolCtx, HDC printDC, COMPtr<IWebUIDelegate> ui, float headerHeight, float footerHeight, UINT page, UINT pageCount)
> 
> Please remove the space before the opening parenthesis (here and elsewhere in
> the patch).

Done.


> Seems like we could share all this coordinate space code if we used
> GraphicsContext a little more. Maybe add a FIXME?

Fixme added.  I'll try to take another pass once I can do more printing support work to factor these out a bit better.

> > +    void spoolPage (void* ctx, WebCore::GraphicsContext* spoolCtx, HDC printDC, COMPtr<IWebUIDelegate> ui, float headerHeight, float footerHeight, UINT page, UINT pageCount);
> > +    void printHeader(void* ctx, COMPtr<IWebUIDelegate> ui, const WebCore::IntRect& pageRect, float headerHeight);
> > +    void printFooter(void* ctx, COMPtr<IWebUIDelegate> ui, const WebCore::IntRect& pageRect, UINT page, UINT pageCount, float headerHeight, float footerHeight);
> 
> Please remove the "ui" parameter names from these declarations.

Done.

> I think drawHeader/drawFooter would be better names, since these functions on
> their own don't do any printing.

Done.
 
> r=me, but please make these changes first (except for maybe the
> WebFrameCG/WebFrameCairo stuff).

Done!

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45846.

[Brent Fulgham]

A quick test of printing via a nightly WebKit using Safari on Windows seems to work properly.