[webkit-reviews] review granted: [Bug 31750] Can focus but not type into content editable block that contains only non-editable content : [Attachment 44038] Patch2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 31750: Can focus but not type into content editable block that contains
only non-editable content
https://bugs.webkit.org/show_bug.cgi?id=31750

Attachment 44038: Patch2
https://bugs.webkit.org/attachment.cgi?id=44038&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
It seems unnecessary to make atEditingBoundary be a member function on Position
and PositionIterator. Can we instead just make those free functions?

> +// A position is considered at editing boundary if one of the following is
true:"

Stray quote mark on the end of the line above.

> +// 1. it is the first position in the node and the next visually equivalent
position
> +//	 is non editable
> +// 2. it is the last position in the node and the previous visually
equivalent position
> +//	 is non editable
> +// 3. is is an editable position and both the next and previous visually
equivalent
> +//	 positions are both non editable

Typo "is is" here. I would put capital letters and periods on these three
conditions.

> +    return (nextPosition.isNotNull() &&
!nextPosition.node()->isContentEditable())
> +	      && (prevPosition.isNotNull() &&
!prevPosition.node()->isContentEditable());

I think this would read better without the parentheses. Also, the second line
should be indented only 4 spaces, not 7 -- we don't try to line things up.

> +Position Position::upstream(EditingBoundaryCrossingRule upRule) const

I'd probably call this boundaryRule or crossingRule or even "rule" rather than
upRule.

>	   return Position();
> -
> +    
>      // iterate forward from there, looking for a qualified position

The patch has a lot of whitespace changes like the above. It would be better to
not have these changes.

> +	       bool lastPosition = (caretOffset == lastOffsetInNode(node()));
> +	       Node* startNode = (lastPosition)? node()->childNode(caretOffset
- 1): node()->childNode(caretOffset);

It should be formatted as "lastPosition ? a : b" rather than "(lastPosition)?
a: b".

> +bool PositionIterator::atEditingBoundary() const
> +{
> +    if (!m_anchorNode)
> +	   return false;
> +	   
> +    return Position(*this).atEditingBoundary();
> +}

I don't think you need a special case for !m_anchorNode here. Do you?

>      // If this is a non-anonymous renderer, then it's simple.

This comment seems wrong now. It's not all that simple any more! Maybe we
should say something more about it rather than "it's simple".

> -    if (Node* node = this->node())
> +    if (Node* node = this->node()) {
> +	   if (!node->isContentEditable()) {
> +	       // prefer a visually equivalent position that is editable

We use sentence format for this type of comment. Capitals and a period at the
end.

> +	       Position pos(node, offset);

It would be better to name this "position" rather than "pos".

> +    int len = textLength();

This should be unsigned rather than int. And should be named a word, like
"length" rather than an abbreviation such as "len".

> +    const UChar* txt = characters();

Instead of "txt" I suggest either "characters" or "text" as the name for this
local variable.

> +    for (int i = 0; i < len; i++)
> +	   if (!style()->isCollapsibleWhiteSpace(txt[i]))
> +	       return false;

WebKit coding style uses braces around a multi-line for statement like this
one.

r=me -- all the comments are optional things