Bug (W_LPARAM_DEF) – WPARAM and LPARAM are incorrectly defined for x64

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alex Christensen]

There are 5 places in Source/WebCore/platform where WPARAM and LPARAM are defined to be unsigned and long, respectively.  This happens to be correct when building for an x86 machine, but this incorrectly redefines the types for x64 machines.  Visual Studio's BaseTsd.h uses #if defined(_WIN64) to change these types to 64-bit unsigned and signed integers, so if we did the same it would not cause compiler errors.

[Alex Christensen]

Created attachment 149342 [details]
Patch

[WebKit Review Bot]

Attachment 149342 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 149393 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 149393 [details]
Patch

You're missing the change.

[Alex Christensen]

Created attachment 153801 [details]
Patch

[Alex Christensen]

no wonder it passed all the tests :)

(In reply to comment #4)
> (From update of attachment 149393 [details])
> You're missing the change.

[Alex Christensen]

I have a new patch with the changes.  Could someone review it?

[Patrick R. Gansterer]

Please don't do copy&past programming. It's already not the best to have the same typdefs three times. So it's a good time to move them in their own header.

[Alexis Menard (darktears)]

Comment on attachment 153801 [details]
Patch

Would it be possible to refactor this a bit more and add this insane copy pasting in some common headers (platform.h for example?)

[Patrick R. Gansterer]

(In reply to comment #9)
> (From update of attachment 153801 [details])
> Would it be possible to refactor this a bit more and add this insane copy pasting in some common headers (platform.h for example?)
IMHO Platform.h is too general. WindowsExtras.h should be ok for this.

[Alexis Menard (darktears)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 153801 [details] [details])
> > Would it be possible to refactor this a bit more and add this insane copy pasting in some common headers (platform.h for example?)
> IMHO Platform.h is too general. WindowsExtras.h should be ok for this.

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 153801 [details] [details])
> > Would it be possible to refactor this a bit more and add this insane copy pasting in some common headers (platform.h for example?)
> IMHO Platform.h is too general. WindowsExtras.h should be ok for this.

Indeed. Maybe we could even refactor the other copy/pasted defines which are not part of this patch to clean that up. (The bug could be renamed or a new one could be created to move later on the other defines).

[Alex Christensen]

Created attachment 158017 [details]
Patch

[Alex Christensen]

Now the files include WindowsExtras.h, which includes <windows.h> if PLATFORM(WIN) is true, which includes the correct definitions.  No duplicate definitions!

[WebKit Review Bot]

Attachment 158017 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/win/WindowsExtras.h:58:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 158021 [details]
Patch

[Alex Christensen]

Created attachment 158026 [details]
Patch

[Alex Christensen]

Created attachment 158028 [details]
Patch

[Alex Christensen]

Created attachment 158036 [details]
Patch

[Alex Christensen]

Sorry about all the patches.  I don't have a mac to test on.

[Patrick R. Gansterer]

Comment on attachment 158036 [details]
Patch

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

> Source/WebCore/platform/PlatformKeyboardEvent.h:31
> +#if PLATFORM(WIN)

please use OS(WINDOWS) here and everywhere else in this patch

[Alex Christensen]

Created attachment 158060 [details]
Patch

[Brent Fulgham]

Comment on attachment 158060 [details]
Patch

Very nice removal of duplicated code.  Thanks for taking this on.

[WebKit Review Bot]

Comment on attachment 158060 [details]
Patch

Clearing flags on attachment: 158060

Committed r125506: <http://trac.webkit.org/changeset/125506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.