Bug 100667 – [EFL][WK2] Avoid useless assignment in EwkViewImpl::setCustomTextEncodingName()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 171226 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 171226 [details]
Patch

LGTM

[Gyuyoung Kim]

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).

[Chris Dumez]

(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.

[WebKit Review Bot]

Comment on attachment 171226 [details]
Patch

Clearing flags on attachment: 171226

Committed r132800: <http://trac.webkit.org/changeset/132800>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

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()

[Chris Dumez]

(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.