[Webkit-unassigned] [Bug 27240] Refactor WebFrame::spoolPages to share with Windows Cairo

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27240


Adam Roben (aroben) <aroben at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32685|review?                     |review+
               Flag|                            |




--- Comment #2 from Adam Roben (aroben) <aroben at apple.com>  2009-07-13 16:00:59 PDT ---
(From update of attachment 32685)
> +#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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.