Bug 74921 – [EFL] Add new commands for Ewk_Editor_Command.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Michal Pakula vel Rutka]

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.

[Michal Pakula vel Rutka]

Created attachment 120009 [details]
proposed patch

[Grzegorz Czajkowski]

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.

[Gyuyoung Kim]

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?

[Michal Pakula vel Rutka]

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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 120009 [details]
proposed patch

Clearing r? flag while the patch is being reworked.

[Michal Pakula vel Rutka]

Created attachment 122215 [details]
move implementation to new files

[Michal Pakula vel Rutka]

Created attachment 122231 [details]
changelog and style fixes

[Michal Pakula vel Rutka]

Created attachment 138327 [details]
rebased patch

previous patch become obsolete as CMakelistEfl was removed and PlatformEfl.cmake was added

[Grzegorz Czajkowski]

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.

[Grzegorz Czajkowski]

(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) ?

[Raphael Kubo da Costa (:rakuco)]

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(...)
  {
      // ...
  }

[Raphael Kubo da Costa (:rakuco)]

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.

[Michal Pakula vel Rutka]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Michal Pakula vel Rutka]

(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.

[Tomasz Morawski]

(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.

[Grzegorz Czajkowski]

(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.

[Michal Pakula vel Rutka]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Michal Pakula vel Rutka]

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

[Michal Pakula vel Rutka]

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.

[Michal Pakula vel Rutka]

(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.

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

(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.

[Michal Pakula vel Rutka]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Raphael Kubo da Costa (:rakuco)]

Please resend the patch now that bug 85046 has landed so we can check if it passes the EFL EWS.

[Michal Pakula vel Rutka]

Created attachment 141167 [details]
rebased patch

rebase patch after https://bugs.webkit.org/attachment.cgi?bugid=85046 has landed

[Raphael Kubo da Costa (:rakuco)]

LGTM now.

[Hajime Morrita]

Comment on attachment 141167 [details]
rebased patch

rubberstamping.

[WebKit Review Bot]

Comment on attachment 141167 [details]
rebased patch

Clearing flags on attachment: 141167

Committed r117239: <http://trac.webkit.org/changeset/117239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.