[webkit-reviews] review granted: [Bug 27240] Refactor WebFrame::spoolPages to share with Windows Cairo : [Attachment 32685] Refactor page spooling

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 27240: Refactor WebFrame::spoolPages to share with Windows Cairo
https://bugs.webkit.org/show_bug.cgi?id=27240

Attachment 32685: Refactor page spooling
https://bugs.webkit.org/attachment.cgi?id=32685&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +#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).