Bug 78559 - [EFL][DRT] Implement TextInputController.
: [EFL][DRT] Implement TextInputController.
Status: RESOLVED FIXED
: WebKit
WebKit EFL
: 528+ (Nightly build)
: Other Linux
: P2 Normal
Assigned To:
:
:
:
: 84710 85122 85123
  Show dependency treegraph
 
Reported: 2012-02-13 17:51 PST by
Modified: 2012-05-08 22:55 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-13 17:51:28 PST
Add initial implementation for TextInputController on DRT.
------- Comment #1 From 2012-02-13 17:54:25 PST -------
Created an attachment (id=126877) [details]
Patch
------- Comment #2 From 2012-02-20 21:28:27 PST -------
(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 #3 From 2012-02-21 00:46:17 PST -------
Created an attachment (id=127931) [details]
Patch
------- Comment #4 From 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] [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 From 2012-03-02 00:55:22 PST -------
I think we can replace TextInputController with internal API.
------- Comment #6 From 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 From 2012-03-26 03:02:03 PST -------
Created an attachment (id=133758) [details]
Patch
------- Comment #8 From 2012-03-26 03:05:57 PST -------
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 From 2012-03-26 21:37:51 PST -------
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 From 2012-03-26 21:53:30 PST -------
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 From 2012-04-27 21:33:59 PST -------
Created an attachment (id=139335) [details]
patch
------- Comment #12 From 2012-05-06 17:37:00 PST -------
Dear Gyuyoung and rakuco,
Could I please get informal review on this patch?
Thanks!
------- Comment #13 From 2012-05-06 18:05:37 PST -------
(From update of attachment 139335 [details])
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 From 2012-05-06 18:13:04 PST -------
Thanks for the review Gyuyoung!
I will be back with a new patch. ;)
------- Comment #15 From 2012-05-06 18:30:46 PST -------
(From update of attachment 139335 [details])
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 From 2012-05-06 19:39:07 PST -------
Thanks for the review rakuco!
I will also figure it out soon. :)
------- Comment #17 From 2012-05-06 19:39:53 PST -------
Created an attachment (id=140457) [details]
patch
------- Comment #18 From 2012-05-06 20:10:16 PST -------
Need to rebase.
------- Comment #19 From 2012-05-06 20:14:04 PST -------
Working. :)
------- Comment #20 From 2012-05-06 21:44:09 PST -------
Created an attachment (id=140470) [details]
patch
------- Comment #21 From 2012-05-06 22:41:50 PST -------
Rebase and build done. :)
------- Comment #22 From 2012-05-07 01:34:05 PST -------
(From update of attachment 140470 [details])
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 From 2012-05-07 23:25:01 PST -------
mrobinson : Could I please get your formal review on this patch? Thanks!
------- Comment #24 From 2012-05-08 11:20:06 PST -------
(From update of attachment 140470 [details])
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 From 2012-05-08 16:43:43 PST -------
gyuyoung : Could you please help me out to land this patch?
------- Comment #26 From 2012-05-08 16:58:03 PST -------
Created an attachment (id=140819) [details]
Patch
------- Comment #27 From 2012-05-08 18:31:58 PST -------
(From update of attachment 140819 [details])
Clearing flags on attachment: 140819

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