Bug 30909 – 10.6 SDK building for 10.5: GraphicsContextCG.cpp:1056: warning: enumeration value 'kCGInterpolationMedium' not handled in switch

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mark Mentovai]

While building WebCore/platform/graphics/cg/GraphicsContextCG.cpp with the Mac OS X 10.6 SDK but targeting Mac OS X 10.5:

cc1plus: warnings being treated as errors
/chrome/trunk/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/cg/GraphicsContextCG.cpp: In member function 'WebCore::InterpolationQuality WebCore::GraphicsContext::imageInterpolationQuality() const':
/chrome/trunk/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/cg/GraphicsContextCG.cpp:1056: warning: enumeration value 'kCGInterpolationMedium' not handled in switch

[Mark Mentovai]

Created attachment 42104 [details]
Provide a default: switch label

[Eric Seidel (no email)]

Better would be to add all the missing values to the enum.  Even if you have to wrap them in #if.

[Mark Mentovai]

Created attachment 42128 [details]
Provide a case kCGInterpolationMedium: label if the 10.6 SDK is in use

I don't like this as much because it gratuitously makes codegen dependent on the SDK selected.

Here it is if you prefer it.

[Mark Mentovai]

Created attachment 42129 [details]
Provide a case kCGInterpolationMedium: label if the 10.6 SDK is in use

Fixed ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 42129 [details]
Provide a case kCGInterpolationMedium: label if the 10.6 SDK is in use

Looks good to me.  The goal is to avoid using default: as it masks warnings like this which are  often useful (although perhaps not in this case).

[Eric Seidel (no email)]

Comment on attachment 42129 [details]
Provide a case kCGInterpolationMedium: label if the 10.6 SDK is in use

I wonder if the HAVE(CG_INTERPOLATION_MEDIUM) definition check should have been altered instead (in platform.h), but this looks fine to me.

[Eric Seidel (no email)]

CCing mark rowe who made the original CGIngerpolationMedium change:
http://trac.webkit.org/changeset/35869

[Mark Rowe (bdash)]

Comment on attachment 42129 [details]
Provide a case kCGInterpolationMedium: label if the 10.6 SDK is in use

We shouldn’t be sprinkling tests of Mac OS X availability macros throughout the code like this.

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 42129 [details])
> We shouldn’t be sprinkling tests of Mac OS X availability macros throughout the
> code like this.

I'm curious what your suggested alternative is.  Would you prefer the "default:" approach?

[Mark Rowe (bdash)]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 42129 [details] [details])
> > We shouldn’t be sprinkling tests of Mac OS X availability macros throughout the
> > code like this.
> 
> I'm curious what your suggested alternative is.  Would you prefer the
> "default:" approach?

One approach could be to do what we do already with Mac OS X availability macros and confine their use to a single, central location.

[Eric Seidel (no email)]

This particular HAVE_ is defined at the top of the file:
http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/cg/GraphicsContextCG.cpp#L46

But Mark could certainly define a second HAVE_ macro there.  If that is a solution you'd be comfortable with, I'm sure he'd be happy to move forward with such.

[Mark Mentovai]

This sounds like a job for a HAVE_ and a USE_ macro.  Is that something everyone can get on board with?

[Mark Rowe (bdash)]

