[webkit-reviews] review denied: [Bug 36359] Double clicking page's last editable inline element doesn't select a word. : [Attachment 57210] patch v7

[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 57210: patch v7
https://bugs.webkit.org/attachment.cgi?id=57210&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
I'd like darin or mitz to comment since I'm not as familiar with this code.

Just a few nits left from me.

Does the SimplifiedBackwardsTextIterator on TextIterator.cpp line 195 need
TextIteratorEndsAtEditingBoundary as well?

WebCore/editing/TextIterator.cpp:1152
 +	 if (!node)
Indentation is off.

WebCore/editing/TextIterator.cpp:1154
 +	if (!m_node)
I didn't quite mean that you'd split up the null-checks here. What I meant was
that if the m_node null-check were part of the if-statement on line 1156
instead, then you could use setCurrentNode for the initial setting of m_node in
the constructor. Then, the only places where m_node is set would be in
setCurrentNode and clearCurrentNode. So, specifically, something like this:

bool SimplifiedBackwardsTextIterator::setCurrentNode(Node* node)
{
    if (!node)
	return false;
    if (m_node && m_behavior == TextIteratorEndsAtEditingBoundary &&
willCrossEditingBoundary(node))
	return false;
    m_node = node;
    return true;
}

WebCore/editing/TextIterator.h:187
 +	bool willCrossEditingBoundary(Node* node) const;
Nit: I prefer the name crossesEditingBoundary

WebCore/dom/Position.h:145
 +	Node* ascentEditingBoundary() const;
This should probably be placed next to atEditingBoundary in the header file as
well.

WebCore/dom/Position.cpp:335
 +	if (!m_anchorNode->document())
Just to clarify, I didn't mean that each null-check had to go on it's own line.
It would be fine to combine 333 and 335 into one if-statement.

WebCore/dom/Position.cpp:331
 +  Node* Position::ascentEditingBoundary() const
How about parentEditingBoundary? I understand what you mean by "ascent", but I
don't think I would get it from calling code without looking at this function
to figure out what it does.