Bug 87654

Summary: [EFL] Refactoring. Get rid of unnecessary singleCharacterString() function
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch v2 (Review remarks are met) none

Description Mikhail Pozdnyakov 2012-05-28 06:10:33 PDT
Evas key events already provide key string, for instance if the User presses 'Return' key the generated Evas_Event_Key_Down(_Up) events will contain following fields:
keyname = "Return", key = "Return", string = "\r", compose = "\r"
So no practical need for singleCharacterString() evaluating the event string from event keyname.
The only place where this function is currently used is DRT, however DRT EventSender also can provide event string itself.
Comment 1 Mikhail Pozdnyakov 2012-05-28 06:25:25 PDT
Created attachment 144345 [details]
patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-05-28 06:34:30 PDT
Comment on attachment 144345 [details]
patch

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

> Tools/DumpRenderTree/efl/EventSender.cpp:124
> +    KeyEventInfo(const CString& keyName, unsigned modifiers)
> +        : keyName(keyName)
> +        , modifiers(modifiers)

Instead of adding a new constructor, I'd change the order of parameters in the other one and make keyString an optional last parameter whose default value is an empty string.
Comment 3 Mikhail Pozdnyakov 2012-05-28 07:05:29 PDT
Created attachment 144353 [details]
patch v2 (Review remarks are met)
Comment 4 Mikhail Pozdnyakov 2012-05-28 07:06:40 PDT
(In reply to comment #2)
> (From update of attachment 144345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144345&action=review
> 
> > Tools/DumpRenderTree/efl/EventSender.cpp:124
> > +    KeyEventInfo(const CString& keyName, unsigned modifiers)
> > +        : keyName(keyName)
> > +        , modifiers(modifiers)
> 
> Instead of adding a new constructor, I'd change the order of parameters in the other one and make keyString an optional last parameter whose default value is an empty string.

done.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-05-28 07:19:52 PDT
Comment on attachment 144353 [details]
patch v2 (Review remarks are met)

Looks good, thanks.
Comment 6 Gyuyoung Kim 2012-05-28 18:14:30 PDT
Comment on attachment 144353 [details]
patch v2 (Review remarks are met)

Looks good to me too.
Comment 7 Csaba Osztrogonác 2012-05-29 04:48:34 PDT
Comment on attachment 144353 [details]
patch v2 (Review remarks are met)

rs=me
Comment 8 WebKit Review Bot 2012-05-29 05:13:18 PDT
Comment on attachment 144353 [details]
patch v2 (Review remarks are met)

Clearing flags on attachment: 144353

Committed r118760: <http://trac.webkit.org/changeset/118760>
Comment 9 WebKit Review Bot 2012-05-29 05:13:22 PDT
All reviewed patches have been landed.  Closing bug.