Bug 90577 – [WK2][EFL] Ewk_View needs to report new resource requests

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

The Ewk_View needs to emit a signal when resource requests are being initiated in order to notify the client.

[Chris Dumez]

Created attachment 150839 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 150839 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_url_request.cpp:49
> +    const char* url;
> +    const char* first_party;
> +    const char* http_method;

Can you give me an example of what they are supposed to contain? document? Did you look at the new Qt apis?

> Source/WebKit2/UIProcess/API/efl/ewk_url_request.cpp:57
> +#define EWK_URL_REQUEST_WK_GET_OR_RETURN(request, wkRequest_, ...)    \
> +    if (!(request)) {                                           \
> +        EINA_LOG_CRIT("request is NULL.");                      \
> +        return __VA_ARGS__;                                    \
> +    }                                                          \
> +    if (!(request)->wkRequest) {                                 \

Somethings wrong with the \s here?

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:34
> +    CString uri;

why uri here and url elsewhere?

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:50
> +    delete resource;

free?

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:59
> +Ewk_Web_Resource* ewk_web_resource_new(const char* uri, bool isMainResource)

uri?

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource_private.h:32
> +
> +Ewk_Web_Resource* ewk_web_resource_new(const char* uri, bool isMainResource);
> +

why dont you name private method differently? They are not easy to see apart.

maybe add a _p at the end or so? or use _q_ like Qt :-) _e_ :-)

[Chris Dumez]

Comment on attachment 150839 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_url_request.cpp:49
>> +    const char* http_method;
> 
> Can you give me an example of what they are supposed to contain? document? Did you look at the new Qt apis?

What they are supposed to contain? you mean the 3 string members? They are merely used for eina stringsharing of what is returned by ResourceRequest accessors.

>> Source/WebKit2/UIProcess/API/efl/ewk_url_request.cpp:57
>> +    if (!(request)->wkRequest) {                                 \
> 
> Somethings wrong with the \s here?

Oops. I'll fix this.

>> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:34
>> +    CString uri;
> 
> why uri here and url elsewhere?

Right, I'll fix this.

>> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:50
>> +    delete resource;
> 
> free?

I'm allocating with new.

>> Source/WebKit2/UIProcess/API/efl/ewk_web_resource_private.h:32
>> +
> 
> why dont you name private method differently? They are not easy to see apart.
> 
> maybe add a _p at the end or so? or use _q_ like Qt :-) _e_ :-)

Well, this is more a port-level decision than relating to this particular patch. I personally don't have any preference and I followed the style that was used before, for both WK1 and WK2 EFL. What usually helps distinguishing them though is the "@internal" annotation which I mistakenly omitted. I'll fix this. If we really want to change the private methods naming, I think we should send an email to the webkit-efl mailing list first and the refactoring should take place in a separate patch.

[Chris Dumez]

Created attachment 150875 [details]
Patch

Take Kenneth's feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 150875 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_url_request.h:83
> +EAPI const char *ewk_request_first_party_get(const Ewk_Url_Request *request);

first_party_domain? Shouldnt documentation describe these things better? like what it a first-party?

> Source/WebKit2/UIProcess/API/efl/ewk_url_request.h:86
> + * Query HTTP method for this request.

so this is http, https? spdy?

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.cpp:51
> +
> +    if (--resource->__ref)
> +        return;
> +    free(resource);

generally I would always prefer a newline after a return, or block

> Source/WebKit2/UIProcess/API/efl/ewk_web_resource.h:75
> +EAPI Eina_Bool ewk_web_resource_main_get(const Ewk_Web_Resource *resource);

Isnt this a bit too Yoda? resource main get? :-) why not just main_resource_get ?

[Chris Dumez]

Created attachment 150880 [details]
Patch

Take Kenneth's feedback into consideration.

[WebKit Review Bot]

Comment on attachment 150880 [details]
Patch

Clearing flags on attachment: 150880

Committed r121889: <http://trac.webkit.org/changeset/121889>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.