Bug 93964 – [EFL][WK2] Refactoring: start using WKEinaSharedString in ewk_ classes

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

WKEinaSharedString in shall be used in ewk_ classes rather than straight C API.

[Mikhail Pozdnyakov]

Created attachment 158548 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 158548 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158548&action=review

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:115
> +    if (!str)
> +        return !m_string;
> +
> +    if (!m_string)
> +        return false;
> +
> +    if (length() != static_cast<size_t>(strlen(str)))
> +        return false;
> +
> +    return !strcmp(m_string, str);

Why is all of this faster than just calling strcmp. Isnt it doing similar internally more optimized?

[Kenneth Rohde Christiansen]

Comment on attachment 158548 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158548&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:770
> +    const char* callBackArgument = static_cast<const char*>(priv->uri);

I think we just call it callback and not CallBack. But please check

> Source/WebKit2/UIProcess/API/efl/ewk_web_error.cpp:102
>  const char* ewk_web_error_url_get(const Ewk_Web_Error* error)
>  {
> -    EWK_WEB_ERROR_WK_GET_OR_RETURN(error, wkError, 0);
> -
> -    WKRetainPtr<WKURLRef> wkUrl(AdoptWK, WKErrorCopyFailingURL(wkError));
> -    Ewk_Web_Error* ewkError = const_cast<Ewk_Web_Error*>(error);
> -    eina_stringshare_replace(&ewkError->url, toImpl(wkUrl.get())->string().utf8().data());
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(error, 0);
>  
>      return error->url;
>  }

There are API tests testing these things?

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:40
>      _Ewk_Web_Resource(const char* _url, bool _isMainResource)
>          : __ref(1)
> -         , url(eina_stringshare_add(_url))
> +         , url(_url)

I think it is OK to use url both places and have url(url). I am pretty sure that works in C++

[Chris Dumez]

Comment on attachment 158548 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158548&action=review

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:106
> +    if (!str)

if (!str || !m_string)
    return str == m_string;

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:109
> +    if (!m_string)

Not needed anymore

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:112
> +    if (length() != static_cast<size_t>(strlen(str)))

I don't think this is needed. This does not seem faster since it needs to iterate.

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:115
>> +    return !strcmp(m_string, str);
> 
> Why is all of this faster than just calling strcmp. Isnt it doing similar internally more optimized?

I think we still need the NULL case check before strcmp(), right?

[Kenneth Rohde Christiansen]

> I think we still need the NULL case check before strcmp(), right?

It is supposed to work on null terminated strings, so I am not sure. At least it is worth checking the implementation.

[Mikhail Pozdnyakov]

(In reply to comment #5)
> > I think we still need the NULL case check before strcmp(), right?
> 
> It is supposed to work on null terminated strings, so I am not sure. At least it is worth checking the implementation.

http://sourceware.org/git/?p=glibc.git;a=blob;f=string/strcmp.c;

There is no checking for NULL

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 158548 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158548&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:770
> > +    const char* callBackArgument = static_cast<const char*>(priv->uri);
> 
> I think we just call it callback and not CallBack. But please check
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_web_error.cpp:102
> >  const char* ewk_web_error_url_get(const Ewk_Web_Error* error)
> >  {
> > -    EWK_WEB_ERROR_WK_GET_OR_RETURN(error, wkError, 0);
> > -
> > -    WKRetainPtr<WKURLRef> wkUrl(AdoptWK, WKErrorCopyFailingURL(wkError));
> > -    Ewk_Web_Error* ewkError = const_cast<Ewk_Web_Error*>(error);
> > -    eina_stringshare_replace(&ewkError->url, toImpl(wkUrl.get())->string().utf8().data());
> > +    EINA_SAFETY_ON_NULL_RETURN_VAL(error, 0);
> >  
> >      return error->url;
> >  }
> 
> There are API tests testing these things?
Not explicitly, but it is used in /Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp

[Mikhail Pozdnyakov]

Created attachment 158555 [details]
patch v2

Kenneth, Chris, thanks for review!

[WebKit Review Bot]

Comment on attachment 158555 [details]
patch v2

Clearing flags on attachment: 158555

Committed r125671: <http://trac.webkit.org/changeset/125671>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.