Bug 78559

Created attachment 126877 [details] Patch

Comment on attachment 126877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126877&action=review > Tools/DumpRenderTree/efl/TextInputController.cpp:14 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of I guess you don't need to claim anything about Apple ;-) > Tools/DumpRenderTree/efl/TextInputController.cpp:40 > +static JSValueRef setMarkedTextCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) Typically we don't give argument names which are not used in the function. Some compiler actually complain about that. > Tools/DumpRenderTree/efl/TextInputController.h:14 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of Ditto for license.

Created attachment 127931 [details] Patch

Done and thank you for reviewing my patch! :) (In reply to comment #2) > (From update of attachment 126877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126877&action=review > > > Tools/DumpRenderTree/efl/TextInputController.cpp:14 > > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > > I guess you don't need to claim anything about Apple ;-) > > > Tools/DumpRenderTree/efl/TextInputController.cpp:40 > > +static JSValueRef setMarkedTextCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception) > > Typically we don't give argument names which are not used in the function. > Some compiler actually complain about that. > > > Tools/DumpRenderTree/efl/TextInputController.h:14 > > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > > Ditto for license.

I think we can replace TextInputController with internal API.

Hi Kangil, thanks for caring this! (In reply to comment #5) > I think we can replace TextInputController with internal API. It depends. Some API is good fit for internals. But many of them needs its own API. These APIs are for emulating user input, especially IME. You can think this more like MockClient than Internals. I think having this implementation inside WebCoreTestSupport could be a great idea. But it should be different object from Internals. Anyway, thanks again to consider better design over existing one. TextInputController is an old resident on DRT world which needs some improvement.

Created attachment 133758 [details] Patch

This patch has excluded functions used in port specific test cases at this stage. IMHO, I shouldn't touch webview related implementations for port specific purposes.

It looks my last explanation was confused. I'm sorry about that. Now, I cannot understand the purpose of this change. Whole purpose of TextInputController is to - inspect platform's text input API state. For mac, it's NSTextInput, and to - emulate user text input against the platform's text input API. It's also NSTextInput on mac. Following this pattern, what you need is to invoke EFL's text input interface through TextInputController And this is required only if EFL port is going to implement EFL's (or some other underlying platform's) text input protocol. If EFL isn't going to support any text input protocol, it makes little sense to support TextInputController since it is designed to test that feature. You can just skip tests which exercise text input functionality. I know both GTK and Qt implement TextInputController in a similar way. But in my opinion, it's kinda autotelic. Having yet another fake implementation just for mocking TextInputController will be a pure waste. I think you can discuss, say, GTK fork and move their TextInputController with this WebCoreTestSupport version. Then you can employ it for EFL. By doing it, - we can avoid code duplication at least. - we can eliminate manual JS binding code. This will help both GTK and EFL port. Als, the class name would be MockTextInputController or something. It cannot be common for the reason.

Your point makes sense. I will discard this and make a patch for EFL only. (In reply to comment #9) > It looks my last explanation was confused. I'm sorry about that. > > Now, I cannot understand the purpose of this change. > Whole purpose of TextInputController is to > > - inspect platform's text input API state. For mac, it's NSTextInput, and to > - emulate user text input against the platform's text input API. It's also NSTextInput on mac. > > Following this pattern, what you need is to invoke EFL's text input interface through TextInputController > And this is required only if EFL port is going to implement EFL's (or some other underlying platform's) > text input protocol. > > If EFL isn't going to support any text input protocol, it makes little sense to support TextInputController > since it is designed to test that feature. You can just skip tests which exercise text input functionality. > > I know both GTK and Qt implement TextInputController in a similar way. > But in my opinion, it's kinda autotelic. > Having yet another fake implementation just for mocking TextInputController > will be a pure waste. > > I think you can discuss, say, GTK fork and move their TextInputController with this > WebCoreTestSupport version. Then you can employ it for EFL. > By doing it, > - we can avoid code duplication at least. > - we can eliminate manual JS binding code. > This will help both GTK and EFL port. > > Als, the class name would be MockTextInputController or something. > It cannot be common for the reason.

Created attachment 139335 [details] patch

Dear Gyuyoung and rakuco, Could I please get informal review on this patch? Thanks!

Comment on attachment 139335 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=139335&action=review Basically, this patch move GTK's implementation to EFL port. So, it seems to me there is no critical issue. > LayoutTests/ChangeLog:8 > + Implemented textInputController for EFL DRT. I think this is unsufficient description for this patch. > Source/WebKit/efl/ChangeLog:8 > + Implemented textInputController for EFL DRT. ditto. > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:643 > +{ Is it better to initialize start and length like gtk port ? I think we need to initialize start and length. http://trac.webkit.org/browser/trunk/Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp#L478 > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:670 > + } Nit : Add an empty line to here. > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:698 > +{ Add a condition to check if start and length variables are 0.

Thanks for the review Gyuyoung! I will be back with a new patch. ;)

Comment on attachment 139335 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=139335&action=review I can't help much here because text editing is not an area I'm familiar with, and most of the patch seems to be common, boilerplate code. It'd be good if some reviewer who works on editing stuff could take a look at this before landing. > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:623 > + String compositionString = String::fromUTF8(text); Could be const. > Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp:629 > +bool DumpRenderTreeSupportEfl::hasComposition(Evas_Object* ewkView) ewkView, page and editor can be const in this method. > Tools/DumpRenderTree/efl/TextInputController.h:28 > +#ifndef TextInputController_h Nit: we generally separate the copyright header from the ifdef guards with an empty line.

Thanks for the review rakuco! I will also figure it out soon. :)

Created attachment 140457 [details] patch

Need to rebase.

Working. :)

Created attachment 140470 [details] patch

Rebase and build done. :)

Comment on attachment 140470 [details] patch I'm also not expert in this area. But, this patch copies GTK's implementation to EFL port. It looks this patch doesn't have critical problems and nits. So, looks fine to me for now. I think editor review or GTK reviewer can review this patch.

mrobinson : Could I please get your formal review on this patch? Thanks!

Comment on attachment 140470 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=140470&action=review > LayoutTests/ChangeLog:4631 > +2012-04-27 Kangil Han <kangil.han@samsung.com> The ChangeLog didn't merge properly here. > Source/WebKit/efl/ChangeLog:173 > +2012-04-27 Kangil Han <kangil.han@samsung.com> Ditto. > Tools/ChangeLog:1658 > +2012-04-27 Kangil Han <kangil.han@samsung.com> Ditto.

gyuyoung : Could you please help me out to land this patch?

Created attachment 140819 [details] Patch

Comment on attachment 140819 [details] Patch Clearing flags on attachment: 140819 Committed r116475: <http://trac.webkit.org/changeset/116475>

All reviewed patches have been landed. Closing bug.