[webkit-reviews] review denied: [Bug 54752] [EFL] Soup authentication feature implementation : [Attachment 95871] patch

[bugzilla-daemon at webkit.org]

Leandro Pereira <leandro at profusion.mobi> has denied Kamil Blank
<k.blank at samsung.com>'s request for review:
Bug 54752: [EFL] Soup authentication feature implementation
https://bugs.webkit.org/show_bug.cgi?id=54752

Attachment 95871: patch
https://bugs.webkit.org/attachment.cgi?id=95871&action=review

------- Additional Comments from Leandro Pereira <leandro at profusion.mobi>
View in context: https://bugs.webkit.org/attachment.cgi?id=95871&action=review

I couldn't figure out how this would work. Could you please provide a simple
explanation of the expected flow?

> Source/WebKit/efl/CMakeListsEfl.txt:106
> +  LIST(APPEND WebKit_SOURCES
> +    efl/ewk/ewk_auth.cpp
> +    efl/ewk/ewk_soup_auth.cpp
> +  )

I'd prefer ewk_auth_soup.cpp, so it is consistent with EFL naming scheme.

> Source/WebKit/efl/ewk/ewk_auth.cpp:27
> +ewk_auth_show_dialog_callback g_auth_show_dialog_callback = 0;

Structures/typedefs in EFL are named
Using_A_Combination_Of_Camel_Case_And_Underscores. Also, this variable should
be static and there's no need to prepend the name with "g_" to denote it is a
global variable.

> Source/WebKit/efl/ewk/ewk_auth.cpp:42
> +void ewk_auth_credentials_set(char* username, char* password, void* data)
> +{
> +#if USE(SOUP)
> +    ewk_soup_auth_credentials_set(username, password, data);

ewk_auth_soup_credentials_set() would be better (makes it easier to grep by
related functions on the source code).

> Source/WebKit/efl/ewk/ewk_auth.h:35
> +typedef void (*ewk_auth_show_dialog_callback)(const char* msg, void* data);

See remark about typedef naming above.

> Source/WebKit/efl/ewk/ewk_soup_auth.cpp:45
> +typedef struct _WebKitAuthData {
> +    SoupMessage* msg;
> +    SoupAuth* auth;
> +    SoupSession* session;
> +} WebKitAuthData;

See remarks about structure naming above. Also, prefix structures with Ewk
rather than WebKit.

> Source/WebKit/efl/ewk/ewk_soup_auth.cpp:94
> +    uri = soup_message_get_uri(msg);

uri isn't used here.

> Source/WebKit/efl/ewk/ewk_soup_auth.cpp:105
> +    if (g_auth_show_dialog_callback)
> +	   g_auth_show_dialog_callback(realm, auth_data);

First parameter of this function is called "msg", yet you just pass realm. How
about passing instead 'url' and 'realm', and having consistent naming on the
function prototype?