Bug 84835 - [EFL] [DRT] EFL's DumpRenderTree should support LayoutTestController's dumpEditingCallbacks()
Summary: [EFL] [DRT] EFL's DumpRenderTree should support LayoutTestController's dumpEd...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mariusz Grzegorczyk
URL:
Keywords:
: 85871 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-25 03:45 PDT by Sudarsana Nagineni (babu)
Modified: 2012-05-18 09:30 PDT (History)
8 users (show)

See Also:


Attachments
[EFL] Editing callbacks implementation (19.77 KB, patch)
2012-05-09 06:51 PDT, Mariusz Grzegorczyk
no flags Details | Formatted Diff | Diff
[EFL] Editing callbacks implementation (19.77 KB, patch)
2012-05-17 02:16 PDT, Mariusz Grzegorczyk
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sudarsana Nagineni (babu) 2012-04-25 03:45:28 PDT
EFL's DumpRenderTree needs to support LayoutTestController's dumpEditingCallbacks() in order to unksip several tests from the skip list.
Comment 1 Mariusz Grzegorczyk 2012-05-04 02:23:58 PDT
(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?
Comment 2 Sudarsana Nagineni (babu) 2012-05-04 02:28:03 PDT
(In reply to comment #1)
> Are you preparing patch for this bug?

No. I have not started working on this yet.
Comment 3 Mariusz Grzegorczyk 2012-05-08 01:15:08 PDT
As discussed with Sudarsana Nagineni I'm preparing patch for this issue.
Comment 4 Mikhail Pozdnyakov 2012-05-08 01:18:43 PDT
*** Bug 85871 has been marked as a duplicate of this bug. ***
Comment 5 Mariusz Grzegorczyk 2012-05-09 06:51:52 PDT
Created attachment 140934 [details]
[EFL] Editing callbacks implementation
Comment 6 Grzegorz Czajkowski 2012-05-09 23:40:35 PDT
What are tests passed with this patch?
You should update Skipped or test_expectations.txt files.
Comment 7 Gyuyoung Kim 2012-05-10 02:09:56 PDT
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.
Comment 8 Mariusz Grzegorczyk 2012-05-17 01:51:32 PDT
(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.
Comment 9 Mariusz Grzegorczyk 2012-05-17 02:16:38 PDT
Created attachment 142444 [details]
[EFL] Editing callbacks implementation

Rebased, update with Gyuyoung's comments.
Comment 10 Mariusz Grzegorczyk 2012-05-17 02:36:52 PDT
(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.
Comment 11 Eric Seidel (no email) 2012-05-17 03:16:02 PDT
Comment on attachment 142444 [details]
[EFL] Editing callbacks implementation

rs=me.
Comment 12 WebKit Review Bot 2012-05-17 04:05:35 PDT
Comment on attachment 142444 [details]
[EFL] Editing callbacks implementation

Clearing flags on attachment: 142444

Committed r117426: <http://trac.webkit.org/changeset/117426>
Comment 13 WebKit Review Bot 2012-05-17 04:05:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-05-18 09:01:00 PDT
Comment on attachment 142444 [details]
[EFL] Editing callbacks implementation

ewk_view.h has not been updated with the new signals being emitted.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-05-18 09:02:42 PDT
Plus API users cannot really use these new signals since the structs are defined in EditorClientEfl.h, which is not installed.
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-05-18 09:07:06 PDT
Please fix these issues ASAP, otherwise I am inclined to revert this until these issues are properly addressed.
Comment 17 Mariusz Grzegorczyk 2012-05-18 09:29:04 PDT
(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.
Comment 18 Mariusz Grzegorczyk 2012-05-18 09:30:45 PDT
Additionally these singals must use internal WebCore's classes not accesible for external applications, so they cannot be exported.