[Webkit-unassigned] [Bug 22891] Scrolling in Windows Cairo Broken if no background color set.

[bugzilla-daemon at webkit.org]

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


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31065|review?                     |review-
               Flag|                            |




------- Comment #20 from eric at webkit.org  2009-06-08 16:26 PDT -------
(From update of attachment 31065)
In general this looks fine.  but I'm not really a windows guy.

Should follow "the create rule" and webkit naming style:
 39 static cairo_t* CairoContextWithHDC(HDC hdc, bool hasAlpha)

createCairoContextWithHDC(...)

Does:
 44     GetObject(bitmap, sizeof(info), &info);

need a later release call?

Extra spaces:
 53     cairo_t* context = cairo_create (image);
 54     cairo_surface_destroy (image);


This shoudl be abstracted into its own function:
90         // Create a bitmap DC in which to draw.
 91         BITMAPINFO bitmapInfo;
 92         bitmapInfo.bmiHeader.biSize          = sizeof(BITMAPINFOHEADER);
which returns the right BITMAPINFO.
BITMAPINFO bitmapInfo = bitmapInfoForSize(destRect.size());


Should be its own function too:
         if (supportAlphaBlend) {
 114             BITMAP bmpInfo;
 115             GetObject(bitmap, sizeof(bmpInfo), &bmpInfo);
 116             int bufferSize = bmpInfo.bmWidthBytes * bmpInfo.bmHeight;
 117             memset(bmpInfo.bmBits, 0, bufferSize);
 118         }
fillWithClearColor(bitmap);


Should again be another local static function:
123         // Apply a translation to our context so that the drawing done will
be at (0,0) of the bitmap.
 124         XFORM xform;
 125         xform.eM11 = 1.0f;
 126         xform.eM12 = 0.0f;
 127         xform.eM21 = 0.0f;
 128         xform.eM22 = 1.0f;
 129         xform.eDx = -dstRect.x();
 130         xform.eDy = -dstRect.y();

XFORM xform = makeXFORM(1, 0, 0, 1, -x, -y);

Breaking it all out like that will make the original method much smaller, and
hopefully make it easier to understand what you're actually trying to do.

Ugly (and error prone!) that we have to do this all manually:
177         // Delete all our junk.
 178         cairo_surface_destroy(image);
 179         ::DeleteDC(hdc);
 180         ::DeleteObject(bitmap);

This is confusing:
         return;
 183     }
 184 
 185     m_data->restore();

I think that restore shoudl be an early return, and the if block removed.

More extra spaces:
     cairo_t* targetRef = cairo_create (image);
 58     cairo_surface_destroy (image);


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