RESOLVED FIXED 99549
[EFL][WK2] Ewk_Url_Scheme_Request has to be refactored 
https://bugs.webkit.org/show_bug.cgi?id=99549
Summary [EFL][WK2] Ewk_Url_Scheme_Request has to be refactored
Mikhail Pozdnyakov
Reported 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
Attachments
patch (8.91 KB, patch)
2012-10-17 01:30 PDT, Mikhail Pozdnyakov 		no flags
DetailsFormatted DiffDiff
patch v2 (8.87 KB, patch)
2012-10-17 02:39 PDT, Mikhail Pozdnyakov 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mikhail Pozdnyakov
Comment 1 2012-10-17 01:30:25 PDT
Created attachment 169119 [details] patch
Chris Dumez
Comment 2 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.
Kenneth Rohde Christiansen
Comment 3 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
Mikhail Pozdnyakov
Comment 4 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
Mikhail Pozdnyakov
Comment 5 2012-10-17 02:39:48 PDT
Created attachment 169138 [details] patch v2 Took comments from Chris and Kenneth into consideration.
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2012-10-17 03:08:45 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.