Bug 80126 – [EFL] Add API to set platform dependent editing behavior

• Component: WebKit EFL
• Status: RESOLVED DUPLICATE

[Sudarsana Nagineni (babu)]

Add API to set platform dependent editing behavior of editable elements.

[Sudarsana Nagineni (babu)]

Created attachment 129860 [details]
Set editing behavior

This patch adds an API to set the platform dependent editing behavior and implement layoutTestController.setEditingBehavior

[Gyuyoung Kim]

Comment on attachment 129860 [details]
Set editing behavior

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

It looks you need to read "Contributing Code" article. http://www.webkit.org/coding/contributing.html

To request review for your patch, you need to set r and cq field as '?'

> Source/WebCore/platform/efl/EflKeyboardUtilities.cpp:79
> +    keyMap().set("BackSpace", "U+0008");

It seems this may be conflict with Bug 80491.

> Tools/ChangeLog:7
> +

In my opinion, you need to write more detail description.

[Sudarsana Nagineni (babu)]

Created attachment 131601 [details]
V2: Path that implements set editing behavior

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 131601 [details]
V2: Path that implements set editing behavior

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

Thanks for the patch. A few more general comments besides those I made inline:
 * You seem to be making 3 separate changes in a single patch (adding tests to the skipped list, adding the backspace stuff and implementing ewk_view_setting_editing_behavior_set). Please split these up into separate patches.
 * Please also add a getter for the ewk_view function you have added, and properly document what the default is.

> LayoutTests/platform/efl/Skipped:211
> +editing/selection/extend-selection-after-double-click.html
> +editing/selection/rtl-move-selection-right-left.html
> +editing/selection/shift-click.html

Are these tests related to this commit? If they aren't, I'd rather commit these entries separately (and they should be sorted alphabetically with the rest of the block).

> LayoutTests/platform/efl/Skipped:233
> +editing/selection/5354455-1.html
> +editing/selection/context-menu-text-selection.html
> +editing/selection/extend-after-mouse-selection.html

Ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:3919
> +Eina_Bool ewk_view_setting_editing_behavior_set(Evas_Object* ewkView, Ewk_Editing_Behavior behavior)

In general, our setters do not return anything; I think we can do the same here.

> Source/WebKit/efl/ewk/ewk_view.h:2371
> +    EWK_EDITING_BEHAVIOR_MAC,
> +    EWK_EDITING_BEHAVIOR_WIN,
> +    EWK_EDITING_BEHAVIOR_UNIX

Please document what each of these values is supposed to mean (ie. how they are different from each other).

> Source/WebKit/efl/ewk/ewk_view.h:2377
> + * Sets the editing behavior of editable elements.

Please briefly explain what editing behavior means, and what the default is.

> Source/WebKit/efl/ewk/ewk_view.h:2384
> +EAPI Eina_Bool ewk_view_setting_editing_behavior_set(Evas_Object* o, Ewk_Editing_Behavior behavior);

This is slightly odd, but the asterisk should be next to "o" here, as we follow EFL's coding style in public headers.

[Sudarsana Nagineni (babu)]

(In reply to comment #4)

Thanks for review!

>  * You seem to be making 3 separate changes in a single patch (adding tests to the skipped list, adding the backspace stuff and implementing ewk_view_setting_editing_behavior_set). Please split these up into separate patches.

I will split this into separate patches.

>  * Please also add a getter for the ewk_view function you have added, and properly document what the default is.

Ok.
 
> > LayoutTests/platform/efl/Skipped:211
> > +editing/selection/extend-selection-after-double-click.html
> > +editing/selection/rtl-move-selection-right-left.html
> > +editing/selection/shift-click.html
> 
> Are these tests related to this commit? If they aren't, I'd rather commit these entries separately (and they should be sorted alphabetically with the rest of the block).
> 
> > LayoutTests/platform/efl/Skipped:233
> > +editing/selection/5354455-1.html
> > +editing/selection/context-menu-text-selection.html
> > +editing/selection/extend-after-mouse-selection.html
> 

These tests are already there in the skip list, but the description was wrong. So, I moved them to correct place. 

> > Source/WebKit/efl/ewk/ewk_view.cpp:3919
> > +Eina_Bool ewk_view_setting_editing_behavior_set(Evas_Object* ewkView, Ewk_Editing_Behavior behavior)
> 
> In general, our setters do not return anything; I think we can do the same here.

Ok, I will change this.

[Sudarsana Nagineni (babu)]

Created attachment 131859 [details]
Patch fixing issues pointed in comment #4

This patch includes only set/get editing behavior implementation. I will create separate bugs for missing LayoutTestController::setEditingBehavior and backspace unicode change.

[Antonio Gomes]

Comment on attachment 131859 [details]
Patch fixing issues pointed in comment #4

The original purpose of EditingBehavior class was to make different ports to behavior equally on the same platform/OS, and make this switch to happen out of the box to the programmers and users.

Could you explain the test cases of this API?

[Antonio Gomes]

> 
> Could you explain the test cases of this API?

test cases -> use cases

[Sudarsana Nagineni (babu)]

(In reply to comment #7)
> (From update of attachment 131859 [details])
> The original purpose of EditingBehavior class was to make different ports to behavior equally on the same platform/OS, and make this switch to happen out of the box to the programmers and users.
> 
Thanks for your review and info. AFAIK, there is no any real use case scenarios currently except using it in EFL LTC and enable some skipped test cases. I think moving this to DumpRenderTreeSupport is a right approach so that we don't really expose this API to users. 

I will update the patch.

[Sudarsana Nagineni (babu)]

Created attachment 133077 [details]
Patch

Added missing implementation setEditingBehavior to EFL's DumpRenderTreeSupport. I will update LayoutTestController::setEditingBehavior implementation and skipped list in bug #81124

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 133077 [details]
Patch

Looks fine to me. Now that this only adds API to DRTSupportEfl used only by LTC, it even makes sense to implement everything together in bug 81124 (bug 81130 is still a separate thing).

[Antonio Gomes]

Comment on attachment 133077 [details]
Patch

Looks good, but no tests unskipped?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #12)
> (From update of attachment 133077 [details])
> Looks good, but no tests unskipped?

The tests (and the LTC) part are being changed in bug 80124, that's why I was suggestig merging it all into a single bug/patch.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 133077 [details]
Patch

Clearing the flags; as mentioned here (and discussed on IRC), we'll land everything in bug 81124.

[Raphael Kubo da Costa (:rakuco)]

*** This bug has been marked as a duplicate of bug 81124 ***