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.
Created attachment 171226 [details] Patch
Comment on attachment 171226 [details] Patch LGTM
Comment on attachment 171226 [details] 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).
(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.
Comment on attachment 171226 [details] Patch Clearing flags on attachment: 171226 Committed r132800: <http://trac.webkit.org/changeset/132800>
All reviewed patches have been landed. Closing bug.
Comment on attachment 171226 [details] 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()
(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.