Bug 85503

Created attachment 140027 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

Comment on attachment 140027 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling View in context: https://bugs.webkit.org/attachment.cgi?id=140027&action=review > Source/WebCore/ChangeLog:12 > + No new tests. Remove above line or write the reason why this patch don't need new test.

Created attachment 140168 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling fix changelog

(In reply to comment #2) > (From update of attachment 140027 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140027&action=review > > > Source/WebCore/ChangeLog:12 > > + No new tests. > > Remove above line or write the reason why this patch don't need new test. Done.

Comment on attachment 140168 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling View in context: https://bugs.webkit.org/attachment.cgi?id=140168&action=review Are there more commits related to adding keys to this file in your queue? > LayoutTests/ChangeLog:3 > + [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling Nit: '[EFL] foo' instead of '[EFL]foo' > Source/WebCore/ChangeLog:3 > + [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling Ditto. > Source/WebCore/ChangeLog:9 > + Corrected valure for printscreen key in the windowsKeyMap. Nit: s/valure/value/. > Source/WebCore/ChangeLog:10 > + Figures and letters keys are added to the keyMap. > + Corrected valure for printscreen key in the windowsKeyMap. > + Capital letters keys are added to the windowsKeyMap. This description can be moved to the respective methods they are related to below. > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:97 > + // Set alphabet to the keyMap. > + const char* lows = "abcdefghijklmnopqrstuvwxyz"; > + const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; > + const int lowsBase = static_cast<int>(*lows); > + const int capsBase = static_cast<int>(*caps); > + > + for (unsigned int i = 0; i < 26; i++) { > + keyMap().set(String(lows + i, 1), String::format("U+%04X", lowsBase + i)); > + keyMap().set(String(caps + i, 1), String::format("U+%04X", capsBase + i)); > + } > + > + // Set digits to the keyMap > + const int digitBase = static_cast<int>('0'); > + for (unsigned int i = 0; i < 10; i++) > + keyMap().set(String::number(i), String::format("U+%04X", digitBase + i)); How about static void addCharacterToKeyMap(char character) { keyMap().set(String(&character, 1), String::format("U+%04X", character)); } static void createKeyMap() { // ... for (char c = 'a'; c <= 'z'; c++) addCharacterToKeyMap(c); for (char c = 'A'; c <= 'Z'; c++) addCharacterToKeyMap(c); for (char c = '0'; c <= '9'; c++) addCharacterToKeyMap(c); // .... } > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:155 > - const char* alphabet = "abcdefghijklmnopqrstuvwxyz"; > + const char* lows = "abcdefghijklmnopqrstuvwxyz"; > + const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; > for (unsigned int i = 0; i < 26; i++) { This can be simplified in a way similar to the one I described above. > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:209 > + if (keyName == "Print") > + return String(""); Are you sure this is correct? Neither the Qt nor the GTK+ port does something similar.

Created attachment 140514 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

(In reply to comment #5) > (From update of attachment 140168 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140168&action=review > > Are there more commits related to adding keys to this file in your queue? > > > LayoutTests/ChangeLog:3 > > + [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling > > Nit: '[EFL] foo' instead of '[EFL]foo' > > > Source/WebCore/ChangeLog:3 > > + [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling > > Ditto. > > > Source/WebCore/ChangeLog:9 > > + Corrected valure for printscreen key in the windowsKeyMap. > > Nit: s/valure/value/. > > > Source/WebCore/ChangeLog:10 > > + Figures and letters keys are added to the keyMap. > > + Corrected valure for printscreen key in the windowsKeyMap. > > + Capital letters keys are added to the windowsKeyMap. > > This description can be moved to the respective methods they are related to below. > > > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:97 > > + // Set alphabet to the keyMap. > > + const char* lows = "abcdefghijklmnopqrstuvwxyz"; > > + const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; > > + const int lowsBase = static_cast<int>(*lows); > > + const int capsBase = static_cast<int>(*caps); > > + > > + for (unsigned int i = 0; i < 26; i++) { > > + keyMap().set(String(lows + i, 1), String::format("U+%04X", lowsBase + i)); > > + keyMap().set(String(caps + i, 1), String::format("U+%04X", capsBase + i)); > > + } > > + > > + // Set digits to the keyMap > > + const int digitBase = static_cast<int>('0'); > > + for (unsigned int i = 0; i < 10; i++) > > + keyMap().set(String::number(i), String::format("U+%04X", digitBase + i)); > > How about > > static void addCharacterToKeyMap(char character) > { > keyMap().set(String(&character, 1), String::format("U+%04X", character)); > } > > static void createKeyMap() > { > // ... > > for (char c = 'a'; c <= 'z'; c++) > addCharacterToKeyMap(c); > for (char c = 'A'; c <= 'Z'; c++) > addCharacterToKeyMap(c); > for (char c = '0'; c <= '9'; c++) > addCharacterToKeyMap(c); > > // .... > } > > > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:155 > > - const char* alphabet = "abcdefghijklmnopqrstuvwxyz"; > > + const char* lows = "abcdefghijklmnopqrstuvwxyz"; > > + const char* caps = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; > > for (unsigned int i = 0; i < 26; i++) { > > This can be simplified in a way similar to the one I described above. > Fixed all above. > > Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:209 > > + if (keyName == "Print") > > + return String(""); > > Are you sure this is correct? Neither the Qt nor the GTK+ port does something similar. This function is used to evaluate text() and unmodifiedText() properties for the event, which should be empty in this case. Actually GTK+ port has something similar as gdk_keyval_to_unicode() returns 0 for printscreen key.

(In reply to comment #5) > (From update of attachment 140168 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140168&action=review > > Are there more commits related to adding keys to this file in your queue? There is also bug85479 fixing numeric pad keys related tests.

Comment on attachment 140514 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling Looks good, thanks!

Comment on attachment 140514 [details] EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling Rejecting attachment 140514 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ng file Source/WebCore/platform/efl/EflKeyboardUtilities.cpp Hunk #2 FAILED at 84. Hunk #3 succeeded at 126 (offset 11 lines). Hunk #4 succeeded at 161 with fuzz 1 (offset 22 lines). Hunk #5 succeeded at 209 (offset 22 lines). 1 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/platform/efl/EflKeyboardUtilities.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Gustavo No..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12703548

Rebase please.

Created attachment 142176 [details] to be landed.

Comment on attachment 142176 [details] to be landed. Clearing flags on attachment: 142176 Committed r117238: <http://trac.webkit.org/changeset/117238>

All reviewed patches have been landed. Closing bug.