Created attachment 113964 [details] Patch

Comment on attachment 113964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113964&action=review > Source/WebCore/bindings/v8/custom/V8GamepadCustom.cpp:37 > +static v8::Handle<v8::Value> createNumberArray(const Gamepad::FloatVector& vector) You should teach the code generator how to do this instead of writing custom bindings. All custom bindings are fully of bugs.

Comment on attachment 113964 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113964&action=review >> Source/WebCore/bindings/v8/custom/V8GamepadCustom.cpp:37 >> +static v8::Handle<v8::Value> createNumberArray(const Gamepad::FloatVector& vector) > > You should teach the code generator how to do this instead of writing custom bindings. All custom bindings are fully of bugs. OK. WIP here: https://bugs.webkit.org/show_bug.cgi?id=71763

Created attachment 114314 [details] remove custom binding code

Created attachment 114533 [details] add basic test + harness

Comment on attachment 114533 [details] add basic test + harness Attachment 114533 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10450568

Created attachment 115215 [details] reupload to re-EWS now that pipelined change 71518 is landed

Comment on attachment 115215 [details] reupload to re-EWS now that pipelined change 71518 is landed Attachment 115215 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10483453 New failing tests: fast/dom/navigator-detached-no-crash.html

Created attachment 115247 [details] test rebase for navigator addition

Created attachment 115845 [details] vendor prefix, update Skippeds

Comment on attachment 115845 [details] vendor prefix, update Skippeds View in context: https://bugs.webkit.org/attachment.cgi?id=115845&action=review > Source/WebCore/WebCore.gypi:2954 > + 'page/Gamepad.cpp', > + 'page/Gamepad.h', > + 'page/GamepadList.cpp', > + 'page/GamepadList.h', Drive by: "page" isn't really the right place for these objects because they have document-scope, not page-scope. I realize that there isn't a good place to put them at the moment. It's fine to put them here for now, but we'll probably end up moving them.

Comment on attachment 115845 [details] vendor prefix, update Skippeds Attachment 115845 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10517207 New failing tests: fast/dom/navigator-detached-no-crash.html

Created attachment 115857 [details] rebase

Comment on attachment 115857 [details] rebase View in context: https://bugs.webkit.org/attachment.cgi?id=115857&action=review If it's not too much trouble, would you be willing to put these files in Source/WebCore/Modules/gamepad ? We've been discussing creating a Modules directory to hold these sorts of features, and your patch seems well timed to be the guinea pig. (If you'd rather not be a guinea pig, we can keep these in page.) Some minor nits below. > Source/WebCore/page/Gamepad.h:26 > + Please include ENABLE(GAMEPAD) guards in these header files. > Source/WebCore/page/GamepadList.h:26 > + ENABLE(GAMEPAD) > Source/WebCore/page/Navigator.idl:66 > + readonly attribute [EnabledAtRuntime] GamepadList webkitGamepads; You can use Conditional=GAMEPAD rather than #if statements.

(In reply to comment #14) > (From update of attachment 115857 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=115857&action=review > > If it's not too much trouble, would you be willing to put these files in Source/WebCore/Modules/gamepad ? We've been discussing creating a Modules directory to hold these sorts of features, and your patch seems well timed to be the guinea pig. (If you'd rather not be a guinea pig, we can keep these in page.) Sure, I can do that if we can get it done soon-ish (I would like to get moving, as there's a lot of dependent code in Chromium). Are we decided on "Modules"? Moved various to Modules/gamepad/, and changed WebCore.gypi and WebCore.gyp/WebCore.gyp for includes. > > Some minor nits below. > > > Source/WebCore/page/Gamepad.h:26 > > + > > Please include ENABLE(GAMEPAD) guards in these header files. > > > Source/WebCore/page/GamepadList.h:26 > > + Done (but moved of course) > ENABLE(GAMEPAD) > > > Source/WebCore/page/Navigator.idl:66 > > + readonly attribute [EnabledAtRuntime] GamepadList webkitGamepads; > > You can use Conditional=GAMEPAD rather than #if statements. Done.

Created attachment 115894 [details] review fixes, move to Modules

Comment on attachment 115894 [details] review fixes, move to Modules Attachment 115894 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10518233

Hmm... so there's a lot of include dirs to update for the various ports. Do you think it might make more sense to do the move to Modules as a separate patch instead?

> Hmm... so there's a lot of include dirs to update for the various ports. Do you think it might make more sense to do the move to Modules as a separate patch instead? Sure, that's fine.

Created attachment 115898 [details] retreat on Modules

(In reply to comment #19) > > Hmm... so there's a lot of include dirs to update for the various ports. Do you think it might make more sense to do the move to Modules as a separate patch instead? > > Sure, that's fine. OK, as soon as this one lands I'll work up a guinea pig move-to-Modules and hunt down all the necessary places to change include dirs.

Comment on attachment 115898 [details] retreat on Modules View in context: https://bugs.webkit.org/attachment.cgi?id=115898&action=review > Source/WebCore/page/Navigator.cpp:35 > +#include "GamepadList.h" Usually we put ifdefs around these sorts of headers so that folks who don't enable this feature don't need to include it. This file is missing a bunch of those ifdefs, which is probably why you didn't add them. > LayoutTests/gamepad/gamepad-api.html:3 > +<script src="gamepad-test.js"></script> Is there some reason to use a gamepad-specific test harness rather than the usual script test harness?

Comment on attachment 115898 [details] retreat on Modules This is fine to land as-is, but consider moving the test to use the usual fast/js testing helper functions as you add more gamepad tests.

Comment on attachment 115898 [details] retreat on Modules Clearing flags on attachment: 115898 Committed r100833: <http://trac.webkit.org/changeset/100833>

All reviewed patches have been landed. Closing bug.

Followup cleanup here: https://bugs.webkit.org/show_bug.cgi?id=72785