Bug 97224 – [EFL] EventSender should mimic CTRL+o emacs shortcut

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

EventSender should mimic CTRL+o emacs shortcut in order for the following test case to pass:
editing/input/emacs-ctrl-o.html

[Chris Dumez]

Created attachment 164933 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 164933 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:104
>  {
> -    Ewk_Should_Insert_Text_Event shouldInsertTextEvent = { text.utf8().data(), range, action };
> +    CString textUTF8 = text.utf8();
> +    Ewk_Should_Insert_Text_Event shouldInsertTextEvent = { textUTF8.data(), range, action };
>      evas_object_smart_callback_call(m_view, "editorclient,text,insert", &shouldInsertTextEvent);
>      return true;
>  }

so what makes sure now that textUTF8 is available at the right point? I mean 'text' before should be available till the end of the method scope, and isnt that the same with the textUTF8 copy now?

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 164933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164933&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:104
> >  {
> > -    Ewk_Should_Insert_Text_Event shouldInsertTextEvent = { text.utf8().data(), range, action };
> > +    CString textUTF8 = text.utf8();
> > +    Ewk_Should_Insert_Text_Event shouldInsertTextEvent = { textUTF8.data(), range, action };
> >      evas_object_smart_callback_call(m_view, "editorclient,text,insert", &shouldInsertTextEvent);
> >      return true;
> >  }
> 
> so what makes sure now that textUTF8 is available at the right point? I mean 'text' before should be available till the end of the method scope, and isnt that the same with the textUTF8 copy now?

The evas_object_smart_callback_call() is synchronous, so we just need to make sure that textUTF8 is available during its call (and it is with my fix).

The issue with the previous call is that String::utf8() returns a temporary CString object, and a pointer to its internal char* representation was stored in the Ewk_Should_Insert_Text_Event structure. When the temporary object gets destroyed (basically after the call), the pointer to the char* points to a area of the memory that is no longer valid. Then we send that pointer to the client...

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 164933 [details]
Patch

The actual bug fix in EditorClientEfl is nice, however I wonder if these tests should really pass for us or if we should just permanently ignore them; testing emacs-like shortcuts makes sense if you have that in your platform (Mac or GTK+, for example). Nothing like that exists for EFL these days, so the test itself passes but we cannot use the feature it tests in the real world.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 164933 [details])
> The actual bug fix in EditorClientEfl is nice, however I wonder if these tests should really pass for us or if we should just permanently ignore them; testing emacs-like shortcuts makes sense if you have that in your platform (Mac or GTK+, for example). Nothing like that exists for EFL these days, so the test itself passes but we cannot use the feature it tests in the real world.


At least now the results are consistent for WK1 and WK2 EFL and we can unskip the test for WebKit2 EFL. I don't think it hurts to mimic the expected behavior here in DRT (Qt port is doing the same).

We could of course mark this test as WONTFIX for EFL port and only land the EditorClientEfl fix. Kenneth, what's your opinion?

[Kenneth Rohde Christiansen]

Comment on attachment 164933 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:100
> +    CString textUTF8 = text.utf8();

I think I would call it protectedText; or so to make it more clear why you are doing this, so that no new refactoring will change it back to the former

> Tools/DumpRenderTree/efl/EventSender.cpp:536
> +    // Mimic the emacs ctrl-o binding by inserting a paragraph
> +    // separator and then putting the cursor back to its original
> +    // position. Allows us to pass emacs-ctrl-o.html

Please write why you are doing this (consistency with WK1 test results etc)

[Chris Dumez]

(In reply to comment #6)
> (From update of attachment 164933 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164933&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp:100
> > +    CString textUTF8 = text.utf8();
> 
> I think I would call it protectedText; or so to make it more clear why you are doing this, so that no new refactoring will change it back to the former

Ok, I'll rename.
BTW, thanks to this fix, WK1 results becomes the same as WK2 (not the ctrl+o minic).

> > Tools/DumpRenderTree/efl/EventSender.cpp:536
> > +    // Mimic the emacs ctrl-o binding by inserting a paragraph
> > +    // separator and then putting the cursor back to its original
> > +    // position. Allows us to pass emacs-ctrl-o.html
> 
> Please write why you are doing this (consistency with WK1 test results etc)

The CTRL+o change is not meant to make results consistent between WK1 and WK2. It just felt weird to commit wrong expectations with my EditorClientEfl fix so I mimicked the ctrl+o hebavior in the same patch as the fix.

[Chris Dumez]

Created attachment 165115 [details]
Patch for landing

Could someone please cq+ ?

[WebKit Review Bot]

Comment on attachment 165115 [details]
Patch for landing

Clearing flags on attachment: 165115

Committed r129212: <http://trac.webkit.org/changeset/129212>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.