[webkit-reviews] review denied: [Bug 36359] Double clicking page's last editable inline element doesn't select a word. : [Attachment 56875] patch v6 - fix lint violation

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied MORITA Hajime <morrita at google.com>'s
request for review:
Bug 36359: Double clicking page's last editable inline element doesn't select a
word.
https://bugs.webkit.org/show_bug.cgi?id=36359

Attachment 56875: patch v6 - fix lint violation
https://bugs.webkit.org/attachment.cgi?id=56875&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
WebCore/ChangeLog:16
 +	    TextIteratorEndsAtEditingBoundary.
These last two sentences are a bit confusing. How about something like this
(assuming this is correct)?

Also, added TextIteratorEndsAtEditingBoundary to BackwardsCharacterIterator,
otherwise, the VisiblePosition returned by BackwardsCharacterIterator might
cross an editing boundary.

WebCore/dom/Node.cpp:1522
 +	Node* documentElement = document()->documentElement();
I know the if-statement below works if documentElement is null, but I would
prefer to be more explicit, both for readability and in case the code below
changed. Can you add a null-check here like there was in the old code?

WebCore/editing/TextIterator.cpp:954
 +	ASSERT(m_behavior == TextIteratorDefaultBehavior || m_behavior ==
TextIteratorEndsAtEditingBoundary);
I like this assert. Can you add one like this to the start of the TextIterator
constructor as well and put a FIXME to add support for
TextIteratorEndsAtEditingBoundary to TextIterator?

WebCore/editing/visible_units.cpp:165
 +	Node* boundary = pos.node()->editingBoundary();
This can return null. We should return VisiblePositon() in that case to match
the old code.

WebCore/editing/visible_units.cpp:80
 +	Node* boundary = pos.node()->editingBoundary();
This can return null. We should return VisiblePositon() in that case to match
the old code.

WebCore/dom/Node.cpp:1517
 +  Node* Node::editingBoundary() const
I like that you moved this into a shared function, but I don't think this
belongs in Node. Really, there shouldn't be editing code in Node. The code
that's there now needs to move elsewhere some day. For now, this could just
live in visible_units.cpp. There are a couple editing methods there and I'm not
sure this will be needed elsewhere. If it turns out to be needed elsewhere, we
might need to find a better home for it. The editing code in general needs a
better home for a lot of code. 

WebCore/editing/TextIterator.cpp:1044
 +		    Node* parentNode =
guardEditingBoundary(parentCrossingShadowBoundaries(m_node));
I think this code is correct, but it took me a good deal of looking at the code
to figure out why. The key point is that we need to guard for crossing editing
boundaries every time we set m_node. How about instead of this method we add
something like the following:

void SimplifiedBackwardsTextIterator::setCurrentNode(Node* node)
{
    if (!m_node || !node)
	m_node = 0;

    if (m_behavior == TextIteratorEndsAtEditingBoundary
	&& m_node->isContentEditable() != node->isContentEditable())
	m_node = 0;

    m_node = node;
}

Then this code could be:
Node* parentNode =
guardEditingBoundary(parentCrossingShadowBoundaries(m_node));
if (!parentNode)
    break;
setCurrentNode(parentNode)
if (!m_node)
// NOTE: I'm not sure if we should break here, or if we should break right
before the last line in the while loop.
    break;

And then below, it would just be setCurrentNode(next).

I'm not too familiar with this code, so tell me if this makes no sense. I just
want something that's a bit more clear that we need to guard whenever we set
m_node.

If you don't want to do all this, the safer, smaller change would be to avoid
trying to mix these two checks into one. Instead, add a crossesEditingBoundary
method that returns a bool. Then break if parentNode crosses an editing
boundary. I realize it's equivalent, but it puts the crossesEditingBoundary
call right before setting m_node that way.

WebCore/editing/TextIterator.cpp:1144
 +	if (!m_node || !node)
I don't feel like this should early return for !m_node (and node for that
matter). It's just there so the next if-statement doesn't crash, right?
Instead, the if-statement below should check m_node and node.

That way, you can make it so that all the places we set m_node actually call
setCurrentNode instead.