[Webkit-unassigned] [Bug 36359] Double clicking page's last editable inline element doesn't select a word.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 27 13:40:36 PDT 2010


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


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57210|review?                     |review-
               Flag|                            |




--- Comment #24 from Ojan Vafai <ojan at chromium.org>  2010-05-27 13:40:34 PST ---
(From update of attachment 57210)
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.

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



More information about the webkit-unassigned mailing list