Bug 108794 – [EFL][WK2] Stop using WebString in ewk_cookie_manager, ewk_form_submission_request and ewk_text_checker

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

[Chris Dumez]

ewk_cookie_manager, ewk_form_submission_request and ewk_text_checker still rely on internal C++ API, in particular WebString.

[Chris Dumez]

Created attachment 186312 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186312 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_cookie_manager.cpp:197
> +        String hostname = toWTFString(wkHostname);

Shouldnt we use WKStringRef instead?

[Mikhail Pozdnyakov]

Comment on attachment 186312 [details]
Patch

LGTM.

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 186312 [details])
> LGTM.

However agree with Kenneth, that using WKStringRef is better.

[Chris Dumez]

Created attachment 186328 [details]
Patch

Take Kenneth's feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 186328 [details]
Patch

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

LGTM

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:51
> -String EwkFormSubmissionRequest::fieldValue(const String& fieldName) const
> +WKEinaSharedString EwkFormSubmissionRequest::fieldValue(const char* fieldName) const
>  {

it is really valuable having these methods when they are called only from one place? and they are very simple

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:79
>          WKStringRef wkKey = static_cast<WKStringRef>(WKArrayGetItemAtIndex(wkKeys.get(), i));

is this really the key or the value?

[Chris Dumez]

Comment on attachment 186328 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:51
>>  {
> 
> it is really valuable having these methods when they are called only from one place? and they are very simple

This is the pattern we have used so far for encapsulation since m_wkValues is private in EwkFormSubmissionRequest currently.
While this pattern could be changed, I think it is not really related to this patch.

>> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:79
>>          WKStringRef wkKey = static_cast<WKStringRef>(WKArrayGetItemAtIndex(wkKeys.get(), i));
> 
> is this really the key or the value?

Those are really the *keys* in m_wkValues dictionary although we could rename to wkFieldName if you would prefer.

[Kenneth Rohde Christiansen]

> This is the pattern we have used so far for encapsulation since m_wkValues is private in EwkFormSubmissionRequest currently.
> While this pattern could be changed, I think it is not really related to this patch.

Sure.

> Those are really the *keys* in m_wkValues dictionary although we could rename to wkFieldName if you would prefer.

I think it would. Apart from this it looks good

[Chris Dumez]

Created attachment 186370 [details]
Patch

Take Kenneth's feedback into consideration.

[Mikhail Pozdnyakov]

Comment on attachment 186370 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request_private.h:48
> +    WKEinaSharedString fieldValue(const char* fieldName) const;

would it be better if this function returns 'const char*' (avoid WKEinaSharedString copy constructor invoke)?

[Chris Dumez]

Created attachment 186383 [details]
Patch

Add new WKEinaSharedString::leakString() and use it, as discussed via IRC with Mikhail and Kenneth.

[Mikhail Pozdnyakov]

Comment on attachment 186383 [details]
Patch

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

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:49
> +    const char* leakString();

think it should return Eina_Stringshare*

[Chris Dumez]

Comment on attachment 186383 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.h:49
>> +    const char* leakString();
> 
> think it should return Eina_Stringshare*

Yes, it is clearer. I'll update. BTW, we use const char* when we mean Eina_Stringshare* in a lot of places. We should probably fix that.

[Chris Dumez]

Created attachment 186391 [details]
Patch

Take Mikhail's feedback into consideration.

[Chris Dumez]

Could a WK2 owner please take a look?

[Alexey Proskuryakov]

Comment on attachment 186391 [details]
Patch

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

This is EFL code, so mostly carrying over others' reviews.

> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:119
> +    m_string = 0;

I don't know if I would expect a method named leakString() to do this.

> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:53
> -    WKRetainPtr<WKStringRef> wkFieldName = adoptWK(toCopiedAPI(fieldName));
> +    WKRetainPtr<WKStringRef> wkFieldName(AdoptWK, WKStringCreateWithUTF8CString(fieldName));

I think that the adoptWK() form is preferred these days, although I'm not sure why.

[Chris Dumez]

Comment on attachment 186391 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/cpp/efl/WKEinaSharedString.cpp:119
>> +    m_string = 0;
> 
> I don't know if I would expect a method named leakString() to do this.

I used OwnPtr::leakPtr() as reference.

>> Source/WebKit2/UIProcess/API/efl/ewk_form_submission_request.cpp:53
>> +    WKRetainPtr<WKStringRef> wkFieldName(AdoptWK, WKStringCreateWithUTF8CString(fieldName));
> 
> I think that the adoptWK() form is preferred these days, although I'm not sure why.

I did not know that. I'll fix before landing.

[Chris Dumez]

Created attachment 188149 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 188149 [details]
Patch for landing

Clearing flags on attachment: 188149

Committed r142794: <http://trac.webkit.org/changeset/142794>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.