Bug 54752 – [EFL] Soup authentication feature implementation

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kamil Blank]

This implementation is based on GTK+ implementation.

In case of authentication request, previously defined callback to application will be called. 
This is a major difference between EFL and GTK implementation where authentication dialog is shown inside webkit.

[Kamil Blank]

Created attachment 82967 [details]
patch

[Gyuyoung Kim]

Comment on attachment 82967 [details]
patch

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

I will contribute ewk_network.cpp / h file soon. Why don't you move these APIs to that ?

> Source/WebKit/efl/ewk/ewk_main.cpp:204
> +void ewk_show_auth_dialog_callback_set(ewk_show_auth_dialog_callback callback)

These API should not locate in ewk_main.cpp

[Kamil Blank]

When can I expect ewk_network in main repo?

[Gyuyoung Kim]

(In reply to comment #3)
> When can I expect ewk_network in main repo?

Hmm, I will make a bug for it soon. But, I,m not sure when it can be landed to mainline. I will try it.

[Gyuyoung Kim]

Hello Kamil,

Why don't you make ewk_authenticate files ?

[Gyuyoung Kim]

(In reply to comment #5)
> Hello Kamil,
> 
> Why don't you make ewk_authenticate files ?

Kamil, how do you think about my suggestion ?

[Kamil Blank]

(In reply to comment #5)
> Hello Kamil,
> 
> Why don't you make ewk_authenticate files ?

Hi Gyuyoung,

Could you please give me more details about your idea? What exactly do you want me to put inside this file(s)?
Do you want me to move headers from ewk_main.h into ewk_authenticate.h?

[Gyuyoung Kim]

(In reply to comment #7)
> (In reply to comment #5)
> > Hello Kamil,
> > 
> > Why don't you make ewk_authenticate files ?
> 
> Hi Gyuyoung,
> 
> Could you please give me more details about your idea? What exactly do you want me to put inside this file(s)?
> Do you want me to move headers from ewk_main.h into ewk_authenticate.h?

Sorry for my late reply. Yes, I think you should move your implementation to new ewk_xxx file (for example, ewk_auth.cpp) Please make new patch. :-)

[Antonio Gomes]

Comment on attachment 82967 [details]
patch

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

Gyuyoung's suggestion. r-'ing it to keep it going ...

> Source/WebKit/efl/ewk/ewk_main.h:59
> +
> +

unneeded extra line.

[Lucas De Marchi]

Comment on attachment 82967 [details]
patch

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

Overall I think most of this code should be shared somehow with gtk. There's no need to use g_slice_new0 / g_slice_free. If this indeed used, it should not be inside ewk/.

> Source/WebKit/efl/ewk/ewk_main.h:48
> +/**
> + * Set callback to be called when authentication is required.
> + */
> +EAPI void ewk_show_auth_dialog_callback_set(ewk_show_auth_dialog_callback);

When you move to the new file, remember to keep namespaces clean. E.g.: this function should be named "ewk_auth_..."

> Source/WebKit/efl/ewk/ewk_main.h:57
> +/**
> + * Calls authentication method for setting credentials.
> + *
> + * @param username username
> + * @param password user password
> + * @param data soup authentication data
> + */
> +EAPI void ewk_auth_credentials_set(char* username, char* password, void* data);

This one is right. Make the others like this.

> Source/WebKit/efl/ewk/ewk_soup_authentication.cpp:3
> +/*
> +    Copyright (C) 2009-2011 Samsung Electronics
> +

Since this is based on the GTK port, you probably need to keep their copyrights. And here it's only 2011 for yours.

> Source/WebKit/efl/ewk/ewk_soup_authentication.h:23
> +
> +#include "ewk_eapi.h"

Remember that this is gone now.

[Kamil Blank]

Created attachment 95871 [details]
patch

I've updated patch. Could you please take a look on it again?

I'd like to also ask for your opinion on naming of some objects/functions copied from GTK port.
Do you think keeping their original names started with webkit_, WebKit etc. is ok or would it be better to rename them with EWK/ewk_ prefixes?

[Leandro Pereira]

Comment on attachment 95871 [details]
patch

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?

[Kamil Blank]

Thank you for the review. 
I followed your suggestions and prepared new patch.

I also added code inside ewk_auth_soup.cpp in case of unset callback - to free memory and unpause soup message.

(In reply to comment #12)
> (From update of attachment 95871 [details])
> 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?

Let's say that we've already got some functionality implemented to get username/password which is of type Ewk_Show_Auth_Dialog_Callback:
  static void auth_dialog_show(const char* realm, const char* uri, void* data);

First, application should call ewk_auth_show_dialog_callback_set(auth_dialog_show) to let WebKit know which function should be called when authentication required.
This function is responsible for getting username/password from user and pass them through ewk_auth_credentials_set(). 

> > 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.

I moved this variable inside ewk_auth_soup.cpp

[Kamil Blank]

Created attachment 96085 [details]
patch

[Leandro Pereira]

Looks better. Two nitpicks, however:

1) I don't see a way around it, but it would be a lot better if we used some already existing function to set the callback function.
2) How difficult would it be to also support this feature when using the Curl network backend?

[Kamil Blank]

(In reply to comment #15)
> Looks better. Two nitpicks, however:
> 
> 1) I don't see a way around it, but it would be a lot better if we used some already existing function to set the callback function.

Ok. I'll replace it to use evas_object_callback functions.

> 2) How difficult would it be to also support this feature when using the Curl network backend?

I'm not familiar with CURL but public API I added is not dependent on soup and I don't think there should be any problems. 
The idea was to make API (ewk_auth) and network backend (ewk_auth_soup) in separate files so if there is a need to implement curl support you just have to create and implement ewk_auth_curl files with expected functionality.

[Kamil Blank]

> 1) I don't see a way around it, but it would be a lot better if we used some already existing function to set the callback function.

I've got problem with the modification we talked. I don't have any handle to ewk_view inside ewk_auth_soup and there is no possibility to emit any signal into it.

[Leandro Pereira]

Comment on attachment 96085 [details]
patch

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

Regarding the function to set the callback, that's exactly what I said about not having a way around it. This seems to be a special case, so I think it is fine leaving as is.

> Source/WebKit/efl/ewk/ewk_auth_soup.cpp:102
> +    auth_data = (Ewk_Auth_Data*)calloc(1, sizeof(Ewk_Auth_Data));
> +    auth_data->msg = msg;
> +    auth_data->auth = auth;
> +    auth_data->session = session;

Just make sure that, if calloc() fails, you return from this function before writing to it (and before pausing/incrementing references).

> Source/WebKit/efl/ewk/ewk_auth_soup.h:37
> +#define EWK_TYPE_SOUP_AUTH_DIALOG            (ewk_auth_soup_dialog_get_type ())
> +#define EWK_SOUP_AUTH_DIALOG(object)         (G_TYPE_CHECK_INSTANCE_CAST ((object), EWK_TYPE_SOUP_AUTH_DIALOG, Ewk_Soup_Auth_Dialog))
> +#define EWK_SOUP_AUTH_DIALOG_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), EWK_TYPE_SOUP_AUTH_DIALOG, Ewk_Soup_Auth_Dialog))
> +#define EWK_IS_SOUP_AUTH_DIALOG(object)      (G_TYPE_CHECK_INSTANCE_TYPE ((object), EWK_TYPE_SOUP_AUTH_DIALOG))
> +#define EWK_IS_SOUP_AUTH_DIALOG_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), EWK_TYPE_SOUP_AUTH_DIALOG))
> +#define EWK_SOUP_AUTH_DIALOG_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), EWK_TYPE_SOUP_AUTH_DIALOG, Ewk_Soup_Auth_Dialog))

Pay attention to spaces before parenthesis.

[Kamil Blank]

Created attachment 97088 [details]
patch

[Kamil Blank]

> > Source/WebKit/efl/ewk/ewk_auth_soup.cpp:102
> > +    auth_data = (Ewk_Auth_Data*)calloc(1, sizeof(Ewk_Auth_Data));
> > +    auth_data->msg = msg;
> > +    auth_data->auth = auth;
> > +    auth_data->session = session;
> 
> Just make sure that, if calloc() fails, you return from this function before writing to it (and before pausing/incrementing references).

Fixed. 
I also moved checking whether callback is set before all other actions.

> > Source/WebKit/efl/ewk/ewk_auth_soup.h:37
> > +#define EWK_TYPE_SOUP_AUTH_DIALOG            (ewk_auth_soup_dialog_get_type ())
> > +#define EWK_SOUP_AUTH_DIALOG(object)         (G_TYPE_CHECK_INSTANCE_CAST ((object), EWK_TYPE_SOUP_AUTH_DIALOG, Ewk_Soup_Auth_Dialog))
> > +#define EWK_SOUP_AUTH_DIALOG_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), EWK_TYPE_SOUP_AUTH_DIALOG, Ewk_Soup_Auth_Dialog))
> > +#define EWK_IS_SOUP_AUTH_DIALOG(object)      (G_TYPE_CHECK_INSTANCE_TYPE ((object), EWK_TYPE_SOUP_AUTH_DIALOG))
> > +#define EWK_IS_SOUP_AUTH_DIALOG_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), EWK_TYPE_SOUP_AUTH_DIALOG))
> > +#define EWK_SOUP_AUTH_DIALOG_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), EWK_TYPE_SOUP_AUTH_DIALOG, Ewk_Soup_Auth_Dialog))
> 
> Pay attention to spaces before parenthesis.

Fixed.

[Leandro Pereira]

Comment on attachment 97088 [details]
patch

Informal r+.

[Kenneth Rohde Christiansen]

Comment on attachment 97088 [details]
patch

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

> Source/WebKit/efl/ChangeLog:7
> +        [EFL] Soup authentication feature implementation.
> +        https://bugs.webkit.org/show_bug.cgi?id=54752
> +

A bit more description on what you fixed, how you did it etc would be welcome

[Kamil Blank]

Created attachment 97247 [details]
patch

Added descriptions.

[Lucas De Marchi]

Committed r88929: <http://trac.webkit.org/changeset/88929>

[Eric Seidel (no email)]

Comment on attachment 97247 [details]
patch

Cleared review? from attachment 97247 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).