[webkit-reviews] review denied: [Bug 49498] focus issue with links that have tooltips : [Attachment 74364] Patch to set the tooltip area

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 49498: focus issue with links that have tooltips
https://bugs.webkit.org/show_bug.cgi?id=49498

Attachment 74364: Patch to set the tooltip area
https://bugs.webkit.org/attachment.cgi?id=74364&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74364&action=review

Great! Just a couple suggestions.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:526
> +    if (Node* node = hit.innerNonSharedNode())
> +	   webkit_web_view_set_tooltip_area(m_webView, node->getRect());
> +    else
> +	   webkit_web_view_set_tooltip_area(m_webView, IntRect());

I think it might be cleaner to simply have this code update the WebView private
data structure.

> WebKit/gtk/webkit/webkitwebview.cpp:1645
> +		   gtk_tooltip_set_tip_area(tooltip, NULL);

NULL should be 0 here.

> WebKit/gtk/webkit/webkitwebview.cpp:4756
> +void webkit_web_view_set_tooltip_area(WebKitWebView* webView, const IntRect&
area)
> +{
> +#if GTK_CHECK_VERSION(2, 12, 0)
> +    WebKitWebViewPrivate* priv = webView->priv;
> +    priv->tooltipArea = area;
> +    // query-tooltip will be triggered by
webkit_web_view_set_tooltip_text().
> +#else
> +    // TODO: Support older GTK+ versions
> +    // See http://bugs.webkit.org/show_bug.cgi?id=15793
> +    notImplemented();
> +#endif
> +}
> +
>  /**

I think the conditional logic is unnecessarily here. It's technically correct,
but this method doesn't use any GTK+ API calls that don't exist in < 2.12.0. I
prefer simply scrapping this and fiddling the private structure directly.
Mainly this is because webkitwebview.cpp is bordering on 5000 lines.