100667 2012-10-29 06:25:14 -0700 [EFL][WK2] Avoid useless assignment in EwkViewImpl::setCustomTextEncodingName() 2012-10-29 10:00:33 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cdumez cdumez gyuyoung.kim kenneth laszlo.gombos lucas.de.marchi rakuco tonikitoo webkit.review.bot oldest_to_newest 753349 0 cdumez 2012-10-29 06:25:14 -0700 We currently do the following in EwkViewImpl::setCustomTextEncodingName(encoding): m_customEncoding = encoding; m_pageProxy->setCustomTextEncodingName(encoding); Then, we assign again m_customEncoding in EwkViewImpl::customTextEncodingName(): String customEncoding = m_pageProxy->customTextEncodingName(); if (customEncoding.isEmpty()) return 0; m_customEncoding = customEncoding.utf8().data(); return m_customEncoding; The assignment in EwkViewImpl::setCustomTextEncodingName() is useless since the value is anyway set when calling EwkViewImpl::setCustomTextEncodingName(encoding). We can therefore get rid of it. 753355 1 171226 cdumez 2012-10-29 06:28:51 -0700 Created attachment 171226 Patch 753363 2 171226 mikhail.pozdnyakov 2012-10-29 06:37:56 -0700 Comment on attachment 171226 Patch LGTM 753380 3 171226 gyuyoung.kim 2012-10-29 06:56:49 -0700 Comment on attachment 171226 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171226&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:817 > + impl->setCustomTextEncodingName(encoding ? encoding : String()); I think you need to use String::fromUTF8(encoding). 753387 4 cdumez 2012-10-29 07:02:12 -0700 (In reply to comment #3) > (From update of attachment 171226 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171226&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:817 > > + impl->setCustomTextEncodingName(encoding ? encoding : String()); > > I think you need to use String::fromUTF8(encoding). Can we really have non-ASCII characters in the encoding name? I don't think so. 753434 5 171226 webkit.review.bot 2012-10-29 07:37:40 -0700 Comment on attachment 171226 Patch Clearing flags on attachment: 171226 Committed r132800: <http://trac.webkit.org/changeset/132800> 753435 6 webkit.review.bot 2012-10-29 07:37:44 -0700 All reviewed patches have been landed. Closing bug. 753485 7 171226 gyuyoung.kim 2012-10-29 08:54:41 -0700 Comment on attachment 171226 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171226&action=review >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:817 >>> + impl->setCustomTextEncodingName(encoding ? encoding : String()); >> >> I think you need to use String::fromUTF8(encoding). > > Can we really have non-ASCII characters in the encoding name? I don't think so. I think this is not related to set non-ASCII character as encoding name. This is to check if const char* is valid utf8 or not, and to convert from utf8() to utf16(). If we use encoding name without converting, encoding name might to be broken when it is converted to String implicitly. I think setCustomTextEncodingName(() will check if encoding character can be adjusted or not. GTK port also uses String::fromUTF8() 753549 8 cdumez 2012-10-29 10:00:33 -0700 (In reply to comment #7) > (From update of attachment 171226 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171226&action=review > > >>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:817 > >>> + impl->setCustomTextEncodingName(encoding ? encoding : String()); > >> > >> I think you need to use String::fromUTF8(encoding). > > > > Can we really have non-ASCII characters in the encoding name? I don't think so. > > I think this is not related to set non-ASCII character as encoding name. This is to check if const char* is valid utf8 or not, and to convert from utf8() to utf16(). If we use encoding name without converting, encoding name might to be broken when it is converted to String implicitly. I think setCustomTextEncodingName(() will check if encoding character can be adjusted or not. GTK port also uses String::fromUTF8() If we don't call String::fromUTF8() then the input string is treated as ASCII. This is safe (and likely faster) as long as the input string only contains ASCII characters. Since I don't think it is valid to have non-ASCII characters in an encoding name, I think my code is correct. 171226 2012-10-29 06:28:51 -0700 2012-10-29 08:54:41 -0700 Patch 100667_custom_encoding.patch text/plain 3230 cdumez ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBiNTY1MWFlLi45YWRmNGMyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjQg QEAKKzIwMTItMTAtMjkgIENocmlzdG9waGUgRHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVs LmNvbT4KKworICAgICAgICBbRUZMXVtXSzJdIEF2b2lkIHVzZWxlc3MgYXNzaWdubWVudCBpbiBF d2tWaWV3SW1wbDo6c2V0Q3VzdG9tVGV4dEVuY29kaW5nTmFtZSgpCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDA2NjcKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZW1vdmUgdXNlbGVzcyBtX2N1c3RvbUVu Y29kaW5nIGFzc2lnbm1lbnQgaW4gRXdrVmlld0ltcGw6OnNldEN1c3RvbVRleHRFbmNvZGluZ05h bWUoKQorICAgICAgICBzaW5jZSB0aGUgbWVtYmVyIGlzIGFueXdheSBhc3NpZ25lZCBpbiBFd2tW aWV3SW1wbDo6Y3VzdG9tVGV4dEVuY29kaW5nTmFtZSgpLgorCisgICAgICAgIEhhdmUgRXdrVmll d0ltcGw6OnNldEN1c3RvbVRleHRFbmNvZGluZ05hbWUoKSB0YWtlIGEgU3RyaW5nIGluIGFyZ3Vt ZW50IGluc3RlYWQKKyAgICAgICAgb2YgYSBjb25zdCBjaGFyKiB0byBtYWtlIHRoZSBBUEkgbW9y ZSBDKysgYW5kIHNpbmNlIHdlIGRvbid0IG5lZWQgYSBjb25zdCBjaGFyKgorICAgICAgICB0byBh c3NpZ24gdG8gbV9jdXN0b21FbmNvZGluZyBhbnltb3JlLgorCisgICAgICAgICogVUlQcm9jZXNz L0FQSS9lZmwvRXdrVmlld0ltcGwuY3BwOgorICAgICAgICAoRXdrVmlld0ltcGw6OnNldEN1c3Rv bVRleHRFbmNvZGluZ05hbWUpOgorICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvZWZsL0V3a1ZpZXdJ bXBsLmg6CisgICAgICAgIChFd2tWaWV3SW1wbCk6CisgICAgICAgICogVUlQcm9jZXNzL0FQSS9l ZmwvZXdrX3ZpZXcuY3BwOgorICAgICAgICAoZXdrX3ZpZXdfc2V0dGluZ19lbmNvZGluZ19jdXN0 b21fc2V0KToKKwogMjAxMi0xMC0yOSAgSm9jZWx5biBUdXJjb3R0ZSAgPGpvY2VseW4udHVyY290 dGVAZGlnaWEuY29tPgogCiAgICAgICAgIFtXSzJdIEVuYWJsZSBkZWxlZ2F0ZWQgc2Nyb2xsaW5n IGFzIHNvb24gYXMgdGhlIEZyYW1lVmlldyBpcyBjcmVhdGVkIHdoZW4gdXNpbmcgZml4ZWQgbGF5 b3V0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9Fd2tWaWV3 SW1wbC5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9Fd2tWaWV3SW1wbC5j cHAKaW5kZXggZjU0ZDc2Zi4uZjVlZWEwYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQ cm9jZXNzL0FQSS9lZmwvRXdrVmlld0ltcGwuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJv Y2Vzcy9BUEkvZWZsL0V3a1ZpZXdJbXBsLmNwcApAQCAtNjE2LDEwICs2MTYsOSBAQCBjb25zdCBj aGFyKiBFd2tWaWV3SW1wbDo6Y3VzdG9tVGV4dEVuY29kaW5nTmFtZSgpIGNvbnN0CiAgICAgcmV0 dXJuIG1fY3VzdG9tRW5jb2Rpbmc7CiB9CiAKLXZvaWQgRXdrVmlld0ltcGw6OnNldEN1c3RvbVRl eHRFbmNvZGluZ05hbWUoY29uc3QgY2hhciogZW5jb2RpbmcpCit2b2lkIEV3a1ZpZXdJbXBsOjpz ZXRDdXN0b21UZXh0RW5jb2RpbmdOYW1lKGNvbnN0IFN0cmluZyYgZW5jb2RpbmcpCiB7Ci0gICAg bV9jdXN0b21FbmNvZGluZyA9IGVuY29kaW5nOwotICAgIG1fcGFnZVByb3h5LT5zZXRDdXN0b21U ZXh0RW5jb2RpbmdOYW1lKGVuY29kaW5nID8gZW5jb2RpbmcgOiBTdHJpbmcoKSk7CisgICAgbV9w YWdlUHJveHktPnNldEN1c3RvbVRleHRFbmNvZGluZ05hbWUoZW5jb2RpbmcpOwogfQogCiB2b2lk IEV3a1ZpZXdJbXBsOjpzZXRNb3VzZUV2ZW50c0VuYWJsZWQoYm9vbCBlbmFibGVkKQpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvRXdrVmlld0ltcGwuaCBiL1Nv dXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL0V3a1ZpZXdJbXBsLmgKaW5kZXggZjRlOTE3 Ny4uYTRiZWMzMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwv RXdrVmlld0ltcGwuaAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9Fd2tW aWV3SW1wbC5oCkBAIC0xMjUsNyArMTI1LDcgQEAgcHVibGljOgogICAgIGNvbnN0IGNoYXIqIHRo ZW1lUGF0aCgpIGNvbnN0OwogICAgIHZvaWQgc2V0VGhlbWVQYXRoKGNvbnN0IGNoYXIqIHRoZW1l KTsKICAgICBjb25zdCBjaGFyKiBjdXN0b21UZXh0RW5jb2RpbmdOYW1lKCkgY29uc3Q7Ci0gICAg dm9pZCBzZXRDdXN0b21UZXh0RW5jb2RpbmdOYW1lKGNvbnN0IGNoYXIqIGVuY29kaW5nKTsKKyAg ICB2b2lkIHNldEN1c3RvbVRleHRFbmNvZGluZ05hbWUoY29uc3QgU3RyaW5nJiBlbmNvZGluZyk7 CiAKICAgICBib29sIG1vdXNlRXZlbnRzRW5hYmxlZCgpIGNvbnN0IHsgcmV0dXJuIG1fbW91c2VF dmVudHNFbmFibGVkOyB9CiAgICAgdm9pZCBzZXRNb3VzZUV2ZW50c0VuYWJsZWQoYm9vbCBlbmFi bGVkKTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192 aWV3LmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3LmNwcApp bmRleCBjMDEyYTA2Li4xOGJkODYyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nl c3MvQVBJL2VmbC9ld2tfdmlldy5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQ SS9lZmwvZXdrX3ZpZXcuY3BwCkBAIC04MTQsNyArODE0LDcgQEAgRWluYV9Cb29sIGV3a192aWV3 X3NldHRpbmdfZW5jb2RpbmdfY3VzdG9tX3NldChFdmFzX09iamVjdCogZXdrVmlldywgY29uc3Qg Y2hhcioKICAgICBFV0tfVklFV19TRF9HRVRfT1JfUkVUVVJOKGV3a1ZpZXcsIHNtYXJ0RGF0YSwg ZmFsc2UpOwogICAgIEVXS19WSUVXX0lNUExfR0VUX09SX1JFVFVSTihzbWFydERhdGEsIGltcGws IGZhbHNlKTsKIAotICAgIGltcGwtPnNldEN1c3RvbVRleHRFbmNvZGluZ05hbWUoZW5jb2Rpbmcp OworICAgIGltcGwtPnNldEN1c3RvbVRleHRFbmNvZGluZ05hbWUoZW5jb2RpbmcgPyBlbmNvZGlu ZyA6IFN0cmluZygpKTsKIAogICAgIHJldHVybiB0cnVlOwogfQo=