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 EFL | Assignee: | 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
Chris Dumez
2013-02-03 23:09:40 PST
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. |