[webkit-reviews] review denied: [Bug 16979] Patch to conditionalize some CG/Cairo support in win32 : [Attachment 18629] Second revision based on Alp and Aroben's comments.

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 18629: Second revision based on Alp and Aroben's comments.
http://bugs.webkit.org/attachment.cgi?id=18629&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 First step of fully native win32 build.

I object to this description of what you're doing. There's nothing "fully
native" about Cairo in my opinion.

I would say "First step of getting Cairo-based WebKit working on Windows again"
or something like that.

The imageFromSelection function looks improperly ifdef'd here. You've put the
entire body of the function inside and if and not provided an else case. I
suggest putting the #if around the entire function, including the function name
and arguments, or providing at least a call to notImplemented().

Also, it would be a lot better to move a function like this into its own file,
one with a CGWin.cpp suffix, rather than have it sitting in a shared file with
an #if around it.

Putting an #if around the body of a function starts us out going in the wrong
direction for platform code. What we try to do is to share declarations in
header files, and have implementations with separate functions in separate
files without sprinkling them with #ifs. Actually using #if inside the
implementations of files is our method of last resort and should only be used
when our platform portability strategy has broken down.

 36 #if PLATFORM(CG)
3537 #include <WebKitSystemInterface/WebKitSystemInterface.h>
 38 #endif

This include should be moved out of the shared includes list into a separate
section below. Please do that when adding #if to an includes list. That's how
we format them.

Also, the if here is PLATFORM(CG), but the if below if PLATFORM(CF). I suggest
using PLATFORM(CG) exclusively for these ifdefs. Since CG is based on CF, you
are guaranteed to have CF if you have CG, and these bits of code are only
helpful if you do have CG. I'm not going to comment individually on each
PLATFORM(CF), but I think they're all wrong.

I think it's a little strange to just #if out a function and leave it doing
nothing when CG is not defined, but you're doing that in a number of places
here. I strongly prefer at least putting #else notImplemented() in these
places.

I really think it doesn't make sense to create a single heavily ifdef'd
GraphicsContextWin.cpp. Instead we should have GraphicsContextCairoWin.cpp and
GraphicsContextCGWin.cpp and put only shared code into GraphicsContextWin.cpp.

 125 #if PLATFORM(CG)
 126	imageHeight = CGImageGetHeight(image);
 127	imageWidth = CGImageGetWidth(image);
 128 #elif PLATFORM(CAIRO)
 129	imageHeight = cairo_image_surface_get_height(image);
 130	imageWidth = cairo_image_surface_get_width(image);
 131 #endif

This code is indented wrong.

While there are no major problems with this patch, there are enough small
mistakes that I'm going to say review- for this round.