[webkit-reviews] review granted: [Bug 193452] [iOS] Make Window virtual key code computation match Mac : [Attachment 359173] Patch and layout tests

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 193452: [iOS] Make Window virtual key code computation match Mac
https://bugs.webkit.org/show_bug.cgi?id=193452

Attachment 359173: Patch and layout tests

https://bugs.webkit.org/attachment.cgi?id=359173&action=review




--- Comment #5 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 359173
  --> https://bugs.webkit.org/attachment.cgi?id=359173
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=359173&action=review

> LayoutTests/fast/events/ios/resources/key-tester.js:39
> +    constructor(key, modifiers = []) {

Put { on new line to be consistent?

> LayoutTests/fast/events/ios/resources/key-tester.js:77
> +	   for (let j = i, k = 0; j; j = Math.floor(j / 2), ++k) {

Can we use more describe variable names than i and j?
It's a bit confusing as to what's happening here.

> LayoutTests/fast/events/ios/resources/key-tester.js:95
> +let disallowedKeyCommands = [

Use const?

> LayoutTests/fast/events/ios/resources/key-tester.js:115
> +let tests = [];

Ditto.

> LayoutTests/fast/events/ios/resources/key-tester.js:181
> +

Maybe remove this blank line?

> LayoutTests/fast/events/ios/resources/key-tester.js:201
> +    numberOfKeyUpsRemaining = currentTest.modifiers.length + 1 /*
non-modifier key */;

Instead of adding a comment, why don't we declare a local variable named
nonModiferKey = ~?

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:425
> +    // Check that this is the type of event that has a keyCode.

Can we just extract the switch below as a helper function like
eventHasKeyCode(~) and remove this comment?
This function is quite comment heavy.

> Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:434
> +    // With the exception of keypad comma, the following corresponds to the
criterion for UIKeyModifierNumericPad.

criterion to decide what?