Bug 99549

Summary: [EFL][WK2] Ewk_Url_Scheme_Request has to be refactored
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit EFLAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99321    
Attachments:
Description Flags
patch
none
patch v2 none

Description Mikhail Pozdnyakov 2012-10-16 23:52:45 PDT
Ewk_Url_Scheme_Request has to be refactored according to the pattern from bug#99321 + it should use WKEinaSharedString
Comment 1 Mikhail Pozdnyakov 2012-10-17 01:30:25 PDT
Created attachment 169119 [details]
patch
Comment 2 Chris Dumez 2012-10-17 01:44:08 PDT
Comment on attachment 169119 [details]
patch

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

> Source/WebKit2/ChangeLog:8
> +        Ewk_Url_Scheme_Requestis inherited from RefCounted, WKEinaSharedString are used,

"Ewk_Url_Scheme_Request is"

> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request_private.h:47
> +    static PassRefPtr<Ewk_Url_Scheme_Request> create(WKSoupRequestManagerRef manager, WKStringRef url, uint64_t requestID)

I think we should pass a WKURLRef here instead of a WKStringRef.

> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request_private.h:56
> +    Ewk_Url_Scheme_Request(WKSoupRequestManagerRef manager, WKStringRef urlString, uint64_t requestID)

Ditto.
Comment 3 Kenneth Rohde Christiansen 2012-10-17 02:04:27 PDT
Comment on attachment 169119 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context_request_manager_client.cpp:43
> +    RefPtr<Ewk_Url_Scheme_Request> schemeRequest = Ewk_Url_Scheme_Request::create(soupRequestManagerRef, adoptWK(WKURLCopyString(urlRef)).get(), requestID);

So you copy the string then you adopt it (turning it into a RetainPtr<WKStringRef>, which you then passes the WKStringRef of (a pointer; const struct OpaqueWKString*), then it gets out of scope, meaning that it will be freed after this function call ends. That doesn't seem right, unless you copy it inbetween
Comment 4 Mikhail Pozdnyakov 2012-10-17 02:33:33 PDT
(In reply to comment #3)
> (From update of attachment 169119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169119&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context_request_manager_client.cpp:43
> > +    RefPtr<Ewk_Url_Scheme_Request> schemeRequest = Ewk_Url_Scheme_Request::create(soupRequestManagerRef, adoptWK(WKURLCopyString(urlRef)).get(), requestID);
> 
> So you copy the string then you adopt it (turning it into a RetainPtr<WKStringRef>, which you then passes the WKStringRef of (a pointer; const struct OpaqueWKString*), then it gets out of scope, meaning that it will be freed after this function call ends. That doesn't seem right, unless you copy it inbetween

It'll be freed after Ewk_Url_Scheme_Request::create() exits because RefPtr will be kept on stack before. However, guess I should pass WKURLRef as Chris recommends
Comment 5 Mikhail Pozdnyakov 2012-10-17 02:39:48 PDT
Created attachment 169138 [details]
patch v2

Took comments from Chris and Kenneth into consideration.
Comment 6 WebKit Review Bot 2012-10-17 03:08:40 PDT
Comment on attachment 169138 [details]
patch v2

Clearing flags on attachment: 169138

Committed r131575: <http://trac.webkit.org/changeset/131575>
Comment 7 WebKit Review Bot 2012-10-17 03:08:45 PDT
All reviewed patches have been landed.  Closing bug.