WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
93964
[EFL][WK2] Refactoring: start using WKEinaSharedString in ewk_ classes
https://bugs.webkit.org/show_bug.cgi?id=93964
Summary
[EFL][WK2] Refactoring: start using WKEinaSharedString in ewk_ classes
Mikhail Pozdnyakov
Reported
2012-08-14 07:02:45 PDT
WKEinaSharedString in shall be used in ewk_ classes rather than straight C API.
Attachments
patch
(28.99 KB, patch)
2012-08-15 05:22 PDT
,
Mikhail Pozdnyakov
no flags
Details
Formatted Diff
Diff
patch v2
(28.97 KB, patch)
2012-08-15 06:06 PDT
,
Mikhail Pozdnyakov
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1
2012-08-15 05:22:36 PDT
Created
attachment 158548
[details]
patch
Kenneth Rohde Christiansen
Comment 2
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?
Kenneth Rohde Christiansen
Comment 3
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++
Chris Dumez
Comment 4
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?
Kenneth Rohde Christiansen
Comment 5
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.
Mikhail Pozdnyakov
Comment 6
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
Mikhail Pozdnyakov
Comment 7
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
Mikhail Pozdnyakov
Comment 8
2012-08-15 06:06:33 PDT
Created
attachment 158555
[details]
patch v2 Kenneth, Chris, thanks for review!
WebKit Review Bot
Comment 9
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
>
WebKit Review Bot
Comment 10
2012-08-15 06:50:35 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug