Bug 108794

Created attachment 186312 [details] Patch

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 on attachment 186312 [details] Patch LGTM.

(In reply to comment #3) > (From update of attachment 186312 [details]) > LGTM. However agree with Kenneth, that using WKStringRef is better.

Created attachment 186328 [details] Patch Take Kenneth's feedback into consideration.

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 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.

> 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

Created attachment 186370 [details] Patch Take Kenneth's feedback into consideration.

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)?

Created attachment 186383 [details] Patch Add new WKEinaSharedString::leakString() and use it, as discussed via IRC with Mikhail and Kenneth.

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 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.

Created attachment 186391 [details] Patch Take Mikhail's feedback into consideration.

Could a WK2 owner please take a look?

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 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.

Created attachment 188149 [details] Patch for landing

Comment on attachment 188149 [details] Patch for landing Clearing flags on attachment: 188149 Committed r142794: <http://trac.webkit.org/changeset/142794>

All reviewed patches have been landed. Closing bug.