Bug 193452

Created attachment 359173 [details] Patch and layout tests

Although the test will fail (or may even timeout) when using a build of WebKit with the public SDK I am curious to see what the results are. I will amend the patch before landing as appropriate to ensure that this patch does not cause test failures.

Attachment 359173 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/PlatformEventFactoryIOS.mm:427: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to Daniel Bates from comment #2) > Although the test will fail (or may even timeout) when using a build of > WebKit with the public SDK I am curious to see what the results are. I will > amend the patch before landing as appropriate to ensure that this patch does > not cause test failures. It didn't fail. I'm a bit surprised, but it could be that the test isn't comprehensive enough (say, testing function keys and arrow keys, et cetera). I do not suspect the test passing represents a bug in the patch. It's hard to keep all the bugs I've fixed and the side effects of each bug in my head. I'm happy the test passes. We'll see if we can keep the test green and avoid skipping it with the public SDK in a future iteration where I make it more comprehensive.

Comment on attachment 359173 [details] 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?

Comment on attachment 359173 [details] 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:166 > + for (let propertyName of ["type", "key", "code", "keyIdentifier", "keyCode", "charCode", "keyCode", "which", "altKey", "ctrlKey", "metaKey", "shiftKey"]) Why not also dump location / keyLocation?

Comment on attachment 359173 [details] 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? OK. >> 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. Changed this code to read: // For each ordinal 1, 2, ... numberOfNonEmptyPermutations we look at its binary representation // and generate a permutation that consists of the entries in anArray at the indices where there // is a one bit in the binary representation. For example, suppose anArray = ["metaKey", "altKey", "ctrlKey"]. // To generate the 5th permutation we look at the binary representation of i = 5 => 0b101. And // compute the permutation to be [ anArray[0], anArray[2] ] = [ "metaKey", "ctrlKey" ] because // the 0th and 2nd bits are ones in the binary representation. for (let i = 1; i <= numberOfNonEmptyPermutations; ++i) { let temp = []; for (let bitmask = i, j = 0; bitmask; bitmask = Math.floor(bitmask / 2), ++j) { if (bitmask % 2) temp.push(anArray[j]); } result.push(temp); } >> LayoutTests/fast/events/ios/resources/key-tester.js:95 >> +let disallowedKeyCommands = [ > > Use const? OK. >> LayoutTests/fast/events/ios/resources/key-tester.js:115 >> +let tests = []; > > Ditto. OK. >> LayoutTests/fast/events/ios/resources/key-tester.js:166 >> + for (let propertyName of ["type", "key", "code", "keyIdentifier", "keyCode", "charCode", "keyCode", "which", "altKey", "ctrlKey", "metaKey", "shiftKey"]) > > Why not also dump location / keyLocation? OK. >> LayoutTests/fast/events/ios/resources/key-tester.js:181 >> + > > Maybe remove this blank line? Will fix. >> 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 = ~? Will change this code to read: const numberOfNonModifierKeys = 1; numberOfKeyUpsRemaining = currentTest.modifiers.length + numberOfNonModifierKeys; >> 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. I'll consider this depending on whether this code can be re-used in other places. I think it can! I may defer doing this cleanup until I find the second call site and choose to do it in its own patch. >> 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? The criterion to decide if an event is from the numeric keypad. UIKit |= event.modifierFlags with UIKeyModifierNumericPad if the event's key code corresponds to one of the key codes in the below switch block or is a keypad comma.

Created attachment 360188 [details] To Land

Committed r240604: <https://trac.webkit.org/changeset/240604>

<rdar://problem/47612503>

(In reply to Daniel Bates from comment #7) > >> LayoutTests/fast/events/ios/resources/key-tester.js:95 > >> +let disallowedKeyCommands = [ > > > > Use const? > > OK. > > >> LayoutTests/fast/events/ios/resources/key-tester.js:115 > >> +let tests = []; > > > > Ditto. > > OK. Oops! I said I would make these changes, but I forgot. When I have a moment, I will look to land a follow up fix.