62849 2011-06-17 01:42:58 -0700 Refactoring: Editor::selectionStartHasMarkerFor should be simplified 2011-11-22 00:59:04 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 1 morrita morrita rniwa oldest_to_newest 422487 0 morrita 2011-06-17 01:42:58 -0700 This testing API has a code duplication, which should be eliminated. 422488 1 97560 morrita 2011-06-17 01:45:59 -0700 Created attachment 97560 Patch 422499 2 97560 rniwa 2011-06-17 02:12:44 -0700 Comment on attachment 97560 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? 422504 3 morrita 2011-06-17 02:25:14 -0700 > 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. 422509 4 97560 rniwa 2011-06-17 02:42:36 -0700 Comment on attachment 97560 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. 422512 5 97560 morrita 2011-06-17 02:52:45 -0700 Comment on attachment 97560 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! 422513 6 97560 rniwa 2011-06-17 02:56:00 -0700 Comment on attachment 97560 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. 422521 7 morrita 2011-06-17 03:11:17 -0700 > 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*. 422524 8 rniwa 2011-06-17 03:18:11 -0700 (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? 422528 9 rniwa 2011-06-17 03:22:11 -0700 To clarify, the selection start's deprecated node is b in this case. 422535 10 morrita 2011-06-17 03:34:54 -0700 > 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.... 422536 11 rniwa 2011-06-17 03:37:47 -0700 (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+ :) 422538 12 morrita 2011-06-17 03:42:48 -0700 > > 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. 422542 13 rniwa 2011-06-17 03:46:24 -0700 (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 :( 507451 14 morrita 2011-11-22 00:59:04 -0800 Original assumption for this refactoring was wrong. Closing. 97560 2011-06-17 01:45:59 -0700 2011-06-17 03:40:33 -0700 Patch bug-62849-20110617174557.patch text/plain 2835 morrita U3VidmVyc2lvbiBSZXZpc2lvbjogODkxMjcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAxMGYzNGRhZTk1Yzg3YWUx NzM4ODg0OTIxZTgxM2M2MzhlNmE5MTliLi5kN2IyODk3MzBjOGFjNTk0NjQ0ZjkzYjlhNGJjMmRm ZjZkNjc5OGFjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDYtMTcgIE1PUklU QSBIYWppbWUgIDxtb3JyaXRhQGdvb2dsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgUmVmYWN0b3Jpbmc6IEVkaXRvcjo6c2VsZWN0aW9uU3Rh cnRIYXNNYXJrZXJGb3Igc2hvdWxkIGJlIHNpbXBsaWZpZWQuCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02Mjg0OQorCisgICAgICAgIE5vIG5ldyB0ZXN0 cy4gTm8gYmVoYXZpb3JhbCBjaGFuZ2UuCisKKyAgICAgICAgKiBlZGl0aW5nL0VkaXRvci5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpFZGl0b3I6OnNlbGVjdGlvblN0YXJ0SGFzTWFya2VyRm9yKToK KwogMjAxMS0wNi0xNiAgWXVyeSBTZW1pa2hhdHNreSAgPHl1cnlzQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBSZXZpZXdlZCBieSBQYXZlbCBGZWxkbWFuLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvZWRpdGluZy9FZGl0b3IuY3BwIGIvU291cmNlL1dlYkNvcmUvZWRpdGluZy9FZGl0b3Iu Y3BwCmluZGV4IDJlNGZjNDEyMzRmNzExZGFkN2VjNzczZTg0NmIyNjdiZTMyMTE4MDQuLmFjNDJi ZjA1NGUwMTViNWJhMGIwNWViZmVlNjVhNDgyMWE1YWQ2MTIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJDb3JlL2VkaXRpbmcvRWRpdG9yLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9lZGl0aW5nL0Vk aXRvci5jcHAKQEAgLTMxODMsNDAgKzMxODMsMTUgQEAgdm9pZCBFZGl0b3I6OnJlc3BvbmRUb0No YW5nZWRTZWxlY3Rpb24oY29uc3QgVmlzaWJsZVNlbGVjdGlvbiYgb2xkU2VsZWN0aW9uLCBGcmEK ICAgICByZXNwb25kVG9DaGFuZ2VkU2VsZWN0aW9uKG9sZFNlbGVjdGlvbik7CiB9CiAKLXN0YXRp YyBOb2RlKiBmaW5kRmlyc3RNYXJrYWJsZShOb2RlKiBub2RlKQotewotICAgIHdoaWxlIChub2Rl KSB7Ci0gICAgICAgIGlmICghbm9kZS0+cmVuZGVyZXIoKSkKLSAgICAgICAgICAgIHJldHVybiAw OwotICAgICAgICBpZiAobm9kZS0+cmVuZGVyZXIoKS0+aXNUZXh0KCkpCi0gICAgICAgICAgICBy ZXR1cm4gbm9kZTsKLSAgICAgICAgaWYgKG5vZGUtPnJlbmRlcmVyKCktPmlzVGV4dENvbnRyb2wo KSkKLSAgICAgICAgICAgIG5vZGUgPSB0b1JlbmRlclRleHRDb250cm9sKG5vZGUtPnJlbmRlcmVy KCkpLT52aXNpYmxlUG9zaXRpb25Gb3JJbmRleCgxKS5kZWVwRXF1aXZhbGVudCgpLmRlcHJlY2F0 ZWROb2RlKCk7Ci0gICAgICAgIGVsc2UgaWYgKG5vZGUtPmZpcnN0Q2hpbGQoKSkKLSAgICAgICAg ICAgIG5vZGUgPSBub2RlLT5maXJzdENoaWxkKCk7Ci0gICAgICAgIGVsc2UKLSAgICAgICAgICAg IG5vZGUgPSBub2RlLT5uZXh0U2libGluZygpOwotICAgIH0KLQotICAgIHJldHVybiAwOwotfQot CiBib29sIEVkaXRvcjo6c2VsZWN0aW9uU3RhcnRIYXNNYXJrZXJGb3IoRG9jdW1lbnRNYXJrZXI6 Ok1hcmtlclR5cGUgbWFya2VyVHlwZSwgaW50IGZyb20sIGludCBsZW5ndGgpIGNvbnN0CiB7Ci0g ICAgTm9kZSogbm9kZSA9IGZpbmRGaXJzdE1hcmthYmxlKG1fZnJhbWUtPnNlbGVjdGlvbigpLT5z dGFydCgpLmRlcHJlY2F0ZWROb2RlKCkpOwotICAgIGlmICghbm9kZSkKKyAgICBpZiAobV9mcmFt ZS0+c2VsZWN0aW9uKCktPmlzTm9uZSgpKQogICAgICAgICByZXR1cm4gZmFsc2U7Ci0KLSAgICB1 bnNpZ25lZCBpbnQgc3RhcnRPZmZzZXQgPSBzdGF0aWNfY2FzdDx1bnNpZ25lZCBpbnQ+KGZyb20p OwotICAgIHVuc2lnbmVkIGludCBlbmRPZmZzZXQgPSBzdGF0aWNfY2FzdDx1bnNpZ25lZCBpbnQ+ KGZyb20gKyBsZW5ndGgpOwotICAgIFZlY3RvcjxEb2N1bWVudE1hcmtlcio+IG1hcmtlcnMgPSBt X2ZyYW1lLT5kb2N1bWVudCgpLT5tYXJrZXJzKCktPm1hcmtlcnNGb3Iobm9kZSk7Ci0gICAgZm9y IChzaXplX3QgaSA9IDA7IGkgPCBtYXJrZXJzLnNpemUoKTsgKytpKSB7Ci0gICAgICAgIERvY3Vt ZW50TWFya2VyKiBtYXJrZXIgPSBtYXJrZXJzW2ldOwotICAgICAgICBpZiAobWFya2VyLT5zdGFy dE9mZnNldCgpIDw9IHN0YXJ0T2Zmc2V0ICYmIGVuZE9mZnNldCA8PSBtYXJrZXItPmVuZE9mZnNl dCgpICYmIG1hcmtlci0+dHlwZSgpID09IG1hcmtlclR5cGUpCi0gICAgICAgICAgICByZXR1cm4g dHJ1ZTsKLSAgICB9Ci0KLSAgICByZXR1cm4gZmFsc2U7CisgICAgUG9zaXRpb24gc3RhcnQgPSBt X2ZyYW1lLT5zZWxlY3Rpb24oKS0+c3RhcnQoKTsKKyAgICBzdGFydC5tb3ZlVG9PZmZzZXQoZnJv bSk7CisgICAgUG9zaXRpb24gZW5kID0gbV9mcmFtZS0+c2VsZWN0aW9uKCktPnN0YXJ0KCk7Cisg ICAgZW5kLm1vdmVUb09mZnNldChmcm9tICsgbGVuZ3RoKTsKKyAgICByZXR1cm4gbV9mcmFtZS0+ ZG9jdW1lbnQoKS0+bWFya2VycygpLT5oYXNNYXJrZXJzKFJhbmdlOjpjcmVhdGUobV9mcmFtZS0+ ZG9jdW1lbnQoKSwgc3RhcnQsIGVuZCkuZ2V0KCksIG1hcmtlclR5cGUpOwogfSAgICAgICAKIAog VGV4dENoZWNraW5nVHlwZU1hc2sgRWRpdG9yOjpyZXNvbHZlVGV4dENoZWNraW5nVHlwZU1hc2so VGV4dENoZWNraW5nVHlwZU1hc2sgdGV4dENoZWNraW5nT3B0aW9ucykK