Bug 32819

Created attachment 45324 [details] Initial proposed patch

Attachment 45324 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebview.cpp:2634: Missing space after , [whitespace/comma] [3] WebKit/gtk/webkit/webkitwebview.cpp:2635: webkit_web_view_query_tooltip is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:2686: Missing space after , [whitespace/comma] [3] WebKit/gtk/webkit/webkitwebview.cpp:2687: Use 0 instead of NULL. [readability/null] [5] WebKit/gtk/webkit/webkitwebview.cpp:2689: Use 0 instead of NULL. [readability/null] [5] WebKit/gtk/webkit/webkitwebview.cpp:4045: webkit_web_view_set_tooltip_text is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:4052: Use 0 instead of NULL. [readability/null] [5] Total errors found: 7

Created attachment 45378 [details] Proposed patch Style review.

Attachment 45378 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebview.cpp:2635: webkit_web_view_query_tooltip is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:4045: webkit_web_view_set_tooltip_text is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2

Comment on attachment 45378 [details] Proposed patch > +#if GTK_CHECK_VERSION(2, 12, 0) > + priv->tooltipText = 0; > + gtk_widget_set_has_tooltip(GTK_WIDGET(webView), TRUE); > + g_signal_connect(webView, "query-tooltip", G_CALLBACK(webkit_web_view_query_tooltip), 0); > +#endif You should override the GtkWidget class method instead of connecting to the signal. > +void webkit_web_view_set_tooltip_text(WebKitWebView* webView, const char* tooltip) > +{ > + WebKitWebViewPrivate* priv = webView->priv; > + g_free(priv->tooltipText); > + if (tooltip && *tooltip != '\0') > + priv->tooltipText = g_strdup(tooltip); > + else > + priv->tooltipText = 0; > +} What about the 'has-tooltip' property? You should probably set it to TRUE/FALSE, as appropriate, here. Otherwise the tooltips will only be displayed when the webview has keyboard focus, as I understand it. The patch looks awesome otherwise, nice work!

(In reply to comment #5) > (From update of attachment 45378 [details]) > > +#if GTK_CHECK_VERSION(2, 12, 0) > > + priv->tooltipText = 0; > > + gtk_widget_set_has_tooltip(GTK_WIDGET(webView), TRUE); > > + g_signal_connect(webView, "query-tooltip", G_CALLBACK(webkit_web_view_query_tooltip), 0); > > +#endif > > You should override the GtkWidget class method instead of connecting to the > signal. > Yep, I'll do it. > > +void webkit_web_view_set_tooltip_text(WebKitWebView* webView, const char* tooltip) > > +{ > > + WebKitWebViewPrivate* priv = webView->priv; > > + g_free(priv->tooltipText); > > + if (tooltip && *tooltip != '\0') > > + priv->tooltipText = g_strdup(tooltip); > > + else > > + priv->tooltipText = 0; > > +} > > What about the 'has-tooltip' property? You should probably set it to > TRUE/FALSE, as appropriate, here. Otherwise the tooltips will only be displayed > when the webview has keyboard focus, as I understand it. The patch looks > awesome otherwise, nice work! AFAIK the property is a widget property and you just have to set it to TRUE once in order to receive the tooltip events. But now that you point out this, one interesting thing would be to avoid the gtkwidget check and use just the webkit one, i.e. set query-tooltip always to FALSE and use gtk_tooltip_trigger_tooltip_query. I'll try to do this. Thanks for the comments :).

Created attachment 46288 [details] Proposed patch I did both modifications, with this approach we do not ask gtk to send queries if webkit did not set a string in the tooltip. I guess it is a better option to avoid launching the query signal when we know there is no string to be rendered. I've also reviewed the gtk+ version check.

Attachment 46288 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebview.cpp:1281: webkit_web_view_query_tooltip is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:4044: webkit_web_view_set_tooltip_text is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2

Comment on attachment 46288 [details] Proposed patch Looks great to me.

Comment on attachment 46288 [details] Proposed patch Clearing flags on attachment: 46288 Committed r53256: <http://trac.webkit.org/changeset/53256>

All reviewed patches have been landed. Closing bug.