Bug 78022 - [Chromium] pack Gamepad shared memory structure
Summary: [Chromium] pack Gamepad shared memory structure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Scott Graham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-07 13:00 PST by Scott Graham
Modified: 2012-02-17 11:45 PST (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Graham 2012-02-07 13:00:25 PST
Use #pragma pack to avoid difference across compiler settings in different modules/platforms.
Comment 1 Scott Graham 2012-02-07 13:02:24 PST
Created attachment 125905 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 WebKit Review Bot 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
Comment 4 Scott Graham 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/
Comment 5 Eric Seidel (no email) 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?
Comment 6 Scott Graham 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.
Comment 7 Scott Graham 2012-02-07 16:01:49 PST
Created attachment 125948 [details]
add compile_asserts
Comment 8 Eric Seidel (no email) 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. :)
Comment 9 WebKit Review Bot 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
Comment 10 Darin Fisher (:fishd, Google) 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?
Comment 11 Scott Graham 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?
Comment 12 James Robinson 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.
Comment 13 Scott Graham 2012-02-08 11:34:41 PST
Created attachment 126125 [details]
add/use WEBKIT_COMPILE_ASSERT
Comment 14 WebKit Review Bot 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
Comment 15 James Robinson 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);
Comment 16 Scott Graham 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.
Comment 17 James Robinson 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.
Comment 18 Scott Graham 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
Comment 19 Adam Barth 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.
Comment 20 Darin Fisher (:fishd, Google) 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?
Comment 21 Scott Graham 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.
Comment 22 Scott Graham 2012-02-08 16:06:26 PST
Created attachment 126174 [details]
webkit_implementation for compile_assert
Comment 23 WebKit Review Bot 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
Comment 24 Scott Graham 2012-02-09 09:04:49 PST
Created attachment 126315 [details]
reup for ews
Comment 25 Scott Graham 2012-02-15 10:30:50 PST
Ping, is this OK to commit now?
Comment 26 WebKit Review Bot 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>
Comment 27 WebKit Review Bot 2012-02-17 11:08:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Review Bot 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.