Bug 108794

Summary: [EFL][WK2] Stop using WebString in ewk_cookie_manager, ewk_form_submission_request and ewk_text_checker
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, benjamin, gyuyoung.kim, kenneth, kling, laszlo.gombos, lucas.de.marchi, mikhail.pozdnyakov, rakuco, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107657    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ap: review+
Patch for landing none

Description Chris Dumez 2013-02-03 23:09:40 PST
ewk_cookie_manager, ewk_form_submission_request and ewk_text_checker still rely on internal C++ API, in particular WebString.
Comment 1 Chris Dumez 2013-02-03 23:14:07 PST
Created attachment 186312 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-04 01:20:11 PST
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?
Comment 3 Mikhail Pozdnyakov 2013-02-04 01:26:02 PST
Comment on attachment 186312 [details]
Patch

LGTM.
Comment 4 Mikhail Pozdnyakov 2013-02-04 01:41:07 PST
(In reply to comment #3)
> (From update of attachment 186312 [details])
> LGTM.

However agree with Kenneth, that using WKStringRef is better.
Comment 5 Chris Dumez 2013-02-04 02:23:39 PST
Created attachment 186328 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 6 Kenneth Rohde Christiansen 2013-02-04 02:43:10 PST
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?
Comment 7 Chris Dumez 2013-02-04 03:43:56 PST
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.
Comment 8 Kenneth Rohde Christiansen 2013-02-04 06:46:23 PST
> 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
Comment 9 Chris Dumez 2013-02-04 07:12:31 PST
Created attachment 186370 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 10 Mikhail Pozdnyakov 2013-02-04 07:40:42 PST
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)?
Comment 11 Chris Dumez 2013-02-04 08:26:43 PST
Created attachment 186383 [details]
Patch

Add new WKEinaSharedString::leakString() and use it, as discussed via IRC with Mikhail and Kenneth.
Comment 12 Mikhail Pozdnyakov 2013-02-04 08:31:34 PST
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*
Comment 13 Chris Dumez 2013-02-04 08:41:37 PST
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.
Comment 14 Chris Dumez 2013-02-04 08:43:12 PST
Created attachment 186391 [details]
Patch

Take Mikhail's feedback into consideration.
Comment 15 Chris Dumez 2013-02-12 14:22:27 PST
Could a WK2 owner please take a look?
Comment 16 Alexey Proskuryakov 2013-02-13 12:27:04 PST
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.
Comment 17 Chris Dumez 2013-02-13 12:31:26 PST
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.
Comment 18 Chris Dumez 2013-02-13 12:38:51 PST
Created attachment 188149 [details]
Patch for landing
Comment 19 WebKit Review Bot 2013-02-13 13:57:41 PST
Comment on attachment 188149 [details]
Patch for landing

Clearing flags on attachment: 188149

Committed r142794: <http://trac.webkit.org/changeset/142794>
Comment 20 WebKit Review Bot 2013-02-13 13:57:48 PST
All reviewed patches have been landed.  Closing bug.