[webkit-reviews] review denied: [Bug 16979] Patch to conditionalize some CG/Cairo support in win32 : [Attachment 18768] Fifth revision based on Eric's comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 29 12:47:01 PST 2008


Adam Roben <aroben at apple.com> has denied Brent Fulgham <bfulgham at gmail.com>'s
request for review:
Bug 16979: Patch to conditionalize some CG/Cairo support in win32
http://bugs.webkit.org/show_bug.cgi?id=16979

Attachment 18768: Fifth revision based on Eric's comments
http://bugs.webkit.org/attachment.cgi?id=18768&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
- * Copyright (C) 2006, 2007 Apple Inc.  All rights reserved.
+ * Copyright (C) 2008 Apple Inc.  All rights reserved.

I think you need to retain the "2006, 2007" for the new *CGWin.cpp files, since
the code in them (which is what the copyright pertains to) dates back to 2006.

+HBITMAP imageFromSelection(Frame* frame, bool forceBlackText)
+{
+    notImplemented();
+}

You're missing a return statement here.

+    setPaintingDisabled(!m_data->m_cgContext);
+    if (m_data->m_cgContext) {

Seems strange to have a member called m_cgContext in
GraphicsContextCairoWin.cpp.

+    size_t imageHeight = 0;
+    size_t imageWidth = 0;
     for (int i = 0; i < frames; ++i) {
-	 CGImageRef image = frameAtIndex(i);
-	 if (CGImageGetHeight(image) == (size_t)srcSize.height() &&
CGImageGetWidth(image) == (size_t)srcSize.width()) {
+	 NativeImagePtr image = frameAtIndex(i);
+	 imageHeight = CGImageGetHeight(image);
+	 imageWidth = CGImageGetWidth(image);
+	 if (imageHeight == static_cast<size_t>(srcSize.height()) && imageWidth
== static_cast<size_t>(srcSize.width())) {

I don't think there's any reason to move the imageHeight and imageWidth
declarations out of the for loop. We also don't tend to use typedefs like
NativeImagePtr within the source files, just in the headers, so you should
change that back to CGImageRef. Ditto for ImageCairoWin.cpp.

+    SelectObject(tempDC, bmp);
+    GraphicsContext gc(tempDC);
+#endif

I don't see what #if or #ifdef this #endif is matching.

Are the *CairoWin.cpp files supposed to compile yet? Or is this just an initial
non-compiling-but-close version? I think landing a compiling version is vastly
preferable to a non-compiling one.

I'm going to r- so that we can get these last few niggling details worked out,
but then this should be good to go!


More information about the webkit-reviews mailing list