The patch adds and fixes signals that are emitted by evas_object_smart_callcabck_call in ewk_view, ewk_frame, ewk_contextmenu.
Created attachment 86584 [details] signals description
LGTM. Demarchi, how do you think ?
Comment on attachment 86584 [details] signals description View in context: https://bugs.webkit.org/attachment.cgi?id=86584&action=review > Source/WebKit/efl/ewk/ewk_contextmenu.h:28 > + * and as an agrument gives the context menu. agrument* > Source/WebKit/efl/ewk/ewk_contextmenu.h:33 > + * and as an argument gives a list with items of context menu. "and it gives a list with items of the context menu as an argument" reads better?
Created attachment 87474 [details] signals description
(In reply to comment #3) > (From update of attachment 86584 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86584&action=review > > > Source/WebKit/efl/ewk/ewk_contextmenu.h:28 > > + * and as an agrument gives the context menu. > > agrument* fixed > > > Source/WebKit/efl/ewk/ewk_contextmenu.h:33 > > + * and as an argument gives a list with items of context menu. > > "and it gives a list with items of the context menu as an argument" reads better? fixed, I introduced the same sequences in ewk_frame.h and ewk_view.h
Created attachment 87477 [details] signals description In previous patch the Changelog file was missed.
Attachment 87477 [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/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 87672 [details] patch Fix check style in ChangeLog.
Antonio, how do you think latest patch ?
Comment on attachment 87672 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=87672&action=review It looks fine, but might need an extra interaction. If ProFusion guys want to validate, it would be know. > Source/WebKit/efl/ewk/ewk_contextmenu.h:29 > + * - "contextmenu,free", Ewk_Context_Menu *: a context menu is freed. freed. Would hidden be better? > Source/WebKit/efl/ewk/ewk_view.h:77 > + * - "link,hover,in", const char *link[2]: reports mouse is over a link > + * it gives the url in link[0] and link's title in link[1] as an argument. I think there could be a period between these two lines. > Source/WebKit/efl/ewk/ewk_view.h:91 > + * - "tooltip,text,set", const char*: sets tooltip text and display if it is currently hidden. displays*
(In reply to comment #10) > (From update of attachment 87672 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87672&action=review > > It looks fine, but might need an extra interaction. If ProFusion guys want to validate, it would be know. /s/know/good
Created attachment 88036 [details] updated patch regarding to Antonio's suggestions
(In reply to comment #10) > (From update of attachment 87672 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87672&action=review > > It looks fine, but might need an extra interaction. If ProFusion guys want to validate, it would be know. > > > Source/WebKit/efl/ewk/ewk_contextmenu.h:29 > > + * - "contextmenu,free", Ewk_Context_Menu *: a context menu is freed. > > freed. Would hidden be better? This signal is emitted when context menu is destroyed (free method is called on: - all items of the context menu - the context menu so, in my opinion we should leave "free" in signal discription > > > Source/WebKit/efl/ewk/ewk_view.h:77 > > + * - "link,hover,in", const char *link[2]: reports mouse is over a link > > + * it gives the url in link[0] and link's title in link[1] as an argument. > > I think there could be a period between these two lines. fixed, description was splitted up into two sentences. > > > Source/WebKit/efl/ewk/ewk_view.h:91 > > + * - "tooltip,text,set", const char*: sets tooltip text and display if it is currently hidden. > > displays* fixed
PreFusion, please cq+- it :)
(In reply to comment #14) > PreFusion, please cq+- it :) ProFusion even :)
Comment on attachment 88036 [details] updated patch regarding to Antonio's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=88036&action=review > Source/WebKit/efl/ewk/ewk_frame.h:51 > - * - "title,changed", const char*: title of the main frame changed. > - * - "uri,changed", const char*: uri of the main frame changed. > - * - "load,document,finished", void: loading of a document has > - * finished on this frame. > + * - "title,changed", const char*: title of the main frame was changed. > + * - "uri,changed", const char*: uri of the main frame was changed. > + * - "load,document,finished", void: frame finished loading the document. Why are you changing these sentences to use past continuous instead of past? While I'm not really against, I don't see a reason to do so. > Source/WebKit/efl/ewk/ewk_frame.h:66 > - * changed due new layout, script actions or any other events. > - * - "navigation,first", void: first navigation occurred. > + * were changed due new layout, script actions or any other events. > + * - "navigation,first", void: first navigation was occurred. And I think here it's weird to use "was occurred".
(In reply to comment #16) > (From update of attachment 88036 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88036&action=review > > > Source/WebKit/efl/ewk/ewk_frame.h:51 > > - * - "title,changed", const char*: title of the main frame changed. > > - * - "uri,changed", const char*: uri of the main frame changed. > > - * - "load,document,finished", void: loading of a document has > > - * finished on this frame. > > + * - "title,changed", const char*: title of the main frame was changed. > > + * - "uri,changed", const char*: uri of the main frame was changed. > > + * - "load,document,finished", void: frame finished loading the document. > > Why are you changing these sentences to use past continuous instead of past? While I'm not really against, I don't see a reason to do so. I tried to use a passive voice instead of past for example, in first case we can write: a) main frame changed the title or b) title of the main frame was changed but c) title of the main frame changed - doesn't sound well in my opinion I would like to encourage you to use a) or b)
Comment on attachment 87672 [details] patch Cleared Antonio Gomes's review+ from obsolete attachment 87672 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Demarchi, Grzegorz fixed this patch again. LGTM. Please give your comment.:-)
Comment on attachment 88036 [details] updated patch regarding to Antonio's suggestions Ok. Let's land this patch.
Comment on attachment 88036 [details] updated patch regarding to Antonio's suggestions Clearing flags on attachment: 88036 Committed r84972: <http://trac.webkit.org/changeset/84972>
All reviewed patches have been landed. Closing bug.