Summary: | [EFL] Update signals description | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Minor | CC: | commit-queue, gyuyoung.kim, kenneth, leandro, l.slachciak, lucas.de.marchi, ryuan.choi, tkent, tonikitoo, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Grzegorz Czajkowski
2011-03-23 00:57:45 PDT
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. |