Bug 99549 - [EFL][WK2] Ewk_Url_Scheme_Request has to be refactored
Summary: [EFL][WK2] Ewk_Url_Scheme_Request has to be refactored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on:
Blocks: 99321
  Show dependency treegraph
 
Reported: 2012-10-16 23:52 PDT by Mikhail Pozdnyakov
Modified: 2012-10-17 03:08 PDT (History)
6 users (show)

See Also:


Attachments
patch (8.91 KB, patch)
2012-10-17 01:30 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (8.87 KB, patch)
2012-10-17 02:39 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.