[Webkit-unassigned] [Bug 28306] double-clicking a word inside <b> beside newline select two words

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=28306


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44748|review?                     |review+
               Flag|                            |




--- Comment #22 from Darin Adler <darin at apple.com>  2009-12-14 10:24:36 PST ---
(From update of attachment 44748)
> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.