128478 2014-02-08 15:35:50 -0800 HTMLTextFormControlElement::setSelectionRange shouldn't use VisiblePosition 2014-02-11 00:53:17 -0800 1 1 1 Unclassified WebKit Forms 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 127832 1 rniwa rniwa ap barraclough darin enrica sam oldest_to_newest 978452 0 rniwa 2014-02-08 15:35:50 -0800 setSelectionRange uses visiblePositionForIndex to obtain start and end positions but we shouldn't need to since we only have br elements and text nodes inside the shadow DOM of inputs and text area elements. We should be able to obtain a regular Position by traversing through the DOM, which is significantly more efficient than creating a VisiblePosition 978461 1 223594 rniwa 2014-02-08 16:25:19 -0800 Created attachment 223594 Cleanup 978756 2 223594 darin 2014-02-09 23:41:45 -0800 Comment on attachment 223594 Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=223594&action=review > Source/WebCore/html/HTMLTextFormControlElement.cpp:55 > +static Position positionForIndex(TextControlInnerTextElement*, unsigned); I suggest taking a reference here rather than a pointer. > Source/WebCore/html/HTMLTextFormControlElement.cpp:324 > int HTMLTextFormControlElement::indexForVisiblePosition(const VisiblePosition& pos) const Why not rename pos to position while you are changing this entire function? > Source/WebCore/html/HTMLTextFormControlElement.cpp:569 > + Text* text = toText(node); This should be: Text& text = toText(*node); Because we know node is non-null. 979083 3 rniwa 2014-02-10 15:00:15 -0800 Committed r163825: <http://trac.webkit.org/changeset/163825> 979323 4 rniwa 2014-02-11 00:53:17 -0800 Oops, I somehow missed these review comments. Fixed them in http://trac.webkit.org/changeset/163846 (In reply to comment #2) > (From update of attachment 223594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=223594&action=review > > > Source/WebCore/html/HTMLTextFormControlElement.cpp:55 > > +static Position positionForIndex(TextControlInnerTextElement*, unsigned); > > I suggest taking a reference here rather than a pointer. Unfortunately the pointer could be null in all 3 call sites :( > > Source/WebCore/html/HTMLTextFormControlElement.cpp:324 > > int HTMLTextFormControlElement::indexForVisiblePosition(const VisiblePosition& pos) const > > Why not rename pos to position while you are changing this entire function? Done. > > Source/WebCore/html/HTMLTextFormControlElement.cpp:569 > > + Text* text = toText(node); > > This should be: > > Text& text = toText(*node); > > Because we know node is non-null. Done. 223594 2014-02-08 16:25:19 -0800 2014-02-09 23:41:45 -0800 Cleanup bug-128478-20140208162518.patch text/plain 6060 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2MzcyOSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMzIEBACisyMDE0LTAyLTA4ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIEhUTUxUZXh0Rm9ybUNvbnRyb2xF bGVtZW50OjpzZXRTZWxlY3Rpb25SYW5nZSBzaG91bGRuJ3QgdXNlIFZpc2libGVQb3NpdGlvbgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTI4NDc4CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkZWQgcG9z aXRpb25Gb3JJbmRleCB0byBjb21wdXRlIFBvc2l0aW9uIGdpdmVuIGEgc2VsZWN0aW9uIGluZGV4 LiBUaGlzIGZ1bmN0aW9uIGRvZXNuJ3QKKyAgICAgICAgc3luY2hyb25vdXNseSB0cmlnZ2VyIGxp a2UgdmlzaWJsZVBvc2l0aW9uRm9ySW5kZXguCisKKyAgICAgICAgQWxzbyBhZGRlZCBhc3NlcnRp b25zIGluIHZpc2libGVQb3NpdGlvbkZvckluZGV4IGFuZCB2aXNpYmxlUG9zaXRpb25Gb3JJbmRl eCB0byBtYWtlIHN1cmUKKyAgICAgICAgdGhleSBhcmUgaW52ZXJzZSBvZiBvbmUgYW5vdGhlci4K KworICAgICAgICAqIGh0bWwvSFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6SFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQ6OnNldFNlbGVjdGlvblJhbmdl KTogVXNlIHBvc2l0aW9uRm9ySW5kZXguIEFsc28gcmVtb3ZlZAorICAgICAgICB0aGUgbm93IHRh dXRvbG9naWNhbCBhc3NlcnRpb25zIHNpbmNlIHdlIHdvdWxkIG5ldmVyIGNyZWF0ZSBhIHBvc2l0 aW9uIG91dHNpZGUgdGhlIGlubmVyIHRleHQKKyAgICAgICAgZWxlbWVudC4KKworICAgICAgICAo V2ViQ29yZTo6SFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQ6OmluZGV4Rm9yVmlzaWJsZVBvc2l0 aW9uKTogRml4ZWQgdGhlIGJ1ZyB3aGVyZSB0aGlzCisgICAgICAgIGZ1bmN0aW9uIGNvdWxkIHJl dHVybiBhIHNlbGVjdGlvbiBpbmRleCBiZXlvbmQgaW5uZXJUZXh0RWxlbWVudCBpbiBzb21lIHR5 cGVzIG9mIGlucHV0CisgICAgICAgIGVsZW1lbnRzIHN1Y2ggYXMgc2VhcmNoIGZpZWxkcy4gZmFz dC9mb3Jtcy9zZWFyY2gtZGlzYWJsZWQtcmVhZG9ubHkuaHRtbCBoaXRzIHRoZSBuZXdseQorICAg ICAgICBhZGRlZCBhc3NlcnRpb24gd2l0aG91dCB0aGlzIGNoYW5nZS4gTm90ZSB3ZSBjYW4ndCB1 c2UgdmlzaWJsZVBvc2l0aW9uRm9ySW5kZXggaGVyZSBhcyB0aGF0CisgICAgICAgIHdvdWxkIHJl c3VsdCBpbiBhIG11dHVhbCByZWN1cnNpb24gd2l0aCB0aGUgYXNzZXJ0aW9uIGluIHZpc2libGVQ b3NpdGlvbkZvckluZGV4LgorCisgICAgICAgIChXZWJDb3JlOjpIVE1MVGV4dEZvcm1Db250cm9s RWxlbWVudDo6dmlzaWJsZVBvc2l0aW9uRm9ySW5kZXgpOiBBZGRlZCBhbiBhc3NlcnRpb24uCisK KyAgICAgICAgKFdlYkNvcmU6OnBvc2l0aW9uRm9ySW5kZXgpOiBBZGRlZC4gSXQncyBoZXJlIHdp dGggcHJvdG90eXBlIGF0IHRoZSBiZWdpbm5pbmcgb2YgdGhlIGZpbGUKKyAgICAgICAgc28gdGhh dCBpdCdzIHJpZ2h0IG5leHQgdG8gSFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQ6OmlubmVyVGV4 dCgpIHdoZXJlIHdlIGRvIGEgc2ltaWxhcgorICAgICAgICBET00gdHJhdmVyc2FsIHRvIG9idGFp biB0aGUgaW5uZXIgdGV4dCB2YWx1ZS4KKwogMjAxNC0wMi0wOCAgQnJhZHkgRWlkc29uICA8YmVp ZHNvbkBhcHBsZS5jb20+CiAKICAgICAgICAgSURCOiBzdG9yYWdlL2luZGV4ZWRkYi9tb3ppbGxh L3ZlcnNpb25jaGFuZ2UtYWJvcnQuaHRtbCBmYWlscwpJbmRleDogU291cmNlL1dlYkNvcmUvaHRt bC9IVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudC5jcHAJKHJldmlzaW9uIDE2Mzcy NikKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQuY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC01Miw2ICs1Miw4IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAog dXNpbmcgbmFtZXNwYWNlIEhUTUxOYW1lczsKIAorc3RhdGljIFBvc2l0aW9uIHBvc2l0aW9uRm9y SW5kZXgoVGV4dENvbnRyb2xJbm5lclRleHRFbGVtZW50KiwgdW5zaWduZWQpOworCiBIVE1MVGV4 dEZvcm1Db250cm9sRWxlbWVudDo6SFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQoY29uc3QgUXVh bGlmaWVkTmFtZSYgdGFnTmFtZSwgRG9jdW1lbnQmIGRvY3VtZW50LCBIVE1MRm9ybUVsZW1lbnQq IGZvcm0pCiAgICAgOiBIVE1MRm9ybUNvbnRyb2xFbGVtZW50V2l0aFN0YXRlKHRhZ05hbWUsIGRv Y3VtZW50LCBmb3JtKQogICAgICwgbV9sYXN0Q2hhbmdlV2FzVXNlckVkaXQoZmFsc2UpCkBAIC0y OTYsMjUgKzI5OCwxOCBAQCB2b2lkIEhUTUxUZXh0Rm9ybUNvbnRyb2xFbGVtZW50OjpzZXRTZWxl CiAgICAgZW5kID0gc3RkOjptYXgoZW5kLCAwKTsKICAgICBzdGFydCA9IHN0ZDo6bWluKHN0ZDo6 bWF4KHN0YXJ0LCAwKSwgZW5kKTsKIAotICAgIGlmICghaGFzVmlzaWJsZVRleHRBcmVhKCpyZW5k ZXJlcigpLCBpbm5lclRleHRFbGVtZW50KCkpKSB7CisgICAgVGV4dENvbnRyb2xJbm5lclRleHRF bGVtZW50KiBpbm5lclRleHQgPSBpbm5lclRleHRFbGVtZW50KCk7CisgICAgaWYgKCFoYXNWaXNp YmxlVGV4dEFyZWEoKnJlbmRlcmVyKCksIGlubmVyVGV4dCkpIHsKICAgICAgICAgY2FjaGVTZWxl Y3Rpb24oc3RhcnQsIGVuZCwgZGlyZWN0aW9uKTsKICAgICAgICAgcmV0dXJuOwogICAgIH0KLSAg ICBWaXNpYmxlUG9zaXRpb24gc3RhcnRQb3NpdGlvbiA9IHZpc2libGVQb3NpdGlvbkZvckluZGV4 KHN0YXJ0KTsKLSAgICBWaXNpYmxlUG9zaXRpb24gZW5kUG9zaXRpb247CisgICAgUG9zaXRpb24g c3RhcnRQb3NpdGlvbiA9IHBvc2l0aW9uRm9ySW5kZXgoaW5uZXJUZXh0LCBzdGFydCk7CisgICAg UG9zaXRpb24gZW5kUG9zaXRpb247CiAgICAgaWYgKHN0YXJ0ID09IGVuZCkKICAgICAgICAgZW5k UG9zaXRpb24gPSBzdGFydFBvc2l0aW9uOwogICAgIGVsc2UKLSAgICAgICAgZW5kUG9zaXRpb24g PSB2aXNpYmxlUG9zaXRpb25Gb3JJbmRleChlbmQpOworICAgICAgICBlbmRQb3NpdGlvbiA9IHBv c2l0aW9uRm9ySW5kZXgoaW5uZXJUZXh0LCBlbmQpOwogCi0jaWYgIVBMQVRGT1JNKElPUykKLSAg ICAvLyBzdGFydFBvc2l0aW9uIGFuZCBlbmRQb3NpdGlvbiBjYW4gYmUgbnVsbCBwb3NpdGlvbiBm b3IgZXhhbXBsZSB3aGVuCi0gICAgLy8gIi13ZWJraXQtdXNlci1zZWxlY3Q6IG5vbmUiIHN0eWxl IGF0dHJpYnV0ZSBpcyBzcGVjaWZpZWQuCi0gICAgaWYgKHN0YXJ0UG9zaXRpb24uaXNOb3ROdWxs KCkgJiYgZW5kUG9zaXRpb24uaXNOb3ROdWxsKCkpIHsKLSAgICAgICAgQVNTRVJUKHN0YXJ0UG9z aXRpb24uZGVlcEVxdWl2YWxlbnQoKS5kZXByZWNhdGVkTm9kZSgpLT5zaGFkb3dIb3N0KCkgPT0g dGhpcwotICAgICAgICAgICAgJiYgZW5kUG9zaXRpb24uZGVlcEVxdWl2YWxlbnQoKS5kZXByZWNh dGVkTm9kZSgpLT5zaGFkb3dIb3N0KCkgPT0gdGhpcyk7Ci0gICAgfQotI2VuZGlmCiAgICAgVmlz aWJsZVNlbGVjdGlvbiBuZXdTZWxlY3Rpb247CiAgICAgaWYgKGRpcmVjdGlvbiA9PSBTZWxlY3Rp b25IYXNCYWNrd2FyZERpcmVjdGlvbikKICAgICAgICAgbmV3U2VsZWN0aW9uID0gVmlzaWJsZVNl bGVjdGlvbihlbmRQb3NpdGlvbiwgc3RhcnRQb3NpdGlvbik7CkBAIC0zMjgsMTUgKzMyMywyMCBA QCB2b2lkIEhUTUxUZXh0Rm9ybUNvbnRyb2xFbGVtZW50OjpzZXRTZWxlCiAKIGludCBIVE1MVGV4 dEZvcm1Db250cm9sRWxlbWVudDo6aW5kZXhGb3JWaXNpYmxlUG9zaXRpb24oY29uc3QgVmlzaWJs ZVBvc2l0aW9uJiBwb3MpIGNvbnN0CiB7Ci0gICAgaWYgKGVuY2xvc2luZ1RleHRGb3JtQ29udHJv bChwb3MuZGVlcEVxdWl2YWxlbnQoKSkgIT0gdGhpcykKKyAgICBUZXh0Q29udHJvbElubmVyVGV4 dEVsZW1lbnQqIGlubmVyVGV4dCA9IGlubmVyVGV4dEVsZW1lbnQoKTsKKyAgICBpZiAoIWlubmVy VGV4dCB8fCAhaW5uZXJUZXh0LT5jb250YWlucyhwb3MuZGVlcEVxdWl2YWxlbnQoKS5hbmNob3JO b2RlKCkpKQogICAgICAgICByZXR1cm4gMDsKICAgICBib29sIGZvclNlbGVjdGlvblByZXNlcnZh dGlvbiA9IGZhbHNlOwotICAgIHJldHVybiBXZWJDb3JlOjppbmRleEZvclZpc2libGVQb3NpdGlv bihpbm5lclRleHRFbGVtZW50KCksIHBvcywgZm9yU2VsZWN0aW9uUHJlc2VydmF0aW9uKTsKKyAg ICB1bnNpZ25lZCBpbmRleCA9IFdlYkNvcmU6OmluZGV4Rm9yVmlzaWJsZVBvc2l0aW9uKGlubmVy VGV4dEVsZW1lbnQoKSwgcG9zLCBmb3JTZWxlY3Rpb25QcmVzZXJ2YXRpb24pOworICAgIEFTU0VS VChWaXNpYmxlUG9zaXRpb24ocG9zaXRpb25Gb3JJbmRleChpbm5lclRleHRFbGVtZW50KCksIGlu ZGV4KSkgPT0gcG9zKTsKKyAgICByZXR1cm4gaW5kZXg7CiB9CiAKIFZpc2libGVQb3NpdGlvbiBI VE1MVGV4dEZvcm1Db250cm9sRWxlbWVudDo6dmlzaWJsZVBvc2l0aW9uRm9ySW5kZXgoaW50IGlu ZGV4KSBjb25zdAogewotICAgIHJldHVybiB2aXNpYmxlUG9zaXRpb25Gb3JJbmRleFVzaW5nQ2hh cmFjdGVySXRlcmF0b3IoaW5uZXJUZXh0RWxlbWVudCgpLCBpbmRleCk7CisgICAgVmlzaWJsZVBv c2l0aW9uIHBvc2l0aW9uID0gcG9zaXRpb25Gb3JJbmRleChpbm5lclRleHRFbGVtZW50KCksIGlu ZGV4KTsKKyAgICBBU1NFUlQoaW5kZXhGb3JWaXNpYmxlUG9zaXRpb24ocG9zaXRpb24pID09IGlu ZGV4KTsKKyAgICByZXR1cm4gcG9zaXRpb247CiB9CiAKIGludCBIVE1MVGV4dEZvcm1Db250cm9s RWxlbWVudDo6c2VsZWN0aW9uU3RhcnQoKSBjb25zdApAQCAtNTU3LDYgKzU1NywyNCBAQCBTdHJp bmcgSFRNTFRleHRGb3JtQ29udHJvbEVsZW1lbnQ6OmlubmVyCiAgICAgcmV0dXJuIGZpbmlzaFRl eHQocmVzdWx0KTsKIH0KIAorc3RhdGljIFBvc2l0aW9uIHBvc2l0aW9uRm9ySW5kZXgoVGV4dENv bnRyb2xJbm5lclRleHRFbGVtZW50KiBpbm5lclRleHQsIHVuc2lnbmVkIGluZGV4KQoreworICAg IHVuc2lnbmVkIHJlbWFpbmluZ0NoYXJhY3RlcnNUb01vdmVGb3J3YXJkID0gaW5kZXg7CisgICAg Zm9yIChOb2RlKiBub2RlID0gaW5uZXJUZXh0OyBub2RlOyBub2RlID0gTm9kZVRyYXZlcnNhbDo6 bmV4dChub2RlLCBpbm5lclRleHQpKSB7CisgICAgICAgIGlmIChub2RlLT5oYXNUYWdOYW1lKGJy VGFnKSkgeworICAgICAgICAgICAgaWYgKCFyZW1haW5pbmdDaGFyYWN0ZXJzVG9Nb3ZlRm9yd2Fy ZCkKKyAgICAgICAgICAgICAgICByZXR1cm4gcG9zaXRpb25CZWZvcmVOb2RlKG5vZGUpOworICAg ICAgICAgICAgcmVtYWluaW5nQ2hhcmFjdGVyc1RvTW92ZUZvcndhcmQtLTsKKyAgICAgICAgfSBl bHNlIGlmIChub2RlLT5pc1RleHROb2RlKCkpIHsKKyAgICAgICAgICAgIFRleHQqIHRleHQgPSB0 b1RleHQobm9kZSk7CisgICAgICAgICAgICBpZiAocmVtYWluaW5nQ2hhcmFjdGVyc1RvTW92ZUZv cndhcmQgPCB0ZXh0LT5sZW5ndGgoKSkKKyAgICAgICAgICAgICAgICByZXR1cm4gUG9zaXRpb24o dGV4dCwgcmVtYWluaW5nQ2hhcmFjdGVyc1RvTW92ZUZvcndhcmQpOworICAgICAgICAgICAgcmVt YWluaW5nQ2hhcmFjdGVyc1RvTW92ZUZvcndhcmQgLT0gdGV4dC0+bGVuZ3RoKCk7CisgICAgICAg IH0KKyAgICB9CisgICAgcmV0dXJuIGxhc3RQb3NpdGlvbkluTm9kZShpbm5lclRleHQpOworfQor CiAjaWYgUExBVEZPUk0oSU9TKQogdm9pZCBIVE1MVGV4dEZvcm1Db250cm9sRWxlbWVudDo6aGlk ZVBsYWNlaG9sZGVyKCkKIHsK