Bug 93275 - Windows 64 bit compliance
Summary: Windows 64 bit compliance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL: https://github.com/achristensen07/web...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-06 09:20 PDT by Alex Christensen
Modified: 2012-08-13 20:20 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.52 KB, patch)
2012-08-06 09:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
patch (4.47 KB, patch)
2012-08-06 09:37 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (2.83 KB, patch)
2012-08-06 10:00 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (3.38 KB, patch)
2012-08-13 10:09 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2012-08-06 09:20:47 PDT
Here are a few fixes from my Windows 64-bit build.  I'll post most of the others as separate bugs.  https://github.com/achristensen07/webkit64prep
Comment 1 Alex Christensen 2012-08-06 09:29:00 PDT
Created attachment 156707 [details]
Patch
Comment 2 Alex Christensen 2012-08-06 09:37:45 PDT
Created attachment 156709 [details]
patch

patch
Comment 3 Alex Christensen 2012-08-06 10:00:15 PDT
Created attachment 156716 [details]
Patch
Comment 4 Patrick R. Gansterer 2012-08-06 14:54:14 PDT
Comment on attachment 156716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156716&action=review

> Tools/WinLauncher/WinLauncher.cpp:232
> -    DefWebKitProc = reinterpret_cast<WNDPROC>(::GetWindowLongPtr(hMainWnd, GWL_WNDPROC));
> -    ::SetWindowLongPtr(hMainWnd, GWL_WNDPROC, reinterpret_cast<LONG_PTR>(WndProc));
> +    DefWebKitProc = reinterpret_cast<WNDPROC>(::GetWindowLongPtr(hMainWnd, GWLP_WNDPROC));
> +    ::SetWindowLongPtr(hMainWnd, GWLP_WNDPROC, reinterpret_cast<LONG_PTR>(WndProc));

i'd like to see usage of http://trac.webkit.org/browser/trunk/Source/WebCore/platform/win/WindowsExtras.h instead, which adds additional abstraction required for OS(WINCE).

> Tools/win/DLLLauncher/DLLLauncherMain.cpp:191
> +#ifdef _M_AMD64
> +    const char* entryPointName = "_dllLauncherEntryPoint";
> +#else
>      const char* entryPointName = "_dllLauncherEntryPoint@8";
> +#endif

isn't there a possibility to export them as extern C without any additional decoration?

> Tools/win/DLLLauncher/DLLLauncherMain.cpp:198
> +#ifdef _M_AMD64
>      const char* entryPointName = "_dllLauncherEntryPoint@16";
> +#else
> +    const char* entryPointName = "_dllLauncherEntryPoint@16";
> +#endif

they look very similar
Comment 5 Alex Christensen 2012-08-06 15:19:10 PDT
(In reply to comment #4)

> i'd like to see usage of http://trac.webkit.org/browser/trunk/Source/WebCore/platform/win/WindowsExtras.h instead, which adds additional abstraction required for OS(WINCE).

That's a good idea, but it would still need to use GWLP_WNDPROC instead of GWL_WNDPROC.

> isn't there a possibility to export them as extern C without any additional decoration?

I don't know why the symbol names are in the source code, but they don't work with the 64-bit dlls, hence the additional 64-bit symbols.

> they look very similar

They are the same.  The first shouldn't have @16.  My bad.
Comment 6 Alex Christensen 2012-08-07 11:09:10 PDT
> i'd like to see usage of http://trac.webkit.org/browser/trunk/Source/WebCore/platform/win/WindowsExtras.h instead, which adds additional abstraction required for OS(WINCE).

How does it know what #if OS(WINCE) means?  Whatever is done in WebKit to define that isn't done in WinLauncher.

> isn't there a possibility to export them as extern C without any additional decoration?

This isn't exporting, it's the parameter given to GetProcAddress to link to an already exported symbol.
Comment 7 Brent Fulgham 2012-08-08 11:14:56 PDT
Comment on attachment 156716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156716&action=review

This is a good start, but needs a few adjustments before landing.  I think Patrick's idea to use the WindowsExtras.h code is good, but I think you will need to add a static value to represent the token you want to get from GetWindowLongPtr.

>> Tools/WinLauncher/WinLauncher.cpp:232
>> +    ::SetWindowLongPtr(hMainWnd, GWLP_WNDPROC, reinterpret_cast<LONG_PTR>(WndProc));
> 
> i'd like to see usage of http://trac.webkit.org/browser/trunk/Source/WebCore/platform/win/WindowsExtras.h instead, which adds additional abstraction required for OS(WINCE).

How would using that help with GWL_WNDPROC versus GWLP_WNDPROC?  GWL_WNDPROC resolves to -4 for win32 (and to nothing in 64-bit), while GWLP_WNDPROC is -4 for 64-bit, and undefined for 32-bit.

I guess we could add a static const "s_wndProc" member to the WindowsExtras.h that would be defined as -4.

>> Tools/win/DLLLauncher/DLLLauncherMain.cpp:198
>> +#endif
> 
> they look very similar

Maybe this should be "#ifdef _M_AMD64 || _WIN64"?
Comment 8 Alex Christensen 2012-08-08 12:12:23 PDT
(In reply to comment #7)

> This is a good start, but needs a few adjustments before landing.  I think Patrick's idea to use the WindowsExtras.h code is good, but I think you will need to add a static value to represent the token you want to get from GetWindowLongPtr.

Try adding #include "WindowsExtras.h" to WinLauncher.cpp.  It doesn't know what to do with the #if OS(WINCE) directive.

> I guess we could add a static const "s_wndProc" member to the WindowsExtras.h that would be defined as -4.

GWLP_WNDPROC is defined for 32-bit and 64-bit.  GWL_WNDPROC is not defined for 64-bit.

> Maybe this should be "#ifdef _M_AMD64 || _WIN64"?

Good idea.  #if defined _M_AMD64 || defined _WIN64
Comment 9 Brent Fulgham 2012-08-08 13:37:52 PDT
(In reply to comment #8)
> (In reply to comment #7)
> 
> > This is a good start, but needs a few adjustments before landing.  I think Patrick's idea to use the WindowsExtras.h code is good, but I think you will need to add a static value to represent the token you want to get from GetWindowLongPtr.
> 
> Try adding #include "WindowsExtras.h" to WinLauncher.cpp.  It doesn't know what to do with the #if OS(WINCE) directive.

Ah, right.  That's internal to WebCore.

Why not do an #if/def around it using the _M_AMD64 and _WIN64.  WINCE won't define these, and can stick to the old way.

Then you don't need to touch WindowsExtras at all, and it should build fine on all systems.

-Brent
Comment 10 Alex Christensen 2012-08-13 10:09:52 PDT
Created attachment 158031 [details]
Patch
Comment 11 Brent Fulgham 2012-08-13 20:01:40 PDT
Comment on attachment 158031 [details]
Patch

Thanks for your patience revising the patch.  These look great.
Comment 12 WebKit Review Bot 2012-08-13 20:20:48 PDT
Comment on attachment 158031 [details]
Patch

Clearing flags on attachment: 158031

Committed r125505: <http://trac.webkit.org/changeset/125505>
Comment 13 WebKit Review Bot 2012-08-13 20:20:52 PDT
All reviewed patches have been landed.  Closing bug.