[webkit-reviews] review denied: [Bug 22891] Scrolling in Windows Cairo Broken if no background color set. : [Attachment 31065] Corrects HDC/cairo_t* Context Synchronization

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 22891: Scrolling in Windows Cairo Broken if no background color set.
https://bugs.webkit.org/show_bug.cgi?id=22891

Attachment 31065: Corrects HDC/cairo_t* Context Synchronization
https://bugs.webkit.org/attachment.cgi?id=31065&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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);