Bug 85503

Summary: [EFL]PlatformKeyboardEvent: figures, letters and printscreen key handling
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85369, 86200    
Attachments:
Description Flags
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling
none
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling
none
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling
gustavo: review+, webkit.review.bot: commit-queue-
to be landed. none

Description Mikhail Pozdnyakov 2012-05-03 08:46:01 PDT
figures, letters and printscreen key should be added to keyMap and windowsKeyMap in order to unskip:
fast/events/special-key-events-in-input-text.html
fast/forms/enter-clicks-buttons.html
and also improve input for
fast/events/key-events-in-input-button.html
fast/events/key-events-in-input-text.html
Comment 1 Mikhail Pozdnyakov 2012-05-03 08:58:15 PDT
Created attachment 140027 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling
Comment 2 Gyuyoung Kim 2012-05-03 19:22:54 PDT
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.
Comment 3 Mikhail Pozdnyakov 2012-05-03 23:45:56 PDT
Created attachment 140168 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

fix changelog
Comment 4 Mikhail Pozdnyakov 2012-05-03 23:47:02 PDT
(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 5 Raphael Kubo da Costa (:rakuco) 2012-05-06 18:20:06 PDT
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.
Comment 6 Mikhail Pozdnyakov 2012-05-07 05:43:04 PDT
Created attachment 140514 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling
Comment 7 Mikhail Pozdnyakov 2012-05-07 05:54:50 PDT
(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.
Comment 8 Mikhail Pozdnyakov 2012-05-07 06:11:25 PDT
(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 9 Raphael Kubo da Costa (:rakuco) 2012-05-07 08:19:36 PDT
Comment on attachment 140514 [details]
EFL' PlatformKeyboardEvent: figures, letters and printscreen key handling

Looks good, thanks!
Comment 10 WebKit Review Bot 2012-05-15 12:22:39 PDT
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
Comment 11 Gyuyoung Kim 2012-05-15 19:04:56 PDT
Rebase please.
Comment 12 Mikhail Pozdnyakov 2012-05-16 00:26:45 PDT
Created attachment 142176 [details]
to be landed.
Comment 13 WebKit Review Bot 2012-05-16 02:11:46 PDT
Comment on attachment 142176 [details]
to be landed.

Clearing flags on attachment: 142176

Committed r117238: <http://trac.webkit.org/changeset/117238>
Comment 14 WebKit Review Bot 2012-05-16 02:11:52 PDT
All reviewed patches have been landed.  Closing bug.