This extends Ewk_Editor_Command with new commands. Also a table of editor command strings was introduced. Numerous calls of _ewk_view_editor_command for each case in ewk_view_execute_editor_command were replaced by one call of functions obtaining a command string from a table for given Ewk_Editor_Command value.
Created attachment 120009 [details] proposed patch
Mostly LGTM. > Source/WebKit/efl/ewk/ewk_view.cpp:1480 > + && (command < static_cast<int>(sizeof(_ewk_view_editor_commands) / sizeof(const char*)))) This line has wrong indent.
Comment on attachment 120009 [details] proposed patch I think editor command can be implemented by new file. For example, ewk_editor.cpp or ewk_editor_command.cpp and so on. As you know, there are many features and functions in ewk_view.cpp. How do you think about my suggestion?
I am OK with this, but into this new file I can put only the ewk_view_editor_command* functions there for now. I will prepare a new patch soon.
Comment on attachment 120009 [details] proposed patch Clearing r? flag while the patch is being reworked.
Created attachment 122215 [details] move implementation to new files
Created attachment 122231 [details] changelog and style fixes
Created attachment 138327 [details] rebased patch previous patch become obsolete as CMakelistEfl was removed and PlatformEfl.cmake was added
View in context: https://bugs.webkit.org/attachment.cgi?id=138327&action=review > Source/WebKit/ChangeLog:8 > + This extends Ewk_Editor_Command with new commands and moving it to new ewk_editor files. Please mention what exactly is changed here. I think description: "Added ewk_editor.cpp and ewk_editor.h files" is sufficient. > Source/WebKit/PlatformEfl.cmake:100 > + efl/ewk/ewk_editor.cpp Please keep alphabetical order.
(In reply to comment #3) > (From update of attachment 120009 [details]) > I think editor command can be implemented by new file. For example, ewk_editor.cpp or ewk_editor_command.cpp and so on. As you know, there are many features and functions in ewk_view.cpp. How do you think about my suggestion? I think it's good idea to split out this feature to new files. What about name ewk_view_editor(h/cpp) ?
Comment on attachment 138327 [details] rebased patch I think separating the enum definition from the _ewk_editor_commands array is error prone; one might inadvertently make the two data structures out of sync and cause havoc. How about doing the following in ewk_editor.cpp (this is similar to what exists in EditorClientQt.cpp): struct EditorCommand { Ewk_Editor_Command ewkCommand; const char editorCommand; }; static const EditorCommand editorCommands[] = { { EWK_EDITOR_COMMAND_INSERT_IMAGE, "InsertImage" }, { EWK_EDITOR_COMMAND_INSERT_TEXT, "InsertText" }, // ... }; static const char* _ewk_editor_command_get(Ewk_Editor_Command command) { static Eina_Hash* commandHash = 0; if (!commandHash) { // Initialize the hash map with Ewk_Editor_Command as key and // a const char* as value. } return static_cast<const char*>(eina_hash_find(...)); } Eina_Bool ewk_editor_command_execute(...) { // ... }
As for the splitting being proposed, I still find it weird to call an ewk_view function without the ewk_view prefix as is the case with ewk_editor_command_execute. You're expected to pass a sort of "self" or "this" as the first parameter and the function name should match it.
(In reply to comment #12) > As for the splitting being proposed, I still find it weird to call an ewk_view function without the ewk_view prefix as is the case with ewk_editor_command_execute. You're expected to pass a sort of "self" or "this" as the first parameter and the function name should match it. In case of a file name proposed by Grzegorz: ewk_view_editor.h/cpp, the function name will be changed to ewk_view_editor. Will it be good for you to use ewkView then? Still it won't be 'self' or 'this', but at least names will match.
(In reply to comment #13) > In case of a file name proposed by Grzegorz: ewk_view_editor.h/cpp, the function name will be changed to ewk_view_editor. Will it be good for you to use ewkView then? Still it won't be 'self' or 'this', but at least names will match. It sounds better; personally, I still don't see much reason not to just put these things into ewk_view.cpp itself, but if you guys prefer to split, ewk_view_editor is better than ewk_editor.
(In reply to comment #11) > (From update of attachment 138327 [details]) > I think separating the enum definition from the _ewk_editor_commands array is error prone; one might inadvertently make the two data structures out of sync and cause havoc. How about doing the following in ewk_editor.cpp (this is similar to what exists in EditorClientQt.cpp): > > struct EditorCommand { > Ewk_Editor_Command ewkCommand; > const char editorCommand; > }; > > static const EditorCommand editorCommands[] = { > { EWK_EDITOR_COMMAND_INSERT_IMAGE, "InsertImage" }, > { EWK_EDITOR_COMMAND_INSERT_TEXT, "InsertText" }, > // ... > }; > > static const char* _ewk_editor_command_get(Ewk_Editor_Command command) > { > static Eina_Hash* commandHash = 0; > > if (!commandHash) { > // Initialize the hash map with Ewk_Editor_Command as key and > // a const char* as value. > } > > return static_cast<const char*>(eina_hash_find(...)); > } > > Eina_Bool ewk_editor_command_execute(...) > { > // ... > } Good idea, I will apply it as soon as naming discussion will be over.
(In reply to comment #14) > (In reply to comment #13) > > In case of a file name proposed by Grzegorz: ewk_view_editor.h/cpp, the function name will be changed to ewk_view_editor. Will it be good for you to use ewkView then? Still it won't be 'self' or 'this', but at least names will match. > > It sounds better; personally, I still don't see much reason not to just put these things into ewk_view.cpp itself, but if you guys prefer to split, ewk_view_editor is better than ewk_editor. I agree with you. I think it should be put into ewk_view.cpp.
(In reply to comment #16) > (In reply to comment #14) > > (In reply to comment #13) > > > In case of a file name proposed by Grzegorz: ewk_view_editor.h/cpp, the function name will be changed to ewk_view_editor. Will it be good for you to use ewkView then? Still it won't be 'self' or 'this', but at least names will match. > > > > It sounds better; personally, I still don't see much reason not to just put these things into ewk_view.cpp itself, but if you guys prefer to split, ewk_view_editor is better than ewk_editor. > I agree with you. I think it should be put into ewk_view.cpp. Ok, those arguments are convincing to keep thie feature into ewk_view files. Thanks for your opinions.
Created attachment 139008 [details] move back implementation to ewk_view Implementation is moved back to ewk_view and a hash table proposed by Raphael was added.
Comment on attachment 139008 [details] move back implementation to ewk_view View in context: https://bugs.webkit.org/attachment.cgi?id=139008&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:321 > + Eina_Hash* editorCommandHash; I didn't understand why this was added to the private data instead of being a static variable inside _ewk_view_editor_command_string_get -- every time a new view is created a new hash will be created with the exact same data. > Source/WebKit/efl/ewk/ewk_view.cpp:1219 > + eina_hash_add(priv->editorCommandHash, &editorCommands[i].ewkEditorCommand, > + editorCommands[i].editorCommandString); We usually don't wrap at 80 characters, so these lines can be merged.
One other thing I'd like to ask is whether other ports implement something similar, and if they don't, if they achieve the functionality implemented here in another way. I started wondering about this after taking a look at bug 82286 -- it's rather unfortunate that we can't exercise the function being changed here in DumpRenderTree.
Comment on attachment 139008 [details] move back implementation to ewk_view View in context: https://bugs.webkit.org/attachment.cgi?id=139008&action=review >> Source/WebKit/efl/ewk/ewk_view.cpp:321 >> + Eina_Hash* editorCommandHash; > > I didn't understand why this was added to the private data instead of being a static variable inside _ewk_view_editor_command_string_get -- every time a new view is created a new hash will be created with the exact same data. I had difficulties with calling eina_hash_free on static Eina_Hash inside function. I will move the hash table back to function and replace it with OwnPtr<Eina_Hash>. >> Source/WebKit/efl/ewk/ewk_view.cpp:1219 >> + editorCommands[i].editorCommandString); > > We usually don't wrap at 80 characters, so these lines can be merged. Done
Created attachment 139166 [details] new patch using OwnPtr A new patch moving hash table back inside static function using OwnPtr approach for freeing Eina_Hash.
(In reply to comment #20) > One other thing I'd like to ask is whether other ports implement something similar, and if they don't, if they achieve the functionality implemented here in another way. > > I started wondering about this after taking a look at bug 82286 -- it's rather unfortunate that we can't exercise the function being changed here in DumpRenderTree. The most similar can be found in QT: QWebPage::triggerAction, where we can call some editing actions from API using enum. I am not 100% sure but I think there are some API function using commands as strings in Chromium (WebFrameImpl::executeCommand) and Windows (WebView::executeCoreCommandByName). All ports mentioned above are using the methods in DumpRendererTree.
Comment on attachment 139166 [details] new patch using OwnPtr Attachment 139166 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12592488
(In reply to comment #21) > (From update of attachment 139008 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139008&action=review > > >> Source/WebKit/efl/ewk/ewk_view.cpp:321 > >> + Eina_Hash* editorCommandHash; > > > > I didn't understand why this was added to the private data instead of being a static variable inside _ewk_view_editor_command_string_get -- every time a new view is created a new hash will be created with the exact same data. > > I had difficulties with calling eina_hash_free on static Eina_Hash inside function. I will move the hash table back to function and replace it with OwnPtr<Eina_Hash>. Note that this approach "leaks" as much as using a static raw pointer -- even if you do call the OwnPtr destructor, it will be done at the program shutdown anyway. I guess that's why this idiom is not found elsewhere in the code base, by the way.
(In reply to comment #25) > (In reply to comment #21) > > (From update of attachment 139008 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=139008&action=review > > > > >> Source/WebKit/efl/ewk/ewk_view.cpp:321 > > >> + Eina_Hash* editorCommandHash; > > > > > > I didn't understand why this was added to the private data instead of being a static variable inside _ewk_view_editor_command_string_get -- every time a new view is created a new hash will be created with the exact same data. > > > > I had difficulties with calling eina_hash_free on static Eina_Hash inside function. I will move the hash table back to function and replace it with OwnPtr<Eina_Hash>. > > Note that this approach "leaks" as much as using a static raw pointer -- even if you do call the OwnPtr destructor, it will be done at the program shutdown anyway. I guess that's why this idiom is not found elsewhere in the code base, by the way. I just thought that it will be better to clean up after myself by calling eina_hash_free explicitly than leaving it unfreed as we destroy the application.
(In reply to comment #26) > > Note that this approach "leaks" as much as using a static raw pointer -- even if you do call the OwnPtr destructor, it will be done at the program shutdown anyway. I guess that's why this idiom is not found elsewhere in the code base, by the way. > > I just thought that it will be better to clean up after myself by calling eina_hash_free explicitly than leaving it unfreed as we destroy the application. In practice this will make no difference, and WebKit itself already does that in many other places, including WebCore. The patch size could be reduced a bit if you just leak the raw pointer, and you wouldn't need to depend on the Eina_Hash OwnPtr bug to land this.
Please resend the patch now that bug 85046 has landed so we can check if it passes the EFL EWS.
Created attachment 141167 [details] rebased patch rebase patch after https://bugs.webkit.org/attachment.cgi?bugid=85046 has landed
LGTM now.
Comment on attachment 141167 [details] rebased patch rubberstamping.
Comment on attachment 141167 [details] rebased patch Clearing flags on attachment: 141167 Committed r117239: <http://trac.webkit.org/changeset/117239>
All reviewed patches have been landed. Closing bug.