This testing API has a code duplication, which should be eliminated.
Created attachment 97560 [details] Patch
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?
> 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.
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.
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!
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.
> 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*.
(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?
To clarify, the selection start's deprecated node is b in this case.
> 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....
(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+ :)
> > 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.
(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 :(
Original assumption for this refactoring was wrong. Closing.