50854 2010-12-10 16:01:11 -0800 REGRESSION(r73799): editing/execCommand/4920488.html fails 2011-01-15 12:45:47 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) All All RESOLVED FIXED P1 Normal --- 52512 1 rniwa rniwa abarth bweinstein darin ddkilzer eae enrica eric inferno jberlin pkasting oldest_to_newest 320473 0 rniwa 2010-12-10 16:01:11 -0800 editing/execCommand/4920488.html fails on all platforms after http://trac.webkit.org/changeset/73799. 320476 1 76277 rniwa 2010-12-10 16:05:20 -0800 Created attachment 76277 Patch 320477 2 76278 rniwa 2010-12-10 16:06:01 -0800 Created attachment 76278 Patch 320483 3 76278 darin 2010-12-10 16:10:14 -0800 Comment on attachment 76278 Patch It would be *much* better if we had a regression test that was using Range directly rather than seeing this only as a side effect of the editing test. I worry that this rewrite of processContents broke other Range operations but we don’t have sufficient test coverage. 320487 4 rniwa 2010-12-10 16:14:22 -0800 (In reply to comment #3) > (From update of attachment 76278 [details]) > It would be *much* better if we had a regression test that was using Range directly rather than seeing this only as a side effect of the editing test. I worry that this rewrite of processContents broke other Range operations but we don’t have sufficient test coverage. Actually, the editing test directly called extractContents. Good thing I converted to a dump as text some time ago, and I was very familiar with the test. We might have missed otherwise. Anyway, thanks for the review. Will be landing. 320490 5 darin 2010-12-10 16:18:12 -0800 (In reply to comment #4) > Actually, the editing test directly called extractContents. Sure, but how much other coverage do we have for this Range function and the closely related functions that share the same back end code. I think we lucked out this time, but I fear we may have other bugs. 320495 6 abarth 2010-12-10 16:26:57 -0800 Sorry, that was my fault. I should have spotted the "n" shadowing in my review. :( 320498 7 rniwa 2010-12-10 16:30:29 -0800 Committed r73818: <http://trac.webkit.org/changeset/73818> 320499 8 rniwa 2010-12-10 16:31:48 -0800 (In reply to comment #6) > Sorry, that was my fault. I should have spotted the "n" shadowing in my review. :( Shadowing was okay. The problem was that it stopped adding the first child (m_end.container()->firstChild()) when i + 1 >= m_end.offset(). 334912 9 ddkilzer 2011-01-15 11:39:54 -0800 (In reply to comment #8) > (In reply to comment #6) > > Sorry, that was my fault. I should have spotted the "n" shadowing in my review. :( > > Shadowing was okay. The problem was that it stopped adding the first child (m_end.container()->firstChild()) when i + 1 >= m_end.offset(). This fix does not increment the loop variable (i) in the do-while loop, so it ignores the end offset of the range and iterates through all children of the endContainer. Follow-up fix in Bug 52512. 334927 10 ddkilzer 2011-01-15 12:45:47 -0800 (In reply to comment #9) > Follow-up fix in Bug 52512. Committed r75882: <http://trac.webkit.org/changeset/75882> 76277 2010-12-10 16:05:20 -0800 2010-12-10 16:05:58 -0800 Patch bug-50854-20101210160519.patch text/plain 1866 rniwa SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3MzgxNikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTItMTAgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUkVH UkVTU0lPTihyNzM3OTkpOiBlZGl0aW5nL2V4ZWNDb21tYW5kLzQ5MjA0ODguaHRtbCBmYWlscwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTA4NTQKKwor ICAgICAgICBUaGUgcmVncmVzc2lvbiB3YXMgY2F1c2VkIGJ5IHRoZSBjaGFuZ2UgaW4gUmFuZ2U6 OnByb2Nlc3NDb250ZW50cydzIG5vdCBleHRyYWN0aW5nCisgICAgICAgIG1fZW5kLmNvbnRhaW5l cigpLT5maXJzdENoaWxkKCkuIEZpeGVkIHRoZSBidWcgYnkgYWx3YXlzIGFkZGluZyB0aGUgZmly c3QgY2hpbGQuCisKKyAgICAgICAgKiBkb20vUmFuZ2UuY3BwOgorICAgICAgICAoV2ViQ29yZTo6 UmFuZ2U6OnByb2Nlc3NDb250ZW50cyk6CisKIDIwMTAtMTItMTAgIEJyaWFuIFdlaW5zdGVpbiAg PGJ3ZWluc3RlaW5AYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYSBDaGFuLgpJ bmRleDogV2ViQ29yZS9kb20vUmFuZ2UuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvZG9tL1Jh bmdlLmNwcAkocmV2aXNpb24gNzM4MDUpCisrKyBXZWJDb3JlL2RvbS9SYW5nZS5jcHAJKHdvcmtp bmcgY29weSkKQEAgLTc5NCwxMSArNzk0LDE1IEBAIFBhc3NSZWZQdHI8RG9jdW1lbnRGcmFnbWVu dD4gUmFuZ2U6OnByb2MKICAgICAgICAgICAgIE5vZGUqIG4gPSBtX2VuZC5jb250YWluZXIoKS0+ Zmlyc3RDaGlsZCgpOwogICAgICAgICAgICAgaWYgKG4gJiYgbV9lbmQub2Zmc2V0KCkpIHsKICAg ICAgICAgICAgICAgICBOb2RlVmVjdG9yIG5vZGVzOwotICAgICAgICAgICAgICAgIGZvciAoaW50 IGkgPSAwOyBpICsgMSA8IG1fZW5kLm9mZnNldCgpICYmIG47IGkrKywgbiA9IG4tPm5leHRTaWJs aW5nKCkpIHsKKyAgICAgICAgICAgICAgICBpbnQgaSA9IDA7CisgICAgICAgICAgICAgICAgZG8g ewogICAgICAgICAgICAgICAgICAgICBub2Rlcy5hcHBlbmQobik7Ci0gICAgICAgICAgICAgICAg fQorICAgICAgICAgICAgICAgICAgICBpZiAoIW4tPm5leHRTaWJsaW5nKCkpCisgICAgICAgICAg ICAgICAgICAgICAgICBicmVhazsKKyAgICAgICAgICAgICAgICAgICAgbiA9IG4tPm5leHRTaWJs aW5nKCk7CisgICAgICAgICAgICAgICAgfSB3aGlsZSAoaSArIDEgPCBtX2VuZC5vZmZzZXQoKSk7 CiAgICAgICAgICAgICAgICAgZm9yIChpbnQgaSA9IG5vZGVzLnNpemUoKSAtIDE7IGkgPj0gMDsg aS0tKSB7Ci0gICAgICAgICAgICAgICAgICAgIE5vZGUqIG4gPSBub2Rlc1tpXS5nZXQoKTsKKyAg ICAgICAgICAgICAgICAgICAgbiA9IG5vZGVzW2ldLmdldCgpOwogICAgICAgICAgICAgICAgICAg ICBpZiAoYWN0aW9uID09IEVYVFJBQ1RfQ09OVEVOVFMpCiAgICAgICAgICAgICAgICAgICAgICAg ICByaWdodENvbnRlbnRzLT5pbnNlcnRCZWZvcmUobiwgcmlnaHRDb250ZW50cy0+Zmlyc3RDaGls ZCgpLCBlYyk7IC8vIHdpbGwgcmVtb3ZlIG4gZnJvbSBpdHMgcGFyZW50CiAgICAgICAgICAgICAg ICAgICAgIGVsc2UgaWYgKGFjdGlvbiA9PSBDTE9ORV9DT05URU5UUykK 76278 2010-12-10 16:06:01 -0800 2010-12-10 16:10:14 -0800 Patch bug-50854-20101210160600.patch text/plain 1852 rniwa SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3MzgxNikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTAtMTItMTAgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUkVH UkVTU0lPTihyNzM3OTkpOiBlZGl0aW5nL2V4ZWNDb21tYW5kLzQ5MjA0ODguaHRtbCBmYWlscwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTA4NTQKKwor ICAgICAgICBUaGUgcmVncmVzc2lvbiB3YXMgY2F1c2VkIGJ5IFJhbmdlOjpwcm9jZXNzQ29udGVu dHMncyBub3QgZXh0cmFjdGluZworICAgICAgICBtX2VuZC5jb250YWluZXIoKS0+Zmlyc3RDaGls ZCgpLiBGaXhlZCB0aGUgYnVnIGJ5IGFsd2F5cyBhZGRpbmcgdGhlIGZpcnN0IGNoaWxkLgorCisg ICAgICAgICogZG9tL1JhbmdlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlJhbmdlOjpwcm9jZXNz Q29udGVudHMpOgorCiAyMDEwLTEyLTEwICBCcmlhbiBXZWluc3RlaW4gIDxid2VpbnN0ZWluQGFw cGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBBZGEgQ2hhbi4KSW5kZXg6IFdlYkNvcmUv ZG9tL1JhbmdlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2RvbS9SYW5nZS5jcHAJKHJldmlz aW9uIDczODA1KQorKysgV2ViQ29yZS9kb20vUmFuZ2UuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC03 OTQsMTEgKzc5NCwxNSBAQCBQYXNzUmVmUHRyPERvY3VtZW50RnJhZ21lbnQ+IFJhbmdlOjpwcm9j CiAgICAgICAgICAgICBOb2RlKiBuID0gbV9lbmQuY29udGFpbmVyKCktPmZpcnN0Q2hpbGQoKTsK ICAgICAgICAgICAgIGlmIChuICYmIG1fZW5kLm9mZnNldCgpKSB7CiAgICAgICAgICAgICAgICAg Tm9kZVZlY3RvciBub2RlczsKLSAgICAgICAgICAgICAgICBmb3IgKGludCBpID0gMDsgaSArIDEg PCBtX2VuZC5vZmZzZXQoKSAmJiBuOyBpKyssIG4gPSBuLT5uZXh0U2libGluZygpKSB7CisgICAg ICAgICAgICAgICAgaW50IGkgPSAwOworICAgICAgICAgICAgICAgIGRvIHsKICAgICAgICAgICAg ICAgICAgICAgbm9kZXMuYXBwZW5kKG4pOwotICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAg ICAgICAgICAgaWYgKCFuLT5uZXh0U2libGluZygpKQorICAgICAgICAgICAgICAgICAgICAgICAg YnJlYWs7CisgICAgICAgICAgICAgICAgICAgIG4gPSBuLT5uZXh0U2libGluZygpOworICAgICAg ICAgICAgICAgIH0gd2hpbGUgKGkgKyAxIDwgbV9lbmQub2Zmc2V0KCkpOwogICAgICAgICAgICAg ICAgIGZvciAoaW50IGkgPSBub2Rlcy5zaXplKCkgLSAxOyBpID49IDA7IGktLSkgewotICAgICAg ICAgICAgICAgICAgICBOb2RlKiBuID0gbm9kZXNbaV0uZ2V0KCk7CisgICAgICAgICAgICAgICAg ICAgIG4gPSBub2Rlc1tpXS5nZXQoKTsKICAgICAgICAgICAgICAgICAgICAgaWYgKGFjdGlvbiA9 PSBFWFRSQUNUX0NPTlRFTlRTKQogICAgICAgICAgICAgICAgICAgICAgICAgcmlnaHRDb250ZW50 cy0+aW5zZXJ0QmVmb3JlKG4sIHJpZ2h0Q29udGVudHMtPmZpcnN0Q2hpbGQoKSwgZWMpOyAvLyB3 aWxsIHJlbW92ZSBuIGZyb20gaXRzIHBhcmVudAogICAgICAgICAgICAgICAgICAgICBlbHNlIGlm IChhY3Rpb24gPT0gQ0xPTkVfQ09OVEVOVFMpCg==