Bug 78559 - [EFL][DRT] Implement TextInputController.
: [EFL][DRT] Implement TextInputController.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit EFL
: 528+ (Nightly build)
: Other Linux
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks: 84710 85122 85123
  Show dependency treegraph
 
Reported: 2012-02-13 17:51 PST by Kangil Han
Modified: 2012-05-08 22:55 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.22 KB, patch)
2012-02-13 17:54 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (10.28 KB, patch)
2012-02-21 00:46 PST, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (12.62 KB, patch)
2012-03-26 03:02 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
patch (22.77 KB, patch)
2012-04-27 21:33 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
patch (23.41 KB, patch)
2012-05-06 19:39 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
patch (23.65 KB, patch)
2012-05-06 21:44 PDT, Kangil Han
no flags Details | Formatted Diff | Diff
Patch (23.73 KB, patch)
2012-05-08 16:58 PDT, Kangil Han
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kangil Han 2012-02-13 17:51:28 PST
Add initial implementation for TextInputController on DRT.
Comment 1 Kangil Han 2012-02-13 17:54:25 PST
Created attachment 126877 [details]
Patch
Comment 2 Hajime Morrita 2012-02-20 21:28:27 PST
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.
Comment 3 Kangil Han 2012-02-21 00:46:17 PST
Created attachment 127931 [details]
Patch
Comment 4 Kangil Han 2012-02-21 00:48:09 PST
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.
Comment 5 Kangil Han 2012-03-02 00:55:22 PST
I think we can replace TextInputController with internal API.
Comment 6 Hajime Morrita 2012-03-04 17:50:11 PST
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.
Comment 7 Kangil Han 2012-03-26 03:02:03 PDT
Created attachment 133758 [details]
Patch
Comment 8 Kangil Han 2012-03-26 03:05:57 PDT
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.
Comment 9 Hajime Morrita 2012-03-26 21:37:51 PDT
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.
Comment 10 Kangil Han 2012-03-26 21:53:30 PDT
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.
Comment 11 Kangil Han 2012-04-27 21:33:59 PDT
Created attachment 139335 [details]
patch
Comment 12 Kangil Han 2012-05-06 17:37:00 PDT
Dear Gyuyoung and rakuco,
Could I please get informal review on this patch?
Thanks!
Comment 13 Gyuyoung Kim 2012-05-06 18:05:37 PDT
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.
Comment 14 Kangil Han 2012-05-06 18:13:04 PDT
Thanks for the review Gyuyoung!
I will be back with a new patch. ;)
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-05-06 18:30:46 PDT
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.
Comment 16 Kangil Han 2012-05-06 19:39:07 PDT
Thanks for the review rakuco!
I will also figure it out soon. :)
Comment 17 Kangil Han 2012-05-06 19:39:53 PDT
Created attachment 140457 [details]
patch
Comment 18 Gyuyoung Kim 2012-05-06 20:10:16 PDT
Need to rebase.
Comment 19 Kangil Han 2012-05-06 20:14:04 PDT
Working. :)
Comment 20 Kangil Han 2012-05-06 21:44:09 PDT
Created attachment 140470 [details]
patch
Comment 21 Kangil Han 2012-05-06 22:41:50 PDT
Rebase and build done. :)
Comment 22 Gyuyoung Kim 2012-05-07 01:34:05 PDT
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.
Comment 23 Kangil Han 2012-05-07 23:25:01 PDT
mrobinson : Could I please get your formal review on this patch? Thanks!
Comment 24 Martin Robinson 2012-05-08 11:20:06 PDT
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.
Comment 25 Kangil Han 2012-05-08 16:43:43 PDT
gyuyoung : Could you please help me out to land this patch?
Comment 26 Kangil Han 2012-05-08 16:58:03 PDT
Created attachment 140819 [details]
Patch
Comment 27 WebKit Review Bot 2012-05-08 18:31:58 PDT
Comment on attachment 140819 [details]
Patch

Clearing flags on attachment: 140819

Committed r116475: <http://trac.webkit.org/changeset/116475>
Comment 28 WebKit Review Bot 2012-05-08 18:32:04 PDT
All reviewed patches have been landed.  Closing bug.