36741 2010-03-29 00:07:08 -0700 Duplicate, slightly divergent implementation of Position[Iterator]::isCandidate() 2017-07-18 08:29:38 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) PC All REOPENED P2 Normal --- 1 rolandsteiner webkit-unassigned adele commit-queue enrica eric leviw mitz rniwa oldest_to_newest 205308 0 rolandsteiner 2010-03-29 00:07:08 -0700 The member function isCandidate is implemented twice, once on the Position class, and once on the PositionIterator class. The latter also does not contain the latest modifications to that function. There should be only one implementation. 205311 1 51879 rolandsteiner 2010-03-29 00:23:14 -0700 Created attachment 51879 patch - consolidate implementations into PositionIterator::isCandidate() As stated in the ChangeLog, the rationale for using PositionIterator::isCandidate() over Position::isCandidate() is that a PositionIterator is cheaper to construct from a Position than vice versa (no call to nodeIndex()). Also, PositionIterator::isCandidate is called in a tight loop within next/previousCandidate (editing/htmlediting.cpp). The code as is doesn't look very nice though, and I believe that PositionIterator and Position should be merged at some point. But that is another story, for another 1001 commits... 207510 2 51879 eric 2010-04-01 17:04:32 -0700 Comment on attachment 51879 patch - consolidate implementations into PositionIterator::isCandidate() I like the idea. Concerned about possible performace troubles, but OK. 207807 3 51879 commit-queue 2010-04-02 01:31:23 -0700 Comment on attachment 51879 patch - consolidate implementations into PositionIterator::isCandidate() Clearing flags on attachment: 51879 Committed r56989: <http://trac.webkit.org/changeset/56989> 207808 4 commit-queue 2010-04-02 01:31:28 -0700 All reviewed patches have been landed. Closing bug. 207879 5 mitz 2010-04-02 09:04:40 -0700 (In reply to comment #1) > a > PositionIterator is cheaper to construct from a Position than vice versa (no > call to nodeIndex()). But there is a call to childNode(). Isn't is just as expensive? I am concerned about the performance impact of this patch. How did you measure it? 208708 6 mrowe 2010-04-05 14:33:45 -0700 This introduced bug 37115. 208761 7 adele 2010-04-05 15:57:30 -0700 I'm considering rolling this out based on the introduced bug, and the performance concerns. I guess it was after the commit, but why were's the performance concerns addressed? 208775 8 adele 2010-04-05 16:18:25 -0700 wow. I meant to say - why weren't the performance concerns addressed? 208781 9 eric 2010-04-05 16:26:01 -0700 You should of course feel free to roll it out. I'm not sure there is any performance data for or against this change. It creates and extra object on the stack (in a relatively hot code path). But that doesn't necessarily mean it's noticeably slower. 208845 10 mrowe 2010-04-05 18:46:40 -0700 I rolled this out in r57110 to address bug 37115. I added a test case that covers that crash in r57111. 208849 11 rolandsteiner 2010-04-05 19:12:12 -0700 [sorry for the belated response] Regarding the performance concerns: Note that even PositionIterator::isCandidate() contains conversions back to Position, so the implementation is sub-optimal in this regard anyway. As mentioned in my initial comment, and also on webkit-dev@ I believe that those classes should be refactored anyway, even more so now that consolidating the divergent implementations apparently even leads to crashes. 208899 12 mrowe 2010-04-05 22:43:26 -0700 (In reply to comment #11) > As mentioned in my initial comment, and also on webkit-dev@ I believe that > those classes should be refactored anyway, even more so now that consolidating > the divergent implementations apparently even leads to crashes. What it says to me is that refactoring needs to be approached carefully as it is easy to unintentionally break something. 209066 13 enrica 2010-04-06 08:51:10 -0700 (In reply to comment #11) > [sorry for the belated response] > > Regarding the performance concerns: Note that even > PositionIterator::isCandidate() contains conversions back to Position, so the > implementation is sub-optimal in this regard anyway. > > As mentioned in my initial comment, and also on webkit-dev@ I believe that > those classes should be refactored anyway, even more so now that consolidating > the divergent implementations apparently even leads to crashes. I've read you e-mail to webkit dev very carefully and, even though I like the idea of refactoring Position, PositionIterator and Visible position, I think it is a major undertaking. There are too many places in the editing code where heavy assumptions are made on what VisiblePosition or Position will provide, that will force you to revisit many of the editing commands. I considered something similar for Selection and VisibleSelection at some point, encouraged by the comment in the VisibleSelection class but I decided not to move forward because the chances of breaking many places were too high. As far as this specific change is concerned, my advice is to run performance tests before making this change. As you know, the purpose of Position iterator is to allow you to move among positions fast and we want to make sure we are not slowing it down. I like the idea of having VisiblePosition work on renderers. isCandidate can be very expensive at times, especially since we introduced the ability to have a selection in an editable container that contains only non editable elements and in empty editable spans within non editable content. 51879 2010-03-29 00:23:14 -0700 2010-06-11 10:43:51 -0700 patch - consolidate implementations into PositionIterator::isCandidate() isc.patch text/plain 3438 rolandsteiner ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg Y2FiZmYxZC4uZDEwMDIyMiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyNCBAQAorMjAxMC0wMy0yOSAgUm9sYW5kIFN0ZWlu ZXIgIDxyb2xhbmRzdGVpbmVyQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCdWcgMzY3NDEgLSAgRHVwbGljYXRlLCBzbGlnaHRs eSBkaXZlcmdlbnQgaW1wbGVtZW50YXRpb24gb2YgUG9zaXRpb25bSXRlcmF0b3JdOjppc0NhbmRp ZGF0ZSgpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0z Njc0MQorICAgICAgICAKKyAgICAgICAgUGF0Y2g6IGNoYW5nZSBQb3NpdGlvbjo6aXNDYW5kaWRp ZGF0ZSgpIHRvIGNhbGwgdGhlIFBvc2l0aW9uSXRlcmF0b3I6OmlzQ2FuZGlkYXRlKCkgdmVyc2lv bi4KKyAgICAgICAgVXBkYXRlIFBvc2l0aW9uSXRlcmF0b3I6OmlzQ2FuZGlkYXRlKCkgdG8gbWly cm9yIFBvc2l0aW9uOjppc0NhbmRpZGF0ZSgpLgorCisgICAgICAgIFJhdGlvbmFsZTogUG9zaXRp b25JdGVyYXRvcjo6aXNDYW5kaWRhdGUoKSBpcyBjYWxsZWQgaW4gYSB0aWdodCBsb29wIHdpdGhp bgorICAgICAgICBuZXh0L3ByZXZpb3VzQ2FuZGlkYXRlKCkuIEFsc28sIGNyZWF0aW9uIG9mIGEg UG9zaXRpb25JdGVyYXRvciBmcm9tIGEgUG9zaXRpb24KKyAgICAgICAgaXMgY2hlYXBlciB0aGFu IHZpY2UtdmVyc2EuCisKKyAgICAgICAgVGVzdHM6IHJhbiBhbGwgdGVzdHMgaW4gJ2VkaXRpbmcn LgorCisgICAgICAgICogZG9tL1Bvc2l0aW9uLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBvc2l0 aW9uOjppc0NhbmRpZGF0ZSk6CisgICAgICAgICogZG9tL1Bvc2l0aW9uSXRlcmF0b3IuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6UG9zaXRpb25JdGVyYXRvcjo6aXNDYW5kaWRhdGUpOgorCiAyMDEw LTAzLTI4ICBBbGV4ZXkgUHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgogCiAgICAgICAgIFJl dmlld2VkIGJ5IFNhbSBXZWluaWcuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2RvbS9Qb3NpdGlvbi5j cHAgYi9XZWJDb3JlL2RvbS9Qb3NpdGlvbi5jcHAKaW5kZXggYjQ3MWE5OS4uNTU3MjAwMiAxMDA2 NDQKLS0tIGEvV2ViQ29yZS9kb20vUG9zaXRpb24uY3BwCisrKyBiL1dlYkNvcmUvZG9tL1Bvc2l0 aW9uLmNwcApAQCAtNzI1LDM4ICs3MjUsNyBAQCBib29sIFBvc2l0aW9uOjpub2RlSXNVc2VyU2Vs ZWN0Tm9uZShOb2RlKiBub2RlKQogCiBib29sIFBvc2l0aW9uOjppc0NhbmRpZGF0ZSgpIGNvbnN0 CiB7Ci0gICAgaWYgKGlzTnVsbCgpKQotICAgICAgICByZXR1cm4gZmFsc2U7Ci0gICAgICAgIAot ICAgIFJlbmRlck9iamVjdCAqcmVuZGVyZXIgPSBub2RlKCktPnJlbmRlcmVyKCk7Ci0gICAgaWYg KCFyZW5kZXJlcikKLSAgICAgICAgcmV0dXJuIGZhbHNlOwotICAgIAotICAgIGlmIChyZW5kZXJl ci0+c3R5bGUoKS0+dmlzaWJpbGl0eSgpICE9IFZJU0lCTEUpCi0gICAgICAgIHJldHVybiBmYWxz ZTsKLQotICAgIGlmIChyZW5kZXJlci0+aXNCUigpKQotICAgICAgICByZXR1cm4gbV9vZmZzZXQg PT0gMCAmJiAhbm9kZUlzVXNlclNlbGVjdE5vbmUobm9kZSgpLT5wYXJlbnQoKSk7Ci0KLSAgICBp ZiAocmVuZGVyZXItPmlzVGV4dCgpKQotICAgICAgICByZXR1cm4gaW5SZW5kZXJlZFRleHQoKSAm JiAhbm9kZUlzVXNlclNlbGVjdE5vbmUobm9kZSgpKTsKLQotICAgIGlmIChpc1RhYmxlRWxlbWVu dChub2RlKCkpIHx8IGVkaXRpbmdJZ25vcmVzQ29udGVudChub2RlKCkpKQotICAgICAgICByZXR1 cm4gKGF0Rmlyc3RFZGl0aW5nUG9zaXRpb25Gb3JOb2RlKCkgfHwgYXRMYXN0RWRpdGluZ1Bvc2l0 aW9uRm9yTm9kZSgpKSAmJiAhbm9kZUlzVXNlclNlbGVjdE5vbmUobm9kZSgpLT5wYXJlbnQoKSk7 Ci0KLSAgICBpZiAobV9hbmNob3JOb2RlLT5oYXNUYWdOYW1lKGh0bWxUYWcpKQotICAgICAgICBy ZXR1cm4gZmFsc2U7Ci0gICAgICAgIAotICAgIGlmIChyZW5kZXJlci0+aXNCbG9ja0Zsb3coKSkg ewotICAgICAgICBpZiAodG9SZW5kZXJCbG9jayhyZW5kZXJlciktPmhlaWdodCgpIHx8IG1fYW5j aG9yTm9kZS0+aGFzVGFnTmFtZShib2R5VGFnKSkgewotICAgICAgICAgICAgaWYgKCFQb3NpdGlv bjo6aGFzUmVuZGVyZWROb25Bbm9ueW1vdXNEZXNjZW5kYW50c1dpdGhIZWlnaHQocmVuZGVyZXIp KQotICAgICAgICAgICAgICAgIHJldHVybiBhdEZpcnN0RWRpdGluZ1Bvc2l0aW9uRm9yTm9kZSgp ICYmICFQb3NpdGlvbjo6bm9kZUlzVXNlclNlbGVjdE5vbmUobm9kZSgpKTsKLSAgICAgICAgICAg IHJldHVybiBtX2FuY2hvck5vZGUtPmlzQ29udGVudEVkaXRhYmxlKCkgJiYgIVBvc2l0aW9uOjpu b2RlSXNVc2VyU2VsZWN0Tm9uZShub2RlKCkpICYmIGF0RWRpdGluZ0JvdW5kYXJ5KCk7Ci0gICAg ICAgIH0KLSAgICB9IGVsc2UKLSAgICAgICAgcmV0dXJuIG1fYW5jaG9yTm9kZS0+aXNDb250ZW50 RWRpdGFibGUoKSAmJiAhUG9zaXRpb246Om5vZGVJc1VzZXJTZWxlY3ROb25lKG5vZGUoKSkgJiYg YXRFZGl0aW5nQm91bmRhcnkoKTsKLQotICAgIHJldHVybiBmYWxzZTsKKyAgICByZXR1cm4gUG9z aXRpb25JdGVyYXRvcigqdGhpcykuaXNDYW5kaWRhdGUoKTsKIH0KIAogYm9vbCBQb3NpdGlvbjo6 aW5SZW5kZXJlZFRleHQoKSBjb25zdApkaWZmIC0tZ2l0IGEvV2ViQ29yZS9kb20vUG9zaXRpb25J dGVyYXRvci5jcHAgYi9XZWJDb3JlL2RvbS9Qb3NpdGlvbkl0ZXJhdG9yLmNwcAppbmRleCBmNWI2 NWY1Li5hZmUzNzM1IDEwMDY0NAotLS0gYS9XZWJDb3JlL2RvbS9Qb3NpdGlvbkl0ZXJhdG9yLmNw cAorKysgYi9XZWJDb3JlL2RvbS9Qb3NpdGlvbkl0ZXJhdG9yLmNwcApAQCAtMTYyLDcgKzE2Miw4 IEBAIGJvb2wgUG9zaXRpb25JdGVyYXRvcjo6aXNDYW5kaWRhdGUoKSBjb25zdAogICAgICAgICAg ICAgICAgIHJldHVybiBhdFN0YXJ0T2ZOb2RlKCkgJiYgIVBvc2l0aW9uOjpub2RlSXNVc2VyU2Vs ZWN0Tm9uZShtX2FuY2hvck5vZGUpOwogICAgICAgICAgICAgcmV0dXJuIG1fYW5jaG9yTm9kZS0+ aXNDb250ZW50RWRpdGFibGUoKSAmJiAhUG9zaXRpb246Om5vZGVJc1VzZXJTZWxlY3ROb25lKG1f YW5jaG9yTm9kZSkgJiYgUG9zaXRpb24oKnRoaXMpLmF0RWRpdGluZ0JvdW5kYXJ5KCk7CiAgICAg ICAgIH0KLSAgICB9CisgICAgfSBlbHNlIAorICAgICAgICByZXR1cm4gbV9hbmNob3JOb2RlLT5p c0NvbnRlbnRFZGl0YWJsZSgpICYmICFQb3NpdGlvbjo6bm9kZUlzVXNlclNlbGVjdE5vbmUobV9h bmNob3JOb2RlKSAmJiBQb3NpdGlvbigqdGhpcykuYXRFZGl0aW5nQm91bmRhcnkoKTsKIAogICAg IHJldHVybiBmYWxzZTsKIH0K