86307 2012-05-12 17:17:32 -0700 Simplify SourceProvider's subclasses 2012-09-01 17:51:39 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 1 benjamin benjamin oldest_to_newest 622431 0 benjamin 2012-05-12 17:17:32 -0700 Simplify SourceProvider's subclasses 622447 1 141591 benjamin 2012-05-12 19:17:14 -0700 Created attachment 141591 Patch 622552 2 141591 darin 2012-05-13 15:59:03 -0700 Comment on attachment 141591 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141591&action=review This is OK as-is, but we might be able to do a bit better. > Source/JavaScriptCore/parser/SourceProvider.h:61 > + const StringImpl* data() const > + { > + return stringImpl(); > + } Why not leave this as a pure virtual function and call it instead of adding a new stringImpl() function? Or maybe this could all be done with the source() function? I am unclear on why there need to be three different functions that all return the source string. Also, all the callers seem to be calling impl() so maybe this should return a const UString& or const String&. That would be one way of saving a bit of reference count churn in the getRange function. I guess the source() function already works that way. > Source/JavaScriptCore/parser/SourceProvider.h:65 > + int length = end - start; Why a local variable for this? Doesn’t seem better than just saying end - start in the function call. > Source/JavaScriptCore/parser/SourceProvider.h:66 > + return UString(stringImpl()).substringSharingImpl(start, length); Would be nice to add a version of substringSharingImpl that works directly on a StringImpl* so we don’t have to churn the reference count an extra time each time this function is called. > Source/JavaScriptCore/parser/SourceProvider.h:72 > + StringImpl* impl = stringImpl(); > + return impl ? impl->length() : 0; There must be a better name than “impl” for this. Maybe “data” or “string”. Words are better than word fragments. 626004 3 benjamin 2012-05-16 16:40:26 -0700 The method source() is actually WebCore specific. Generalizing it to JSC is a good idea, I will look into that. 710298 4 benjamin 2012-09-01 17:51:39 -0700 With the UString change, this problem has radically changed. I close this bug as it is now superseded by https://bugs.webkit.org/show_bug.cgi?id=95635 141591 2012-05-12 19:17:14 -0700 2012-05-13 15:59:03 -0700 Patch bug-86307-20120512191713.patch text/plain 7175 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2ODYwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAx YzkzZjFkMTdjZTViNDA0ZjU4NWE4MjQ5MjdjZTFiMTBlYjFkZTUxLi43YWM5NThkZTU3MjhiMGI3 MWJjYTc0NTU0NWUxNWEzM2IxNTBjZDQ5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMiBAQAorMjAxMi0wNS0xMiAgQmVuamFtaW4gUG91bGFpbiAgPGJlbmphbWluQHdlYmtp dC5vcmc+CisKKyAgICAgICAgU2ltcGxpZnkgU291cmNlUHJvdmlkZXIncyBzdWJjbGFzc2VzCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NjMwNworCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFsbCB0aGUgc3Vi Y2xhc3NlcyBvZiBTb3VyY2VQcm92aWRlciBhcmUgYmFzZWQgb24gU3RyaW5nSW1wbCAoYmVpbmcg aW4gVVN0cmluZyBvciBTdHJpbmcpCisgICAgICAgIGFuZCB3ZXJlIHJlLWltcGxlbWVudGluZyB0 aGUgc2FtZSAzIG1ldGhvZHMgdGhlIHNhbWUgd2F5LgorCisgICAgICAgIFRoZSBpbXBsZW1lbnRh dGlvbiBpcyBwdXNoZWQgdXAgdG8gU291cmNlUHJvdmlkZXIgYW5kIGFsbCB0aGUgc3ViY2xhc3Mg aGF2ZSB0byBkbworICAgICAgICBpcyBwcm92aWRlIGEgU3RyaW5nSW1wbCBmb3IgdGhlIGN1cnJl bnQgc291cmNlLgorCisgICAgICAgICogcGFyc2VyL1NvdXJjZVByb3ZpZGVyLmg6CisgICAgICAg IChKU0M6OlNvdXJjZVByb3ZpZGVyOjpkYXRhKToKKyAgICAgICAgKFNvdXJjZVByb3ZpZGVyKToK KyAgICAgICAgKEpTQzo6U291cmNlUHJvdmlkZXI6OmdldFJhbmdlKToKKyAgICAgICAgKEpTQzo6 U291cmNlUHJvdmlkZXI6Omxlbmd0aCk6CisKIDIwMTItMDUtMTEgIEdhdmluIEJhcnJhY2xvdWdo ICA8YmFycmFjbG91Z2hAYXBwbGUuY29tPgogCiAgICAgICAgIEludHJvZHVjZSBQcm9wZXJ0eU5h bWUgY2xhc3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZwppbmRleCAwM2I3Nzk3NDg2YmM5NTMyNmJmNjFjODNhZmJjODg2NTNl YTg1YTM2Li5lNjRjMDY4YzIyMzVlMTkzZjBlNmRlOGQ4NWNiYTUyMDRmZWIzYzYwIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTItMDUtMTIgIEJlbmphbWluIFBvdWxhaW4gIDxiZW5q YW1pbkB3ZWJraXQub3JnPgorCisgICAgICAgIFNpbXBsaWZ5IFNvdXJjZVByb3ZpZGVyJ3Mgc3Vi Y2xhc3NlcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 ODYzMDcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBS ZW1vdmUgdGhlIG1ldGhvZHMgdGhhdCBoYXZlIGJlY29tZSB1c2VsZXNzLgorCisgICAgICAgIEFk ZCBPVkVSUklERSB0byBtZXRob2RzIHdoZW4gcG9zc2libGUuCisKKyAgICAgICAgKiBiaW5kaW5n cy9qcy9DYWNoZWRTY3JpcHRTb3VyY2VQcm92aWRlci5oOgorICAgICAgICAqIGJpbmRpbmdzL2pz L1N0cmluZ1NvdXJjZVByb3ZpZGVyLmg6CisgICAgICAgIChTdHJpbmdTb3VyY2VQcm92aWRlcik6 CisKIDIwMTItMDUtMTIgIFBoaWxpcCBSb2dlcnMgIDxwZHJAZ29vZ2xlLmNvbT4KIAogICAgICAg ICBDbGVhbnVwIGJlZm9yZSBjaGFuZ2luZyBhdHRyaWJ1dGVOYW1lIGluIFNWRyA8YW5pbWF0ZT4K ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9wYXJzZXIvU291cmNlUHJvdmlkZXIu aCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9wYXJzZXIvU291cmNlUHJvdmlkZXIuaAppbmRleCAy MGJlNjNjYWNkNDhmYTJiOTY5MTJlNTUwMDA3OTZlMjc1NDJkZDE3Li40OWUwNjQwNTVjMWMxODZl MTZlNDljMjYxNjBhZmNmM2NjN2Q0OTYyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvcGFyc2VyL1NvdXJjZVByb3ZpZGVyLmgKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3Bh cnNlci9Tb3VyY2VQcm92aWRlci5oCkBAIC01NSw5ICs1NSwyMiBAQCBuYW1lc3BhY2UgSlNDIHsK ICAgICAgICAgICAgICAgICBkZWxldGUgbV9jYWNoZTsKICAgICAgICAgfQogCi0gICAgICAgIHZp cnR1YWwgVVN0cmluZyBnZXRSYW5nZShpbnQgc3RhcnQsIGludCBlbmQpIGNvbnN0ID0gMDsKLSAg ICAgICAgdmlydHVhbCBjb25zdCBTdHJpbmdJbXBsKiBkYXRhKCkgY29uc3QgPSAwOwotICAgICAg ICB2aXJ0dWFsIGludCBsZW5ndGgoKSBjb25zdCA9IDA7CisgICAgICAgIGNvbnN0IFN0cmluZ0lt cGwqIGRhdGEoKSBjb25zdAorICAgICAgICB7CisgICAgICAgICAgICByZXR1cm4gc3RyaW5nSW1w bCgpOworICAgICAgICB9CisKKyAgICAgICAgVVN0cmluZyBnZXRSYW5nZShpbnQgc3RhcnQsIGlu dCBlbmQpIGNvbnN0CisgICAgICAgIHsKKyAgICAgICAgICAgIGludCBsZW5ndGggPSBlbmQgLSBz dGFydDsKKyAgICAgICAgICAgIHJldHVybiBVU3RyaW5nKHN0cmluZ0ltcGwoKSkuc3Vic3RyaW5n U2hhcmluZ0ltcGwoc3RhcnQsIGxlbmd0aCk7CisgICAgICAgIH0KKworICAgICAgICBpbnQgbGVu Z3RoKCkgY29uc3QKKyAgICAgICAgeworICAgICAgICAgICAgU3RyaW5nSW1wbCogaW1wbCA9IHN0 cmluZ0ltcGwoKTsKKyAgICAgICAgICAgIHJldHVybiBpbXBsID8gaW1wbC0+bGVuZ3RoKCkgOiAw OworICAgICAgICB9CiAgICAgICAgIAogICAgICAgICBjb25zdCBVU3RyaW5nJiB1cmwoKSB7IHJl dHVybiBtX3VybDsgfQogICAgICAgICBUZXh0UG9zaXRpb24gc3RhcnRQb3NpdGlvbigpIGNvbnN0 IHsgcmV0dXJuIG1fc3RhcnRQb3NpdGlvbjsgfQpAQCAtNzEsNiArODQsNyBAQCBuYW1lc3BhY2Ug SlNDIHsKICAgICAgICAgCiAgICAgcHJpdmF0ZToKICAgICAgICAgdmlydHVhbCB2b2lkIGNhY2hl U2l6ZUNoYW5nZWQoaW50IGRlbHRhKSB7IFVOVVNFRF9QQVJBTShkZWx0YSk7IH0KKyAgICAgICAg dmlydHVhbCBTdHJpbmdJbXBsKiBzdHJpbmdJbXBsKCkgY29uc3QgPSAwOwogCiAgICAgICAgIFVT dHJpbmcgbV91cmw7CiAgICAgICAgIFRleHRQb3NpdGlvbiBtX3N0YXJ0UG9zaXRpb247CkBAIC04 NiwxOSArMTAwLDEzIEBAIG5hbWVzcGFjZSBKU0MgewogICAgICAgICAgICAgcmV0dXJuIGFkb3B0 UmVmKG5ldyBVU3RyaW5nU291cmNlUHJvdmlkZXIoc291cmNlLCB1cmwsIHN0YXJ0UG9zaXRpb24p KTsKICAgICAgICAgfQogCi0gICAgICAgIHZpcnR1YWwgVVN0cmluZyBnZXRSYW5nZShpbnQgc3Rh cnQsIGludCBlbmQpIGNvbnN0IE9WRVJSSURFCi0gICAgICAgIHsKLSAgICAgICAgICAgIHJldHVy biBtX3NvdXJjZS5zdWJzdHJpbmdTaGFyaW5nSW1wbChzdGFydCwgZW5kIC0gc3RhcnQpOwotICAg ICAgICB9Ci0gICAgICAgIGNvbnN0IFN0cmluZ0ltcGwqIGRhdGEoKSBjb25zdCB7IHJldHVybiBt X3NvdXJjZS5pbXBsKCk7IH0KLSAgICAgICAgaW50IGxlbmd0aCgpIGNvbnN0IHsgcmV0dXJuIG1f c291cmNlLmxlbmd0aCgpOyB9Ci0KICAgICBwcml2YXRlOgogICAgICAgICBVU3RyaW5nU291cmNl UHJvdmlkZXIoY29uc3QgVVN0cmluZyYgc291cmNlLCBjb25zdCBVU3RyaW5nJiB1cmwsIGNvbnN0 IFRleHRQb3NpdGlvbiYgc3RhcnRQb3NpdGlvbikKICAgICAgICAgICAgIDogU291cmNlUHJvdmlk ZXIodXJsLCBzdGFydFBvc2l0aW9uKQogICAgICAgICAgICAgLCBtX3NvdXJjZShzb3VyY2UpCiAg ICAgICAgIHsKICAgICAgICAgfQorICAgICAgICB2aXJ0dWFsIFN0cmluZ0ltcGwqIHN0cmluZ0lt cGwoKSBjb25zdCBPVkVSUklERSB7IHJldHVybiBtX3NvdXJjZS5pbXBsKCk7IH0KIAogICAgICAg ICBVU3RyaW5nIG1fc291cmNlOwogICAgIH07CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9i aW5kaW5ncy9qcy9DYWNoZWRTY3JpcHRTb3VyY2VQcm92aWRlci5oIGIvU291cmNlL1dlYkNvcmUv YmluZGluZ3MvanMvQ2FjaGVkU2NyaXB0U291cmNlUHJvdmlkZXIuaAppbmRleCA4ZWZiNDgxNzUz NDM2NmRjNTQ3OTk4NmIzYjBhZDJmODg1MDZiYzYxLi5hYzdkODdhZmRjNmY2NDk0M2U4Y2Q5ZTFi NWQ0YWU3NmFjN2EzODZkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9D YWNoZWRTY3JpcHRTb3VyY2VQcm92aWRlci5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdz L2pzL0NhY2hlZFNjcmlwdFNvdXJjZVByb3ZpZGVyLmgKQEAgLTQ1LDIxICs0NSw3IEBAIG5hbWVz cGFjZSBXZWJDb3JlIHsKICAgICAgICAgICAgIG1fY2FjaGVkU2NyaXB0LT5yZW1vdmVDbGllbnQo dGhpcyk7CiAgICAgICAgIH0KIAotICAgICAgICB2aXJ0dWFsIEpTQzo6VVN0cmluZyBnZXRSYW5n ZShpbnQgc3RhcnQsIGludCBlbmQpIGNvbnN0IE9WRVJSSURFCi0gICAgICAgIHsKLSAgICAgICAg ICAgIGludCBsZW5ndGggPSBlbmQgLSBzdGFydDsKLSAgICAgICAgICAgIEFTU0VSVChsZW5ndGgg Pj0gMCk7Ci0gICAgICAgICAgICBBU1NFUlQoc3RhcnQgKyBsZW5ndGggPD0gdGhpcy0+bGVuZ3Ro KCkpOwotCi0gICAgICAgICAgICBTdHJpbmcgc2NyaXB0ID0gbV9jYWNoZWRTY3JpcHQtPnNjcmlw dCgpOwotICAgICAgICAgICAgcmV0dXJuIEpTQzo6VVN0cmluZyhTdHJpbmdJbXBsOjpjcmVhdGUo c2NyaXB0LmltcGwoKSwgc3RhcnQsIGxlbmd0aCkpOwotICAgICAgICB9Ci0KLSAgICAgICAgY29u c3QgU3RyaW5nSW1wbCogZGF0YSgpIGNvbnN0IHsgcmV0dXJuIG1fY2FjaGVkU2NyaXB0LT5zY3Jp cHQoKS5pbXBsKCk7IH0KLSAgICAgICAgaW50IGxlbmd0aCgpIGNvbnN0IHsgcmV0dXJuIG1fY2Fj aGVkU2NyaXB0LT5zY3JpcHQoKS5sZW5ndGgoKTsgfQotICAgICAgICBjb25zdCBTdHJpbmcmIHNv dXJjZSgpIGNvbnN0IHsgcmV0dXJuIG1fY2FjaGVkU2NyaXB0LT5zY3JpcHQoKTsgfQotCi0gICAg ICAgIHZpcnR1YWwgdm9pZCBjYWNoZVNpemVDaGFuZ2VkKGludCBkZWx0YSkgCisgICAgICAgIHZp cnR1YWwgdm9pZCBjYWNoZVNpemVDaGFuZ2VkKGludCBkZWx0YSkgT1ZFUlJJREUKICAgICAgICAg eyAKICAgICAgICAgICAgIG1fY2FjaGVkU2NyaXB0LT5zb3VyY2VQcm92aWRlckNhY2hlU2l6ZUNo YW5nZWQoZGVsdGEpOwogICAgICAgICB9CkBAIC03Miw2ICs1OCwxNiBAQCBuYW1lc3BhY2UgV2Vi Q29yZSB7CiAgICAgICAgICAgICBtX2NhY2hlZFNjcmlwdC0+YWRkQ2xpZW50KHRoaXMpOwogICAg ICAgICB9CiAKKyAgICAgICAgdmlydHVhbCBTdHJpbmdJbXBsKiBzdHJpbmdJbXBsKCkgY29uc3Qg T1ZFUlJJREUKKyAgICAgICAgeworICAgICAgICAgICAgcmV0dXJuIG1fY2FjaGVkU2NyaXB0LT5z Y3JpcHQoKS5pbXBsKCk7CisgICAgICAgIH0KKworICAgICAgICB2aXJ0dWFsIGNvbnN0IFN0cmlu ZyYgc291cmNlKCkgY29uc3QgT1ZFUlJJREUKKyAgICAgICAgeworICAgICAgICAgICAgcmV0dXJu IG1fY2FjaGVkU2NyaXB0LT5zY3JpcHQoKTsKKyAgICAgICAgfQorCiAgICAgICAgIENhY2hlZFJl c291cmNlSGFuZGxlPENhY2hlZFNjcmlwdD4gbV9jYWNoZWRTY3JpcHQ7CiAgICAgfTsKIApkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvU3RyaW5nU291cmNlUHJvdmlkZXIu aCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL1N0cmluZ1NvdXJjZVByb3ZpZGVyLmgKaW5k ZXggOGI2NzE4ODkxMmI3N2U2NDg5ODc2NzgwMjc4MzY1NzFlMTFjM2EzZS4uYmNjYjJhYzQyMGIx N2JiMWYxYzZjNjYwODdhNDEyOTQ0YzdmZjkyNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv YmluZGluZ3MvanMvU3RyaW5nU291cmNlUHJvdmlkZXIuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9i aW5kaW5ncy9qcy9TdHJpbmdTb3VyY2VQcm92aWRlci5oCkBAIC00MiwyNSArNDIsMjIgQEAgbmFt ZXNwYWNlIFdlYkNvcmUgewogICAgICAgICAgICAgcmV0dXJuIGFkb3B0UmVmKG5ldyBTdHJpbmdT b3VyY2VQcm92aWRlcihzb3VyY2UsIHVybCwgc3RhcnRQb3NpdGlvbikpOwogICAgICAgICB9CiAK LSAgICAgICAgdmlydHVhbCBKU0M6OlVTdHJpbmcgZ2V0UmFuZ2UoaW50IHN0YXJ0LCBpbnQgZW5k KSBjb25zdCBPVkVSUklERQotICAgICAgICB7Ci0gICAgICAgICAgICBpbnQgbGVuZ3RoID0gZW5k IC0gc3RhcnQ7Ci0gICAgICAgICAgICBBU1NFUlQobGVuZ3RoID49IDApOwotICAgICAgICAgICAg QVNTRVJUKHN0YXJ0ICsgbGVuZ3RoIDw9IHRoaXMtPmxlbmd0aCgpKTsKLQotICAgICAgICAgICAg cmV0dXJuIEpTQzo6VVN0cmluZyhTdHJpbmdJbXBsOjpjcmVhdGUobV9zb3VyY2UuaW1wbCgpLCBz dGFydCwgbGVuZ3RoKSk7Ci0gICAgICAgIH0KLQotICAgICAgICBjb25zdCBTdHJpbmdJbXBsKiBk YXRhKCkgY29uc3QgeyByZXR1cm4gbV9zb3VyY2UuaW1wbCgpOyB9Ci0gICAgICAgIGludCBsZW5n dGgoKSBjb25zdCB7IHJldHVybiBtX3NvdXJjZS5sZW5ndGgoKTsgfQotICAgICAgICBjb25zdCBT dHJpbmcmIHNvdXJjZSgpIGNvbnN0IHsgcmV0dXJuIG1fc291cmNlOyB9Ci0KICAgICBwcml2YXRl OgogICAgICAgICBTdHJpbmdTb3VyY2VQcm92aWRlcihjb25zdCBTdHJpbmcmIHNvdXJjZSwgY29u c3QgU3RyaW5nJiB1cmwsIGNvbnN0IFRleHRQb3NpdGlvbiYgc3RhcnRQb3NpdGlvbikKICAgICAg ICAgICAgIDogU2NyaXB0U291cmNlUHJvdmlkZXIoc3RyaW5nVG9VU3RyaW5nKHVybCksIHN0YXJ0 UG9zaXRpb24pCiAgICAgICAgICAgICAsIG1fc291cmNlKHNvdXJjZSkKICAgICAgICAgewogICAg ICAgICB9CisKKyAgICAgICAgdmlydHVhbCBTdHJpbmdJbXBsKiBzdHJpbmdJbXBsKCkgY29uc3Qg T1ZFUlJJREUKKyAgICAgICAgeworICAgICAgICAgICAgcmV0dXJuIG1fc291cmNlLmltcGwoKTsK KyAgICAgICAgfQorCisgICAgICAgIHZpcnR1YWwgY29uc3QgU3RyaW5nJiBzb3VyY2UoKSBjb25z dCBPVkVSUklERQorICAgICAgICB7CisgICAgICAgICAgICByZXR1cm4gbV9zb3VyY2U7CisgICAg ICAgIH0KICAgICAgICAgCiAgICAgICAgIFN0cmluZyBtX3NvdXJjZTsKICAgICB9Owo=