Bug 84835 – [EFL] [DRT] EFL's DumpRenderTree should support LayoutTestController's dumpEditingCallbacks()

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

[Sudarsana Nagineni (babu)]

EFL's DumpRenderTree needs to support LayoutTestController's dumpEditingCallbacks() in order to unksip several tests from the skip list.

[Mariusz Grzegorczyk]

(In reply to comment #0)
> EFL's DumpRenderTree needs to support LayoutTestController's dumpEditingCallbacks() in order to unksip several tests from the skip list.

Are you preparing patch for this bug?

[Sudarsana Nagineni (babu)]

(In reply to comment #1)
> Are you preparing patch for this bug?

No. I have not started working on this yet.

[Mariusz Grzegorczyk]

As discussed with Sudarsana Nagineni I'm preparing patch for this issue.

[Mikhail Pozdnyakov]

*** Bug 85871 has been marked as a duplicate of this bug. ***

[Mariusz Grzegorczyk]

Created attachment 140934 [details]
[EFL] Editing callbacks implementation

[Grzegorz Czajkowski]

What are tests passed with this patch?
You should update Skipped or test_expectations.txt files.

[Gyuyoung Kim]

Comment on attachment 140934 [details]
[EFL] Editing callbacks implementation

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:95
> +bool EditorClientEfl::shouldInsertText(const String& string, Range* range, EditorInsertAction action)

string is ambiguous variable name. I think it is good to use more clear name.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:61
> +static const char* insertActionString(WebCore::EditorInsertAction action)

Why do you return const char* instead of WTF::String ? I think it is better to return consistent value with above functions.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:87
> +void shouldBeginEditing(void*, Evas_Object*, void* event_info)

Do not use _ in parameter.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:95
> +void shouldEndEditing(void*, Evas_Object*, void* event_info)

ditto.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:103
> +void shouldInsertNode(void*, Evas_Object*, void* event_info)

ditto.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:113
> +void shouldInsertText(void*, Evas_Object*, void* event_info)

ditto.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:122
> +void shouldDeleteRange(void*, Evas_Object*, void* event_info)

ditto.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:130
> +void shouldChangeSelectedRange(void*, Evas_Object*, void* event_info)

ditto.

> Tools/DumpRenderTree/efl/EditingCallbacks.cpp:140
> +void shouldApplyStyle(void*, Evas_Object*, void* event_info)

ditto.

[Mariusz Grzegorczyk]

(In reply to comment #7)

> > Tools/DumpRenderTree/efl/EditingCallbacks.cpp:61
> > +static const char* insertActionString(WebCore::EditorInsertAction action)
> 
> Why do you return const char* instead of WTF::String ? I think it is better to return consistent value with above functions.


dumpPath and dumpRange generates strings, so WTF::String is used to simplify, but other helper functions just returns one of predefined strings, so to avoid conversion to String and than to char* it is better to leave it as is. Same policy is chosen in gtk port. See EditingCallbacks.cpp in gtk.

[Mariusz Grzegorczyk]

Created attachment 142444 [details]
[EFL] Editing callbacks implementation

Rebased, update with Gyuyoung's comments.

[Mariusz Grzegorczyk]

(In reply to comment #6)
> What are tests passed with this patch?
> You should update Skipped or test_expectations.txt files.

Skipped and test_expectations.txt will be updated soon in separate patch under this bug.

[Eric Seidel (no email)]

Comment on attachment 142444 [details]
[EFL] Editing callbacks implementation

rs=me.

[WebKit Review Bot]

Comment on attachment 142444 [details]
[EFL] Editing callbacks implementation

Clearing flags on attachment: 142444

Committed r117426: <http://trac.webkit.org/changeset/117426>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 142444 [details]
[EFL] Editing callbacks implementation

ewk_view.h has not been updated with the new signals being emitted.

[Raphael Kubo da Costa (:rakuco)]

Plus API users cannot really use these new signals since the structs are defined in EditorClientEfl.h, which is not installed.

[Raphael Kubo da Costa (:rakuco)]

Please fix these issues ASAP, otherwise I am inclined to revert this until these issues are properly addressed.

[Mariusz Grzegorczyk]

(In reply to comment #16)
> Please fix these issues ASAP, otherwise I am inclined to revert this until these issues are properly addressed.

From the beginning they were NOT being exported to API. They are used internally by DRT. In previous patch they were in ewk_private.h not ewk_view.h, so also were not installed. In update patch I only moved them from ewk_private.h to EditorClientEfl.h because of some refactoring done in ewk_*_private.h files.

[Mariusz Grzegorczyk]

Additionally these singals must use internal WebCore's classes not accesible for external applications, so they cannot be exported.