Bug 16979 – Patch to conditionalize some CG/Cairo support in win32

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This is a fairly contained patch that excludes various CoreGraphics and CoreFoundation-based implementations in the win32 build.  Some Cairo support is added (based on older WebKit builds).

A second bug will be filed with changes to the vcproj structure to avoid compiling unsupported files under a 'pure' win32 build.

[Brent Fulgham]

Created attachment 18613 [details]
Initial patch to WebKit to support building without CG/CF.

Note that this patch builds cleanly using the normal win32 target.  It is not complete for a 'pure' win32 target, but this patch removes a large number of compiler errors when compiling without CG.  A future patch to correct the vcproj files will be filed as a separate bug to allow a full build under win32.

[Alp Toker]

Comment on attachment 18613 [details]
Initial patch to WebKit to support building without CG/CF.

This is looking good. One issue with your approach to conditional that I think needs cleanup described below. I'd also like a quick second opinion from a Win port hacker before landing this since I don't maintain this module.

>Index: platform/graphics/win/GraphicsContextWin.cpp
>===================================================================
>--- platform/graphics/win/GraphicsContextWin.cpp	(revision 29726)
>+++ platform/graphics/win/GraphicsContextWin.cpp	(working copy)
>@@ -29,11 +29,15 @@
> #include "AffineTransform.h"
> #include "NotImplemented.h"
> #include "Path.h"
>-#include <CoreGraphics/CGBitmapContext.h>
>-#include <WebKitSystemInterface/WebKitSystemInterface.h>
> #include <wtf/MathExtras.h>
> 
>+#if PLATFORM(CG)
>+#include <CoreGraphics/CGBitmapContext.h>
>+#include <WebKitSystemInterface/WebKitSystemInterface.h>
> #include "GraphicsContextPlatformPrivate.h"
>+#else
>+#include <cairo-win32.h>
>+#endif

Please make this #elif PLATFORM(CAIRO)

