Bug 90577

Created attachment 150839 [details] Patch

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_ :-)

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.

Created attachment 150875 [details] Patch Take Kenneth's feedback into consideration.

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 ?

Created attachment 150880 [details] Patch Take Kenneth's feedback into consideration.

Comment on attachment 150880 [details] Patch Clearing flags on attachment: 150880 Committed r121889: <http://trac.webkit.org/changeset/121889>

All reviewed patches have been landed. Closing bug.