Bug 23671 – PLATFORM(CF) should be set when building for Qt on Darwin

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Adler]

We discussed on the mailing list setting PLATFORM(CF) when building for Qt on Darwin. This will change lines like this:

    #if PLATFORM(CF) || (PLATFORM(QT) && PLATFORM(DARWIN))

to this:

    #if PLATFORM(CF)

And we'll also have to make sure the #if statements are in the right order in any files that check for both CF and QT.

[Laszlo Gombos]

Created attachment 42249 [details]
first try.

[Darin Adler]

Comment on attachment 42249 [details]
first try.

> -#if PLATFORM(CF)
> +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)
>  #include "LegacyWebArchive.h"
>  #endif

This will change behavior for Chromium builds that are targeting Windows and Unix, so I think it's incorrect.

I'm not an expert on Chromium ifdefs, but I assume that something along the lines of:

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

Or:

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

would do. Maybe the best way to write it is:

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

Otherwise, the patch looks good.

[Laszlo Gombos]

Created attachment 42299 [details]
2nd try.

Thanks Darin for the review. I overlooked the test in ArchiveFactory.cpp - thanks for the correction.
Made the change in the patch as last suggested.

[WebKit Commit Bot]

Comment on attachment 42299 [details]
2nd try.

Clearing flags on attachment: 42299

Committed r50438: <http://trac.webkit.org/changeset/50438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

(In reply to comment #2)
> (From update of attachment 42249 [details])
> > -#if PLATFORM(CF)
> > +#if PLATFORM(MAC) || PLATFORM(CHROMIUM)
> >  #include "LegacyWebArchive.h"
> >  #endif
> 
> This will change behavior for Chromium builds that are targeting Windows and
> Unix, so I think it's incorrect.
> 
> I'm not an expert on Chromium ifdefs, but I assume that something along the
> lines of:
> 
>     #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
> 
> Or:
> 
>     #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(CF))
> 
> would do. Maybe the best way to write it is:
> 
>     #if PLATFORM(CF) && (PLATFORM(MAC) || PLATFORM(CHROMIUM))
> 
> Otherwise, the patch looks good.

Making this change disables web archive support on Windows!  That’s clearly wrong.

[Mark Rowe (bdash)]

(In reply to comment #6)
> Making this change disables web archive support on Windows!  That’s clearly
> wrong.

I fixed this in r50439.