Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212813008:
Gamepad API: tests for gamepad events (Closed)
|
Created:
6 years, 9 months ago by kbalazs Modified:
6 years, 8 months ago CC:
blink-reviews, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionGamepad API: tests for gamepad events
This CL adds a basic test for 'gamepadconnected' and 'gamepaddisconnected' events and extends the api test to check the differences between the prefixed and non-prefixed api's.
BUG=344556
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170461
Patch Set 1 #
Total comments: 17
Patch Set 2 : cleanups #
Total comments: 20
Patch Set 3 : incorp comments #
Total comments: 6
Patch Set 4 : check prototypes #
Messages
Total messages: 13 (0 generated)
Just some test style issues, hopefully useful. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... File LayoutTests/gamepad/gamepad-api.html (right): https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-api.html:12: shouldBeEqualToString("Object.prototype.toString.call(webkitGamepads)", "[object WebKitGamepadList]"); This is a curious way to do a type test -- how about shouldBeTrue("webkitGamepads instanceof WebKitGamepadList"); instead? Similarly for all other uses of Object.prototype.toString.call() here, either use 'instanceof' or 'typeof' if a primitive type. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-api.html:16: shouldBe("webkitGamepads.item(0)", "null"); shouldBeNull() instead? https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-api.html:21: shouldBe("gamepads[0]", "undefined"); You can use shouldBeUndefined() instead here. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... File LayoutTests/gamepad/gamepad-events-basic.html (right): https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:9: shouldBe("event.gamepad.id", "'MockStick 3000'"); Use shouldBeString() https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:13: shouldBe("event.gamepad.buttons[0].pressed", "true"); shouldBeTrue() https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:15: shouldBe("event.gamepad.buttons[1].pressed", "false"); shouldBeFalse() https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:24: shouldBe("event.gamepad.id", "'MockStick 3000'"); shouldBeString() https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:48: console.log("FAIL: no gamepadController available.") testFailed() seems a better fit. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:51: <p>Basic test for 'gamepadconnected' and 'gamepaddisconnected' events.</p> Put this in a call to description() instead, at the top.
On 2014/03/26 21:43:46, sof wrote: > Just some test style issues, hopefully useful. > > https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... > File LayoutTests/gamepad/gamepad-api.html (right): > > https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... > LayoutTests/gamepad/gamepad-api.html:12: > shouldBeEqualToString("Object.prototype.toString.call(webkitGamepads)", "[object > WebKitGamepadList]"); > This is a curious way to do a type test -- how about > > shouldBeTrue("webkitGamepads instanceof WebKitGamepadList"); > > instead? > > Similarly for all other uses of Object.prototype.toString.call() here, either > use 'instanceof' or 'typeof' if a primitive type. WebKitGamepadList is not exposed as a javascript type, it's type is just Object. Same is true for GamepadList and GamepadButton. I got this Object.prototype.toString.call() from another test but now I'm not sure why I couldn't just use toString(), I will try that. A grep for '\[object' in LayoutTests tells me that this type of type testing is not uncommon.
On 2014/03/26 23:03:06, kbalazs wrote: > On 2014/03/26 21:43:46, sof wrote: > > Just some test style issues, hopefully useful. > > > > > https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... > > File LayoutTests/gamepad/gamepad-api.html (right): > > > > > https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... > > LayoutTests/gamepad/gamepad-api.html:12: > > shouldBeEqualToString("Object.prototype.toString.call(webkitGamepads)", > "[object > > WebKitGamepadList]"); > > This is a curious way to do a type test -- how about > > > > shouldBeTrue("webkitGamepads instanceof WebKitGamepadList"); > > > > instead? > > > > Similarly for all other uses of Object.prototype.toString.call() here, either > > use 'instanceof' or 'typeof' if a primitive type. > > WebKitGamepadList is not exposed as a javascript type, it's type is just Object. > Same is true for GamepadList and GamepadButton. I got this > Object.prototype.toString.call() from another test but now I'm not sure why I > couldn't just use toString(), I will try that. A grep for '\[object' in > LayoutTests tells me that this type of type testing is not uncommon. Err, toString() only works for the trivial cases of course, not for strings or numbers, so for consistency I think I stick to the Object.prototype.toString.call() form.
https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... File LayoutTests/gamepad/gamepad-api.html (right): https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-api.html:16: shouldBe("webkitGamepads.item(0)", "null"); On 2014/03/26 21:43:46, sof wrote: > shouldBeNull() instead? Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-api.html:21: shouldBe("gamepads[0]", "undefined"); On 2014/03/26 21:43:46, sof wrote: > You can use shouldBeUndefined() instead here. Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... File LayoutTests/gamepad/gamepad-events-basic.html (right): https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:9: shouldBe("event.gamepad.id", "'MockStick 3000'"); On 2014/03/26 21:43:46, sof wrote: > Use shouldBeString() Yes, shouldBeEqualToString :) Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:13: shouldBe("event.gamepad.buttons[0].pressed", "true"); On 2014/03/26 21:43:46, sof wrote: > shouldBeTrue() Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:15: shouldBe("event.gamepad.buttons[1].pressed", "false"); On 2014/03/26 21:43:46, sof wrote: > shouldBeFalse() Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:24: shouldBe("event.gamepad.id", "'MockStick 3000'"); On 2014/03/26 21:43:46, sof wrote: > shouldBeString() Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:48: console.log("FAIL: no gamepadController available.") On 2014/03/26 21:43:46, sof wrote: > testFailed() seems a better fit. Done. https://codereview.chromium.org/212813008/diff/1/LayoutTests/gamepad/gamepad-... LayoutTests/gamepad/gamepad-events-basic.html:51: <p>Basic test for 'gamepadconnected' and 'gamepaddisconnected' events.</p> On 2014/03/26 21:43:46, sof wrote: > Put this in a call to description() instead, at the top. Done.
https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... File LayoutTests/gamepad/gamepad-api.html (right): https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:27: shouldNotThrow("window.addEventListener('gamepadconnected', function(){}, false)"); I don't believe addEventListener can ever throw. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:28: shouldNotThrow("window.addEventListener('gamepaddisconnected', function(){}, false)"); Ditto. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:37: shouldBeEqualToString("Object.prototype.toString.call(gamepad)", "[object WebKitGamepad]"); This test relies a lot on the String serialization of objects. I think it would be better to check the object prototype instead. e.g. shouldBe("gamepad.__proto__", "WebKitGamepad.prototype"); Note that this will not work for non-exposed types such as GamepadList. Then again, I am not sure we should test those types since they are not exposed and internal to Blink. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... File LayoutTests/gamepad/gamepad-events-basic.html (right): https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:6: jsTestIsAsync = true; https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:10: debug("Gamepad connected"); You should probably check the event type as well, e.g.: window.testEvent = event; shouldBe("testEvent.__proto__", "GamepadEvent.prototype"); shouldBe("testEvent.__proto__.__proto__", "Event.prototype"); https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:25: debug("Gamepad disconnected"); Should check event type, same as above. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:29: if (window.testRunner) finishJSTest(); https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:33: if (window.testRunner) Not needed if you do the "jsTestIsAsync = true;" at the top. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:35: window.addEventListener('gamepadconnected', onConnected, false); nit: ", false" is unnecessary (this is the default) https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:36: window.addEventListener('gamepaddisconnected', onDisconnected, false); Done.
https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... File LayoutTests/gamepad/gamepad-api.html (right): https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:27: shouldNotThrow("window.addEventListener('gamepadconnected', function(){}, false)"); On 2014/03/27 13:27:24, Chris Dumez wrote: > I don't believe addEventListener can ever throw. No, not really. Removed the events from this test. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:28: shouldNotThrow("window.addEventListener('gamepaddisconnected', function(){}, false)"); On 2014/03/27 13:27:24, Chris Dumez wrote: > Ditto. Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:37: shouldBeEqualToString("Object.prototype.toString.call(gamepad)", "[object WebKitGamepad]"); On 2014/03/27 13:27:24, Chris Dumez wrote: > This test relies a lot on the String serialization of objects. I think it would > be better to check the object prototype instead. e.g. > shouldBe("gamepad.__proto__", "WebKitGamepad.prototype"); > > Note that this will not work for non-exposed types such as GamepadList. Then > again, I am not sure we should test those types since they are not exposed and > internal to Blink. Using __proto__ where possible and not checking non-exposed types now. I still kept checking the builtin types of properties (via string serialization) mostly to verify the difference between the prefixed and non-prefixed api. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... File LayoutTests/gamepad/gamepad-events-basic.html (right): https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:6: On 2014/03/27 13:27:24, Chris Dumez wrote: > jsTestIsAsync = true; Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:10: debug("Gamepad connected"); On 2014/03/27 13:27:24, Chris Dumez wrote: > You should probably check the event type as well, e.g.: > window.testEvent = event; > shouldBe("testEvent.__proto__", "GamepadEvent.prototype"); > shouldBe("testEvent.__proto__.__proto__", "Event.prototype"); Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:25: debug("Gamepad disconnected"); On 2014/03/27 13:27:24, Chris Dumez wrote: > Should check event type, same as above. Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:29: if (window.testRunner) On 2014/03/27 13:27:24, Chris Dumez wrote: > finishJSTest(); Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:33: if (window.testRunner) On 2014/03/27 13:27:24, Chris Dumez wrote: > Not needed if you do the "jsTestIsAsync = true;" at the top. Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:35: window.addEventListener('gamepadconnected', onConnected, false); On 2014/03/27 13:27:24, Chris Dumez wrote: > nit: ", false" is unnecessary (this is the default) Done. https://codereview.chromium.org/212813008/diff/10001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-events-basic.html:36: window.addEventListener('gamepaddisconnected', onDisconnected, false); On 2014/03/27 13:27:24, Chris Dumez wrote: > Done. Done.
https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... File LayoutTests/gamepad/gamepad-api.html (right): https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:14: shouldBeEqualToString("Object.prototype.toString.call(webkitGamepads.item)", "[object Function]") shouldBe("webkitGamepads.item.__proto__", "Function.prototype"); https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:20: shouldBeEqualToString("Object.prototype.toString.call(gamepads.item)", "[object Function]"); Ditto. https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:31: shouldBeEqualToString("Object.prototype.toString.call(gamepad.id)", "[object String]"); You could use __proto__ for those JS builtin types as well, e.g. ShouldBe("gamepad.id.__proto", "String.prototype");
https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... File LayoutTests/gamepad/gamepad-api.html (right): https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:14: shouldBeEqualToString("Object.prototype.toString.call(webkitGamepads.item)", "[object Function]") On 2014/03/29 00:26:35, Chris Dumez wrote: > shouldBe("webkitGamepads.item.__proto__", "Function.prototype"); Done. https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:20: shouldBeEqualToString("Object.prototype.toString.call(gamepads.item)", "[object Function]"); On 2014/03/29 00:26:35, Chris Dumez wrote: > Ditto. Done. https://codereview.chromium.org/212813008/diff/30001/LayoutTests/gamepad/game... LayoutTests/gamepad/gamepad-api.html:31: shouldBeEqualToString("Object.prototype.toString.call(gamepad.id)", "[object String]"); On 2014/03/29 00:26:35, Chris Dumez wrote: > You could use __proto__ for those JS builtin types as well, e.g. > ShouldBe("gamepad.id.__proto", "String.prototype"); Done.
lgtm
The CQ bit was checked by b.kelemen@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/212813008/50001
Message was sent while issue was closed.
Change committed as 170461 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