> 
> using namespace std;
> 
>@@ -41,6 +45,7 @@ namespace WebCore {
> 
> class SVGResourceImage;
> 
>+#if PLATFORM(CG)
> static CGContextRef CGContextWithHDC(HDC hdc)
> {
>     HBITMAP bitmap = (HBITMAP)GetCurrentObject(hdc, OBJ_BITMAP);
>@@ -76,9 +81,11 @@ GraphicsContext::GraphicsContext(HDC hdc
>         setPlatformStrokeColor(strokeColor());
>     }
> }
>+#endif
> 
> HDC GraphicsContext::getWindowsContext(const IntRect& dstRect, bool supportAlphaBlend)
> {
>+#if PLATFORM(CG)
>     if (inTransparencyLayer()) {
>         if (dstRect.isEmpty())
>             return 0;
>@@ -132,12 +139,37 @@ HDC GraphicsContext::getWindowsContext(c
>     CGContextFlush(platformContext());
>     m_data->save();
>     return m_data->m_hdc;
>+#else
>+    // This is probably wrong, and definitely out of date.  Pulled from old SVN
>+    cairo_surface_t* surface = cairo_get_target(platformContext());
>+    HDC hdc = cairo_win32_surface_get_dc(surface);   
>+    SaveDC(hdc);
>+
>+    // FIXME: We need to make sure a clip is really set on the HDC.
>+    // Call SetWorldTransform to honor the current Cairo transform.
>+    SetGraphicsMode(hdc, GM_ADVANCED); // We need this call for themes to honor world transforms.
>+    cairo_matrix_t mat;
>+    cairo_get_matrix(platformContext(), &mat);
>+    XFORM xform;
>+    xform.eM11 = mat.xx;
>+    xform.eM12 = mat.xy;
>+    xform.eM21 = mat.yx;
>+    xform.eM22 = mat.yy;
>+    xform.eDx = mat.x0;
>+    xform.eDy = mat.y0;
>+    SetWorldTransform(hdc, &xform);
>+
>+    return hdc;
>+#endif

#elif PLATFORM(CAIRO) again. In this case, also add _another_ #else with a notImplemented() after the CAIRO case.

> }
> 
>+#if PLATFORM(CG)
> bool GraphicsContext::inTransparencyLayer() const { return m_data->m_transparencyCount; }
>+#endif
> 
> void GraphicsContext::releaseWindowsContext(HDC hdc, const IntRect& dstRect, bool supportAlphaBlend)
> {
>+#if PLATFORM(CG)
>     if (hdc && inTransparencyLayer()) {
>         if (dstRect.isEmpty())
>             return;
>@@ -169,8 +201,15 @@ void GraphicsContext::releaseWindowsCont
>     }
> 
>     m_data->restore();
>+#else
>+    cairo_surface_t* surface = cairo_get_target(platformContext());
>+    HDC hdc2 = cairo_win32_surface_get_dc(surface);
>+    RestoreDC(hdc2, -1);
>+    cairo_surface_mark_dirty(surface);
>+#endif

Ditto here and all the other places.

[Alp Toker]

On second thought, the "#else with a notImplemented() after the CAIRO case" may be overkill. At your discretion.

[Adam Roben (:aroben)]

(In reply to comment #3)
> On second thought, the "#else with a notImplemented() after the CAIRO case" may
> be overkill. At your discretion.

Yeah, I think a build failure is as good as or better than all those notImplemented()s.

[Adam Roben (:aroben)]

Comment on attachment 18613 [details]
Initial patch to WebKit to support building without CG/CF.

I don't think it's so great to have all these #ifdefs within each "Win" file. I think we're taking "Win" too literally here -- it doesn't (and shouldn't) mean "any port building on Windows" (for example, Qt/win doesn't use these files). Right now "Win" means "the Windows port which depends on CG, CF, CFNetwork, etc.". Clearly "Win" is not a great name for this. We should try to come up with a better name for the current port and a good name for the port you're starting to create. Then we can separate the implementations into separate files for each port, instead of tossing all these #ifdefs in.

I've CCed Darin because I know he's been thinking about this naming issue as well.

[Alp Toker]

(In reply to comment #5)
> (From update of attachment 18613 [details] [edit])
> I don't think it's so great to have all these #ifdefs within each "Win" file. I
> think we're taking "Win" too literally here -- it doesn't (and shouldn't) mean
> "any port building on Windows" (for example, Qt/win doesn't use these files).
> Right now "Win" means "the Windows port which depends on CG, CF, CFNetwork,
> etc.". Clearly "Win" is not a great name for this. We should try to come up
> with a better name for the current port and a good name for the port you're
> starting to create. Then we can separate the implementations into separate
> files for each port, instead of tossing all these #ifdefs in.

I don't think the conditionals hurt readability here, but obviously you spend more time working with this code, so you may disagree.

I think there are benefits in sharing the code rather than forking it into a new port. WIN should probably be made to only the "native" parts of the Windows ports, which is a slight semantic change, but can be introduced incrementally without vast search and replace or build system changes.

Which source files are you worried about, specifically?

[Adam Roben (:aroben)]

(In reply to comment #6)
> I think there are benefits in sharing the code rather than forking it into a
> new port. WIN should probably be made to only the "native" parts of the Windows
> ports, which is a slight semantic change, but can be introduced incrementally
> without vast search and replace or build system changes.

(I'm not sure what "native" really means here. Is Cairo a "native" Win32 library? It doesn't seem to be any more so than CoreGraphics, for example. So we'll probably need a more descriptive name than "native" for whatever this port becomes.)

I'm not suggesting that we abandon all code sharing. But there are many places in this patch where the entire body of a function is #ifdef'd, which I think is much less readable than splitting the two versions of the function into separate files. Places where only parts of a function body are #ifdef'd are probably good candidates for refactoring, perhaps by splitting out the #ifdef'd parts into a helper function.

> Which source files are you worried about, specifically?

For example, SearchPopupMenuWin.cpp is not really useful if you don't have CF, as the entire implementation is based on it, so I don't see any benefit to sharing this file with a port that doesn't use CF.

[Alp Toker]

(In reply to comment #7)
> (In reply to comment #6)
> > I think there are benefits in sharing the code rather than forking it into a
> > new port. WIN should probably be made to only the "native" parts of the Windows
> > ports, which is a slight semantic change, but can be introduced incrementally
> > without vast search and replace or build system changes.
> 
> (I'm not sure what "native" really means here. Is Cairo a "native" Win32

By "native", I mean the code that uses Win32 API directly, for example places where we use GDI directly. I'm suggesting that WIN can refer to these bits.

Cairo or CG certainly don't fall into the "native" category -- they have their own platform defines.

> > Which source files are you worried about, specifically?
> 
> For example, SearchPopupMenuWin.cpp is not really useful if you don't have CF,
> as the entire implementation is based on it, so I don't see any benefit to
> sharing this file with a port that doesn't use CF.
> 

Many of the *Win source files only use Win32 directly and aren't Apple-specific at all, so it makes sense to continue to use Win as an umbrella port name here.

Files which are mostly Apple-Win should probably be given a new name. I'd propose moving SearchPopupMenuWin.cpp to SearchPopupMenuAWin.cpp and creating a new SearchPopupMenuWin.cpp that uses native Win32 (Cairo doesn't come into the picture here) for now. I actually suspect there won't be all that many files that need renaming to AWin.

My AWin proposal is just to get the debate going, you may have better ideas :-)

I think it'll become more clear how to elegantly split the code modules as the port develops and starts to work.

[Brent Fulgham]

(In reply to comment #7)
> (I'm not sure what "native" really means here. Is Cairo a "native" Win32
> library? It doesn't seem to be any more so than CoreGraphics, for example. So
> we'll probably need a more descriptive name than "native" for whatever this
> port becomes.)

It's more 'native' in that someone could legally take the code and do something with it.  At the moment, I can't use a CG-based WebKit to do anything but play at home.

And for all practical purposes, a Cairo back-end is probably as close to a 'native' win32 build as we are likely to get, as replicating the Cairo functionality in GDI/GDI+/etc. seems like a waste of time.  The only reason I am pursuing the Cairo work is that I can't ship WebKit with my application using the CG-based backend.

[Brent Fulgham]

Created attachment 18624 [details]
Revised patch based on Alp's comments.

[Adam Roben (:aroben)]

Comment on attachment 18624 [details]
Revised patch based on Alp's comments.

Looks like this contains some Drosera changes you didn't intend to include.

[Adam Roben (:aroben)]

(In reply to comment #8)
> By "native", I mean the code that uses Win32 API directly, for example places
> where we use GDI directly. I'm suggesting that WIN can refer to these bits.

That seems OK. "Win32" might be even more explicit, but I'm not sure I prefer it.

> Many of the *Win source files only use Win32 directly and aren't Apple-specific
> at all, so it makes sense to continue to use Win as an umbrella port name here.
> 
> Files which are mostly Apple-Win should probably be given a new name. I'd
> propose moving SearchPopupMenuWin.cpp to SearchPopupMenuAWin.cpp and creating a
> new SearchPopupMenuWin.cpp that uses native Win32 (Cairo doesn't come into the
> picture here) for now.

Probably SearchPopupMenuCF.cpp is more appropriate (see below).

> My AWin proposal is just to get the debate going, you may have better ideas :-)
> 
> I think it'll become more clear how to elegantly split the code modules as the
> port develops and starts to work.

It seems like we need to have a distinction between configuration options and ports. I think of configuration options as things like "enable SVG as image support", "use Cairo for the graphics library", etc. The configuration options generally map to our preprocessor macros like PLATFORM() and ENABLE(). Ports I think of as bundles of configuration options, and the configuration options used for a particular port may change over time.

So, I think that something like an AWin suffix is too port-oriented, when it should really be configuration-option-based. We already have a lot of configuration-option-based filenames (*CG.cpp, *Cairo.cpp, etc.), so I think we should stick with that method.

[Adam Roben (:aroben)]

(In reply to comment #9)
> (In reply to comment #7)
> > (I'm not sure what "native" really means here. Is Cairo a "native" Win32
> > library? It doesn't seem to be any more so than CoreGraphics, for example. So
> > we'll probably need a more descriptive name than "native" for whatever this
> > port becomes.)
> 
> It's more 'native' in that someone could legally take the code and do something
> with it.  At the moment, I can't use a CG-based WebKit to do anything but play
> at home.

I wouldn't really consider that a definition of the word "native". But I don't want to mince words too much. I just don't think "native" is really a meaningful name for a port, since all ports on a particular platform could be considered more or less "native" depending on how many libraries they use to abstract away the underlying OS.

[Alp Toker]

Adam,

Agreed on all points. SearchPopupMenuCF.cpp is fine, and probably better than introducing a new port-oriented suffix.

[Brent Fulgham]

Created attachment 18629 [details]
Second revision based on Alp and Aroben's comments.

[Brent Fulgham]

(In reply to comment #14)
> Agreed on all points. SearchPopupMenuCF.cpp is fine, and probably better than
> introducing a new port-oriented suffix.

I think this sounds great, too.  It makes more sense to talk about win-cg, win-cairo, mac-cg, mac-cairo, etc...

In that case, the various files and even the PLATFORM(CG/CAIRO/whatever) are probably fine as-is, with the glaring exception of figuring out what to do with th e vcproj files...

[Darin Adler]

Comment on attachment 18629 [details]
Second revision based on Alp and Aroben's comments.

+        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.

[Brent Fulgham]

From comment #4 From Adam Roben 2008-01-23 08:34 PDT [reply]
> > On second thought, the "#else with a notImplemented() after the CAIRO case" may
> > be overkill. At your discretion.
>
> Yeah, I think a build failure is as good as or better than all those
> notImplemented()s.

(In reply to comment #17)
> 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().

Okay guys, I've been asked to removed the 'notImplemented' calls in one review, and now am asked to  add them back in for the current review.  Could the Apple folks at least be consistent with how this should work?

As for the other comments, I'm revising the patch to break into multiple files as you suggest (and as discussed in the earlier comments and IRC discussion).

[Darin Adler]

(In reply to comment #18)
> Okay guys, I've been asked to removed the 'notImplemented' calls in one review,
> and now am asked to add them back in for the current review.

I don't think I asked you to add them back. I told you to put the #if around the entire function.

My take:

    - A build failure is OK.
    - A notImplemented() is OK.
    - A broken function that compiles but does the wrong thing at runtime is much less OK.

If the function isn't implemented, either #if it so it's entirely omitted, or put something there to indicate it's not implemented. The imageFromSelection looks to me it will compile, but just do the wrong thing. It would be better to put the #if *around* imageFromSelection rathe than compiling an empty function.

[Brent Fulgham]

Created attachment 18731 [details]
Third revision based on Darin and Alp's comments.

New patch that breaks implementation files into CG/Cairo-specific components.  Also uses the CGWin/CairoWin naming conventions.

Patch removes PLATFORM(CF) guards, since our first step is building with CF/CFNetwork, but Cairo instead of CG.  Therefore, those changes were out of scope for this change.

[Adam Roben (:aroben)]

Comment on attachment 18731 [details]
Third revision based on Darin and Alp's comments.

This is really staring to look nice. I think breaking these methods up into separate files is really a good way to go.

It looks like some of your new files have the svn:executable property set on them. You should remove that by running

svn propdel svn:exectuable file1 file2 file3...

I'll post more review comments soon.

[Adam Roben (:aroben)]

Comment on attachment 18731 [details]
Third revision based on Darin and Alp's comments.

I think it would be nicer to use 'svn cp' when creating the *CGWin.cpp files. For example:

svn cp FrameWin.cpp FrameCGWin.cpp

That way it'll be easier to track the changes to the methods defined in the *CGWin.cpp files across the split in history.

The #includes in FrameCairoWin.cpp could probably be reduced quite a bit.

A number of your new methods in the *CairoWin.cpp files throughout this patch are missing return statements.

+GraphicsContext::GraphicsContext(HDC hdc)
+//    : m_common(createGraphicsContextPrivate())
+//    , m_data(new GraphicsContextPlatformPrivate(CGContextWithHDC(hdc)))
+{
+//    CGContextRelease(m_data->m_cgContext);

We generally don't like to commit commented-out code.

I think this is very close to being ready to go in to Subversion.

[Brent Fulgham]

Created attachment 18740 [details]
Fourth revision based on Adam's comments.

* Uses 'svn cp' to create CG-based files
* Added missing return statements in Cairo stub implementations.
* Removes unnecessary header includes.

[Eric Seidel (no email)]

Comment on attachment 18740 [details]
Fourth revision based on Adam's comments.

Once you're inside a CG file, there is no need to conditionalize the header include (see FrameCGWin.cpp).

What's
// FIXME: Ideally we'd have an isPluginView() here but we can't add that to the open source tree right now.
?

We use static_cast in c++ code instead of c-style casts:
(float)printRect.height()

Unnecessary float-double conversion here:
float printedPagesHeight = 0.0;
0.0f instead.

Spaces after commas:
pages.append(IntRect(0,printedPagesHeight,currPageWidth,currPageHeight));

Again:
IntRect ir((int)fr.x(), (int)fr.y(),(int)fr.width(),(int)fr.height());

If I'm picking on you about existing code that's somehow showing up as + lines, my apologies... I should be picking on aroben or whatever Apple windows hacker violated their own published guidelines. :)

We don't generally commit commented out code:
71         /*if (fillColor.hasAlpha() || graphicsContext->inTransparencyLayer()) {
 72             // GDI can't handle drawing transparent text.  We have to draw into a mask.  We draw black text on a white-filled background.
 73             // We also do this when inside transparency layers, since GDI also can't draw onto a surface with alpha.
 74             graphicsContext->save();
 75             graphicsContext->setFillColor(Color::white);
 76             textDrawingDC = graphicsContext->getWindowsBitmapContext(textRect);
 77             SetTextColor(hdc, RGB(0, 0, 0));
 78         } else*/

braces are on the same line as the if:
    if (platformData.syntheticOblique())
 143     {

No commented out code:
//#include "FontFallbackList.h"

It looks like someone could come by with a second pass and push more code down into a shared GraphicsContextCG between the Mac impl and the Win impl.

Our naming style is:
setShouldApplyMacAscentHack(bool)
bool shouldApplyMacAscentHack()
not "get"

In general this patch looks great!

I'll let aroben or darin comment once more, but I think you're just about ready to go.

[Brent Fulgham]

(In reply to comment #24)
> (From update of attachment 18740 [details] [edit])
> Once you're inside a CG file, there is no need to conditionalize the header
> include (see FrameCGWin.cpp).

Some of these issues are caused by the use of 'svn cp' to generate the initial "CG" variant files.  The final version of FrameCGWin.cpp does *not* have any conditionalization around the Core Graphics headers.

> If I'm picking on you about existing code that's somehow showing up as + lines,
> my apologies... I should be picking on aroben or whatever Apple windows hacker
> violated their own published guidelines. :)
> 

Most of the items you mention are from existing code that was copied over from the 'parent' class; however, it's good to get it right now (it may be a while before anyone looks at this again).

I'll make these changes and update the patch.
 

[Brent Fulgham]

Created attachment 18768 [details]
Fifth revision based on Eric's comments

- Better use of 'float' declarations.
- Change C-style casts to static_cast
- Remove commented-out code
- Rename "getShouldApplyMacAscentHack" to "shouldApplyMacAscentHack"
- Correct brace position on some if statements

[Adam Roben (:aroben)]

Comment on attachment 18768 [details]
Fifth revision based on Eric's comments

- * 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!

[Brent Fulgham]

Created attachment 18805 [details]
Sixth revision based on Adam's comments.

Another round of revisions.  This version is known to build under Cairo on Windows, but this patch does not touch several other files that need to be updated (RenderThemeSafari, PlatformScrollBarSafari).

I would really like to get this first series of changes committed before attacking the other stuff outside platform/graphics.

[Brent Fulgham]

Fixing things so that the *CairoWin stuff builds cleanly required touching the GraphicsContextCairo, so I would like a GTK user (e.g., alp) to confirm this doesn't break anything.

[Alp Toker]

(In reply to comment #24)
> (From update of attachment 18740 [details] [edit])
> Once you're inside a CG file, there is no need to conditionalize the header
> include (see FrameCGWin.cpp).
> 
> What's
> // FIXME: Ideally we'd have an isPluginView() here but we can't add that to the
> open source tree right now.
> ?
> 
> We use static_cast in c++ code instead of c-style casts:
> (float)printRect.height()
> 
> Unnecessary float-double conversion here:
> float printedPagesHeight = 0.0;
> 0.0f instead.
> 
> Spaces after commas:
> pages.append(IntRect(0,printedPagesHeight,currPageWidth,currPageHeight));
> 
> Again:
> IntRect ir((int)fr.x(), (int)fr.y(),(int)fr.width(),(int)fr.height());
> 
> If I'm picking on you about existing code that's somehow showing up as + lines,
> my apologies... I should be picking on aroben or whatever Apple windows hacker
> violated their own published guidelines. :)
> 

It's probably not worth trying to clean up this code. The print spooler has already been abstracted and just needs to be svn mv'd down into WebCore before we can kill the Win implementation completely. Fixing trivial style violations in existing code, especially code that's near end of life, makes the important parts of this patch harder to review.

[Alp Toker]

(In reply to comment #30)
> It's probably not worth trying to clean up this code. The print spooler has
> already been abstracted and just needs to be svn mv'd down into WebCore before
> we can kill the Win implementation completely. Fixing trivial style violations
> in existing code, especially code that's near end of life, makes the important
> parts of this patch harder to review.
> 

Now that the coding style changes have been done, it's probably not worth removing them. Just worth noting this for future reference.

The WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivate.h changes look spurious in the patch. Is this just an artifact of git/svn cp? I see the whole content of the file is added and then removed.

Patch looks good apart from these and some nit-picks (I think it's a bit too early to split some of these files -- I have a feeling they're going to merge back in a few commits as platform-specific code is removed.) Still, best to get this landed now and continue to develop it in tree.

Looks fine on the GTK+ side.

[Brent Fulgham]

Created attachment 18811 [details]
Correct typo to Sixth patch

I manually edited the patch because the VCProj changes are a total mess right now, so I wanted to limit it to the original set of CG-only file additions.

[Adam Roben (:aroben)]

(In reply to comment #31)
> The WebCore/platform/graphics/cairo/GraphicsContextPlatformPrivate.h changes
> look spurious in the patch. Is this just an artifact of git/svn cp? I see the
> whole content of the file is added and then removed.

I think you're getting confused by the (admittedly confusing) output from svn-create-patch. When there's an 'svn cp' done followed by modifications to the new file, svn-create-patch creates two diffs for the file, one that adds it, and one that applies the modifications.

[Adam Roben (:aroben)]

Comment on attachment 18811 [details]
Correct typo to Sixth patch

+    if (dc) {
+        cairo_surface_t* surface = cairo_win32_surface_create(dc);
+        m_data->cr = cairo_create(surface);
+	m_data->m_hdc = dc;
+    } else {
+        setPaintingDisabled(true);
+        m_data->cr = 0;
+	m_data->m_hdc = 0;
+    }

There are some tabs here that will have to be removed before landing.

 void BitmapImage::drawFrameMatchingSourceSize(GraphicsContext* ctxt, const FloatRect& dstRect, const IntSize& srcSize, CompositeOperator compositeOp)
 {
     int frames = frameCount();
+    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 still don't think imageHeight and imageWidth should be moved out of the loop like this, and we should be using CGImageRef instead of NativeImagePtr. Similar comments apply to ImageCairoWin.cpp.

It's great that this builds now! These last couple of issues are the only problems I see.

[Brent Fulgham]

(In reply to comment #34)
>  void BitmapImage::drawFrameMatchingSourceSize(GraphicsContext* ctxt, const
> FloatRect& dstRect, const IntSize& srcSize, CompositeOperator compositeOp)
>  {
>      int frames = frameCount();
> +    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 still don't think imageHeight and imageWidth should be moved out of the loop
> like this, and we should be using CGImageRef instead of NativeImagePtr. Similar
> comments apply to ImageCairoWin.cpp.

Oh -- I forgot about this change.  I had done this when I was trying to fit Ciaro and CG code in the same file, so by lifting the height/width calculation out, I could #if/def around just those two lines and handle both cases.

It's not needed now, and I have now returned it to the original form. 

[Brent Fulgham]

Created attachment 18829 [details]
Seventh revision based on Adam's comments.

* Corrected drawFrameMatchingSourceSize implementations (in CG and Cairo code).
* Added path "cg/" and "cairo/" to include of Graphic
* Removed tabs.

[Adam Roben (:aroben)]

Comment on attachment 18829 [details]
Seventh revision based on Adam's comments.

-#include "GraphicsContextPlatformPrivate.h"
+#include "cg/GraphicsContextPlatformPrivate.h"

While this works, it's not the way we do it. Instead of adding "cg/" to the header path, you should add the cg directory to the project's include path (defined in the .vcproj file).

Otherwise looks good, though.

[Brent Fulgham]

Created attachment 18835 [details]
Eighth revision based on Adam's comments.

- Changed name of GraphicsContextPlatformPrivate.h to GraphicsContextPlatformPrivateCG.h and GraphicsContextPlatformPrivateCairo.h to avoid name conflict.
- Removed use of "cg/" and "cairo/" path specifiers in include statements.

[Adam Roben (:aroben)]

Comment on attachment 18835 [details]
Eighth revision based on Adam's comments.

Index: WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h
===================================================================
--- WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h	(revision 29726)
+++ WebCore/platform/graphics/cg/GraphicsContextPlatformPrivateCG.h	(working copy)
@@ -23,6 +23,10 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
  */
 
+#include "config.h"

This is wrong. We only include config.h from source files, not from header files. I don't think the #if PLATFORM(CG) in this file is needed anymore now that it has a CG-specific name. Ditto for GraphicsContextPlatformPrivateCairo.h.

[Brent Fulgham]

Created attachment 18914 [details]
Ninth patch based on Adam's comments.

* Removed "config.h" includes from GraphicsContexPlatformPrivate[CG|Cairo].h".
* Removed PLATFORM(CG)/PLATFORM(CAIRO) from GraphicsContexPlatformPrivate[CG|Cairo].h".

[Adam Roben (:aroben)]

Comment on attachment 18914 [details]
Ninth patch based on Adam's comments.

r=me

[Brent Fulgham]

Created attachment 18975 [details]
Tenth revision to patch cleanly against today's SVN.

Regenerated patch against today's SVN.

[Brent Fulgham]

Created attachment 18976 [details]
Try #11 -- redo 'svn cp' events to allow clean patch.

Update to build against current svn rev.

[Mark Rowe (bdash)]

Landed in r30056.  Thanks for your patience and persistence with this!