78022 – [Chromium] pack Gamepad shared memory structure

RESOLVED FIXED 78022

[Chromium] pack Gamepad shared memory structure

https://bugs.webkit.org/show_bug.cgi?id=78022

Summary [Chromium] pack Gamepad shared memory structure

Scott Graham

Reported 2012-02-07 13:00:25 PST

Use #pragma pack to avoid difference across compiler settings in different modules/platforms.

Attachments
Patch (2.20 KB, patch) 2012-02-07 13:02 PST, Scott Graham	no flags	Details Formatted Diff Diff
add compile_asserts (2.35 KB, patch) 2012-02-07 16:01 PST, Scott Graham	no flags	Details Formatted Diff Diff
add/use WEBKIT_COMPILE_ASSERT (3.10 KB, patch) 2012-02-08 11:34 PST, Scott Graham	no flags	Details Formatted Diff Diff
webkit_implementation for compile_assert (2.65 KB, patch) 2012-02-08 16:06 PST, Scott Graham	no flags	Details Formatted Diff Diff
reup for ews (2.65 KB, patch) 2012-02-09 09:04 PST, Scott Graham	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Scott Graham

Comment 1 2012-02-07 13:02:24 PST

Created attachment 125905 [details] Patch

WebKit Review Bot

Comment 2 2012-02-07 13:04:27 PST

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

WebKit Review Bot

Comment 3 2012-02-07 13:28:02 PST

Comment on attachment 125905 [details] Patch Attachment 125905 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11453063

Scott Graham

Comment 4 2012-02-07 13:40:57 PST

The EWS failure is what the patch is trying to fix. The other side (which temporarily disables a static assert until this lands+rolls) is here: http://chromiumcodereview.appspot.com/9348029/

Eric Seidel (no email)

Comment 5 2012-02-07 15:46:48 PST

Comment on attachment 125905 [details] Patch OK. Seems if we're going to do this we should have a COMPILE_ASSERT to validate the size of these?

Scott Graham

Comment 6 2012-02-07 15:49:54 PST

