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'.
Created attachment 167446 [details] Patch
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?
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.
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?
(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.
(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.
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.
Created attachment 168632 [details] Patch
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
(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.
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"
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.
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.
Created attachment 168642 [details] patch for landing.
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.
(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.
Created attachment 168657 [details] patch Applied the comments by Kenneth.
Comment on attachment 168657 [details] patch Clearing flags on attachment: 168657 Committed r131347: <http://trac.webkit.org/changeset/131347>
All reviewed patches have been landed. Closing bug.