Bug 78022

Summary: [Chromium] pack Gamepad shared memory structure
Product: WebKit Reporter: Scott Graham <scottmg>
Component: WebCore Misc.Assignee: Scott Graham <scottmg>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, fishd, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
add compile_asserts
none
add/use WEBKIT_COMPILE_ASSERT
none
webkit_implementation for compile_assert
none
reup for ews none

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
DetailsFormatted DiffDiff
add compile_asserts (2.35 KB, patch)
2012-02-07 16:01 PST, Scott Graham 		no flags
DetailsFormatted DiffDiff
add/use WEBKIT_COMPILE_ASSERT (3.10 KB, patch)
2012-02-08 11:34 PST, Scott Graham 		no flags
DetailsFormatted DiffDiff
webkit_implementation for compile_assert (2.65 KB, patch)
2012-02-08 16:06 PST, Scott Graham 		no flags
DetailsFormatted DiffDiff
reup for ews (2.65 KB, patch)
2012-02-09 09:04 PST, Scott Graham 		no flags
DetailsFormatted DiffDiff
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.