(In reply to comment #5) > (From update of attachment 125905 [details]) > OK. Seems if we're going to do this we should have a COMPILE_ASSERT to validate the size of these? There's one in Chromium code that the size matches there (which is the failure above). You mean here that the size is a specific number? Sure, that seems reasonable.

Scott Graham

Comment 7 2012-02-07 16:01:49 PST

Created attachment 125948 [details] add compile_asserts

Eric Seidel (no email)

Comment 8 2012-02-07 16:05:15 PST

Comment on attachment 125948 [details] add compile_asserts View in context: https://bugs.webkit.org/attachment.cgi?id=125948&action=review Oh, at first I didn't realize these were Chromium headers. Looks fine. As the style-bot says, fishd is supposed to approve this sort of thing. Given that he's out this week, I think it's OK to move foward with this r=me, TBR=fishd. > Source/WebKit/chromium/public/platform/WebGamepads.h:50 > +COMPILE_ASSERT(sizeof(WebGamepads) == 1864, WebGamepads_has_wrong_size); WebKit has used this type of thing in the past to find compiler configuration bugs between platforms. :)

WebKit Review Bot

Comment 9 2012-02-07 18:07:40 PST

Comment on attachment 125948 [details] add compile_asserts Attachment 125948 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11480001

Darin Fisher (:fishd, Google)

Comment 10 2012-02-08 11:09:29 PST

Comment on attachment 125948 [details] add compile_asserts View in context: https://bugs.webkit.org/attachment.cgi?id=125948&action=review > Source/WebKit/chromium/public/platform/WebGamepad.h:74 > +COMPILE_ASSERT(sizeof(WebGamepad) == 465, WebGamepad_has_wrong_size); I don't think you can use this macro in WebKit API header files. Where is it defined?

Scott Graham

Comment 11 2012-02-08 11:22:58 PST

(In reply to comment #10) > (From update of attachment 125948 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125948&action=review > > > Source/WebKit/chromium/public/platform/WebGamepad.h:74 > > +COMPILE_ASSERT(sizeof(WebGamepad) == 465, WebGamepad_has_wrong_size); > > I don't think you can use this macro in WebKit API header files. Where is it defined? wtf/Assertions.h. Is that OK, or would it be better to have something in WebCommon.h analogous to WEBKIT_ASSERT there?

James Robinson

Comment 12 2012-02-08 11:25:16 PST

People using the WebKit API do not (and should not) have wtf/ on their include path, so that won't work.

Scott Graham

Comment 13 2012-02-08 11:34:41 PST

Created attachment 126125 [details] add/use WEBKIT_COMPILE_ASSERT

WebKit Review Bot

Comment 14 2012-02-08 11:42:47 PST

Comment on attachment 126125 [details] add/use WEBKIT_COMPILE_ASSERT Attachment 126125 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11463504

James Robinson

Comment 15 2012-02-08 13:49:39 PST

You're breaking this COMPILE_ASSERT in pepper code: // Ensure conversion from WebKit::WebGamepads to PP_GamepadsData_Dev is safe. // See also DCHECKs in SampleGamepads below. COMPILE_ASSERT(sizeof(WebKit::WebGamepads) == sizeof(PP_GamepadsData_Dev), size_difference); COMPILE_ASSERT(sizeof(WebKit::WebGamepad) == sizeof(PP_GamepadData_Dev), size_difference);

Scott Graham

Comment 16 2012-02-08 13:53:00 PST

(In reply to comment #15) > You're breaking this COMPILE_ASSERT in pepper code: > > // Ensure conversion from WebKit::WebGamepads to PP_GamepadsData_Dev is safe. > // See also DCHECKs in SampleGamepads below. > COMPILE_ASSERT(sizeof(WebKit::WebGamepads) == sizeof(PP_GamepadsData_Dev), > size_difference); > COMPILE_ASSERT(sizeof(WebKit::WebGamepad) == sizeof(PP_GamepadData_Dev), > size_difference); That's what this is fixing. It's landed in ppapi already, but lkgr hasn't updated yet.

James Robinson

Comment 17 2012-02-08 14:01:10 PST

LKGR is irrelevant. The revision pointed to by Source/WebKit/chromium/DEPS is what this bot is running, so you need to update that to make this pass.

Scott Graham

Comment 18 2012-02-08 14:12:07 PST

(In reply to comment #17) > LKGR is irrelevant. The revision pointed to by Source/WebKit/chromium/DEPS is what this bot is running, so you need to update that to make this pass. Ah, oops, thanks. I guess that should be a separate patch? https://bugs.webkit.org/show_bug.cgi?id=78152

Adam Barth

Comment 19 2012-02-08 14:24:58 PST

> Ah, oops, thanks. I guess that should be a separate patch? https://bugs.webkit.org/show_bug.cgi?id=78152 Yep. Thanks.

Darin Fisher (:fishd, Google)

Comment 20 2012-02-08 15:09:12 PST

Comment on attachment 126125 [details] add/use WEBKIT_COMPILE_ASSERT View in context: https://bugs.webkit.org/attachment.cgi?id=126125&action=review > Source/Platform/chromium/public/WebCommon.h:124 > +#define WEBKIT_COMPILE_ASSERT(exp, name) typedef int dummy##name[(exp) ? 1 : -1] an alternative to introducing this macro would to be only perform the compile assert in a .cpp file, or within a "#if WEBKIT_IMPLEMENTATION" section of a header file. maybe that would be better than replicating the compile assert macro?

Scott Graham

Comment 21 2012-02-08 15:29:31 PST

(In reply to comment #20) > (From update of attachment 126125 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126125&action=review > > > Source/Platform/chromium/public/WebCommon.h:124 > > +#define WEBKIT_COMPILE_ASSERT(exp, name) typedef int dummy##name[(exp) ? 1 : -1] > > an alternative to introducing this macro would to be only perform the compile assert in a .cpp file, or within a "#if WEBKIT_IMPLEMENTATION" section of a header file. maybe that would be better than replicating the compile assert macro? It looked like it belonged there since there was a WEBKIT_ASSERT, but I didn't realize how little that's used. It seems best to keep the assert next to the definition of the structure, so I guess in WEBKIT_IMPLEMENTATION is probably best.

Scott Graham

Comment 22 2012-02-08 16:06:26 PST

Created attachment 126174 [details] webkit_implementation for compile_assert

WebKit Review Bot

Comment 23 2012-02-08 17:00:18 PST

Comment on attachment 126174 [details] webkit_implementation for compile_assert Attachment 126174 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11459716

Scott Graham

Comment 24 2012-02-09 09:04:49 PST

Created attachment 126315 [details] reup for ews

Scott Graham

Comment 25 2012-02-15 10:30:50 PST

Ping, is this OK to commit now?

WebKit Review Bot

Comment 26 2012-02-17 11:08:38 PST

Comment on attachment 126315 [details] reup for ews Clearing flags on attachment: 126315 Committed r108099: <http://trac.webkit.org/changeset/108099>

WebKit Review Bot

Comment 27 2012-02-17 11:08:46 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 28 2012-02-17 11:45:48 PST

The commit-queue encountered the following flaky tests while processing attachment 126315 [details]: http/tests/workers/text-encoding.html bug 78917 (author: dimich@chromium.org) The commit-queue is continuing to process your patch.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Scott Graham

Reported

2012-02-07 13:00 PST

Modified

2012-02-17 11:45 PST History

CC List

6 users Show

URL

Keywords

Depends on

Blocks