Bug 62849 – Refactoring: Editor::selectionStartHasMarkerFor should be simplified

• Component: HTML Editing
• Status: RESOLVED INVALID

[Hajime Morrita]

This testing API has a code duplication, which should be eliminated.

[Hajime Morrita]

Created attachment 97560 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 97560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97560&action=review

> Source/WebCore/editing/Editor.cpp:-3216
> -        if (marker->startOffset() <= startOffset && endOffset <= marker->endOffset() && marker->type() == markerType)
> -            return true;

It seems like we used to return true and we now return false if we have a zero-width marker at from=length but I guess we don't have to worry about that in practice?

[Hajime Morrita]

> It seems like we used to return true and we now return false if we have a zero-width marker at from=length but I guess we don't have to worry about that in practice?
Yes. And in theory, we need should have a way to detect zero-length marker.

[Ryosuke Niwa]

Comment on attachment 97560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97560&action=review

(In reply to comment #3)
> > It seems like we used to return true and we now return false if we have a zero-width marker at from=length but I guess we don't have to worry about that in practice?
> Yes. And in theory, we need should have a way to detect zero-length marker.

Document markets aren't exposed to the Web, are they?  We may want to assert somewhere that we're not adding a zero-length marker.

> Source/WebCore/editing/Editor.cpp:-3194
> -        if (node->renderer()->isTextControl())
> -            node = toRenderTextControl(node->renderer())->visiblePositionForIndex(1).deepEquivalent().deprecatedNode();

We're no longer looking for the text node, is that really fine?  Do we have a test coverage for this?

> Source/WebCore/editing/Editor.cpp:-3198
> -        else if (node->firstChild())
> -            node = node->firstChild();
> -        else
> -            node = node->nextSibling();

We don't do this either.  Maybe you want to canonicalize the selection start using VisiblePosition and get the deepEquivalent?  It seems like that's what this code is trying to do.

[Hajime Morrita]

Comment on attachment 97560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97560&action=review

>> Source/WebCore/editing/Editor.cpp:-3194
>> -            node = toRenderTextControl(node->renderer())->visiblePositionForIndex(1).deepEquivalent().deprecatedNode();
> 
> We're no longer looking for the text node, is that really fine?  Do we have a test coverage for this?

Traversing to text node is done by DocumentMarkerController. 
Our test case cannot handle this because our
selection anchors always go to leaf if it has any text in the subtree.

>> Source/WebCore/editing/Editor.cpp:-3198
>> -            node = node->nextSibling();
> 
> We don't do this either.  Maybe you want to canonicalize the selection start using VisiblePosition and get the deepEquivalent?  It seems like that's what this code is trying to do.

Yeah, that's another reason to remove this function. It's done by DocumentMarker anyway.


Anyway, thank you much for taking a look at this on such a midnight!

[Ryosuke Niwa]

Comment on attachment 97560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97560&action=review

>>> Source/WebCore/editing/Editor.cpp:-3194
>>> -            node = toRenderTextControl(node->renderer())->visiblePositionForIndex(1).deepEquivalent().deprecatedNode();
>> 
>> We're no longer looking for the text node, is that really fine?  Do we have a test coverage for this?
> 
> Traversing to text node is done by DocumentMarkerController. 
> Our test case cannot handle this because our
> selection anchors always go to leaf if it has any text in the subtree.

I'm confused.  DocumentMarkerController::hasMarkers doesn't contain any code that looks for text node.

[Hajime Morrita]

> I'm confused.  DocumentMarkerController::hasMarkers doesn't contain any code that looks for text node.
The marker is associated only for Text node. 
And hasMarker() uses traverseNextNode() for traversing, which walks up to leaf nodes.
Text nodes appear only as leaf nodes. So we never misses marked text node. 
Sorry for unclear explanation.

I think DocumentMarkerController should hold node refrences as Text*, instead of Node*.

[Ryosuke Niwa]

(In reply to comment #7)
> > I'm confused.  DocumentMarkerController::hasMarkers doesn't contain any code that looks for text node.
> The marker is associated only for Text node. 
> And hasMarker() uses traverseNextNode() for traversing, which walks up to leaf nodes.
> Text nodes appear only as leaf nodes. So we never misses marked text node. 
> Sorry for unclear explanation.
> 
> I think DocumentMarkerController should hold node refrences as Text*, instead of Node*.

But selectionStartHasMarkerFor's from and to are now with respect to selection start's node instead of whatever text node we used to use, right?  So it seems like we'll be including more markers.

E.g. consider a case where we have |<b>hello</b> where | is the selection start.  If we passed (from,to)=(0,1), we used to catch markekers on "h" but now we catch markers on "hello".  Am I missing something?

[Ryosuke Niwa]

To clarify, the selection start's deprecated node is b in this case.

[Hajime Morrita]

> E.g. consider a case where we have |<b>hello</b> where | is the selection start.  If we passed (from,to)=(0,1), we used to catch markekers on "h" but now we catch markers on "hello".  Am I missing something?
Wow, you are right. I need to fix it using TextIterator or something....

[Ryosuke Niwa]

(In reply to comment #10)
> > E.g. consider a case where we have |<b>hello</b> where | is the selection start.  If we passed (from,to)=(0,1), we used to catch markekers on "h" but now we catch markers on "hello".  Am I missing something?
> Wow, you are right. I need to fix it using TextIterator or something....

I'm glad I pointed this out before giving r+ :)

[Hajime Morrita]

> 
> I'm glad I pointed this out before giving r+ :)
Yeah, thanks for doing this ;-)
Actually I'm confused the assumption about Positions which FrameSelection returns.
I thought it's always Text. But it looks no longer (or hasn't been) true.

[Ryosuke Niwa]

(In reply to comment #12)
> Yeah, thanks for doing this ;-)
> Actually I'm confused the assumption about Positions which FrameSelection returns.
> I thought it's always Text. But it looks no longer (or hasn't been) true.

It's not.  FrameSelection's start and end are usually canonicalized but there are cases in which they're not validated.  Take a look at VisibleSelection::setWithoutValidation.  We'll really need to clean up the whole FrameSelection class at some point :(

[Hajime Morrita]

Original assumption for this refactoring was wrong. Closing.