EventSender should mimic CTRL+o emacs shortcut in order for the following test case to pass: editing/input/emacs-ctrl-o.html
Created attachment 164933 [details] Patch
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?
(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...
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.
(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?
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)
(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.
Created attachment 165115 [details] Patch for landing Could someone please cq+ ?
Comment on attachment 165115 [details] Patch for landing Clearing flags on attachment: 165115 Committed r129212: <http://trac.webkit.org/changeset/129212>
All reviewed patches have been landed. Closing bug.