Bug 42722

Summary: WebKit on Windows should build optionally with an unversioned ICU DLL
Product: WebKit Reporter: Steve Falkenburg <sfalken>
Component: New BugsAssignee: Steve Falkenburg <sfalken>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
Patch
none
Patch aroben: review+

Description Steve Falkenburg 2010-07-20 20:01:58 PDT
WebKit on Windows should build optionally with an unversioned ICU DLL
Comment 1 Steve Falkenburg 2010-07-20 20:24:26 PDT
Created attachment 62142 [details]
Patch
Comment 2 Steve Falkenburg 2010-07-20 21:47:08 PDT
Created attachment 62143 [details]
Patch
Comment 3 Mark Rowe (bdash) 2010-07-20 21:50:39 PDT
Comment on attachment 62143 [details]
Patch

Is there a reason to force the include of ICUVersion.h rather than having the project prefix header include it explicitly?
Comment 4 Steve Falkenburg 2010-07-20 22:00:59 PDT
It could probably be moved to <wtf/Platform.h>. That's where we include QuartzCorePresent.h, which is somewhat similar to this.

The reason the patch puts it in vcproj/vsprops files is that I needed to edit those files for this patch anyway, so I figured I'd keep the changes localized. This change will be rolled out once we have the new unversioned ICU in the tree.
Comment 5 Adam Roben (:aroben) 2010-07-21 04:57:22 PDT
Comment on attachment 62143 [details]
Patch

> +        Since the versioned and unversioned ICU have different filenames (libicuuc.lib vs icuuc.lib)
> +        we copy the ICU lib to an intermediate location under obj with a common name.

I think you meant "under lib".

It would be nicer to add a new ICU.vsprops file that contains the ICU-specific settings (AdditionalDependencies, AdditionalLibraryDirectories, VCPreLinkEventTool, ForcedIncludes). Then each project that links against ICU could just include it.

r=me
Comment 6 Adam Barth 2010-08-10 22:41:00 PDT
Comment on attachment 62143 [details]
Patch

Is this patch still current?
Comment 7 Adam Roben (:aroben) 2010-08-16 07:16:06 PDT
(In reply to comment #6)
> (From update of attachment 62143 [details])
> Is this patch still current?

No, it was landed in r63833.