(In reply to comment #12)
> This sounds like a job for a HAVE_ and a USE_ macro.  Is that something
> everyone can get on board with?

I’m not sure how what you’re trying to do fits in to the HAVE_ and USE_ style. Can you elaborate?

[Mark Mentovai]

The key problem here is that WebCore doesn't typically seem to distinguish between SDK (MAX_ALLOWED) and deployment target (MIN_REQUIRED).  I suspect that this is because within the Apple build, the SDK and the deployment target are always (or usually) equal.  That's not always the case, though.

I think what we have here is a case where we really want to say:

If the deployment target >= 10.6, then USE_CGINTERPOLATION_MEDIUM.  I think that this is undisputed.

The problem occurs when we don't fall into that configuration, but the SDK >= 10.6.  In that case, we HAVE_CGINTERPOLATION_MEDIUM, but we shouldn't USE_CGINTERPOLATION_MEDIUM.

USE_CGINTERPOLATION_MEDIUM implies HAVE_CGINTERPOLATION_MEDIUM (but the availability macros take care of this for you.)

USE_CGINTERPOLATION_MEDIUM determines whether you actually want to use kCGInterpolationMedium.  If !USE_CGINTERPOLATION_MEDIUM, you don't want to map anything TO kCGInterpolationMedium in setImageInterpolationQuality().

HAVE_CGINTERPOLATION_MEDIUM tells you if the constant exists in the headers you're building against.  If HAVE_CGINTERPOLATION_MEDIUM, in the switch block in imageInterpolationQuality(), you need to handle kCGInterpolationMedium, or provide a default case, even if !USE_CGINTERPOLATION_MEDIUM.  Obviously, if !HAVE_CGINTERPOLATION_MEDIUM, you can't do anything with it at all, because it doesn't exist.

Is that clearer?

[Mark Rowe (bdash)]

(In reply to comment #14)
> The key problem here is that WebCore doesn't typically seem to distinguish
> between SDK (MAX_ALLOWED) and deployment target (MIN_REQUIRED).  I suspect that
> this is because within the Apple build, the SDK and the deployment target are
> always (or usually) equal.  That's not always the case, though.

The existing code, and the version checks within, is written to the assumption that the binary will run on the same OS version that it was built on.  Bug 30526 is an attempt to move away from that assumption in a more general fashion.

> Is that clearer?

Thanks.  This doesn’t match any of our existing HAVE_ / USE_ macros but it seems sufficiently obvious that it is a good way to approach this particular problem.

[Mark Mentovai]

(In reply to comment #15)
> Thanks.  This doesn’t match any of our existing HAVE_ / USE_ macros but it
> seems sufficiently obvious that it is a good way to approach this particular
> problem.

OK.  So in advance of bug 30526 landing, where would you like the macros to live?  I'm inclined to just move the to the top of GraphicsContextCG.cpp because that's where the existing macro lives, but I'm happy to put them anywhere you'd like.

[Mark Mentovai]

bdash?  It'd be nice if you could validate what I suggested in comment 16 before I throw it out there to you.  You expressed concern about putting availability macros in this file.  If you don't want them in here, where would you like me to put them until bug 30526 makes the world better for everyone?

[Mark Mentovai]

Created attachment 42405 [details]
Provide a case kCGInterpolationMedium: label and put the defines in config.h

Let's try this one, then?

[Mark Mentovai]

Created attachment 42406 [details]
Provide a case kCGInterpolationMedium: label and put the defines in config.h

[Darin Adler]

Comment on attachment 42406 [details]
Provide a case kCGInterpolationMedium: label and put the defines in config.h

This seems to be getting out of hand. Is this the only place we have to do something like this? I expect this sort of thing to come up in many other places as well -- WebKit historically has not supported being built with SDKs and cross compiling. If we follow this pattern, config.h is going to get pretty ridiculous.

Unless this really is the only case like this, in which case the complexity might be OK. But I still think the conditionals and complexity should be limited to GraphicsContextCG.cpp if possible.

[Mark Mentovai]

Created attachment 42410 [details]
Check MAC_OS_X_VERSION_MAX_ALLOWED and MAC_OS_X_VERSION_MIN_REQUIRED directly

[Eric Seidel (no email)]

Comment on attachment 42410 [details]
Check MAC_OS_X_VERSION_MAX_ALLOWED and MAC_OS_X_VERSION_MIN_REQUIRED directly

I'm sorry this simple change has taken so much back-and-forth.  I think this patch looks good.  Although personally I would prefer the HAVE/USE stuff to be defined only in the CG file, since it makes little sense for all ports to see it (Which they do with it in config.h).

+    // Fall through to InterpolationHigh if kCGInterpolationMedium is not usable
should have a period after a full sentence.

Anyway, LGTM, but I would like to see the defines you added to config.h moved back to platform/graphics/cg/GraphicsContextCG.cpp instead.  I'll mark r+ and let you land this manually with modifications.  Mark or Darin can give further comments/instructions if they disagree with that approach.

[Mark Rowe (bdash)]

Mark and I have been discussing this on IRC for a while now and I think we’ve settled on a solution that keeps the CG-specific bits in one file while keeping the Mac OS X availability macros out of the way.

[Mark Mentovai]

Created attachment 42418 [details]
Add TARGETING_TIGER and TARGETING_LEOPARD

[Mark Rowe (bdash)]

Comment on attachment 42418 [details]
Add TARGETING_TIGER and TARGETING_LEOPARD

> Index: WebCore/platform/graphics/cg/GraphicsContextCG.cpp
> ===================================================================
> --- WebCore/platform/graphics/cg/GraphicsContextCG.cpp	(revision 50468)
> +++ WebCore/platform/graphics/cg/GraphicsContextCG.cpp	(working copy)
> @@ -43,10 +43,16 @@
>  #include <wtf/OwnArrayPtr.h>
>  #include <wtf/RetainPtr.h>
>  
> -#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> +#if PLATFORM(DARWIN) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)

CoreGraphics is well above the Darwin layer so it’s odd to suggest that Darwin is somehow relevant to this decision.

> @@ -1061,9 +1067,15 @@ InterpolationQuality GraphicsContext::im
>      case kCGInterpolationLow:
>          return InterpolationLow;
>  #if HAVE(CG_INTERPOLATION_MEDIUM)
> +    // kCGInterpolationMedium is known to be present in the CGInterpolationQuality enum.
>      case kCGInterpolationMedium:
> +#if USE(CG_INTERPOLATION_MEDIUM)
> +        // Only map to InterpolationMedium if targeting a system that understands it.
>          return InterpolationMedium;
> -#endif
> +#else
> +        return InterpolationDefault;
> +#endif  // USE(CG_INTERPOLATION_MEDIUM)
> +#endif  // HAVE(CG_INTERPOLATION_MEDIUM)
>      case kCGInterpolationHigh:
>          return InterpolationHigh;
>      }

It’s surprising to me that the !USE(CG_INTERPOLATION_MEDIUM) case returns InterpolationDefault while !HAVE(CG_INTERPOLATION_MEDIUM) returns InterpolationHigh.  Is there some reason for that difference?

[Mark Mentovai]

(In reply to comment #25)
> CoreGraphics is well above the Darwin layer so it’s odd to suggest that Darwin
> is somehow relevant to this decision.

The original code used PLATFORM(MAC), which suggested that kCGInterpolationMedium is not available on Windows.  It also suggested that kCGInterpolationMedium was not available to Mac Chromium, which was an incorrect assumption.

Since this is a CG file, it's being built on Darwin, I figured it'd be safe to assume that the presence and usability of kCGInterpolationMedium could be gated on the headers and the target.  If building on Darwin but not PLATFORM(MAC) and not USE(CG), then this file won't be seen at all.

Here's another option:

#if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)

but it's starting to get really verbose there, and it's still not strictly right, because there might be other "platforms" beyond MAC and CHROMIUM+DARWIN that run on the Mac and use Mac-style CG.

This is a pattern that comes up for Chromium a lot, and it seems like we need to rehash it each and every time, without any real consensus on how to treat it.  What do you prefer, Mark?

> > @@ -1061,9 +1067,15 @@ InterpolationQuality GraphicsContext::im
> >      case kCGInterpolationLow:
> >          return InterpolationLow;
> >  #if HAVE(CG_INTERPOLATION_MEDIUM)
> > +    // kCGInterpolationMedium is known to be present in the CGInterpolationQuality enum.
> >      case kCGInterpolationMedium:
> > +#if USE(CG_INTERPOLATION_MEDIUM)
> > +        // Only map to InterpolationMedium if targeting a system that understands it.
> >          return InterpolationMedium;
> > -#endif
> > +#else
> > +        return InterpolationDefault;
> > +#endif  // USE(CG_INTERPOLATION_MEDIUM)
> > +#endif  // HAVE(CG_INTERPOLATION_MEDIUM)
> >      case kCGInterpolationHigh:
> >          return InterpolationHigh;
> >      }
> 
> It’s surprising to me that the !USE(CG_INTERPOLATION_MEDIUM) case returns
> InterpolationDefault while !HAVE(CG_INTERPOLATION_MEDIUM) returns
> InterpolationHigh.  Is there some reason for that difference?

I don't think that's the case.

Given kCGInterpolationMedium:
HAVE  USE    RESULT
true  true   InterpolationMedium
true  false  InterpolationDefault
false true   InterpolationDefault
false false  InterpolationDefault

I think you may be taking the patch out of context.  If !HAVE, and kCGInterpolationMedium is encountered at runtime, none of the cases in this switch will be matched, and flow will fall through to the |return InterpolationDefault;| outside of the switch body.  This is not shown in the diff, which missed it by one line.

[Mark Rowe (bdash)]

(In reply to comment #26)
> (In reply to comment #25)
> > CoreGraphics is well above the Darwin layer so it’s odd to suggest that Darwin
> > is somehow relevant to this decision.
> 
> The original code used PLATFORM(MAC), which suggested that
> kCGInterpolationMedium is not available on Windows.

It’s not at the moment though it will be in the future.

> Since this is a CG file, it's being built on Darwin, I figured it'd be safe to
> assume that the presence and usability of kCGInterpolationMedium could be gated
> on the headers and the target.  If building on Darwin but not PLATFORM(MAC) and
> not USE(CG), then this file won't be seen at all.

I’m not sure what you’re trying to say here.

> Here's another option:
> 
> #if (PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)) &&
> !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> 
> but it's starting to get really verbose there, and it's still not strictly
> right, because there might be other "platforms" beyond MAC and CHROMIUM+DARWIN
> that run on the Mac and use Mac-style CG.
> 
> This is a pattern that comes up for Chromium a lot, and it seems like we need
> to rehash it each and every time, without any real consensus on how to treat
> it.  What do you prefer, Mark?

I’d prefer that this was handled in a manner consistent with how this has been handled in other places.  This will allow us to easily update the code at a point in the future when we revisit how the platform defines are handled.  Something like:

#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
// Building on 10.6: kCGInterpolationMedium is defined in the CGInterpolationQuality enum.
#define HAVE_CG_INTERPOLATION_MEDIUM 1
#endif

#if !defined(TARGETING_TIGER) && !defined(TARGETING_LEOPARD)
// Targeting 10.6: use kCGInterpolationMedium.
#define WTF_USE_CG_INTERPOLATION_MEDIUM 1
#endif

#endif

Which reminds me… The “Building on 10.6” and “Targeting 10.6” comments are too specific.  Given how the BUILDING_ON_ and TARGETING_ macros work this code is not specific to 10.6, but is for all OS versions >= 10.6.

> I don't think that's the case.

You’re right.  I misread the code.

So overall this looks fine, but we need to get the #if’s straightened out.

[Mark Mentovai]

Created attachment 42531 [details]
Final version

[Mark Rowe (bdash)]

Comment on attachment 42531 [details]
Final version

Yay!

[WebKit Commit Bot]

Comment on attachment 42531 [details]
Final version

Clearing flags on attachment: 42531

Committed r50546: <http://trac.webkit.org/changeset/50546>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.