Bug 93275 – Windows 64 bit compliance

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alex Christensen]

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

[Alex Christensen]

Created attachment 156707 [details]
Patch

[Alex Christensen]

Created attachment 156709 [details]
patch

patch

[Alex Christensen]

Created attachment 156716 [details]
Patch

[Patrick R. Gansterer]

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

[Alex Christensen]

(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.

[Alex Christensen]

> 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.

[Brent Fulgham]

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"?

[Alex Christensen]

(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

[Brent Fulgham]

(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

[Alex Christensen]

Created attachment 158031 [details]
Patch

[Brent Fulgham]

Comment on attachment 158031 [details]
Patch

Thanks for your patience revising the patch.  These look great.

[WebKit Review Bot]

Comment on attachment 158031 [details]
Patch

Clearing flags on attachment: 158031

Committed r125505: <http://trac.webkit.org/changeset/125505>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.