[webkit-reviews] review granted: [Bug 118888] Visual Studio files need x64 configuration : [Attachment 207057] patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 118888: Visual Studio files need x64 configuration
https://bugs.webkit.org/show_bug.cgi?id=118888

Attachment 207057: patch
https://bugs.webkit.org/attachment.cgi?id=207057&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=207057&action=review


Wow!  That was a big patch, but it looks good.	I made a few notes for future
reference; we should try to clean them up as we make actual configuration
changes to support 64-bit.

>
Source/JavaScriptCore/JavaScriptCore.vcxproj/LLInt/LLIntDesiredOffsets/LLIntDes
iredOffsets.vcxproj:105
> +    <Import Project="$(WebKit_Libraries)\tools\vsprops\common.props" />

When I did this locally, I needed to make a "common_x64.props" file.  We'll
probably need to do something like this in your second bug.

> Source/WTF/WTF.vcxproj/WTF.vcxproj.filters:217
> +    <ClCompile Include="..\wtf\text\AtomicStringTable.cpp" />

Interesting -- I wonder why this was missing before your patch?

> Source/WebKit/WebKit.vcxproj/Interfaces/Interfaces.vcxproj:185
> +	 <WarningLevel>Level3</WarningLevel>

Note: We should be setting this stuff in a property sheet!  Reminder for later
:-)

> Source/WebKit/WebKit.vcxproj/WebKitGUID/WebKitGUID.vcxproj.filters:12
> +    <ClCompile
Include="$(ConfigurationBuildDir)\obj32\WebKit\Interfaces\AccessibleRelation_i.
c" />

Good catch!  We'll need to make sure we have obj64 cases soon...

>
Tools/DumpRenderTree/DumpRenderTree.vcxproj/DumpRenderTree/DumpRenderTree.vcxpr
oj.filters:108
> +    <ClCompile Include="..\..\JavaScriptThreading.cpp" />

Huh.  Wonder why this wasn't already in the filters file?

>
Tools/DumpRenderTree/DumpRenderTree.vcxproj/DumpRenderTree/DumpRenderTreeLaunch
er.vcxproj:171
> +	 <AdditionalOptions>/SAFESEH %(AdditionalOptions)</AdditionalOptions>

Seems like this should be in a property sheet.	Note for future work!