[webkit-reviews] review denied: [Bug 24187] RTL: tooltip does not get its directionality from its element's directionality : [Attachment 31868] patch w/o layout test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 24187: RTL: tooltip does not get its directionality from its element's
directionality
https://bugs.webkit.org/show_bug.cgi?id=24187

Attachment 31868: patch w/o layout test
https://bugs.webkit.org/attachment.cgi?id=31868&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks like a step in the right "direction". I see a few problems.

> +    TextDirection toolTipDirection = LTR;
>      // First priority is a potential toolTip representing a spelling or
grammar error
>      String toolTip = result.spellingToolTip();
> +    toolTipDirection = result.spellingToolTipDirection();

There's no need to initialize toolTipDirection twice here. You should just
define it where it's initialized instead of first setting it LTR two lines
before.

>			   toolTip = form->action();
> +			   if (form->renderer())
> +			       toolTipDirection =
form->renderer()->style()->direction();

It seems strange to leave toolTipDirection set to
result.spellingToolTipDirection() if there is a form with no renderer. I
suggest setting toolTipDirection to LTR in that case, or not setting the
toolTip at all in that case.

> -	   if (toolTip.isEmpty())
> +	   if (toolTip.isEmpty()) {
>	       // FIXME: Need to pass this URL through userVisibleString once
that's in WebCore
>	       toolTip = result.absoluteLinkURL().string();
> +	       // URL always display as LTR, no need to reset
toolTipDirectionality.
> +	   }

Comment is fine, but code is wrong. toolTipDirection will be
result.spellingToolTipDirection(), not LTR.

>			   toolTip = String::adopt(names);
> +			   // FIXME: is filename always LTR?

Again, this leaves toolTipDirection set to result.spellingToolTipDirection(),
not LTR.

> +    return (m_innerNonSharedNode->renderer()) ?
> +		  (m_innerNonSharedNode->renderer()->style()->direction()) :
LTR;

The formatting here is a little messy. We don't usually use extra parentheses
like the ones here, nor do we indent the way you're indenting here. Maybe use
an if statement instead of ? : and a local variable to more consistent with the
other early exist in this function.

> +TextDirection HitTestResult::titleDirection() const
> +{
> +    for (Node* titleNode = m_innerNode.get(); titleNode; titleNode =
titleNode->parentNode()) {
> +	   if (titleNode->isElementNode()) {
> +	       String title = static_cast<Element*>(titleNode)->title();
> +	       if (!title.isEmpty())
> +		   return (titleNode->renderer()) ?
(titleNode->renderer()->style()->direction()) : LTR;
> +	   }
> +    }
> +    return LTR;
> +}

I don't like the fact that this function repeats the logic about finding title
elements; it's really unclear to me that the first element with a non-empty
title is the right one to return. Can this share code with HitTestResult::title
in some way?

Also, the extra parentheses in the ? : line don't seem good. Maybe put the
renderer into a local variable.

> -void WebChromeClient::setToolTip(const String& toolTip)
> +void WebChromeClient::setToolTip(const String& toolTip,
WebCore::TextDirection dir)

You need to leave out the name for the TextDirection argument, because
otherwise we'll get an unused parameter warning and so this will not compile.

Also, I don't think you need the WebCore:: prefix inside the file because it
has using namespace WebCore at the top.

What about other platforms with setToolTip functions? Won't this break the
build for those platforms? Please include fixes for that in the patch.