WKEinaSharedString in shall be used in ewk_ classes rather than straight C API.
Created attachment 158548 [details] patch
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?
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++
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?
> 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.
(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
(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
Created attachment 158555 [details] patch v2 Kenneth, Chris, thanks for review!
Comment on attachment 158555 [details] patch v2 Clearing flags on attachment: 158555 Committed r125671: <http://trac.webkit.org/changeset/125671>
All reviewed patches have been landed. Closing bug.