Bug 98589 – [EFL][WK2] Provide implementation for PageClientImpl::toolTipChanged()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jinwoo Song]

Implement PageClientImple::toolTipChanged() to emit signal 'tooltip,text,set' with a tooltip text,
and if tooltip must be actually removed, text will be 0 or '\0'.

[Jinwoo Song]

Created attachment 167446 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 167446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1080
> + * Emits signal: "tooltip,text,set" with a tooltip string. If tooltip must be actually removed, text will be 0 or '\0'.

Isn't it better to emit a different signal for tooltip removal?

[Chris Dumez]

Comment on attachment 167446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review

The use for this seems pretty limited (inexistent?) on mobile devices, isn't it? I mean there is no mouseover so when would we show tooltips?

> Source/WebKit2/ChangeLog:8
> +        Implement PageClientImple::toolTipChanged() to emit signal 'tooltip,text,set' with a tooltip text,

"PageClientImpl"

Why do we emit a signal on the view? It seems to me like the view should be able to take care of drawing tooltips by itself...
Unless there is a good reason to delegate this to the browser, I would rather we keep as much functionality as possible in the view. Otherwise, it becomes painful to write a browser based on WebKit EFL.

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1078
> + * @param text Text to set tooltip.

"Tooltip text to set" ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:66
> + * - "tooltip,text,set", const char*: tooltip text is set.

"Requests the given tooltip text to be shown" ? I would also indicate here that NULL will be passed if the tooltip text should be removed.

[Kenneth Rohde Christiansen]

Comment on attachment 167446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1080
>> + * Emits signal: "tooltip,text,set" with a tooltip string. If tooltip must be actually removed, text will be 0 or '\0'.
> 
> Isn't it better to emit a different signal for tooltip removal?

So what about info that you get a temporary string and that it must be copied or so? We dont document such things?

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 167446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1080
> >> + * Emits signal: "tooltip,text,set" with a tooltip string. If tooltip must be actually removed, text will be 0 or '\0'.
> > 
> > Isn't it better to emit a different signal for tooltip removal?
> 
> So what about info that you get a temporary string and that it must be copied or so? We dont document such things?

I dont 't think this is an issue. The string will be available in the callback. If the app wants to keep the string after the callback is executed then its needs to strdup it. I think this is common practice.

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 167446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1080
> > + * Emits signal: "tooltip,text,set" with a tooltip string. If tooltip must be actually removed, text will be 0 or '\0'.
> 
> Isn't it better to emit a different signal for tooltip removal?

From the application side, different signal may be preferable as they do not need to check the text. I'll change as your suggestion.

[Jinwoo Song]

Comment on attachment 167446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review

>> Source/WebKit2/ChangeLog:8
>> +        Implement PageClientImple::toolTipChanged() to emit signal 'tooltip,text,set' with a tooltip text,
> 
> "PageClientImpl"
> 
> Why do we emit a signal on the view? It seems to me like the view should be able to take care of drawing tooltips by itself...
> Unless there is a good reason to delegate this to the browser, I would rather we keep as much functionality as possible in the view. Otherwise, it becomes painful to write a browser based on WebKit EFL.

In my thought, tooltip is a kind of UI element such as toolbar or status bar. Applications may want their own tooltip UX or even they do not want to show it.
Also, I think applications can easily draw the tooltip with elm_object_tooltip_set() so it is not too painful.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1078
>> + * @param text Text to set tooltip.
> 
> "Tooltip text to set" ?

I'll fix it.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:66
>> + * - "tooltip,text,set", const char*: tooltip text is set.
> 
> "Requests the given tooltip text to be shown" ? I would also indicate here that NULL will be passed if the tooltip text should be removed.

I'll add a "toolip,text,unset' signal for NULL case following Raphael's suggestion.

[Jinwoo Song]

Created attachment 168632 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 168632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168632&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1131
> + * Reports that the tooltip text was changed.
> + *
> + * Emits signal: "tooltip,text,set" with a tooltip text to set.
> + *               "tooltip,text,unset" if tooltip must be actually removed.

