Bug 78559 – [EFL][DRT] Implement TextInputController.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

Add initial implementation for TextInputController on DRT.

[Kangil Han]

Created attachment 126877 [details]
Patch

[Hajime Morrita]

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.

[Kangil Han]

Created attachment 127931 [details]
Patch

[Kangil Han]

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.

[Kangil Han]

I think we can replace TextInputController with internal API.

[Hajime Morrita]

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.

[Kangil Han]

Created attachment 133758 [details]
Patch

[Kangil Han]

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.

[Hajime Morrita]

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.

[Kangil Han]

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.

[Kangil Han]

Created attachment 139335 [details]
patch

[Kangil Han]

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

[Gyuyoung Kim]

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.

[Kangil Han]

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

[Raphael Kubo da Costa (:rakuco)]

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.

[Kangil Han]

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

[Kangil Han]

Created attachment 140457 [details]
patch

[Gyuyoung Kim]

Need to rebase.

[Kangil Han]

Working. :)

[Kangil Han]

Created attachment 140470 [details]
patch

[Kangil Han]

Rebase and build done. :)

[Gyuyoung Kim]

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.

[Kangil Han]

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

[Martin Robinson]

Comment on attachment 140470 [details]
patch

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

> LayoutTests/ChangeLog:4631
> [phone]  Kangil Han  <[e-mail]>

The ChangeLog didn't merge properly here.

> Source/WebKit/efl/ChangeLog:173
> [phone]  Kangil Han  <[e-mail]>

Ditto.

> Tools/ChangeLog:1658
> [phone]  Kangil Han  <[e-mail]>

Ditto.

[Kangil Han]

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

[Kangil Han]

Created attachment 140819 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140819 [details]
Patch

Clearing flags on attachment: 140819

Committed r116475: <http://trac.webkit.org/changeset/116475>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.