Bug 56904 – [EFL] Update signals description

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

The patch adds and fixes signals that are emitted by evas_object_smart_callcabck_call in ewk_view, ewk_frame, ewk_contextmenu.

[Grzegorz Czajkowski]

Created attachment 86584 [details]
signals description

[Gyuyoung Kim]

LGTM. Demarchi, how do you think ?

[Antonio Gomes]

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?

[Grzegorz Czajkowski]

Created attachment 87474 [details]
signals description

[Grzegorz Czajkowski]

(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

[Grzegorz Czajkowski]

Created attachment 87477 [details]
signals description

In previous patch the Changelog file was missed.

[WebKit Review Bot]

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.

[Grzegorz Czajkowski]

Created attachment 87672 [details]
patch

Fix check style in ChangeLog.

[Gyuyoung Kim]

Antonio, how do you think latest patch ?

[Antonio Gomes]

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*

[Antonio Gomes]

(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

[Grzegorz Czajkowski]

Created attachment 88036 [details]
updated patch regarding to Antonio's suggestions

[Grzegorz Czajkowski]

(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

[Antonio Gomes]

PreFusion, please cq+- it :)

[Antonio Gomes]

(In reply to comment #14)
> PreFusion, please cq+- it :)

ProFusion even :)

[Lucas De Marchi]

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

[Grzegorz Czajkowski]

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

[Eric Seidel (no email)]

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.

[Gyuyoung Kim]

Demarchi, Grzegorz fixed this patch again. LGTM. Please give your comment.:-)

[Lucas De Marchi]

Comment on attachment 88036 [details]
updated patch regarding to Antonio's suggestions

Ok. Let's land this patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.