115848 2013-05-08 22:11:05 -0700 Make TextCodecICU not depend on TextEncoding 2013-05-11 16:18:20 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ap ap benjamin commit-queue darin oldest_to_newest 886596 0 ap 2013-05-08 22:11:05 -0700 TextEncoding is a WebCore concept that builds on TextCodec, which is a platform API wrapper, so using the former in TextCodecICU is confusing. It's used for two reasons: 1. Passing an encoding as TextEncoding guarantees that the encoding name has been already canonicalized. 2. We use TextEncoding::displayString() to tweak Asian encoders, so that they have desired behavior with backslash. I think that even though the string transformation is the same in both cases, using displayString for this is conceptually unsound. 886597 1 201135 ap 2013-05-08 22:14:16 -0700 Created attachment 201135 proposed patch I'm looking into this because I plan to add an ICU codec name pointer to TextCodecICU for <rdar://problem/13823864>. 886599 2 commit-queue 2013-05-08 22:15:36 -0700 Attachment 201135 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/text/TextCodecICU.cpp', u'Source/WebCore/platform/text/TextCodecICU.h']" exit_code: 1 Source/WebCore/platform/text/TextCodecICU.cpp:339: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style. 886703 3 201135 benjamin 2013-05-09 01:46:11 -0700 Comment on attachment 201135 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=201135&action=review > Source/WebCore/platform/text/TextCodecICU.cpp:62 > + // TextEncoding name is always atomic, so the string is never deleted. I am a little confused by "atomic" here. What do you mean? 886887 4 201135 ap 2013-05-09 08:45:22 -0700 Comment on attachment 201135 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=201135&action=review >> Source/WebCore/platform/text/TextCodecICU.cpp:62 >> + // TextEncoding name is always atomic, so the string is never deleted. > > I am a little confused by "atomic" here. What do you mean? TextEncodingRegistry has a map for encodings, see atomicCanonicalTextEncodingName(). 887684 5 201135 darin 2013-05-10 17:22:57 -0700 Comment on attachment 201135 proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=201135&action=review > Source/WebCore/platform/text/TextCodecICU.cpp:32 > +#include "TextEncodingRegistry.h" Why is this include needed? >>> Source/WebCore/platform/text/TextCodecICU.cpp:62 >>> + // TextEncoding name is always atomic, so the string is never deleted. >> >> I am a little confused by "atomic" here. What do you mean? > > TextEncodingRegistry has a map for encodings, see atomicCanonicalTextEncodingName(). Maybe word more like this? // TextEncoding name is always one from atomicCanonicalTextEncodingName, guaranteed to never be deleted. > Source/WebCore/platform/text/TextCodecICU.cpp:230 > - const char* name = m_encoding.name(); > - m_needsGBKFallbacks = name[0] == 'G' && name[1] == 'B' && name[2] == 'K' && !name[3]; > + m_needsGBKFallbacks = m_encodingName[0] == 'G' && m_encodingName[1] == 'B' && m_encodingName[2] == 'K' && !m_encodingName[3]; Seems to be a slight decrease in readability. The original author (me) thought that using a local made it more readable. It wasn’t performance optimization, but expression readability optimization. > Source/WebCore/platform/text/TextCodecICU.cpp:437 > + if (shouldShowBackslashAsCurrencySymbolIn(m_encodingName)) { Why not optimize further by searching for '\\' and not making a copy if there is no '\\' character? > Source/WebCore/platform/text/TextCodecICU.cpp:439 > + copy.append(characters, length); > + copy.replace('\\', 0xA5); This copies the characters twice. Instead, we could just use Vector<UChar> and modify the copy in place. > Source/WebCore/platform/text/TextCodecICU.h:45 > + TextCodecICU(const char* encoding); Maybe mark this explicit? > Source/WebCore/platform/text/TextCodecICU.h:59 > + const char* m_encodingName; Maybe even const char* const? 887745 6 ap 2013-05-10 22:45:20 -0700 >> +#include "TextEncodingRegistry.h" > > Why is this include needed? We use shouldShowBackslashAsCurrencySymbolIn() that's declared in this header. > Seems to be a slight decrease in readability. The original author (me) thought that using a local made it > more readable. It wasn’t performance optimization, but expression readability optimization. For reference, this change was <http://trac.webkit.org/changeset/21227/trunk/WebCore/platform/TextCodecICU.cpp>. Before the change, the code would implicitly enumerate non-base codecs just to construct TextEncoding("GBK"). Interestingly, that's a historic example of why it's undesirable to use TextEncoding in TextCodecICU.cpp. I'm unsure why we can't just use strcmp, and in fact, I have a local patch that moves this code, and changes it to use strcmp. Made the other suggested changes, except for optimization ideas - this is just encode(), which is probably not worth any added risk. Committed <http://trac.webkit.org/changeset/149924>. 887854 7 darin 2013-05-11 16:18:20 -0700 (In reply to comment #6) > > Seems to be a slight decrease in readability. The original author (me) thought that using a local made it > > more readable. It wasn’t performance optimization, but expression readability optimization. > > For reference, this change was <http://trac.webkit.org/changeset/21227/trunk/WebCore/platform/TextCodecICU.cpp>. Before the change, the code would implicitly enumerate non-base codecs just to construct TextEncoding("GBK"). Interestingly, that's a historic example of why it's undesirable to use TextEncoding in TextCodecICU.cpp. > > I'm unsure why we can't just use strcmp I think that when I wrote this I thought that the four explicit comparisons would be faster than a strcmp. Depends how aggressive compilers are at optimizing strcmp, particularly when a string constant is involved. And also whether code size or speed is more important. For readability, I think strcmp and memcmp are some of the worst functions. Remembering that "0 means equal" is confusing enough that we made an exception to our "== 0" rule for it, an exception that most people probably don’t realize it exists. But a comparison with "GBK" is obviously better, if only there was a clearer way than strcmp ;-) 201135 2013-05-08 22:14:16 -0700 2013-05-10 17:22:57 -0700 proposed patch EncodingString.txt text/plain 5993 ap SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0OTc5MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEzLTA1LTA4ICBBbGV4ZXkg UHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgorCisgICAgICAgIE1ha2UgVGV4dENvZGVjSUNV IG5vdCBkZXBlbmQgb24gVGV4dEVuY29kaW5nCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xMTU4NDgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICAqIHBsYXRmb3JtL3RleHQvVGV4dENvZGVjSUNVLmNwcDoKKyAg ICAgICAgKFdlYkNvcmU6OlRleHRDb2RlY0lDVTo6Y3JlYXRlKToKKyAgICAgICAgKFdlYkNvcmU6 OlRleHRDb2RlY0lDVTo6VGV4dENvZGVjSUNVKToKKyAgICAgICAgKFdlYkNvcmU6OlRleHRDb2Rl Y0lDVTo6Y3JlYXRlSUNVQ29udmVydGVyKToKKyAgICAgICAgKFdlYkNvcmU6OlRleHRDb2RlY0lD VTo6ZGVjb2RlKToKKyAgICAgICAgKFdlYkNvcmU6OlRleHRDb2RlY0lDVTo6ZW5jb2RlKToKKyAg ICAgICAgKiBwbGF0Zm9ybS90ZXh0L1RleHRDb2RlY0lDVS5oOgorICAgICAgICBVc2UgYSBwbGFp biBlbmNvZGluZyBzdHJpbmcgaW4gcGxhdGZvcm0gZW5jb2RlciB3cmFwcGVyLCBub3QgYSBoaWdo ZXIgbGV2ZWwgY29uY2VwdC4KKwogMjAxMy0wNS0wOCAgRGF2aWQgS2lsemVyICA8ZGRraWx6ZXJA YXBwbGUuY29tPgogCiAgICAgICAgIEJVSUxEIEZJWDogRG9uJ3QgdXNlIGFkb3B0Q0YoKSB3aXRo IE5TRGljdGlvbmFyeSAqIG9iamVjdApJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4 dC9UZXh0Q29kZWNJQ1UuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L3RleHQvVGV4dENvZGVjSUNVLmNwcAkocmV2aXNpb24gMTQ5NTMxKQorKysgU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vdGV4dC9UZXh0Q29kZWNJQ1UuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yOCw2 ICsyOCw4IEBACiAjaWYgVVNFKElDVV9VTklDT0RFKQogI2luY2x1ZGUgIlRleHRDb2RlY0lDVS5o IgogCisjaW5jbHVkZSAiVGV4dEVuY29kaW5nLmgiCisjaW5jbHVkZSAiVGV4dEVuY29kaW5nUmVn aXN0cnkuaCIKICNpbmNsdWRlICJUaHJlYWRHbG9iYWxEYXRhLmgiCiAjaW5jbHVkZSA8dW5pY29k ZS91Y252Lmg+CiAjaW5jbHVkZSA8dW5pY29kZS91Y252X2NiLmg+CkBAIC01Nyw3ICs1OSw4IEBA IHN0YXRpYyBVQ29udmVydGVyKiYgY2FjaGVkQ29udmVydGVySUNVKCkKIAogUGFzc093blB0cjxU ZXh0Q29kZWM+IFRleHRDb2RlY0lDVTo6Y3JlYXRlKGNvbnN0IFRleHRFbmNvZGluZyYgZW5jb2Rp bmcsIGNvbnN0IHZvaWQqKQogewotICAgIHJldHVybiBhZG9wdFB0cihuZXcgVGV4dENvZGVjSUNV KGVuY29kaW5nKSk7CisgICAgLy8gVGV4dEVuY29kaW5nIG5hbWUgaXMgYWx3YXlzIGF0b21pYywg c28gdGhlIHN0cmluZyBpcyBuZXZlciBkZWxldGVkLgorICAgIHJldHVybiBhZG9wdFB0cihuZXcg VGV4dENvZGVjSUNVKGVuY29kaW5nLm5hbWUoKSkpOwogfQogCiB2b2lkIFRleHRDb2RlY0lDVTo6 cmVnaXN0ZXJFbmNvZGluZ05hbWVzKEVuY29kaW5nTmFtZVJlZ2lzdHJhciByZWdpc3RyYXIpCkBA IC0xOTcsOCArMjAwLDggQEAgdm9pZCBUZXh0Q29kZWNJQ1U6OnJlZ2lzdGVyQ29kZWNzKFRleHRD bwogICAgIH0KIH0KIAotVGV4dENvZGVjSUNVOjpUZXh0Q29kZWNJQ1UoY29uc3QgVGV4dEVuY29k aW5nJiBlbmNvZGluZykKLSAgICA6IG1fZW5jb2RpbmcoZW5jb2RpbmcpCitUZXh0Q29kZWNJQ1U6 OlRleHRDb2RlY0lDVShjb25zdCBjaGFyKiBlbmNvZGluZykKKyAgICA6IG1fZW5jb2RpbmdOYW1l KGVuY29kaW5nKQogICAgICwgbV9jb252ZXJ0ZXJJQ1UoMCkKICAgICAsIG1fbmVlZHNHQktGYWxs YmFja3MoZmFsc2UpCiB7CkBAIC0yMjQsOCArMjI3LDcgQEAgdm9pZCBUZXh0Q29kZWNJQ1U6OmNy ZWF0ZUlDVUNvbnZlcnRlcigpIAogewogICAgIEFTU0VSVCghbV9jb252ZXJ0ZXJJQ1UpOwogCi0g ICAgY29uc3QgY2hhciogbmFtZSA9IG1fZW5jb2RpbmcubmFtZSgpOwotICAgIG1fbmVlZHNHQktG YWxsYmFja3MgPSBuYW1lWzBdID09ICdHJyAmJiBuYW1lWzFdID09ICdCJyAmJiBuYW1lWzJdID09 ICdLJyAmJiAhbmFtZVszXTsKKyAgICBtX25lZWRzR0JLRmFsbGJhY2tzID0gbV9lbmNvZGluZ05h bWVbMF0gPT0gJ0cnICYmIG1fZW5jb2RpbmdOYW1lWzFdID09ICdCJyAmJiBtX2VuY29kaW5nTmFt ZVsyXSA9PSAnSycgJiYgIW1fZW5jb2RpbmdOYW1lWzNdOwogCiAgICAgVUVycm9yQ29kZSBlcnI7 CiAKQEAgLTIzMyw3ICsyMzUsNyBAQCB2b2lkIFRleHRDb2RlY0lDVTo6Y3JlYXRlSUNVQ29udmVy dGVyKCkgCiAgICAgaWYgKGNhY2hlZENvbnZlcnRlcikgewogICAgICAgICBlcnIgPSBVX1pFUk9f RVJST1I7CiAgICAgICAgIGNvbnN0IGNoYXIqIGNhY2hlZE5hbWUgPSB1Y252X2dldE5hbWUoY2Fj aGVkQ29udmVydGVyLCAmZXJyKTsKLSAgICAgICAgaWYgKFVfU1VDQ0VTUyhlcnIpICYmIG1fZW5j b2RpbmcgPT0gY2FjaGVkTmFtZSkgeworICAgICAgICBpZiAoVV9TVUNDRVNTKGVycikgJiYgbV9l bmNvZGluZ05hbWUgPT0gY2FjaGVkTmFtZSkgewogICAgICAgICAgICAgbV9jb252ZXJ0ZXJJQ1Ug PSBjYWNoZWRDb252ZXJ0ZXI7CiAgICAgICAgICAgICBjYWNoZWRDb252ZXJ0ZXIgPSAwOwogICAg ICAgICAgICAgcmV0dXJuOwpAQCAtMjQxLDEwICsyNDMsMTAgQEAgdm9pZCBUZXh0Q29kZWNJQ1U6 OmNyZWF0ZUlDVUNvbnZlcnRlcigpIAogICAgIH0KIAogICAgIGVyciA9IFVfWkVST19FUlJPUjsK LSAgICBtX2NvbnZlcnRlcklDVSA9IHVjbnZfb3BlbihtX2VuY29kaW5nLm5hbWUoKSwgJmVycik7 CisgICAgbV9jb252ZXJ0ZXJJQ1UgPSB1Y252X29wZW4obV9lbmNvZGluZ05hbWUsICZlcnIpOwog I2lmICFMT0dfRElTQUJMRUQKICAgICBpZiAoZXJyID09IFVfQU1CSUdVT1VTX0FMSUFTX1dBUk5J TkcpCi0gICAgICAgIExPR19FUlJPUigiSUNVIGFtYmlndW91cyBhbGlhcyB3YXJuaW5nIGZvciBl bmNvZGluZzogJXMiLCBtX2VuY29kaW5nLm5hbWUoKSk7CisgICAgICAgIExPR19FUlJPUigiSUNV IGFtYmlndW91cyBhbGlhcyB3YXJuaW5nIGZvciBlbmNvZGluZzogJXMiLCBtX2VuY29kaW5nTmFt ZSk7CiAjZW5kaWYKICAgICBpZiAobV9jb252ZXJ0ZXJJQ1UpCiAgICAgICAgIHVjbnZfc2V0RmFs bGJhY2sobV9jb252ZXJ0ZXJJQ1UsIFRSVUUpOwpAQCAtMzM0LDcgKzMzNiw3IEBAIFN0cmluZyBU ZXh0Q29kZWNJQ1U6OmRlY29kZShjb25zdCBjaGFyKiAKIAogICAgIC8vIDxodHRwOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzAxND4KICAgICAvLyBTaW1wbGlmaWVkIENoaW5l c2UgcGFnZXMgdXNlIHRoZSBjb2RlIEEzQTAgdG8gbWVhbiAiZnVsbC13aWR0aCBzcGFjZSIsIGJ1 dCBJQ1UgZGVjb2RlcyBpdCBhcyBVK0U1RTUuCi0gICAgaWYgKHN0cmNtcChtX2VuY29kaW5nLm5h bWUoKSwgIkdCSyIpID09IDAgfHwgc3RyY2FzZWNtcChtX2VuY29kaW5nLm5hbWUoKSwgImdiMTgw MzAiKSA9PSAwKQorICAgIGlmIChzdHJjbXAobV9lbmNvZGluZ05hbWUsICJHQksiKSA9PSAwIHx8 IHN0cmNhc2VjbXAobV9lbmNvZGluZ05hbWUsICJnYjE4MDMwIikgPT0gMCkKICAgICAgICAgcmVz dWx0U3RyaW5nLnJlcGxhY2UoMHhFNUU1LCBpZGVvZ3JhcGhpY1NwYWNlKTsKIAogICAgIHJldHVy biByZXN1bHRTdHJpbmc7CkBAIC00MjksMTEgKzQzMSwxOCBAQCBDU3RyaW5nIFRleHRDb2RlY0lD VTo6ZW5jb2RlKGNvbnN0IFVDaGFyCiAgICAgLy8gRklYTUU6IFdlIHNob3VsZCBzZWUgaWYgdGhl cmUgaXMgImZvcmNlIEFTQ0lJIHJhbmdlIiBtb2RlIGluIElDVTsKICAgICAvLyB1bnRpbCB0aGVu LCB3ZSBjaGFuZ2UgdGhlIGJhY2tzbGFzaCBpbnRvIGEgeWVuIHNpZ24uCiAgICAgLy8gRW5jb2Rp bmcgd2lsbCBjaGFuZ2UgdGhlIHllbiBzaWduIGJhY2sgaW50byBhIGJhY2tzbGFzaC4KLSAgICBT dHJpbmcgY29weShjaGFyYWN0ZXJzLCBsZW5ndGgpOwotICAgIGNvcHkgPSBtX2VuY29kaW5nLmRp c3BsYXlTdHJpbmcoY29weS5pbXBsKCkpOwotCi0gICAgY29uc3QgVUNoYXIqIHNvdXJjZSA9IGNv cHkuY2hhcmFjdGVycygpOwotICAgIGNvbnN0IFVDaGFyKiBzb3VyY2VMaW1pdCA9IHNvdXJjZSAr IGNvcHkubGVuZ3RoKCk7CisgICAgU3RyaW5nIGNvcHk7CisgICAgY29uc3QgVUNoYXIqIHNvdXJj ZTsKKyAgICBjb25zdCBVQ2hhciogc291cmNlTGltaXQ7CisgICAgaWYgKHNob3VsZFNob3dCYWNr c2xhc2hBc0N1cnJlbmN5U3ltYm9sSW4obV9lbmNvZGluZ05hbWUpKSB7CisgICAgICAgIGNvcHku YXBwZW5kKGNoYXJhY3RlcnMsIGxlbmd0aCk7CisgICAgICAgIGNvcHkucmVwbGFjZSgnXFwnLCAw eEE1KTsKKyAgICAgICAgc291cmNlID0gY29weS5jaGFyYWN0ZXJzKCk7CisgICAgICAgIHNvdXJj ZUxpbWl0ID0gc291cmNlICsgY29weS5sZW5ndGgoKTsKKyAgICB9IGVsc2UgeworICAgICAgICBz b3VyY2UgPSBjaGFyYWN0ZXJzOworICAgICAgICBzb3VyY2VMaW1pdCA9IHNvdXJjZSArIGxlbmd0 aDsKKyAgICB9CiAKICAgICBVRXJyb3JDb2RlIGVyciA9IFVfWkVST19FUlJPUjsKIApJbmRleDog U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9UZXh0Q29kZWNJQ1UuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L1RleHRDb2RlY0lDVS5oCShyZXZpc2lvbiAx NDk1MzEpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L1RleHRDb2RlY0lDVS5oCSh3 b3JraW5nIGNvcHkpCkBAIC0yOCw3ICsyOCw2IEBACiAjZGVmaW5lIFRleHRDb2RlY0lDVV9oCiAK ICNpbmNsdWRlICJUZXh0Q29kZWMuaCIKLSNpbmNsdWRlICJUZXh0RW5jb2RpbmcuaCIKICNpbmNs dWRlIDx1bmljb2RlL3V0eXBlcy5oPgogCiB0eXBlZGVmIHN0cnVjdCBVQ29udmVydGVyIFVDb252 ZXJ0ZXI7CkBAIC00Myw3ICs0Miw3IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICAgICAgdmly dHVhbCB+VGV4dENvZGVjSUNVKCk7CiAKICAgICBwcml2YXRlOgotICAgICAgICBUZXh0Q29kZWNJ Q1UoY29uc3QgVGV4dEVuY29kaW5nJik7CisgICAgICAgIFRleHRDb2RlY0lDVShjb25zdCBjaGFy KiBlbmNvZGluZyk7CiAgICAgICAgIHN0YXRpYyBQYXNzT3duUHRyPFRleHRDb2RlYz4gY3JlYXRl KGNvbnN0IFRleHRFbmNvZGluZyYsIGNvbnN0IHZvaWQqKTsKIAogICAgICAgICB2aXJ0dWFsIFN0 cmluZyBkZWNvZGUoY29uc3QgY2hhciosIHNpemVfdCBsZW5ndGgsIGJvb2wgZmx1c2gsIGJvb2wg c3RvcE9uRXJyb3IsIGJvb2wmIHNhd0Vycm9yKTsKQEAgLTU3LDcgKzU2LDcgQEAgbmFtZXNwYWNl IFdlYkNvcmUgewogICAgICAgICBpbnQgZGVjb2RlVG9CdWZmZXIoVUNoYXIqIGJ1ZmZlciwgVUNo YXIqIGJ1ZmZlckxpbWl0LCBjb25zdCBjaGFyKiYgc291cmNlLAogICAgICAgICAgICAgY29uc3Qg Y2hhciogc291cmNlTGltaXQsIGludDMyX3QqIG9mZnNldHMsIGJvb2wgZmx1c2gsIFVFcnJvckNv ZGUmIGVycik7CiAKLSAgICAgICAgVGV4dEVuY29kaW5nIG1fZW5jb2Rpbmc7CisgICAgICAgIGNv bnN0IGNoYXIqIG1fZW5jb2RpbmdOYW1lOwogICAgICAgICBtdXRhYmxlIFVDb252ZXJ0ZXIqIG1f Y29udmVydGVySUNVOwogICAgICAgICBtdXRhYmxlIGJvb2wgbV9uZWVkc0dCS0ZhbGxiYWNrczsK ICAgICB9Owo=