Bug 23671

Summary: PLATFORM(CF) should be set when building for Qt on Darwin
Product: WebKit Reporter: Darin Adler <darin>
Component: PlatformAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hausmann, laszlo.gombos, vestbo
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Other   
Bug Depends on:    
Bug Blocks: 29800    
Attachments:
Description Flags
first try.
darin: review-
2nd try. none

Darin Adler
Reported 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.
Attachments
first try. (6.24 KB, patch)
2009-10-30 20:29 PDT, Laszlo Gombos 		darin: review-
DetailsFormatted DiffDiff
2nd try. (6.31 KB, patch)
2009-11-01 21:25 PST, Laszlo Gombos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Laszlo Gombos
Comment 1 2009-10-30 20:29:28 PDT
Created attachment 42249 [details] first try.
Darin Adler
Comment 2 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.
Laszlo Gombos
Comment 3 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.
WebKit Commit Bot
Comment 4 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>
WebKit Commit Bot
Comment 5 2009-11-02 19:24:36 PST
All reviewed patches have been landed. Closing bug.
Mark Rowe (bdash)
Comment 6 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.
Mark Rowe (bdash)
Comment 7 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.
Note You need to log in before you can comment on or make changes to this bug.