24979 2009-03-31 20:04:24 -0700 Avoid type-coercion into AtomicString in AtomicString methods 2009-04-06 09:11:53 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Mac All RESOLVED FIXED P2 Normal --- 1 mbelshe mbelshe ap oldest_to_newest 116132 0 mbelshe 2009-03-31 20:04:24 -0700 When using AtomicString.find() AtomicStirng.contains() AtomicString.startsWith() AtomicString.endsWith() if you pass a const char* string literal, the argument will be coerced into a String, then into an AtomicString, and then ripped out. This causes unnecessary churn on the AtomicString table. 116135 1 29155 mbelshe 2009-03-31 20:56:50 -0700 Created attachment 29155 bug24979 116136 2 mbelshe 2009-03-31 20:58:34 -0700 Two notes on the patch: a. Document.cpp and FrameTree.cpp needed small changes because they use AtomicString methods which are ambiguous now that we have the new methods plumbed in. b. I could make these methods chain - e.g. have the one which takes the AtomicString argument in turn call the one which takes the string argument. I don't know which style is preferred on the WebKit side. Just let me know if you have a preference. 116147 3 ap 2009-04-01 00:10:09 -0700 My suggestion was to replace AtomicString methods with String ones, not to add new methods. Unless we're using them with AtomicStrings very often, a string() call here and there should probably be better than having to disambiguate similar methods. What do you think? 116212 4 29155 darin 2009-04-01 13:33:17 -0700 Comment on attachment 29155 bug24979 > for (frame = m_thisFrame; frame; frame = frame->tree()->parent()) { > - if (frame->tree()->name().startsWith(framePathPrefix)) > + if (frame->tree()->name().startsWith(String(framePathPrefix))) It would be much better to overload for const char* too rather than adding the explicit casts to String. We don't want to create any destroy String objects either. 116582 5 29268 mbelshe 2009-04-05 15:24:17 -0700 Created attachment 29268 updated patch Removed the versions which take AtomicStrings. startsWith() and endsWith() have no versions which take const char* anyway, so they now take Strings. find() and contains() now offer both const char* and String variants. 116609 6 29268 ap 2009-04-06 04:04:09 -0700 Comment on attachment 29268 updated patch r=me We should eventually change these methods to take named enum constants instead of booleans for caseSensitive - a call like str.contains("foo", true) is quite confusing, str.contains("foo", CaseSensitive) would be much better. But that's definitely a separate issue. 116615 7 darin 2009-04-06 09:01:40 -0700 (In reply to comment #5) > startsWith() and endsWith() > have no versions which take const char* anyway, so they now take Strings. Side note: In my opinion that's a temporary situation. Any time we find ourselves doing a string operation on a const char* constant, I think it's a good optimization technique to add the const char* override to avoid the memory allocation. Similarly, I think we need to come up with an approach that will allow us to do this for substrings and other UChar*/length pairs. 116618 8 fishd 2009-04-06 09:11:12 -0700 Landed as: http://trac.webkit.org/changeset/42245 116619 9 darin 2009-04-06 09:11:53 -0700 (In reply to comment #6) > We should eventually change these methods to take named enum constants instead > of booleans for caseSensitive Or use two separate functions for the two variants. 29155 2009-03-31 20:56:50 -0700 2009-04-05 15:22:20 -0700 bug24979 bug24979.patch text/plain 4080 mbelshe SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MjE0NikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDktMDMtMzEgIE1pa2UgQmVsc2hlICA8bWlrZUBiZWxzaGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjQ5NzkKKyAgICAgICAgQWRkIGFjY2Vz c29ycyB0byBBdG9taWNTdHJpbmcgd2hpY2ggdGFrZSBTdHJpbmcgYXJndW1lbnRzCisgICAgICAg IHNvIHRoYXQgd2UgZG9uJ3QgYWNjaWRlbnRhbGx5IHRocmFzaCB0aGUgQXRvbWljU3RyaW5nIHRh YmxlLgorCisgICAgICAgICogYmluZGluZ3MvdjgvY3VzdG9tL1Y4Tm9kZUN1c3RvbS5jcHA6CisK IDIwMDktMDMtMzEgIEVyaWMgQ2FybHNvbiAgPGVyaWMuY2FybHNvbkBhcHBsZS5jb20+CiAKICAg ICAgICAgUmV2aWV3ZWQgYnkgU2ltb24gRnJhc2VyLgpJbmRleDogV2ViQ29yZS9kb20vRG9jdW1l bnQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvZG9tL0RvY3VtZW50LmNwcAkocmV2aXNpb24g NDIxNDYpCisrKyBXZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIz MzAsNyArMjMzMCw3IEBAIHZvaWQgRG9jdW1lbnQ6OnJlY2FsY1N0eWxlU2VsZWN0b3IoKQogICAg ICAgICAgICAgICAgICAgICAvLyBpdCBpcyBsb2FkaW5nIGJ1dCB3ZSBzaG91bGQgc3RpbGwgZGVj aWRlIHdoaWNoIHN0eWxlIHNoZWV0IHNldCB0byB1c2UKICAgICAgICAgICAgICAgICAgICAgaWYg KCFlbmFibGVkVmlhU2NyaXB0ICYmICF0aXRsZS5pc0VtcHR5KCkgJiYgbV9wcmVmZXJyZWRTdHls ZXNoZWV0U2V0LmlzRW1wdHkoKSkgewogICAgICAgICAgICAgICAgICAgICAgICAgY29uc3QgQXRv bWljU3RyaW5nJiByZWwgPSBlLT5nZXRBdHRyaWJ1dGUocmVsQXR0cik7Ci0gICAgICAgICAgICAg ICAgICAgICAgICBpZiAoIXJlbC5jb250YWlucygiYWx0ZXJuYXRlIikpIHsKKyAgICAgICAgICAg ICAgICAgICAgICAgIGlmICghcmVsLmNvbnRhaW5zKFN0cmluZygiYWx0ZXJuYXRlIikpKSB7CiAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgbV9wcmVmZXJyZWRTdHlsZXNoZWV0U2V0ID0gdGl0 bGU7CiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbV9zZWxlY3RlZFN0eWxlc2hlZXRTZXQg PSB0aXRsZTsKICAgICAgICAgICAgICAgICAgICAgICAgIH0KQEAgLTIzNjUsNyArMjM2NSw3IEBA IHZvaWQgRG9jdW1lbnQ6OnJlY2FsY1N0eWxlU2VsZWN0b3IoKQogICAgICAgICAgICAgICAgICAg ICAvLyB1cyBhcyB0aGUgcHJlZmVycmVkIHNldC4gIE90aGVyd2lzZSwganVzdCBpZ25vcmUKICAg ICAgICAgICAgICAgICAgICAgLy8gdGhpcyBzaGVldC4KICAgICAgICAgICAgICAgICAgICAgQXRv bWljU3RyaW5nIHJlbCA9IGUtPmdldEF0dHJpYnV0ZShyZWxBdHRyKTsKLSAgICAgICAgICAgICAg ICAgICAgaWYgKGUtPmhhc0xvY2FsTmFtZShzdHlsZVRhZykgfHwgIXJlbC5jb250YWlucygiYWx0 ZXJuYXRlIikpCisgICAgICAgICAgICAgICAgICAgIGlmIChlLT5oYXNMb2NhbE5hbWUoc3R5bGVU YWcpIHx8ICFyZWwuY29udGFpbnMoU3RyaW5nKCJhbHRlcm5hdGUiKSkpCiAgICAgICAgICAgICAg ICAgICAgICAgICBtX3ByZWZlcnJlZFN0eWxlc2hlZXRTZXQgPSBtX3NlbGVjdGVkU3R5bGVzaGVl dFNldCA9IHRpdGxlOwogICAgICAgICAgICAgICAgIH0KIApJbmRleDogV2ViQ29yZS9wYWdlL0Zy YW1lVHJlZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wYWdlL0ZyYW1lVHJlZS5jcHAJKHJl dmlzaW9uIDQyMTQ2KQorKysgV2ViQ29yZS9wYWdlL0ZyYW1lVHJlZS5jcHAJKHdvcmtpbmcgY29w eSkKQEAgLTExOSw3ICsxMTksNyBAQCBBdG9taWNTdHJpbmcgRnJhbWVUcmVlOjp1bmlxdWVDaGls ZE5hbWUoCiAgICAgVmVjdG9yPEZyYW1lKiwgMTY+IGNoYWluOwogICAgIEZyYW1lKiBmcmFtZTsK ICAgICBmb3IgKGZyYW1lID0gbV90aGlzRnJhbWU7IGZyYW1lOyBmcmFtZSA9IGZyYW1lLT50cmVl KCktPnBhcmVudCgpKSB7Ci0gICAgICAgIGlmIChmcmFtZS0+dHJlZSgpLT5uYW1lKCkuc3RhcnRz V2l0aChmcmFtZVBhdGhQcmVmaXgpKQorICAgICAgICBpZiAoZnJhbWUtPnRyZWUoKS0+bmFtZSgp LnN0YXJ0c1dpdGgoU3RyaW5nKGZyYW1lUGF0aFByZWZpeCkpKQogICAgICAgICAgICAgYnJlYWs7 CiAgICAgICAgIGNoYWluLmFwcGVuZChmcmFtZSk7CiAgICAgfQpJbmRleDogV2ViQ29yZS9wbGF0 Zm9ybS90ZXh0L0F0b21pY1N0cmluZy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0v dGV4dC9BdG9taWNTdHJpbmcuaAkocmV2aXNpb24gNDIxNDYpCisrKyBXZWJDb3JlL3BsYXRmb3Jt L3RleHQvQXRvbWljU3RyaW5nLmgJKHdvcmtpbmcgY29weSkKQEAgLTY3LDE1ICs2NywyMyBAQCBw dWJsaWM6CiAgICAgVUNoYXIgb3BlcmF0b3JbXSh1bnNpZ25lZCBpbnQgaSkgY29uc3QgeyByZXR1 cm4gbV9zdHJpbmdbaV07IH0KICAgICAKICAgICBib29sIGNvbnRhaW5zKFVDaGFyIGMpIGNvbnN0 IHsgcmV0dXJuIG1fc3RyaW5nLmNvbnRhaW5zKGMpOyB9CisgICAgYm9vbCBjb250YWlucyhjb25z dCBTdHJpbmcmIHMsIGJvb2wgY2FzZVNlbnNpdGl2ZSA9IHRydWUpIGNvbnN0CisgICAgICAgIHsg cmV0dXJuIG1fc3RyaW5nLmNvbnRhaW5zKHMsIGNhc2VTZW5zaXRpdmUpOyB9CiAgICAgYm9vbCBj b250YWlucyhjb25zdCBBdG9taWNTdHJpbmcmIHMsIGJvb2wgY2FzZVNlbnNpdGl2ZSA9IHRydWUp IGNvbnN0CiAgICAgICAgIHsgcmV0dXJuIG1fc3RyaW5nLmNvbnRhaW5zKHMuc3RyaW5nKCksIGNh c2VTZW5zaXRpdmUpOyB9CiAKICAgICBpbnQgZmluZChVQ2hhciBjLCBpbnQgc3RhcnQgPSAwKSBj b25zdCB7IHJldHVybiBtX3N0cmluZy5maW5kKGMsIHN0YXJ0KTsgfQorICAgIGludCBmaW5kKGNv bnN0IFN0cmluZyYgcywgaW50IHN0YXJ0ID0gMCwgYm9vbCBjYXNlU2VudGl0aXZlID0gdHJ1ZSkg Y29uc3QKKyAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuZmluZChzLCBzdGFydCwgY2FzZVNlbnRp dGl2ZSk7IH0KICAgICBpbnQgZmluZChjb25zdCBBdG9taWNTdHJpbmcmIHMsIGludCBzdGFydCA9 IDAsIGJvb2wgY2FzZVNlbnRpdGl2ZSA9IHRydWUpIGNvbnN0CiAgICAgICAgIHsgcmV0dXJuIG1f c3RyaW5nLmZpbmQocy5zdHJpbmcoKSwgc3RhcnQsIGNhc2VTZW50aXRpdmUpOyB9CiAgICAgCisg ICAgYm9vbCBzdGFydHNXaXRoKGNvbnN0IFN0cmluZyYgcywgYm9vbCBjYXNlU2Vuc2l0aXZlID0g dHJ1ZSkgY29uc3QKKyAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuc3RhcnRzV2l0aChzLCBjYXNl U2Vuc2l0aXZlKTsgfQogICAgIGJvb2wgc3RhcnRzV2l0aChjb25zdCBBdG9taWNTdHJpbmcmIHMs IGJvb2wgY2FzZVNlbnNpdGl2ZSA9IHRydWUpIGNvbnN0CiAgICAgICAgIHsgcmV0dXJuIG1fc3Ry aW5nLnN0YXJ0c1dpdGgocy5zdHJpbmcoKSwgY2FzZVNlbnNpdGl2ZSk7IH0KKyAgICBib29sIGVu ZHNXaXRoKGNvbnN0IFN0cmluZyYgcywgYm9vbCBjYXNlU2Vuc2l0aXZlID0gdHJ1ZSkgY29uc3QK KyAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuZW5kc1dpdGgocywgY2FzZVNlbnNpdGl2ZSk7IH0K ICAgICBib29sIGVuZHNXaXRoKGNvbnN0IEF0b21pY1N0cmluZyYgcywgYm9vbCBjYXNlU2Vuc2l0 aXZlID0gdHJ1ZSkgY29uc3QKICAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuZW5kc1dpdGgocy5z dHJpbmcoKSwgY2FzZVNlbnNpdGl2ZSk7IH0KICAgICAK 29268 2009-04-05 15:24:17 -0700 2009-04-06 04:04:09 -0700 updated patch bug24979.patch text/plain 2599 mbelshe SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MjIzNCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDktMDQtMDUgIE1pa2UgQmVsc2hlICA8bWlrZUBiZWxzaGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjQ5NzkKKyAgICAgICAgQWRkIGFjY2Vz c29ycyB0byBBdG9taWNTdHJpbmcgd2hpY2ggdGFrZSBTdHJpbmcvY29uc3QgY2hhciogYXJndW1l bnRzCisgICAgICAgIHNvIHRoYXQgd2UgZG9uJ3QgYWNjaWRlbnRhbGx5IHRocmFzaCB0aGUgQXRv bWljU3RyaW5nIHRhYmxlLgorCisgICAgICAgICogcGxhdGZvcm0vdGV4dC9BdG9taWNTdHJpbmcu aDoKKwogMjAwOS0wNC0wNSAgRXJpayBMLiBCdW5jZSAgPGVsYnVuY2VAeGVuZG9tLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBIYXVzbWFubi4KSW5kZXg6IFdlYkNvcmUvcGxhdGZv cm0vdGV4dC9BdG9taWNTdHJpbmcuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL3Rl eHQvQXRvbWljU3RyaW5nLmgJKHJldmlzaW9uIDQyMjM0KQorKysgV2ViQ29yZS9wbGF0Zm9ybS90 ZXh0L0F0b21pY1N0cmluZy5oCSh3b3JraW5nIGNvcHkpCkBAIC02NywxNyArNjcsMjEgQEAgcHVi bGljOgogICAgIFVDaGFyIG9wZXJhdG9yW10odW5zaWduZWQgaW50IGkpIGNvbnN0IHsgcmV0dXJu IG1fc3RyaW5nW2ldOyB9CiAgICAgCiAgICAgYm9vbCBjb250YWlucyhVQ2hhciBjKSBjb25zdCB7 IHJldHVybiBtX3N0cmluZy5jb250YWlucyhjKTsgfQotICAgIGJvb2wgY29udGFpbnMoY29uc3Qg QXRvbWljU3RyaW5nJiBzLCBib29sIGNhc2VTZW5zaXRpdmUgPSB0cnVlKSBjb25zdAotICAgICAg ICB7IHJldHVybiBtX3N0cmluZy5jb250YWlucyhzLnN0cmluZygpLCBjYXNlU2Vuc2l0aXZlKTsg fQorICAgIGJvb2wgY29udGFpbnMoY29uc3QgY2hhciogcywgYm9vbCBjYXNlU2Vuc2l0aXZlID0g dHJ1ZSkgY29uc3QKKyAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuY29udGFpbnMocywgY2FzZVNl bnNpdGl2ZSk7IH0KKyAgICBib29sIGNvbnRhaW5zKGNvbnN0IFN0cmluZyYgcywgYm9vbCBjYXNl U2Vuc2l0aXZlID0gdHJ1ZSkgY29uc3QKKyAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuY29udGFp bnMocywgY2FzZVNlbnNpdGl2ZSk7IH0KIAogICAgIGludCBmaW5kKFVDaGFyIGMsIGludCBzdGFy dCA9IDApIGNvbnN0IHsgcmV0dXJuIG1fc3RyaW5nLmZpbmQoYywgc3RhcnQpOyB9Ci0gICAgaW50 IGZpbmQoY29uc3QgQXRvbWljU3RyaW5nJiBzLCBpbnQgc3RhcnQgPSAwLCBib29sIGNhc2VTZW50 aXRpdmUgPSB0cnVlKSBjb25zdAotICAgICAgICB7IHJldHVybiBtX3N0cmluZy5maW5kKHMuc3Ry aW5nKCksIHN0YXJ0LCBjYXNlU2VudGl0aXZlKTsgfQorICAgIGludCBmaW5kKGNvbnN0IGNoYXIq IHMsIGludCBzdGFydCA9IDAsIGJvb2wgY2FzZVNlbnRpdGl2ZSA9IHRydWUpIGNvbnN0CisgICAg ICAgIHsgcmV0dXJuIG1fc3RyaW5nLmZpbmQocywgc3RhcnQsIGNhc2VTZW50aXRpdmUpOyB9Cisg ICAgaW50IGZpbmQoY29uc3QgU3RyaW5nJiBzLCBpbnQgc3RhcnQgPSAwLCBib29sIGNhc2VTZW50 aXRpdmUgPSB0cnVlKSBjb25zdAorICAgICAgICB7IHJldHVybiBtX3N0cmluZy5maW5kKHMsIHN0 YXJ0LCBjYXNlU2VudGl0aXZlKTsgfQogICAgIAotICAgIGJvb2wgc3RhcnRzV2l0aChjb25zdCBB dG9taWNTdHJpbmcmIHMsIGJvb2wgY2FzZVNlbnNpdGl2ZSA9IHRydWUpIGNvbnN0Ci0gICAgICAg IHsgcmV0dXJuIG1fc3RyaW5nLnN0YXJ0c1dpdGgocy5zdHJpbmcoKSwgY2FzZVNlbnNpdGl2ZSk7 IH0KLSAgICBib29sIGVuZHNXaXRoKGNvbnN0IEF0b21pY1N0cmluZyYgcywgYm9vbCBjYXNlU2Vu c2l0aXZlID0gdHJ1ZSkgY29uc3QKLSAgICAgICAgeyByZXR1cm4gbV9zdHJpbmcuZW5kc1dpdGgo cy5zdHJpbmcoKSwgY2FzZVNlbnNpdGl2ZSk7IH0KKyAgICBib29sIHN0YXJ0c1dpdGgoY29uc3Qg U3RyaW5nJiBzLCBib29sIGNhc2VTZW5zaXRpdmUgPSB0cnVlKSBjb25zdAorICAgICAgICB7IHJl dHVybiBtX3N0cmluZy5zdGFydHNXaXRoKHMsIGNhc2VTZW5zaXRpdmUpOyB9CisgICAgYm9vbCBl bmRzV2l0aChjb25zdCBTdHJpbmcmIHMsIGJvb2wgY2FzZVNlbnNpdGl2ZSA9IHRydWUpIGNvbnN0 CisgICAgICAgIHsgcmV0dXJuIG1fc3RyaW5nLmVuZHNXaXRoKHMsIGNhc2VTZW5zaXRpdmUpOyB9 CiAgICAgCiAgICAgaW50IHRvSW50KGJvb2wqIG9rID0gMCkgY29uc3QgeyByZXR1cm4gbV9zdHJp bmcudG9JbnQob2spOyB9CiAgICAgZG91YmxlIHRvRG91YmxlKGJvb2wqIG9rID0gMCkgY29uc3Qg eyByZXR1cm4gbV9zdHJpbmcudG9Eb3VibGUob2spOyB9Cg==