Bug 84710

Created attachment 139411 [details] patch

Comment on attachment 139411 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=139411&action=review > LayoutTests/ChangeLog:8 > + Non-visible key string, i.e. right arrow key, shouldn't be displayed on the input field. I don't understand what is problem in this description. > Tools/DumpRenderTree/efl/EventSender.cpp:107 > + KeyEventInfo(const CString& keyName, EvasKeyModifier modifiers, Eina_Bool hasVisibleString) Please do not use Eina_Bool. > Tools/DumpRenderTree/efl/EventSender.cpp:116 > + Eina_Bool hasVisibleString; We decided to use standard boolean instead of Eina_Bool except for public APIs. > Tools/DumpRenderTree/efl/EventSender.cpp:447 > + return new KeyEventInfo(character.get()->ustring().utf8(), modifiers, EINA_TRUE); Use true instead of EINA_TRUE. > Tools/DumpRenderTree/efl/EventSender.cpp:449 > + return new KeyEventInfo(keyName, modifiers, EINA_FALSE); ditto.

Created attachment 139421 [details] patch

Thanks for reviewing! I've fixed all mentioned parts.

Created attachment 139431 [details] patch

Comment on attachment 139431 [details] patch Could you please rebase and upload this?

Yes, I will as soon as BUG 78559 is landed. ;)

Created attachment 140848 [details] Patch

Rebase done. :)

Created attachment 140851 [details] Patch

Rebase again since chromium-ews complained about device space. :)

gyuyoung, rakuco : Could I please get informal review on this patch? Thanks!

Comment on attachment 140851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140851&action=review > Tools/DumpRenderTree/efl/EventSender.cpp:387 > + return CString(); Hmm, why do you return CString() instead of character->ustring().utf8()? It looks we need to return character itself when character is not supported by this function. > Tools/DumpRenderTree/efl/EventSender.cpp:451 > + return CString(); ditto.

In this case, blank string indicates pressed key is not a character. So 'character->ustring().utf8()' is happened when creating KeyEventInfo. (In reply to comment #13) > (From update of attachment 140851 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140851&action=review > > > Tools/DumpRenderTree/efl/EventSender.cpp:387 > > + return CString(); > > Hmm, why do you return CString() instead of character->ustring().utf8()? It looks we need to return character itself when character is not supported by this function. > > > Tools/DumpRenderTree/efl/EventSender.cpp:451 > > + return CString(); > > ditto.

gyuyoung : Any more comment(s)?

Comment on attachment 140851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140851&action=review > Source/WebCore/ChangeLog:14 > + (WebCore::singleCharacterString): Please mention what is being changed and why here. > Tools/DumpRenderTree/efl/EventSender.cpp:481 > + if (keyName.isNull()) > + return new KeyEventInfo(character.get()->ustring().utf8(), modifiers, true); > + if (keyName == "Return" || keyName == "Tab" || keyName == "BackSpace" || keyName == "space") > + return new KeyEventInfo(keyName, modifiers, true); > + > + return new KeyEventInfo(keyName, modifiers, false); This change of logic here is very confusing, it took me a while to figure out what the code was supposed to do. The code should be more explicit in what it does. My suggestion: add another CString member, `keyString' (or something better) to KeyEventInfo which is then passed as the 4th parameter to evas_event_feed_key_{down,up} so you don't need to put additional meaning into keyName itself. You can even make keyPadNameFromJSValue and keyNameFromJSValue return a KeyEventInfo themselves if that makes the code more simple.

Created attachment 141667 [details] Patch

(In reply to comment #16) > (From update of attachment 140851 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140851&action=review > > > Source/WebCore/ChangeLog:14 > > + (WebCore::singleCharacterString): > > Please mention what is being changed and why here. > > > Tools/DumpRenderTree/efl/EventSender.cpp:481 > > + if (keyName.isNull()) > > + return new KeyEventInfo(character.get()->ustring().utf8(), modifiers, true); > > + if (keyName == "Return" || keyName == "Tab" || keyName == "BackSpace" || keyName == "space") > > + return new KeyEventInfo(keyName, modifiers, true); > > + > > + return new KeyEventInfo(keyName, modifiers, false); > > This change of logic here is very confusing, it took me a while to figure out what the code was supposed to do. The code should be more explicit in what it does. > > My suggestion: add another CString member, `keyString' (or something better) to KeyEventInfo which is then passed as the 4th parameter to evas_event_feed_key_{down,up} so you don't need to put additional meaning into keyName itself. You can even make keyPadNameFromJSValue and keyNameFromJSValue return a KeyEventInfo themselves if that makes the code more simple. Good suggestion rakuco! Thanks! Please review my latest patch if I've got your idea. :)

rakuco: Could I please get your review?

Comment on attachment 141667 [details] Patch Looks good now, thank you. If it does not cause any regression in other tests, informal r+.

Comment on attachment 141667 [details] Patch Clearing flags on attachment: 141667 Committed r117218: <http://trac.webkit.org/changeset/117218>

All reviewed patches have been landed. Closing bug.

There was another bug marked as blocked by this issue in test_expectations.txt // EventSender doesn't provide correct key string BUGWK84710 : fast/forms/textarea-type-spaces.html = TEXT resulting in an unexpected pass. I am removing that line from test_expectations.txt in gardening bug 86584.

drott: Thanks!