Bug 35607 - Allow building smoothly on win32 and win64 using GCC
Summary: Allow building smoothly on win32 and win64 using GCC
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 34922 (view as bug list)
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-03-02 14:33 PST by Fridrich Strba
Modified: 2010-04-27 03:09 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.09 KB, patch)
2010-03-02 14:36 PST, Fridrich Strba
no flags Details | Formatted Diff | Diff
split part of the previos patch for JavaScriptCore (3.60 KB, patch)
2010-03-02 14:51 PST, Fridrich Strba
no flags Details | Formatted Diff | Diff
split part of the previous patch for WebCore (5.50 KB, patch)
2010-03-02 14:51 PST, Fridrich Strba
no flags Details | Formatted Diff | Diff
Modified patch for WebCore according to discussion in this bug (6.20 KB, patch)
2010-03-03 09:35 PST, Fridrich Strba
no flags Details | Formatted Diff | Diff
patch with fixes required by QtWebkit for mingw64 -v4 (9.52 KB, patch)
2010-03-25 14:42 PDT, vanboxem.ruben
no flags Details | Formatted Diff | Diff
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore (3.85 KB, patch)
2010-03-26 01:26 PDT, vanboxem.ruben
oliver: review-
Details | Formatted Diff | Diff
patch with fixes required by QtWebkit for mingw64 -WebCore (5.75 KB, patch)
2010-03-26 01:30 PDT, vanboxem.ruben
no flags Details | Formatted Diff | Diff
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore (3.86 KB, patch)
2010-03-30 14:38 PDT, vanboxem.ruben
no flags Details | Formatted Diff | Diff
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore (3.85 KB, patch)
2010-04-02 11:46 PDT, vanboxem.ruben
no flags Details | Formatted Diff | Diff
patch with fixes required by QtWebkit for mingw64 -WebCore (5.74 KB, patch)
2010-04-02 11:46 PDT, vanboxem.ruben
aroben: review-
Details | Formatted Diff | Diff
WebCore patch for mingw-w64 (5.80 KB, patch)
2010-04-18 11:19 PDT, vanboxem.ruben
aroben: review-
Details | Formatted Diff | Diff
WebCore patch for mingw-w64 (6.12 KB, patch)
2010-04-26 06:55 PDT, vanboxem.ruben
commit-queue: commit-queue-
Details | Formatted Diff | Diff
WebCore patch for mingw-w64 (updated CPU<->PLATFORM) (6.11 KB, patch)
2010-04-26 07:24 PDT, vanboxem.ruben
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fridrich Strba 2010-03-02 14:33:07 PST
Webkit GTK+ and QT does not build smoothly on win32 and win64 using GCC compiler and different header-sets and runtimes out there (http://mingw-w64.sf.net and http://www.mingw.org). The following patch is solving the issues and allows basically to build Webkit GTK+ and QT on both win32 and win64 using GCC (MinGW).
Comment 1 Fridrich Strba 2010-03-02 14:36:26 PST
Created attachment 49850 [details]
Patch
Comment 2 Fridrich Strba 2010-03-02 14:37:29 PST
Comment on attachment 49850 [details]
Patch

This patch fixes some build issues on win32 and especially on win64 with MinGW (GCC) compilers.
Comment 3 Fridrich Strba 2010-03-02 14:51:04 PST
Created attachment 49852 [details]
split part of the previos patch for JavaScriptCore
Comment 4 Fridrich Strba 2010-03-02 14:51:43 PST
Created attachment 49853 [details]
split part of the previous patch for WebCore
Comment 5 Oliver Hunt 2010-03-02 14:53:20 PST
Comment on attachment 49852 [details]
split part of the previos patch for JavaScriptCore

r=me
Comment 6 Adam Roben (:aroben) 2010-03-02 15:03:46 PST
I don't think the OS(WINDOWS) checks are necessarily correct. Should we be using COMPILER(MINGW) more instead?
Comment 7 Fridrich Strba 2010-03-02 15:27:56 PST
Adam, what I tried to do was to put OS(WINDOWS) whenever the stuff was related to how the behaviour of functions in MS CRT is or if the functions called were part of the Win32 API and thus independent from the compiler. Whenever there were differences between different compilers and even different header-sets, I tried to be granular in defines. But I don't mind replacing the different OS(WINDOWS) by COMPILER(MSVC) || COMPILER(MINGW), just that I don't know about compiler out there that builds for windows but distributes its own runtime.
Comment 8 WebKit Commit Bot 2010-03-03 00:07:53 PST
Comment on attachment 49852 [details]
split part of the previos patch for JavaScriptCore

Clearing flags on attachment: 49852

Committed r55453: <http://trac.webkit.org/changeset/55453>
Comment 9 Adam Roben (:aroben) 2010-03-03 05:58:03 PST
(In reply to comment #7)
> Adam, what I tried to do was to put OS(WINDOWS) whenever the stuff was related
> to how the behaviour of functions in MS CRT is or if the functions called were
> part of the Win32 API and thus independent from the compiler. Whenever there
> were differences between different compilers and even different header-sets, I
> tried to be granular in defines. But I don't mind replacing the different
> OS(WINDOWS) by COMPILER(MSVC) || COMPILER(MINGW), just that I don't know about
> compiler out there that builds for windows but distributes its own runtime.

Thanks for the explanation, Fridrich. This sounds fine to me.
Comment 10 Adam Roben (:aroben) 2010-03-03 05:59:00 PST
Comment on attachment 49853 [details]
split part of the previous patch for WebCore

> +++ WebCore/platform/Arena.h	(working copy)
> @@ -44,7 +44,11 @@
>  
>  namespace WebCore {
>  
> +#if OS(WINDOWS) && CPU(X86_64)
> +typedef unsigned long long uword;
> +#else
>  typedef unsigned long uword;
> +#endif

Why do we need this different definition for 64-bit on Windows, but not for 64-bit on Mac?

The rest of this patch looks good.
Comment 11 Fridrich Strba 2010-03-03 08:17:45 PST
Adam, the fact is that since Mac is based on a Unix system, I assume that long is 32 bit on 32 bit system and 64 bit on 64 bit system. As normally all Unices use the LP64 model.
On windows, long is 32bit on both 32-bit and 64-bit windows whereas the 64-bit type is long long or __int64. Long long is built-in type for both MSVC since .NET 2003 and for gcc.
This is just to assure that the type there is able to hold a pointer because we cast a pointer into it at a certain point of time.
Comment 12 Darin Adler 2010-03-03 08:24:49 PST
(In reply to comment #11)
> Adam, the fact is that since Mac is based on a Unix system, I assume that long
> is 32 bit on 32 bit system and 64 bit on 64 bit system. As normally all Unices
> use the LP64 model.
> On windows, long is 32bit on both 32-bit and 64-bit windows whereas the 64-bit
> type is long long or __int64. Long long is built-in type for both MSVC since
> .NET 2003 and for gcc.
> This is just to assure that the type there is able to hold a pointer because we
> cast a pointer into it at a certain point of time.

We should use uintptr_t then, instead of all the ifdefs.
Comment 13 Fridrich Strba 2010-03-03 08:37:25 PST
Adam, yeah, that would be nice, just that I have the impression that some older visual studio versions did not have it. If though the uintptr_t compiles on all platforms that you people and the google people care about, I am more then happy to have less ifdefs in the code.
Comment 14 Darin Adler 2010-03-03 08:43:15 PST
(In reply to comment #13)
> Adam, yeah, that would be nice, just that I have the impression that some older
> visual studio versions did not have it.

That was me, not Adam.

I don’t think WebKit compiles under these “older visual studio versions” and if we did want it to, we could take the approach I describe below.

> If though the uintptr_t compiles on all
> platforms that you people and the google people care about, I am more then
> happy to have less ifdefs in the code.

The uintptr_t type is already used in 21 different source files in JavaScriptCore and WebCore.

If we encounter a deficient platform where the type is missing, we'll typically patch around that with a header in WTF, the way we do for functions like round in <wtf/MathExtras.h> for example, or in a global include file such as "config.h" or something included by "config.h".
Comment 15 Fridrich Strba 2010-03-03 08:44:55 PST
Darin, I am more then happy with uintptr_t. Is it possible to fix it when landing or do I have to make the whole rediff?
Comment 16 Darin Adler 2010-03-03 08:51:17 PST
(In reply to comment #15)
> Darin, I am more then happy with uintptr_t. Is it possible to fix it when
> landing or do I have to make the whole rediff?

Yes, we need to post a new patch so that things like the early warning system have a chance to chew on the patch. You make it sound like a major chore, but it should be straightforward.
Comment 17 Fridrich Strba 2010-03-03 08:53:15 PST
Darin, the only limitation this thing could have is that the Arena.h does not include any header, so basically only compiler builtin types can be used there. I am just wondering whether landing the patch as it is and fixing later would not be preferable. The uintptr_t is in different headers on different platforms and not sure whether we would not need a new configure check and so on ???
Comment 18 Darin Adler 2010-03-03 08:59:30 PST
(In reply to comment #17)
> Darin, the only limitation this thing could have is that the Arena.h does not
> include any header, so basically only compiler builtin types can be used there.

It can include whatever headers are needed.

> I am just wondering whether landing the patch as it is and fixing later would
> not be preferable.

Absolutely not.

> The uintptr_t is in different headers on different platforms
> and not sure whether we would not need a new configure check and so on ???

No, we do not need a new configure check. To use uintptr_t we just need to include <stdint.h>, and that will work on all platforms.

Do not add an #if for this.
Comment 19 Fridrich Strba 2010-03-03 09:01:03 PST
ok, rediffing now with that change
Comment 20 Fridrich Strba 2010-03-03 09:35:36 PST
Created attachment 49916 [details]
Modified patch for WebCore according to discussion in this bug
Comment 21 Fridrich Strba 2010-03-03 09:37:18 PST
Darin, I will though need to see whether this builds on your platforms. Could you please fix minor issues that I cannot test here?
Comment 22 Darin Adler 2010-03-03 10:02:31 PST
(In reply to comment #21)
> Darin, I will though need to see whether this builds on your platforms. Could
> you please fix minor issues that I cannot test here?

Maybe you misunderstand how this works. While I do have time to make a few comments on your patch I don't have time to install it on various platforms myself and test it. Perhaps someone else does.
Comment 23 Eric Seidel (no email) 2010-03-03 10:29:55 PST
The EWS (early warning system) bots (the bubbles below your patch link) will show whether it builds on various platforms.  Unfortunately because you're not a committer yet, the Mac EWS will not run (and will stay purple -- meaning it couldn't run, but that the patch isn't necessarily broken).
Comment 24 Fridrich Strba 2010-03-04 10:22:14 PST
Anything else to modify in this patch?
Comment 25 Jesus Sanchez-Palencia 2010-03-15 08:14:06 PDT
Could you please take a look at https://bugs.webkit.org/show_bug.cgi?id=36118


Thanks
Comment 26 Fridrich Strba 2010-03-15 12:52:24 PDT
Adam, would it be possible to review the remaining patch? I modified everything according the discussion in this issue and now I am kind of stuck not knowing the way to give it a forward kick.
Comment 27 Eric Seidel (no email) 2010-03-15 13:06:05 PDT
Adam Roben or Steve Falkenburg are good reviewers for windows patches.  cc'd.
Comment 28 vanboxem.ruben 2010-03-23 03:12:20 PDT
Hi, I've been working on the same problem in bug https://bugs.webkit.org/show_bug.cgi?id=34922 and have some remarks to this patch:
I'll be talking about mingw64 (=mingw-w64/w32) and mingw.org (=old MinGW)

 - OS(WINDOWS) && CPU(X86_64) is not garanteed to be only mingw-w64 in the future. Mingw.org could make a 64-bit compiler and not implement several functions provided by mingw64.
 - !defined(__MINGW64_VERSION_MAJOR) is against WebKit style, you should use a custom COMPILER define (like I did in my patch).
 - You missed a lot of code by the way, including stuff in JavaScriptCore.

I find it surprising that this patch was accepted with above issues present. Please reconsider. If you need it, I can upload my patch here, adapted to the modifications made here (mine dates from before this patch, only a problem in the JavaScriptCore portion of the code.

Thanks
Comment 29 Benjamin Poulain 2010-03-24 14:32:55 PDT
*** Bug 34922 has been marked as a duplicate of this bug. ***
Comment 30 Fridrich Strba 2010-03-24 22:42:15 PDT
Several comments:
If I made OS(WINDOWS) && CPU(X86_64), this means that the define is not linked to a particular compiler, but to a way how windows internals are on x64. For instance to get the thread information about the current thread.
Concerning the use of ifdef __MINGW64_VERSION_MAJOR, I agree that your solution in JavaScriptCore/wtf/Platform.h is more elegant and preferable
Concerning the parts that I left out: I did only put into this patch the stuff that I was using to build the Gtk+ port of webkit. I have still some patches for qt-4.6.2's version of WebKit and JavaScriptCore, but was not really feeling such an urge to try to push them up.
Please, move your last patch to this issue and let us work together to get it upstreamed.
Comment 31 vanboxem.ruben 2010-03-25 14:42:34 PDT
Created attachment 51678 [details]
patch with fixes required by QtWebkit for mingw64 -v4

Here's my additions/patches for your patch (with the COMPILER(MINGW64) stuff). I can remove the check on line 554 of Collector.cpp for the compiler, but I left it in for now, as it was originally there, probably for a good reason (Intel Compiler maybe?). There is no reason to assume another win64 compiler provides the functions used in that code block. I hope I got everything and didn't mess up :)

Thanks for being so open-minded :)
Comment 32 Fridrich Strba 2010-03-26 00:42:05 PDT
Ruben, I would have two things:
1) Split the patch please for WebCore and JavaScriptCore, because different people will review the two.
2) Please, use LONG_PTR instead of intptr_t for casts in function SetWindowLongPtr. Like that we avoid some problems if the underlying type of intptr_t and LONG_PTR is not the same in some implementation (it is possible to have the first int and the second long on some 32-bit systems).
Apart that, I would say that it is ok.
Comment 33 Fridrich Strba 2010-03-26 01:07:15 PDT
I looked a bit closer and another two things:
3) In WebCore/platform/text/TextStream.cpp and WebCore/platform/text/TextStream.h, please remove the " && (COMPILER(MSVC) || COMPILER(MINGW64))
"
It has nothing to do with the compiler, just with the size of pointers in on x64 windows. Whichever compiler will be used will need a 64-bit version of this function.
4) In JavaScriptCore/runtime/Collector.cpp, please remove the  && (COMPILER(MSVC) || COMPILER(MINGW64)).
Reason: The NtCurrentTeb() is not depending on any compiler. It is a standard entry point in ntdll.dll found already in Windows XP x64 (I don't have any older version of x64 system). This is the reliable way of getting the stack base on x64 platform. A Win32 API implementation that does not have that function defined in winnt.h and and the symbol in ntdll.dll import library is better off not to try to do x64 compilation because without the information about the stack base, webkit will not work, so if the compilation breaks there, it is a good thing :)
Comment 34 vanboxem.ruben 2010-03-26 01:26:54 PDT
Created attachment 51718 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore
Comment 35 vanboxem.ruben 2010-03-26 01:30:50 PDT
Created attachment 51719 [details]
patch with fixes required by QtWebkit for mingw64 -WebCore

Here's the split patches with the requested modifications. I did not know about the ntcurrentteb function being standard, and there was a compiler check present, that's why I left it in there. Thanks for the explanation.
Comment 36 Fridrich Strba 2010-03-26 01:34:09 PDT
Ruben: No problem. There are some restrictive checks like this in WebKit. Thanks for splitting the patches. Let us make now the patches to go up :)
Comment 37 vanboxem.ruben 2010-03-30 02:47:42 PDT
Is there any progress on this?
Comment 38 Oliver Hunt 2010-03-30 03:36:38 PDT
Comment on attachment 51718 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

no space after # in preprocessor directives, no space before the ('s either -- for nesting preprocessor directives we indent the entire line, not the content after the #

Otherwise looks fine
Comment 39 vanboxem.ruben 2010-03-30 14:38:42 PDT
Created attachment 52085 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

Indentation fixed, please also check linew 517-526 (not modified in my patch) because they go against your indentation guidelines...
Comment 40 Eric Seidel (no email) 2010-04-02 11:42:55 PDT
Comment on attachment 52085 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

LGTM.
Comment 41 Eric Seidel (no email) 2010-04-02 11:43:29 PDT
Comment on attachment 52085 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

Hmm... Please put your real name in the ChangeLogs.  You may need to set CHANGELOG_NAME in your environment.  See prepare-ChangeLog for more info.
Comment 42 vanboxem.ruben 2010-04-02 11:46:02 PDT
Created attachment 52432 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore
Comment 43 vanboxem.ruben 2010-04-02 11:46:58 PDT
Created attachment 52433 [details]
patch with fixes required by QtWebkit for mingw64 -WebCore

Real name in place in both Changelogs.
Comment 44 Eric Seidel (no email) 2010-04-02 11:49:22 PDT
Comment on attachment 52432 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

Do we have these patches on two bugs?
Comment 45 Eric Seidel (no email) 2010-04-02 12:01:53 PDT
No, I just got confused.  You happen to be uploading patches right as I was reviewing. :)
Comment 46 vanboxem.ruben 2010-04-02 12:04:15 PDT
Yeah, When I'm behind my laptop, I get these things out fast. All I changed was the name of *me* in the changelog :)
Comment 47 WebKit Commit Bot 2010-04-02 14:53:15 PDT
Comment on attachment 52432 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

Clearing flags on attachment: 52432

Committed r57025: <http://trac.webkit.org/changeset/57025>
Comment 48 Eric Seidel (no email) 2010-04-06 23:42:58 PDT
Comment on attachment 52085 [details]
patch with fixes required by QtWebkit for mingw64 - JavaScriptCore

Cleared Eric Seidel's review+ from obsolete attachment 52085 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 49 vanboxem.ruben 2010-04-11 07:46:21 PDT
Has someone had the time to check the WebCore part of the patch? Thanks!
Comment 50 Adam Roben (:aroben) 2010-04-15 12:14:15 PDT
Comment on attachment 52433 [details]
patch with fixes required by QtWebkit for mingw64 -WebCore

> +++ WebCore/ChangeLog	(working copy)
> @@ -23807,6 +23807,26 @@
>          (WebCore::GraphicsContext::fillRect): Ditto
>          (WebCore::GraphicsContext::strokeRect): Ditto
>  
> +2010-03-25  Ruben Van Boxem  <vanboxem.ruben@gmail.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Mingw-w64 fixes for WebCore
> +        https://bugs.webkit.org/show_bug.cgi?id=35607
> +
> +        No new tests.
> +
> +        * platform/Arena.h: fix pointer precision loss error with mingw-w64
> +        * platform/graphics/transforms/TransformationMatrix.h: let mingw64 use msvc codepath
> +        * platform/text/TextStream.cpp: let mingw-w64 use msvc codepath
> +        * platform/text/TextStream.h: let mingw-w64 use msvc codepath
> +        * plugins/PluginView.cpp: fix pointer precision loss error with mingw-w64
> +        (WebCore::PluginView::stop):
> +        * plugins/win/PluginViewWin.cpp: let mingw-w64 use msvc codepath
> +        (WebCore::PluginView::paintIntoTransformedContext):
> +        (WebCore::PluginView::setNPWindowRect):
> +        (WebCore::PluginView::platformStart):
> +

You should put your entry at the top of the ChangeLog file, and update the date.

> +++ WebCore/platform/Arena.h	(working copy)
> @@ -44,7 +44,7 @@
>  
>  namespace WebCore {
>  
> -typedef unsigned long uword;
> +typedef uintptr_t uword;

I know that some uses of uword are meant to hold pointers, but what about ArenaPool::mask?

> -#if OS(WINDOWS) && PLATFORM(X86_64) && COMPILER(MSVC)
> +#if OS(WINDOWS) && CPU(X86_64)
>  TextStream& TextStream::operator<<(__int64 i)
>  {
>      char buffer[printBufferSize];
> Index: WebCore/platform/text/TextStream.h
> ===================================================================
> --- WebCore/platform/text/TextStream.h	(revision 56567)
> +++ WebCore/platform/text/TextStream.h	(working copy)
> @@ -45,7 +45,7 @@ public:
>      TextStream& operator<<(const char*);
>      TextStream& operator<<(void*);
>      TextStream& operator<<(const String&);
> -#if OS(WINDOWS) && PLATFORM(X86_64) && COMPILER(MSVC)
> +#if OS(WINDOWS) && CPU(X86_64)
>      TextStream& operator<<(unsigned __int64);
>      TextStream& operator<<(__int64);
>  #endif

Will this screw up the 64-bit Qt Windows build? (Is there such a thing?)

> @@ -544,7 +544,7 @@ void PluginView::paintIntoTransformedCon
>  
>      NPEvent npEvent;
>      npEvent.event = WM_WINDOWPOSCHANGED;
> -    npEvent.lParam = reinterpret_cast<uint32>(&windowpos);
> +    npEvent.lParam = reinterpret_cast<uintptr_t>(&windowpos);
>      npEvent.wParam = 0;
>  
>      dispatchNPEvent(npEvent);
> @@ -552,7 +552,7 @@ void PluginView::paintIntoTransformedCon
>      setNPWindowRect(frameRect());
>  
>      npEvent.event = WM_PAINT;
> -    npEvent.wParam = reinterpret_cast<uint32>(hdc);
> +    npEvent.wParam = reinterpret_cast<uintptr_t>(hdc);

These fields are declared to be of type uint32, so these casts look wrong.

> @@ -829,7 +829,7 @@ void PluginView::setNPWindowRect(const I
>  #else
>          WNDPROC currentWndProc = (WNDPROC)GetWindowLongPtr(platformPluginWidget(), GWLP_WNDPROC);
>          if (currentWndProc != PluginViewWndProc)
> -            m_pluginWndProc = (WNDPROC)SetWindowLongPtr(platformPluginWidget(), GWLP_WNDPROC, (LONG)PluginViewWndProc);
> +            m_pluginWndProc = (WNDPROC)SetWindowLongPtr(platformPluginWidget(), GWLP_WNDPROC, (intptr_t)PluginViewWndProc);

That should be LONG_PTR, not intptr_t.
Comment 51 vanboxem.ruben 2010-04-15 13:59:43 PDT
Yes, there is a 64 bit Qt build (either MSVC or since very recently mingw-w64, thanks to me :)

I'm gonna upload a new patch later, first to answer your questions (back-to-front):

 - LONG_PTR indeed, thought I fixed that :s
 - Windows x64 has 64-bit pointer, and this includes window handles and all that. MSVC warns, gcc errors out. That's why the casts need fixing. I'm not sure how to handle this now in WebCore/bridge/npapi.h. A Win32 handle now returns intptr_t's for x64 compatibility, not int or anything like that (see msdn).
 - This errors out on gcc, so yes, necessary
 - That I do not know. But it is also used as a value to store a pointer cast to void* which requires 64 bits of space on x64 windows, not provided by unsigned long on Win64. Again, gcc errors out, MSVC only warns. IMHO, I think nothing will break, as uintptr_t has the right type on mac and linux, and adapts to either
32-bit integer type on Windows x86, or
64-bit integer type on Windows x64. So I think it is the best choice.

PS: the Qt 4.7 can now be built with mingw-w64 with "configure -platform win32-g++ -no-webkit [-sse sse2 -mmx -3dnow]"
Comment 52 Adam Roben (:aroben) 2010-04-15 14:13:24 PDT
(In reply to comment #51)
>  - Windows x64 has 64-bit pointer, and this includes window handles and all
> that. MSVC warns, gcc errors out. That's why the casts need fixing. I'm not
> sure how to handle this now in WebCore/bridge/npapi.h. A Win32 handle now
> returns intptr_t's for x64 compatibility, not int or anything like that (see
> msdn).

Seems like we should figure out what plug-ins expect in 64-bit. (Are there any 64-bit plug-ins on Windows?) Looking at Firefox's code might help here.

>  - That I do not know. But it is also used as a value to store a pointer cast
> to void* which requires 64 bits of space on x64 windows, not provided by
> unsigned long on Win64. Again, gcc errors out, MSVC only warns. IMHO, I think
> nothing will break, as uintptr_t has the right type on mac and linux, and
> adapts to either
> 32-bit integer type on Windows x86, or
> 64-bit integer type on Windows x64. So I think it is the best choice.

OK, thanks for the explanation.
Comment 53 vanboxem.ruben 2010-04-16 01:57:26 PDT
Well, I have no idea of how to handle this properly (no familiarity whatsoever with netscape plugin api, webkit api or anything at all). Here's what's important on Windows side: http://msdn.microsoft.com/en-us/library/aa384242%28VS.9%29.aspx.

And yes, there is at least one 64-bit plugin for Windows: Java. There probably are no more because of the chicken and the egg story... Flash will surely be late as usual (linux x64 Flash is still alpha!)

PS: for info on Firefox x64 (although quite old, it's the only site that ever cared enough  to provide builds): http://www.mozilla-x86-64.com/
Comment 54 Adam Treat 2010-04-16 09:07:23 PDT
Comment on attachment 49916 [details]
Modified patch for WebCore according to discussion in this bug

I believe this patch has been replaced so marking it as r- to get it out of review queue.
Comment 55 vanboxem.ruben 2010-04-18 11:19:21 PDT
Created attachment 53630 [details]
WebCore patch for mingw-w64

npapi.h: uint32 is not correct for lparam and wparam on WIN64 (see links below). Nothing changes for existing 32-bit code or other platforms.
Arena.h: pointers need to be cast to 64-bit types, uintptr_t handles platform and bitness differences nicely
TexStream.*: int64 needs to be used on all WIN64 platforms, not just msvc
PluginView.cpp: cast to LONG_PTR instead of LONG (see link below)
PluginViewWin.cpp: fix pointer casts (this change is now reflected in npapi.h)

MSDN:
(64-bit guidelines)
http://msdn.microsoft.com/en-us/library/aa384242%28v=VS.9%29.aspx
(LONG_PTR)
http://msdn.microsoft.com/en-us/library/aa383751%28VS.9%29.aspx
Intel (WPARAM, LPARAM, see point 5):
http://software.intel.com/en-us/articles/seven-steps-of-migrating-a-program-to-a-64-bit-system/

I hope this is enough to convinve everyone :) Otherwise I'd be happy to elaborate.
Comment 56 Adam Roben (:aroben) 2010-04-19 08:45:24 PDT
(In reply to comment #55)
> Created an attachment (id=53630) [details]
> WebCore patch for mingw-w64

Thanks for explaining your changes, both here and in the ChangeLog.

> npapi.h: uint32 is not correct for lparam and wparam on WIN64 (see links
> below). Nothing changes for existing 32-bit code or other platforms.

None of the links you provided mention the behavior of NPAPI on 64-bit Windows. This one:

> Intel (WPARAM, LPARAM, see point 5):
> http://software.intel.com/en-us/articles/seven-steps-of-migrating-a-program-to-a-64-bit-system/

Talks about WPARAM and LPARAM from windef.h. But I don't see any evidence that _NPEvent's wParam and lParam are supposed to follow those definitions. Do you have information regarding what 64-bit NPAPI plug-ins expect on Windows, or what other browsers do in 64-bit?
Comment 57 vanboxem.ruben 2010-04-19 12:09:07 PDT
I believe I have found a reason/explanation why the uintptr_t's are necessary in npapi.h:

First we have (lines 3203-3257, specifically 3249-3250):
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp
Where a MSG's wparam and lparam are put in a _NPEvent's wparam and lparam.
Checking MSDN for MSG:
http://msdn.microsoft.com/en-us/library/ms644958%28VS.9%29.aspx
defines these as WPARAM and LPARAM. I believe this provides solid proof in linking the two variables/data types.

I have provided Mozilla with a bug report on this issue, as they seem to be the maintainers of the NPAPI:
https://bugzilla.mozilla.org/show_bug.cgi?id=560298

Please feel free to confirm that bug and get this solved upstream (I think?). Thanks
Comment 58 vanboxem.ruben 2010-04-26 00:42:44 PDT
I bring good tidings!

Mozilla has fixed the npapi.h bug, which makes my patch completely legitimate.

See https://bugzilla.mozilla.org/show_bug.cgi?id=560298 for an already resolved bug report.
Comment 59 Adam Roben (:aroben) 2010-04-26 06:33:18 PDT
Comment on attachment 53630 [details]
WebCore patch for mingw-w64

> +        *  WebCore/bridge/npapi.h: lparam and wparam are 64-bit on WIN64, uintptr_t keeps 32-bit compatibility

You should mention that this change matches Firefox. You could even reference the bug you filed.

> +        *  WebCore/platform/Arena.h: fix pointer cast on win64

This comment is a little strange. You changed a typedef, but the comment talks about a cast. I think you should clear this up.

It might also be worth mentioning that unsigned long is 64 bits on 64-bit Mac builds, but only 32 bits on 64-bit Windows builds, while uintptr_t is 64 bits on both.

Let's get these ChangeLog comments cleared up and then I'll r+ the patch.

Thanks for sticking with it!
Comment 60 vanboxem.ruben 2010-04-26 06:55:48 PDT
Created attachment 54290 [details]
WebCore patch for mingw-w64

Changelog clarifications as per request.
Comment 61 Adam Roben (:aroben) 2010-04-26 07:01:55 PDT
Comment on attachment 54290 [details]
WebCore patch for mingw-w64

r=me!
Comment 62 vanboxem.ruben 2010-04-26 07:24:27 PDT
Created attachment 54295 [details]
WebCore patch for mingw-w64 (updated CPU<->PLATFORM)

Previous patch did not apply do to CPU replacing PLATFORM in some files somewhere between the old patch and now. Apologies, please re-review+. Thanks

(PS: I did not obsolete the old one because it is queue. Should I?)
Comment 63 Adam Roben (:aroben) 2010-04-26 07:30:32 PDT
(In reply to comment #62)
> (PS: I did not obsolete the old one because it is queue. Should I?)

Yes, you should. I'll do that for you.
Comment 64 vanboxem.ruben 2010-04-26 07:33:17 PDT
Muchos gracias! Now to get this in Qt 4.7...
Comment 65 WebKit Commit Bot 2010-04-26 07:41:34 PDT
Comment on attachment 54290 [details]
WebCore patch for mingw-w64

Rejecting patch 54290 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1
Last 500 characters of output:
extStream.cpp.rej
(Stripping trailing CRs from patch.)
patching file WebCore/platform/text/TextStream.h
Hunk #1 FAILED at 45.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/platform/text/TextStream.h.rej
(Stripping trailing CRs from patch.)
patching file WebCore/plugins/PluginView.cpp
(Stripping trailing CRs from patch.)
patching file WebCore/plugins/win/PluginViewWin.cpp
Hunk #4 FAILED at 978.
1 out of 4 hunks FAILED -- saving rejects to file WebCore/plugins/win/PluginViewWin.cpp.rej

Full output: http://webkit-commit-queue.appspot.com/results/1915004
Comment 66 WebKit Commit Bot 2010-04-26 07:58:13 PDT
Comment on attachment 54295 [details]
WebCore patch for mingw-w64 (updated CPU<->PLATFORM)

Clearing flags on attachment: 54295

Committed r58256: <http://trac.webkit.org/changeset/58256>
Comment 67 WebKit Commit Bot 2010-04-26 07:58:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 68 Simon Hausmann 2010-04-27 03:08:23 PDT
Revision r57025 cherry-picked into qtwebkit-2.0 with commit a5c47be0fac33d86cd78b9e17854643a589cf864
Revision r58256 cherry-picked into qtwebkit-2.0 with commit b8b715b36dd157c1f872c5d7e62e68e57458c05f