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

[bugzilla-daemon at webkit.org]

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





--- Comment #23 from MORITA Hajime <morrita at google.com>  2010-05-27 01:47:58 PST ---
Hi Ojan, thank you for reviewing!

>  +          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.
Ya, this is what I meant. I replaced with this one. Thank you for your suggestion!

> 
> 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?
Fixed. The function itself was moved to Position::ascentEditingBoundary()

> 
> 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?
Fixed.

> 
> 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.
Fixed.

> 
> 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.
Fixed.

> 
> 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. 
Agreed that Node should not have editing related methods.
I moved this to Position class where there are many editing related method around.

> 
> 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:
Sounds nice. I introduced setCurrentNode().

> 
> 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.
Fixed to make null checks separate.

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