Created attachment 82425 [details] patch I'd like to add new functionalities to be executed by EditorCommand - InsertText, InsertImage. I decided to do it in a different way than previous commands: "SelectWord", "SelectLine" etc. I believe that having one function for all commands is much better than tens of similar functions (what is possible as EditorCommand has a lot of commands defined). Due to this fact I was also thinking about replacing existing functions (listed below) by additional enums in Ewk_Editor_Command but I'm not sure whether removing API is a good idea, what do you think? List of existing editor commands: EAPI Eina_Bool ewk_view_select_none(Evas_Object *o); EAPI Eina_Bool ewk_view_select_all(Evas_Object *o); EAPI Eina_Bool ewk_view_select_paragraph(Evas_Object *o); EAPI Eina_Bool ewk_view_select_sentence(Evas_Object *o); EAPI Eina_Bool ewk_view_select_line(Evas_Object *o); EAPI Eina_Bool ewk_view_select_word(Evas_Object *o);
Attachment 82425 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/efl/ewk/ewk_view.h:363: The parameter name "o" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/efl/ewk/ewk_view.h:363: The parameter name "command" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #1) > Attachment 82425 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 > > Source/WebKit/efl/ewk/ewk_view.h:363: The parameter name "o" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit/efl/ewk/ewk_view.h:363: The parameter name "command" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 2 in 3 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. To fix these style errors, you have to remove parameter name as below, EAPI Eina_Bool ewk_view_execute_editor_command(Evas_Object*, const Ewk_Editor_Command, const char *value); However, other APIs have parameter name in ewk_view.h. I don't know if we should remove parameter name in this patch. Lucas, how do you think about this ?
Comment on attachment 82425 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82425&action=review > Source/WebKit/efl/ChangeLog:6 > + Please add this bug url to here
Created attachment 82433 [details] patch + url added
Attachment 82433 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/efl/ewk/ewk_view.h:363: The parameter name "o" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/efl/ewk/ewk_view.h:363: The parameter name "command" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
> To fix these style errors, you have to remove parameter name as below, It's probably about the names which are used here. Looks like check-webkit-style doesn't accept names like: "o" and "command". As ewk uses "o" for "object" - I don't think it's a good idea to change it in one place. I can only change "command" but there won't be better name for it.
I add WebKit EFL reviewers to CC
Comment on attachment 82433 [details] patch Waiting for EFL API review...
Created attachment 83059 [details] patch Patch adjusted to the style fixes in ewk_view.h
Created attachment 83061 [details] patch Removed unnecessary line
Comment on attachment 83061 [details] patch LGTM
Comment on attachment 83061 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=83061&action=review > Source/WebKit/efl/ewk/ewk_view.h:365 > +EAPI Eina_Bool ewk_view_execute_editor_command(Evas_Object*, const Ewk_Editor_Command, const char *value); Please add parameters to this API. Style error checker doesn't recognize ewk header files now.
Created attachment 84587 [details] patch Restored params names.
Could someone please review this patch?
(In reply to comment #0) > Created an attachment (id=82425) [details] > patch > > I'd like to add new functionalities to be executed by EditorCommand - InsertText, InsertImage. > I decided to do it in a different way than previous commands: "SelectWord", "SelectLine" etc. > I believe that having one function for all commands is much better than tens of similar functions > (what is possible as EditorCommand has a lot of commands defined). > > Due to this fact I was also thinking about replacing existing functions (listed below) by additional enums in Ewk_Editor_Command > but I'm not sure whether removing API is a good idea, what do you think? > > List of existing editor commands: > EAPI Eina_Bool ewk_view_select_none(Evas_Object *o); > EAPI Eina_Bool ewk_view_select_all(Evas_Object *o); > EAPI Eina_Bool ewk_view_select_paragraph(Evas_Object *o); > EAPI Eina_Bool ewk_view_select_sentence(Evas_Object *o); > EAPI Eina_Bool ewk_view_select_line(Evas_Object *o); > EAPI Eina_Bool ewk_view_select_word(Evas_Object *o); IMO it's ok to remove these too, and merge with this new one that you implemented. Feel free to add this later. thanks
Comment on attachment 84587 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=84587&action=review > Source/WebKit/efl/ewk/ewk_view.h:361 > + EWK_EDITOR_COMMAND_INSERT_TEXT = 1 you do not need "= 1" here.
Hi, Kamil Please, remove the "= 1" Antonio mentioned before landing this patch. We do our best to enforce webkit's coding style. Thanks.
Created attachment 91263 [details] patch Removed unnecessary code from previous patch.
The commit-queue encountered the following flaky tests while processing attachment 91263 [details]: java/lc3/JSObject/ToObject-001.html bug 53091 (author: ap@webkit.org) The commit-queue is continuing to process your patch.
Comment on attachment 91263 [details] patch Clearing flags on attachment: 91263 Committed r85295: <http://trac.webkit.org/changeset/85295>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/85295 might have broken Windows 7 Release (Tests)