Bug 93964

Summary: [EFL][WK2] Refactoring: start using WKEinaSharedString in ewk_ classes
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch v2 none

Description Mikhail Pozdnyakov 2012-08-14 07:02:45 PDT
WKEinaSharedString in shall be used in ewk_ classes rather than straight C API.
Comment 1 Mikhail Pozdnyakov 2012-08-15 05:22:36 PDT
Created attachment 158548 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2012-08-15 05:30:41 PDT
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 3 Kenneth Rohde Christiansen 2012-08-15 05:38:02 PDT
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 4 Chris Dumez 2012-08-15 05:48:46 PDT
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?
Comment 5 Kenneth Rohde Christiansen 2012-08-15 05:52:30 PDT
> 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.
Comment 6 Mikhail Pozdnyakov 2012-08-15 05:59:49 PDT
(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
Comment 7 Mikhail Pozdnyakov 2012-08-15 06:03:43 PDT
(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
Comment 8 Mikhail Pozdnyakov 2012-08-15 06:06:33 PDT
Created attachment 158555 [details]
patch v2

Kenneth, Chris, thanks for review!
Comment 9 WebKit Review Bot 2012-08-15 06:50:30 PDT
Comment on attachment 158555 [details]
patch v2

Clearing flags on attachment: 158555

Committed r125671: <http://trac.webkit.org/changeset/125671>
Comment 10 WebKit Review Bot 2012-08-15 06:50:35 PDT
All reviewed patches have been landed.  Closing bug.