[webkit-reviews] review granted: [Bug 28306] double-clicking a word inside <b> beside newline select two words : [Attachment 44748] patch update 4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted MORITA Hajime <morrita at gmail.com>'s
request for review:
Bug 28306: double-clicking a word inside <b> beside newline select two words
https://bugs.webkit.org/show_bug.cgi?id=28306

Attachment 44748: patch update 4
https://bugs.webkit.org/attachment.cgi?id=44748&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +static int collapsedSpaceLength(Node* textNode, int textEnd)
> +{
> +    RenderText* renderer = toRenderText(textNode->renderer());
> +
> +    const UChar* characters = renderer->text()->characters();
> +    int length = static_cast<int>(renderer->text()->length());

Do we really need a typecast here? Does some compiler generate a warning
without it? I'd prefer to see that omitted.

> +    for (int i = textEnd; i < length; ++i) {
> +	   if (!renderer->style()->isCollapsibleWhiteSpace(characters[i]))
> +	       return i - textEnd;
> +    }
> +
> +    return length - textEnd;
> +}
> +
> +// For the purpose of word boundary detection,
> +// we should iterate all visible text and trailing (collapsed) whitespaces. 

> +static int iterableMaxOffset(Node* node)
> +{
> +    int offset = caretMaxOffset(node);
> +
> +    if (node->renderer() && node->renderer()->isText())
> +	   offset += collapsedSpaceLength(node, offset);
> +
> +    return offset;
> +}

The factoring here is a bit strange. The caller gets the renderer and then
checks that it's a RenderText. But then it calls the function passing the
Node*. Since it has done the type checking, it should pass a RenderText* in
rather than a Node* the type checking and type casting should be in the same
function.

> -	   m_offset = m_node ? caretMaxOffset(m_node) : 0;
> +	   m_offset = m_node ? iterableMaxOffset(m_node) : 0;

I am not a huge fan of the name "iterable" here. It would be useful to write
out a couple sentences explaining what the sentence does, and then maybe
someone could extract some key words to come up with a good name for the
function.

r=me, but maybe consider my comments and revise again? Would be OK to land
exactly as-is.