Bug 7984 – ifdef win32 specific code in GraphicsContextCairo.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Michael Emmel]

Index: GraphicsContextCairo.cpp
===================================================================
--- GraphicsContextCairo.cpp    (revision 13479)
+++ GraphicsContextCairo.cpp    (working copy)
@@ -33,8 +33,12 @@
 #include "IntRect.h"

 #include <cairo.h>
+
+#ifdef WIN32
 #include <cairo-win32.h>
+#endif

+
 #ifndef M_PI
 #define M_PI 3.14159265358979323846
 #endif
@@ -75,6 +79,7 @@
     cairo_destroy(context);
 }

+#ifdef WIN32
 GraphicsContext::GraphicsContext(HDC dc)
     : m_common(createGraphicsContextPrivate())
     , m_data(new GraphicsContextPlatformPrivate)
@@ -82,6 +87,7 @@
     cairo_surface_t* surface = cairo_win32_surface_create(dc);
     m_data->context = cairo_create(surface);
 }
+#endif

 GraphicsContext::GraphicsContext(cairo_t* context)
     : m_common(createGraphicsContextPrivate())

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> Index: GraphicsContextCairo.cpp

Again, please post the patch as a file attachment and mark the "?" review flag.  Thanks!

[Michael Emmel]

Created attachment 7312 [details]
Patch with WIN32 ifdefs and LINUX ifdefs


The patch also contains my Linux ifdefs it thats a problem I'll resubmit the patch without them but in the interm if I'd like to include them so the reviewer can comment on how they would like me to submit the Linux specific ifdefs at a later date

[David Kilzer (:ddkilzer)]

Comment on attachment 7312 [details]
Patch with WIN32 ifdefs and LINUX ifdefs

Set review? flag on attachment.

[Eric Seidel (no email)]

Comment on attachment 7312 [details]
Patch with WIN32 ifdefs and LINUX ifdefs

These should be changed to use PLATFORM(WIN_OS) and PLATFORM(LINUX_OS) macros.  They also should just be made into an ||

#if PLATFORM(WIN_OS) || PLATFORM(LINUX_OS)
cairo_t* platformContext() const
#endif

also, we do not commit commented out code, so 
//typedef struct _GdkDrawable GdkDrawable

It looks like in total you're not adding anything to the file.  You should just change the required #if lines to include linux in an or instead of duplicating them.

[Eric Seidel (no email)]

Comment on attachment 7312 [details]
Patch with WIN32 ifdefs and LINUX ifdefs

These should be changed to use PLATFORM(WIN_OS) and PLATFORM(LINUX_OS) macros.  They also should just be made into an ||

#if PLATFORM(WIN_OS) || PLATFORM(LINUX_OS)
cairo_t* platformContext() const
#endif

also, we do not commit commented out code, so 
//typedef struct _GdkDrawable GdkDrawable

It looks like in total you're not adding anything to the file.  You should just change the required #if lines to include linux in an or instead of duplicating them.

[Michael Emmel]

Actually I think in this case it should be


#if TOOLKIT(WIN32) || TOOLKIT(GTK)

Not PLATFORM since this code is windowing toolkit specific not OS specific ?

[Justin Haygood]

PLATFORM(WIN) is for WIN_OS and not using any other subsystem (such as KDE, GTK, etc..)

you should add a PLATFORM(GTK) to Platform.h using a similar method to how KDE is (basically a define).

(In reply to comment #6)
> Actually I think in this case it should be
> 
> 
> #if TOOLKIT(WIN32) || TOOLKIT(GTK)
> 
> Not PLATFORM since this code is windowing toolkit specific not OS specific ?
> 

[Michael Emmel]

Okay So we can have PLATFORM(LINUX) &&  PLATFORM(GTK) to specifiy GTK on Linux
For GTK we even have more GDK_DIRECTFB AND GDK_X11 and GDK_WIN32 GDK_OSX  OSX can for example support 3 of those. Is this okay if I need one of the GTK variants ?
Similar is true for KDE and WTK. 

At this point the toolkit could provide the variant macro in fact for cairo this is true I think we have 

CAIRO_HAS_XLIB_SURFACE

Gdk has similar sub variant defines internally.

So I'd so there is not PLATFORM(GDK_DIRECTFB) since this define should be internal in the toolkit.

Now this mean the Cairo code thats started this bug should actually use

#if CAIRO_HAS_WIN32_SURFACE

Not PLATFORM but the GTK code would be PLATFORM(GTK) ?

[Mark Rowe (bdash)]

The changes in question was landed in r14807 as part of the patch for bug 8515.