Bug 23671 - PLATFORM(CF) should be set when building for Qt on Darwin
Summary: PLATFORM(CF) should be set when building for Qt on Darwin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Other
: P2 Normal
Assignee: Simon Hausmann
URL:
Keywords: Qt
Depends on:
Blocks: Qt45
  Show dependency treegraph
 
Reported: 2009-02-01 07:05 PST by Darin Adler
Modified: 2009-11-02 20:01 PST (History)
4 users (show)

See Also:


Attachments
first try. (6.24 KB, patch)
2009-10-30 20:29 PDT, Laszlo Gombos
darin: review-
Details | Formatted Diff | Diff
2nd try. (6.31 KB, patch)
2009-11-01 21:25 PST, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-02-01 07:05:35 PST
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.
Comment 1 Laszlo Gombos 2009-10-30 20:29:28 PDT
Created attachment 42249 [details]
first try.
Comment 2 Darin Adler 2009-10-31 15:50:32 PDT
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.
Comment 3 Laszlo Gombos 2009-11-01 21:25:41 PST
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.
Comment 4 WebKit Commit Bot 2009-11-02 19:24:31 PST
Comment on attachment 42299 [details]
2nd try.

Clearing flags on attachment: 42299

Committed r50438: <http://trac.webkit.org/changeset/50438>
Comment 5 WebKit Commit Bot 2009-11-02 19:24:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Mark Rowe (bdash) 2009-11-02 19:50:58 PST
(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.
Comment 7 Mark Rowe (bdash) 2009-11-02 20:01:25 PST
(In reply to comment #6)
> Making this change disables web archive support on Windows!  That’s clearly
> wrong.

I fixed this in r50439.