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.
Created attachment 42249 [details] first try.
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.
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 on attachment 42299 [details] 2nd try. Clearing flags on attachment: 42299 Committed r50438: <http://trac.webkit.org/changeset/50438>
All reviewed patches have been landed. Closing bug.
(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.
(In reply to comment #6) > Making this change disables web archive support on Windows! That’s clearly > wrong. I fixed this in r50439.