I dont think we normally have to document internal methods. Internal methods tend to change so documentation becomes stale

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 167446 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review
> 
> >> Source/WebKit2/ChangeLog:8
> >> +        Implement PageClientImple::toolTipChanged() to emit signal 'tooltip,text,set' with a tooltip text,
> > 
> > "PageClientImpl"
> > 
> > Why do we emit a signal on the view? It seems to me like the view should be able to take care of drawing tooltips by itself...
> > Unless there is a good reason to delegate this to the browser, I would rather we keep as much functionality as possible in the view. Otherwise, it becomes painful to write a browser based on WebKit EFL.
> 
> In my thought, tooltip is a kind of UI element such as toolbar or status bar. Applications may want their own tooltip UX or even they do not want to show it.
> Also, I think applications can easily draw the tooltip with elm_object_tooltip_set() so it is not too painful.

If you would use smart functions instead of signals, this would allow apps to draw tooltips by themselves if they want to and we could fallback to a "default" tooltip drawing behavior in ewk_view.

[Chris Dumez]

Comment on attachment 168632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168632&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1135
> +    if (text && text[0] != '\0')

if (text && *text) ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:68
> + * - "tooltip,text,unset", void: tooltip text must be actaully removed.

"actually"

[Jinwoo Song]

Comment on attachment 167446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167446&action=review

>>>> Source/WebKit2/ChangeLog:8
>>>> +        Implement PageClientImple::toolTipChanged() to emit signal 'tooltip,text,set' with a tooltip text,
>>> 
>>> "PageClientImpl"
>>> 
>>> Why do we emit a signal on the view? It seems to me like the view should be able to take care of drawing tooltips by itself...
>>> Unless there is a good reason to delegate this to the browser, I would rather we keep as much functionality as possible in the view. Otherwise, it becomes painful to write a browser based on WebKit EFL.
>> 
>> In my thought, tooltip is a kind of UI element such as toolbar or status bar. Applications may want their own tooltip UX or even they do not want to show it.
>> Also, I think applications can easily draw the tooltip with elm_object_tooltip_set() so it is not too painful.
> 
> If you would use smart functions instead of signals, this would allow apps to draw tooltips by themselves if they want to and we could fallback to a "default" tooltip drawing behavior in ewk_view.

Yes I know I can use the smart function if we want to support the tooltip in the webkit but it seems that tooltip is not an essential feature to support the fallback.

[Jinwoo Song]

Comment on attachment 168632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168632&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1131
>> + *               "tooltip,text,unset" if tooltip must be actually removed.
> 
> I dont think we normally have to document internal methods. Internal methods tend to change so documentation becomes stale

I see your point. I just added the document following the previous things.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1135
>> +    if (text && text[0] != '\0')
> 
> if (text && *text) ?

I'll fix.

>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:68
>> + * - "tooltip,text,unset", void: tooltip text must be actaully removed.
> 
> "actually"

I'll fix typo.

[Jinwoo Song]

Created attachment 168642 [details]
patch for landing.

[Kenneth Rohde Christiansen]

Comment on attachment 168642 [details]
patch for landing.

View in context: https://bugs.webkit.org/attachment.cgi?id=168642&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1131
> + * Reports that the tooltip text was changed.
> + *
> + * Emits signal: "tooltip,text,set" with a tooltip text to set.
> + *               "tooltip,text,unset" if tooltip must be actually removed.

I would just remove this documentation.

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:68
> + * - "tooltip,text,unset", void: tooltip text must be actually removed.

bad english: tooltip was unset?

 * - "tooltip,text,set", const char*: tooltip was set.
 * - "tooltip,text,unset", void: tooltip was unset.

[Jinwoo Song]

(In reply to comment #15)
> (From update of attachment 168642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168642&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:1131
> > + * Reports that the tooltip text was changed.
> > + *
> > + * Emits signal: "tooltip,text,set" with a tooltip text to set.
> > + *               "tooltip,text,unset" if tooltip must be actually removed.
> 
> I would just remove this documentation.
> 
Done.
> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:68
> > + * - "tooltip,text,unset", void: tooltip text must be actually removed.
> 
> bad english: tooltip was unset?
> 
>  * - "tooltip,text,set", const char*: tooltip was set.
>  * - "tooltip,text,unset", void: tooltip was unset.
Fixed.

[Jinwoo Song]

Created attachment 168657 [details]
patch

Applied the comments by Kenneth.

[WebKit Review Bot]

Comment on attachment 168657 [details]
patch

Clearing flags on attachment: 168657

Committed r131347: <http://trac.webkit.org/changeset/131347>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.