Add API to set platform dependent editing behavior of editable elements.
Created attachment 129860 [details] Set editing behavior This patch adds an API to set the platform dependent editing behavior and implement layoutTestController.setEditingBehavior
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.
Created attachment 131601 [details] V2: Path that implements set editing behavior
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.
(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.
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.
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?
> > Could you explain the test cases of this API? test cases -> use cases
(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.
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
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).
Comment on attachment 133077 [details] Patch Looks good, but no tests unskipped?
(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.
Comment on attachment 133077 [details] Patch Clearing the flags; as mentioned here (and discussed on IRC), we'll land everything in bug 81124.
*** This bug has been marked as a duplicate of bug 81124 ***