100580 2012-10-26 16:47:14 -0700 buildHTTPHeaders() should use a StringBuilder instead of a Vector<UChar> 2012-10-27 14:00:52 -0700 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff webkit.review.bot oldest_to_newest 752582 0 msaboff 2012-10-26 16:47:14 -0700 In Plugins/PluginView.cpp, the function static String buildHTTPHeaders() uses a Vector<UChar> to construct HTTP headers. In should be changed to use a StringBuilder. 752620 1 171063 msaboff 2012-10-26 17:44:57 -0700 Created attachment 171063 Patch 752659 2 171063 eric.carlson 2012-10-26 19:30:54 -0700 Comment on attachment 171063 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171063&action=review > Source/WebKit2/ChangeLog:4 > + WKStringCopyCFString() should directly use 8 bit Strings data instead of up converting > + https://bugs.webkit.org/show_bug.cgi?id=100579 This is the wrong bug title and number. > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:167 > String separator(": "); Is there a benefit to creating this String rather than just appending the characters directly - stringBuilder.append(": ", 2) ? 752706 3 171081 msaboff 2012-10-27 00:02:33 -0700 Created attachment 171081 Fixed bug title and URL in ChangeLog - Rest patch unchanged (In reply to comment #2) > (From update of attachment 171063 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171063&action=review > > > Source/WebKit2/ChangeLog:4 > > + WKStringCopyCFString() should directly use 8 bit Strings data instead of up converting > > + https://bugs.webkit.org/show_bug.cgi?id=100579 > > This is the wrong bug title and number. Indeed! Fixed this. > > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:167 > > String separator(": "); > > Is there a benefit to creating this String rather than just appending the characters directly - stringBuilder.append(": ", 2) ? I'm inclined to keep separator as a named string variable. The append(": ", 2) construct doesn't say what it is for and could be prone to errors if either the string or length was changed without changing the other. This is a weak argument and I could be convinced the other way. 752798 4 171081 webkit.review.bot 2012-10-27 11:17:46 -0700 Comment on attachment 171081 Fixed bug title and URL in ChangeLog - Rest patch unchanged Clearing flags on attachment: 171081 Committed r132734: <http://trac.webkit.org/changeset/132734> 752799 5 webkit.review.bot 2012-10-27 11:17:49 -0700 All reviewed patches have been landed. Closing bug. 752835 6 171063 darin 2012-10-27 14:00:52 -0700 Comment on attachment 171063 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171063&action=review >>> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:167 >>> String separator(": "); >> >> Is there a benefit to creating this String rather than just appending the characters directly - stringBuilder.append(": ", 2) ? > > I'm inclined to keep separator as a named string variable. The append(": ", 2) construct doesn't say what it is for and could be prone to errors if either the string or length was changed without changing the other. This is a weak argument and I could be convinced the other way. stringBuilder.appendLiteral(": ") is what we should use 171063 2012-10-26 17:44:57 -0700 2012-10-27 14:00:52 -0700 Patch 100580.patch text/plain 2800 msaboff SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMjcxMykKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTEwLTI2ICBNaWNoYWVs IFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIFdLU3RyaW5nQ29weUNGU3Ry aW5nKCkgc2hvdWxkIGRpcmVjdGx5IHVzZSA4IGJpdCBTdHJpbmdzIGRhdGEgaW5zdGVhZCBvZiB1 cCBjb252ZXJ0aW5nCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMDA1NzkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBSZXBsYWNlZCBWZWN0b3I8VUNoYXI+IHdpdGggU3RyaW5nQnVpbGRlciBpbiB0aGUgSFRU UCBoZWFkZXIgY29uc3RydWN0aW9uIGZ1bmN0aW9uIGJ1aWxkSFRUUEhlYWRlcnMuCisgICAgICAg IFRoaXMgZWxpbWluYXRlcyA4IC0+IDE2IGJpdCB1cCBjb252ZXJzaW9uIG9mIHRoZSBzdHJpbmdz IGludm9sdmVkLgorCisgICAgICAgICogV2ViUHJvY2Vzcy9QbHVnaW5zL1BsdWdpblZpZXcuY3Bw OgorICAgICAgICAoV2ViS2l0OjpidWlsZEhUVFBIZWFkZXJzKToKKwogMjAxMi0xMC0yNiAgQW5k ZXJzIENhcmxzc29uICA8YW5kZXJzY2FAYXBwbGUuY29tPgogCiAgICAgICAgIENyYXNoIHdoZW4g bWFraW5nIE5QUnVudGltZSBjYWxscyB3aXRoIGEgbnVsbCBOUFAgcG9pbnRlcgpJbmRleDogU291 cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5zL1BsdWdpblZpZXcuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvUGx1Z2lucy9QbHVnaW5WaWV3LmNwcAkocmV2 aXNpb24gMTMyNTEwKQorKysgU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9QbHVnaW5zL1BsdWdp blZpZXcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02MSw2ICs2MSw3IEBACiAjaW5jbHVkZSA8V2Vi Q29yZS9TZWN1cml0eVBvbGljeS5oPgogI2luY2x1ZGUgPFdlYkNvcmUvU2V0dGluZ3MuaD4KICNp bmNsdWRlIDxXZWJDb3JlL1VzZXJHZXN0dXJlSW5kaWNhdG9yLmg+CisjaW5jbHVkZSA8d3RmL3Rl eHQvU3RyaW5nQnVpbGRlci5oPgogCiB1c2luZyBuYW1lc3BhY2UgSlNDOwogdXNpbmcgbmFtZXNw YWNlIFdlYkNvcmU7CkBAIC0xNjIsMjMgKzE2MywyMyBAQCBzdGF0aWMgU3RyaW5nIGJ1aWxkSFRU UEhlYWRlcnMoY29uc3QgUmVzCiAgICAgaWYgKCFyZXNwb25zZS5pc0hUVFAoKSkKICAgICAgICAg cmV0dXJuIFN0cmluZygpOwogCi0gICAgVmVjdG9yPFVDaGFyPiBzdHJpbmdCdWlsZGVyOworICAg IFN0cmluZ0J1aWxkZXIgc3RyaW5nQnVpbGRlcjsKICAgICBTdHJpbmcgc2VwYXJhdG9yKCI6ICIp OwogICAgIAogICAgIFN0cmluZyBzdGF0dXNMaW5lID0gU3RyaW5nOjpmb3JtYXQoIkhUVFAgJWQg IiwgcmVzcG9uc2UuaHR0cFN0YXR1c0NvZGUoKSk7Ci0gICAgc3RyaW5nQnVpbGRlci5hcHBlbmQo c3RhdHVzTGluZS5jaGFyYWN0ZXJzKCksIHN0YXR1c0xpbmUubGVuZ3RoKCkpOwotICAgIHN0cmlu Z0J1aWxkZXIuYXBwZW5kKHJlc3BvbnNlLmh0dHBTdGF0dXNUZXh0KCkuY2hhcmFjdGVycygpLCBy ZXNwb25zZS5odHRwU3RhdHVzVGV4dCgpLmxlbmd0aCgpKTsKKyAgICBzdHJpbmdCdWlsZGVyLmFw cGVuZChzdGF0dXNMaW5lKTsKKyAgICBzdHJpbmdCdWlsZGVyLmFwcGVuZChyZXNwb25zZS5odHRw U3RhdHVzVGV4dCgpKTsKICAgICBzdHJpbmdCdWlsZGVyLmFwcGVuZCgnXG4nKTsKICAgICAKICAg ICBIVFRQSGVhZGVyTWFwOjpjb25zdF9pdGVyYXRvciBlbmQgPSByZXNwb25zZS5odHRwSGVhZGVy RmllbGRzKCkuZW5kKCk7CiAgICAgZm9yIChIVFRQSGVhZGVyTWFwOjpjb25zdF9pdGVyYXRvciBp dCA9IHJlc3BvbnNlLmh0dHBIZWFkZXJGaWVsZHMoKS5iZWdpbigpOyBpdCAhPSBlbmQ7ICsraXQp IHsKLSAgICAgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoaXQtPmtleS5jaGFyYWN0ZXJzKCksIGl0 LT5rZXkubGVuZ3RoKCkpOwotICAgICAgICBzdHJpbmdCdWlsZGVyLmFwcGVuZChzZXBhcmF0b3Iu Y2hhcmFjdGVycygpLCBzZXBhcmF0b3IubGVuZ3RoKCkpOwotICAgICAgICBzdHJpbmdCdWlsZGVy LmFwcGVuZChpdC0+dmFsdWUuY2hhcmFjdGVycygpLCBpdC0+dmFsdWUubGVuZ3RoKCkpOworICAg ICAgICBzdHJpbmdCdWlsZGVyLmFwcGVuZChpdC0+a2V5KTsKKyAgICAgICAgc3RyaW5nQnVpbGRl ci5hcHBlbmQoc2VwYXJhdG9yKTsKKyAgICAgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoaXQtPnZh bHVlKTsKICAgICAgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoJ1xuJyk7CiAgICAgfQogICAgIAot ICAgIFN0cmluZyBoZWFkZXJzID0gU3RyaW5nOjphZG9wdChzdHJpbmdCdWlsZGVyKTsKKyAgICBT dHJpbmcgaGVhZGVycyA9IHN0cmluZ0J1aWxkZXIudG9TdHJpbmcoKTsKICAgICAKICAgICAvLyBJ ZiB0aGUgY29udGVudCBpcyBlbmNvZGVkIChtb3N0IGxpa2VseSBjb21wcmVzc2VkKSwgdGhlbiBk b24ndCBzZW5kIGl0cyBsZW5ndGggdG8gdGhlIHBsdWdpbiwKICAgICAvLyB3aGljaCBpcyBvbmx5 IGludGVyZXN0ZWQgaW4gdGhlIGRlY29kZWQgbGVuZ3RoLCBub3QgeWV0IGtub3duIGF0IHRoZSBt b21lbnQuCg== 171081 2012-10-27 00:02:33 -0700 2012-10-27 11:17:45 -0700 Fixed bug title and URL in ChangeLog - Rest patch unchanged 100580-2.patch text/plain 2786 msaboff SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzMjcxMykKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTEwLTI2ICBNaWNoYWVs IFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIGJ1aWxkSFRUUEhlYWRlcnMo KSBzaG91bGQgdXNlIGEgU3RyaW5nQnVpbGRlciBpbnN0ZWFkIG9mIGEgVmVjdG9yPFVDaGFyPgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAwNTgwCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUmVwbGFjZWQg VmVjdG9yPFVDaGFyPiB3aXRoIFN0cmluZ0J1aWxkZXIgaW4gdGhlIEhUVFAgaGVhZGVyIGNvbnN0 cnVjdGlvbiBmdW5jdGlvbiBidWlsZEhUVFBIZWFkZXJzLgorICAgICAgICBUaGlzIGVsaW1pbmF0 ZXMgOCAtPiAxNiBiaXQgdXAgY29udmVyc2lvbiBvZiB0aGUgc3RyaW5ncyBpbnZvbHZlZC4KKwor ICAgICAgICAqIFdlYlByb2Nlc3MvUGx1Z2lucy9QbHVnaW5WaWV3LmNwcDoKKyAgICAgICAgKFdl YktpdDo6YnVpbGRIVFRQSGVhZGVycyk6CisKIDIwMTItMTAtMjYgIEFuZGVycyBDYXJsc3NvbiAg PGFuZGVyc2NhQGFwcGxlLmNvbT4KIAogICAgICAgICBDcmFzaCB3aGVuIG1ha2luZyBOUFJ1bnRp bWUgY2FsbHMgd2l0aCBhIG51bGwgTlBQIHBvaW50ZXIKSW5kZXg6IFNvdXJjZS9XZWJLaXQyL1dl YlByb2Nlc3MvUGx1Z2lucy9QbHVnaW5WaWV3LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9XZWJQcm9jZXNzL1BsdWdpbnMvUGx1Z2luVmlldy5jcHAJKHJldmlzaW9uIDEzMjUxMCkK KysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvUGx1Z2lucy9QbHVnaW5WaWV3LmNwcAkod29y a2luZyBjb3B5KQpAQCAtNjEsNiArNjEsNyBAQAogI2luY2x1ZGUgPFdlYkNvcmUvU2VjdXJpdHlQ b2xpY3kuaD4KICNpbmNsdWRlIDxXZWJDb3JlL1NldHRpbmdzLmg+CiAjaW5jbHVkZSA8V2ViQ29y ZS9Vc2VyR2VzdHVyZUluZGljYXRvci5oPgorI2luY2x1ZGUgPHd0Zi90ZXh0L1N0cmluZ0J1aWxk ZXIuaD4KIAogdXNpbmcgbmFtZXNwYWNlIEpTQzsKIHVzaW5nIG5hbWVzcGFjZSBXZWJDb3JlOwpA QCAtMTYyLDIzICsxNjMsMjMgQEAgc3RhdGljIFN0cmluZyBidWlsZEhUVFBIZWFkZXJzKGNvbnN0 IFJlcwogICAgIGlmICghcmVzcG9uc2UuaXNIVFRQKCkpCiAgICAgICAgIHJldHVybiBTdHJpbmco KTsKIAotICAgIFZlY3RvcjxVQ2hhcj4gc3RyaW5nQnVpbGRlcjsKKyAgICBTdHJpbmdCdWlsZGVy IHN0cmluZ0J1aWxkZXI7CiAgICAgU3RyaW5nIHNlcGFyYXRvcigiOiAiKTsKICAgICAKICAgICBT dHJpbmcgc3RhdHVzTGluZSA9IFN0cmluZzo6Zm9ybWF0KCJIVFRQICVkICIsIHJlc3BvbnNlLmh0 dHBTdGF0dXNDb2RlKCkpOwotICAgIHN0cmluZ0J1aWxkZXIuYXBwZW5kKHN0YXR1c0xpbmUuY2hh cmFjdGVycygpLCBzdGF0dXNMaW5lLmxlbmd0aCgpKTsKLSAgICBzdHJpbmdCdWlsZGVyLmFwcGVu ZChyZXNwb25zZS5odHRwU3RhdHVzVGV4dCgpLmNoYXJhY3RlcnMoKSwgcmVzcG9uc2UuaHR0cFN0 YXR1c1RleHQoKS5sZW5ndGgoKSk7CisgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoc3RhdHVzTGlu ZSk7CisgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQocmVzcG9uc2UuaHR0cFN0YXR1c1RleHQoKSk7 CiAgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoJ1xuJyk7CiAgICAgCiAgICAgSFRUUEhlYWRlck1h cDo6Y29uc3RfaXRlcmF0b3IgZW5kID0gcmVzcG9uc2UuaHR0cEhlYWRlckZpZWxkcygpLmVuZCgp OwogICAgIGZvciAoSFRUUEhlYWRlck1hcDo6Y29uc3RfaXRlcmF0b3IgaXQgPSByZXNwb25zZS5o dHRwSGVhZGVyRmllbGRzKCkuYmVnaW4oKTsgaXQgIT0gZW5kOyArK2l0KSB7Ci0gICAgICAgIHN0 cmluZ0J1aWxkZXIuYXBwZW5kKGl0LT5rZXkuY2hhcmFjdGVycygpLCBpdC0+a2V5Lmxlbmd0aCgp KTsKLSAgICAgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoc2VwYXJhdG9yLmNoYXJhY3RlcnMoKSwg c2VwYXJhdG9yLmxlbmd0aCgpKTsKLSAgICAgICAgc3RyaW5nQnVpbGRlci5hcHBlbmQoaXQtPnZh bHVlLmNoYXJhY3RlcnMoKSwgaXQtPnZhbHVlLmxlbmd0aCgpKTsKKyAgICAgICAgc3RyaW5nQnVp bGRlci5hcHBlbmQoaXQtPmtleSk7CisgICAgICAgIHN0cmluZ0J1aWxkZXIuYXBwZW5kKHNlcGFy YXRvcik7CisgICAgICAgIHN0cmluZ0J1aWxkZXIuYXBwZW5kKGl0LT52YWx1ZSk7CiAgICAgICAg IHN0cmluZ0J1aWxkZXIuYXBwZW5kKCdcbicpOwogICAgIH0KICAgICAKLSAgICBTdHJpbmcgaGVh ZGVycyA9IFN0cmluZzo6YWRvcHQoc3RyaW5nQnVpbGRlcik7CisgICAgU3RyaW5nIGhlYWRlcnMg PSBzdHJpbmdCdWlsZGVyLnRvU3RyaW5nKCk7CiAgICAgCiAgICAgLy8gSWYgdGhlIGNvbnRlbnQg aXMgZW5jb2RlZCAobW9zdCBsaWtlbHkgY29tcHJlc3NlZCksIHRoZW4gZG9uJ3Qgc2VuZCBpdHMg bGVuZ3RoIHRvIHRoZSBwbHVnaW4sCiAgICAgLy8gd2hpY2ggaXMgb25seSBpbnRlcmVzdGVkIGlu IHRoZSBkZWNvZGVkIGxlbmd0aCwgbm90IHlldCBrbm93biBhdCB0aGUgbW9tZW50Lgo=