116318 2013-05-17 08:20:09 -0700 [CSS] Minor cleanups in CSS variables handling 2013-05-17 14:53:08 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 csaavedra csaavedra commit-queue esprehn+autocc glenn macpherson menard rniwa oldest_to_newest 890397 0 csaavedra 2013-05-17 08:20:09 -0700 [CSS] Minor cleanups in CSS variables handling 890400 1 202091 csaavedra 2013-05-17 08:23:24 -0700 Created attachment 202091 Patch 890423 2 202091 darin 2013-05-17 09:09:07 -0700 Comment on attachment 202091 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202091&action=review > Source/WebCore/css/CSSParser.cpp:-407 > - ASSERT(m_length >= position + length); > - > - RefPtr<StringImpl> result; > - > - if (is8Bit()) { > - result = StringImpl::create(characters8() + position, length); > - } else { > - result = StringImpl::create(characters16() + position, length); > - } > - > - return AtomicString(result); Here’s the correct way to write this: ASSERT(m_length >= length); ASSERT(m_length >= position + length); if (is8Bit()) return AtomicString(characters8() + position, length); return AtomicString(characters16() + position, length); There is no reason to call StringImpl::create, and in fact that costs memory allocation and deallocation when the string already exists in the atomic string table. > Source/WebCore/css/CSSParser.cpp:3313 > - AtomicString variableName = name.substring(prefixLength, name.length() - prefixLength); > + AtomicString variableName(String(name).impl(), prefixLength, name.length() - prefixLength); Like the old code, this does unnecessary memory allocation, so you should do what I suggested instead. 890461 3 csaavedra 2013-05-17 09:52:23 -0700 (In reply to comment #2) > ASSERT(m_length >= length); Isn't that implicit below? > ASSERT(m_length >= position + length); 890472 4 202105 csaavedra 2013-05-17 09:59:17 -0700 Created attachment 202105 Patch 890586 5 202105 rniwa 2013-05-17 12:30:25 -0700 Comment on attachment 202105 Patch Sorry, I should have caught this when you were landing the first patch. 890685 6 csaavedra 2013-05-17 14:53:08 -0700 Committed r150302: <http://trac.webkit.org/changeset/150302> 202091 2013-05-17 08:23:24 -0700 2013-05-17 09:59:09 -0700 Patch bug-116318-20130517182304.patch text/plain 3769 csaavedra U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwMjQ0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMmYwYWM0OTNhYjFkMTc1 MTdlNzRjMTc1YTJlNTU0Mjk2NmYwOWNiYy4uMTlmNGVjZWZiNDE4OGI4ODhiMTg3ZjVhNTkwNWY1 NTJiOWEzMTM4OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEzLTA1LTE3ICBDbGF1 ZGlvIFNhYXZlZHJhICA8Y3NhYXZlZHJhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0NTU10gTWlu b3IgY2xlYW51cHMgaW4gQ1NTIHZhcmlhYmxlcyBoYW5kbGluZworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE2MzE4CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBvbmx5IGEgY2xlYW51 cC4KKworICAgICAgICAqIGNzcy9DU1NQYXJzZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Q1NT UGFyc2VyOjpjcmVhdGVQcmltaXRpdmVWYXJpYWJsZU5hbWVWYWx1ZSk6CisgICAgICAgIFJlbW92 ZSBpbnRlcm1lZGlhdGUgdmFyaWFibGUuCisgICAgICAgIChXZWJDb3JlOjpDU1NQYXJzZXI6OnN0 b3JlVmFyaWFibGVEZWNsYXJhdGlvbik6IERpcmVjdGx5CisgICAgICAgIGNyZWF0ZSBhIHN1YnN0 cmluZywgbm8gbmVlZCBmb3IgYW4gZXh0cmEgbWV0aG9kLgorICAgICAgICAqIGNzcy9DU1NQYXJz ZXJWYWx1ZXMuaDogUmVtb3ZlIHN1YnN0cmluZygpIG1ldGhvZCBkZWNsYXJhdGlvbi4KKwogMjAx My0wNS0xNiAgSmVzdXMgU2FuY2hlei1QYWxlbmNpYSAgPGplc3VzLnBhbGVuY2lhQG9wZW5ib3Nz YS5vcmc+CiAKICAgICAgICAgQWRkIHN0dWJzIGZvciBDb250ZXh0TWVudSBhbmQgQ29udGV4dE1l bnVJdGVtCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTUGFyc2VyLmNwcCBiL1Nv dXJjZS9XZWJDb3JlL2Nzcy9DU1NQYXJzZXIuY3BwCmluZGV4IDY5MGI3NDU2NzFjNWFjYWU5Yzhh ZTAzZmFjNTNiNjcwODIyNDZlZWMuLjlkMTI2MDNlMzczMzM5Yjk4NWY0N2ViMzE0ZmNkMDljYTc0 MDQ0M2IgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NQYXJzZXIuY3BwCisrKyBi L1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NQYXJzZXIuY3BwCkBAIC0zOTEsMjMgKzM5MSw2IEBAIHZv aWQgQ1NTUGFyc2VyU3RyaW5nOjpsb3dlcigpCiAgICAgbWFrZUxvd2VyKGNoYXJhY3RlcnMxNigp LCBjaGFyYWN0ZXJzMTYoKSwgbGVuZ3RoKCkpOwogfQogCi0jaWYgRU5BQkxFKENTU19WQVJJQUJM RVMpCi1BdG9taWNTdHJpbmcgQ1NTUGFyc2VyU3RyaW5nOjpzdWJzdHJpbmcodW5zaWduZWQgcG9z aXRpb24sIHVuc2lnbmVkIGxlbmd0aCkgY29uc3QKLXsKLSAgICBBU1NFUlQobV9sZW5ndGggPj0g cG9zaXRpb24gKyBsZW5ndGgpOwotCi0gICAgUmVmUHRyPFN0cmluZ0ltcGw+IHJlc3VsdDsKLQot ICAgIGlmIChpczhCaXQoKSkgewotICAgICAgICByZXN1bHQgPSBTdHJpbmdJbXBsOjpjcmVhdGUo Y2hhcmFjdGVyczgoKSArIHBvc2l0aW9uLCBsZW5ndGgpOwotICAgIH0gZWxzZSB7Ci0gICAgICAg IHJlc3VsdCA9IFN0cmluZ0ltcGw6OmNyZWF0ZShjaGFyYWN0ZXJzMTYoKSArIHBvc2l0aW9uLCBs ZW5ndGgpOwotICAgIH0KLQotICAgIHJldHVybiBBdG9taWNTdHJpbmcocmVzdWx0KTsKLX0KLSNl bmRpZgotCiB2b2lkIENTU1BhcnNlcjo6c2V0dXBQYXJzZXIoY29uc3QgY2hhciogcHJlZml4LCB1 bnNpZ25lZCBwcmVmaXhMZW5ndGgsIGNvbnN0IFN0cmluZyYgc3RyaW5nLCBjb25zdCBjaGFyKiBz dWZmaXgsIHVuc2lnbmVkIHN1ZmZpeExlbmd0aCkKIHsKICAgICBtX3BhcnNlZFRleHRQcmVmaXhM ZW5ndGggPSBwcmVmaXhMZW5ndGg7CkBAIC0xNzU2LDggKzE3MzksNyBAQCBpbmxpbmUgUGFzc1Jl ZlB0cjxDU1NQcmltaXRpdmVWYWx1ZT4gQ1NTUGFyc2VyOjpjcmVhdGVQcmltaXRpdmVTdHJpbmdW YWx1ZShDU1NQYQogaW5saW5lIFBhc3NSZWZQdHI8Q1NTUHJpbWl0aXZlVmFsdWU+IENTU1BhcnNl cjo6Y3JlYXRlUHJpbWl0aXZlVmFyaWFibGVOYW1lVmFsdWUoQ1NTUGFyc2VyVmFsdWUqIHZhbHVl KQogewogICAgIEFTU0VSVCh2YWx1ZS0+dW5pdCA9PSBDU1NQcmltaXRpdmVWYWx1ZTo6Q1NTX1ZB UklBQkxFX05BTUUpOwotICAgIEF0b21pY1N0cmluZyB2YXJpYWJsZU5hbWUgPSBTdHJpbmcodmFs dWUtPnN0cmluZyk7Ci0gICAgcmV0dXJuIENTU1ByaW1pdGl2ZVZhbHVlOjpjcmVhdGUodmFyaWFi bGVOYW1lLCBDU1NQcmltaXRpdmVWYWx1ZTo6Q1NTX1ZBUklBQkxFX05BTUUpOworICAgIHJldHVy biBDU1NQcmltaXRpdmVWYWx1ZTo6Y3JlYXRlKHZhbHVlLT5zdHJpbmcsIENTU1ByaW1pdGl2ZVZh bHVlOjpDU1NfVkFSSUFCTEVfTkFNRSk7CiB9CiAjZW5kaWYKIApAQCAtMzMyOCw3ICszMzEwLDcg QEAgdm9pZCBDU1NQYXJzZXI6OnN0b3JlVmFyaWFibGVEZWNsYXJhdGlvbihjb25zdCBDU1NQYXJz ZXJTdHJpbmcmIG5hbWUsIFBhc3NPd25QdHIKICAgICBzdGF0aWMgY29uc3QgdW5zaWduZWQgcHJl Zml4TGVuZ3RoID0gc2l6ZW9mKCItd2Via2l0LXZhci0iKSAtIDE7CiAgICAgCiAgICAgQVNTRVJU KG5hbWUubGVuZ3RoKCkgPiBwcmVmaXhMZW5ndGgpOwotICAgIEF0b21pY1N0cmluZyB2YXJpYWJs ZU5hbWUgPSBuYW1lLnN1YnN0cmluZyhwcmVmaXhMZW5ndGgsIG5hbWUubGVuZ3RoKCkgLSBwcmVm aXhMZW5ndGgpOworICAgIEF0b21pY1N0cmluZyB2YXJpYWJsZU5hbWUoU3RyaW5nKG5hbWUpLmlt cGwoKSwgcHJlZml4TGVuZ3RoLCBuYW1lLmxlbmd0aCgpIC0gcHJlZml4TGVuZ3RoKTsKIAogICAg IFN0cmluZ0J1aWxkZXIgYnVpbGRlcjsKICAgICBmb3IgKHVuc2lnbmVkIGkgPSAwLCBzaXplID0g dmFsdWUtPnNpemUoKTsgaSA8IHNpemU7IGkrKykgewpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvY3NzL0NTU1BhcnNlclZhbHVlcy5oIGIvU291cmNlL1dlYkNvcmUvY3NzL0NTU1BhcnNlclZh bHVlcy5oCmluZGV4IDE4Zjc2ZTE0NTlmODA0MWM5OTIwZjQwOGI1NmY1YzFkMTdjNTA2NzUuLjkx N2RiYWFkNWM0NDExNTNiZTkxYmI1ZDY3ZTQ3NmVhYWJmZGMyNjIgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2Nzcy9DU1NQYXJzZXJWYWx1ZXMuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9jc3Mv Q1NTUGFyc2VyVmFsdWVzLmgKQEAgLTk0LDEwICs5NCw2IEBAIHN0cnVjdCBDU1NQYXJzZXJTdHJp bmcgewogICAgIG9wZXJhdG9yIFN0cmluZygpIGNvbnN0IHsgcmV0dXJuIGlzOEJpdCgpID8gU3Ry aW5nKG1fZGF0YS5jaGFyYWN0ZXJzOCwgbV9sZW5ndGgpIDogU3RyaW5nKG1fZGF0YS5jaGFyYWN0 ZXJzMTYsIG1fbGVuZ3RoKTsgfQogICAgIG9wZXJhdG9yIEF0b21pY1N0cmluZygpIGNvbnN0IHsg cmV0dXJuIGlzOEJpdCgpID8gQXRvbWljU3RyaW5nKG1fZGF0YS5jaGFyYWN0ZXJzOCwgbV9sZW5n dGgpIDogQXRvbWljU3RyaW5nKG1fZGF0YS5jaGFyYWN0ZXJzMTYsIG1fbGVuZ3RoKTsgfQogCi0j aWYgRU5BQkxFKENTU19WQVJJQUJMRVMpCi0gICAgQXRvbWljU3RyaW5nIHN1YnN0cmluZyh1bnNp Z25lZCBwb3NpdGlvbiwgdW5zaWduZWQgbGVuZ3RoKSBjb25zdDsKLSNlbmRpZgotCiAgICAgdW5p b24gewogICAgICAgICBMQ2hhciogY2hhcmFjdGVyczg7CiAgICAgICAgIFVDaGFyKiBjaGFyYWN0 ZXJzMTY7Cg== 202105 2013-05-17 09:59:17 -0700 2013-05-17 12:30:24 -0700 Patch bug-116318-20130517195857.patch text/plain 2179 csaavedra U3VidmVyc2lvbiBSZXZpc2lvbjogMTUwMjYzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjRiYTgyYjk3OWI2Nzc4 YmJiNDIyZDMzNDI5ZDNlNmU4MWFkNWVmYy4uY2FkZWUxYjU0YjJlZGZiOTI2OTBkMjJlYjVlYjRj MmJhODZlMjAwZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEzLTA1LTE3ICBDbGF1 ZGlvIFNhYXZlZHJhICA8Y3NhYXZlZHJhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0NTU10gTWlu b3IgY2xlYW51cHMgaW4gQ1NTIHZhcmlhYmxlcyBoYW5kbGluZworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTE2MzE4CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBvbmx5IGEgY2xlYW51 cC4KKworICAgICAgICAqIGNzcy9DU1NQYXJzZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Q1NT UGFyc2VyU3RyaW5nOjpzdWJzdHJpbmcpOiBPcHRpbWl6ZS4KKyAgICAgICAgKFdlYkNvcmU6OkNT U1BhcnNlcjo6Y3JlYXRlUHJpbWl0aXZlVmFyaWFibGVOYW1lVmFsdWUpOgorICAgICAgICBSZW1v dmUgaW50ZXJtZWRpYXRlIHZhcmlhYmxlLgorCiAyMDEzLTA1LTE3ICBBbGJlcnRvIEdhcmNpYSAg PGFnYXJjaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBGaXggY29kZSB0aGF0IGV4cGVjdHMgUGFn ZTo6Y2hyb21lKCkgdG8gcmV0dXJuIGEgcG9pbnRlcgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNv cmUvY3NzL0NTU1BhcnNlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTUGFyc2VyLmNwcApp bmRleCA0ZWRmYmFiNmU4ZmQ3NmI2NjNiNDRlYjZhMjYxNDljNDUyNTgxZjMzLi4wYmU3YWIzYWUz NGRkZGVkMGZiZWEzMDk0NjZhZDM5OGQyYjNjZDBlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9jc3MvQ1NTUGFyc2VyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTUGFyc2VyLmNw cApAQCAtMzk2LDE1ICszOTYsOSBAQCBBdG9taWNTdHJpbmcgQ1NTUGFyc2VyU3RyaW5nOjpzdWJz dHJpbmcodW5zaWduZWQgcG9zaXRpb24sIHVuc2lnbmVkIGxlbmd0aCkgY29ucwogewogICAgIEFT U0VSVChtX2xlbmd0aCA+PSBwb3NpdGlvbiArIGxlbmd0aCk7CiAKLSAgICBSZWZQdHI8U3RyaW5n SW1wbD4gcmVzdWx0OwotCi0gICAgaWYgKGlzOEJpdCgpKSB7Ci0gICAgICAgIHJlc3VsdCA9IFN0 cmluZ0ltcGw6OmNyZWF0ZShjaGFyYWN0ZXJzOCgpICsgcG9zaXRpb24sIGxlbmd0aCk7Ci0gICAg fSBlbHNlIHsKLSAgICAgICAgcmVzdWx0ID0gU3RyaW5nSW1wbDo6Y3JlYXRlKGNoYXJhY3RlcnMx NigpICsgcG9zaXRpb24sIGxlbmd0aCk7Ci0gICAgfQotCi0gICAgcmV0dXJuIEF0b21pY1N0cmlu ZyhyZXN1bHQpOworICAgIGlmIChpczhCaXQoKSkKKyAgICAgICAgcmV0dXJuIEF0b21pY1N0cmlu ZyhjaGFyYWN0ZXJzOCgpICsgcG9zaXRpb24sIGxlbmd0aCk7CisgICAgcmV0dXJuIEF0b21pY1N0 cmluZyhjaGFyYWN0ZXJzMTYoKSArIHBvc2l0aW9uLCBsZW5ndGgpOwogfQogI2VuZGlmCiAKQEAg LTE3NTYsOCArMTc1MCw3IEBAIGlubGluZSBQYXNzUmVmUHRyPENTU1ByaW1pdGl2ZVZhbHVlPiBD U1NQYXJzZXI6OmNyZWF0ZVByaW1pdGl2ZVN0cmluZ1ZhbHVlKENTU1BhCiBpbmxpbmUgUGFzc1Jl ZlB0cjxDU1NQcmltaXRpdmVWYWx1ZT4gQ1NTUGFyc2VyOjpjcmVhdGVQcmltaXRpdmVWYXJpYWJs ZU5hbWVWYWx1ZShDU1NQYXJzZXJWYWx1ZSogdmFsdWUpCiB7CiAgICAgQVNTRVJUKHZhbHVlLT51 bml0ID09IENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfVkFSSUFCTEVfTkFNRSk7Ci0gICAgQXRvbWlj U3RyaW5nIHZhcmlhYmxlTmFtZSA9IFN0cmluZyh2YWx1ZS0+c3RyaW5nKTsKLSAgICByZXR1cm4g Q1NTUHJpbWl0aXZlVmFsdWU6OmNyZWF0ZSh2YXJpYWJsZU5hbWUsIENTU1ByaW1pdGl2ZVZhbHVl OjpDU1NfVkFSSUFCTEVfTkFNRSk7CisgICAgcmV0dXJuIENTU1ByaW1pdGl2ZVZhbHVlOjpjcmVh dGUodmFsdWUtPnN0cmluZywgQ1NTUHJpbWl0aXZlVmFsdWU6OkNTU19WQVJJQUJMRV9OQU1FKTsK IH0KICNlbmRpZgogCg==