[webkit-reviews] review denied: [Bug 28173] [GTK] Remove keyring optional features : [Attachment 34547] removekeyringv2.patch

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Xan Lopez <xan.lopez at gmail.com>'s
request for review:
Bug 28173: [GTK] Remove keyring optional features
https://bugs.webkit.org/show_bug.cgi?id=28173

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

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> -#if USE(GNOMEKEYRING)
>      GtkWidget* checkButton;
> -#endif

Is there a reason why you removed the conditional but not the block inside it?
There are a few instances of this in your patch and would be nice to know why
they're not removed.
>      char *username;
>      char *password;
>  } WebKitAuthData;
> @@ -103,54 +98,38 @@ static void free_authData(WebKitAuthData* authData)
>      g_slice_free(WebKitAuthData, authData);
>  }
>  
>  static void save_password_callback(SoupMessage* msg, WebKitAuthData*
authData)
>  {
>      /* Check only for Success status codes (2xx) */
> -    if (msg->status_code >= 200 && msg->status_code < 300) {
> -	   SoupURI* uri = soup_message_get_uri(authData->msg);
> -	   gnome_keyring_set_network_password(NULL,
> -					      authData->username,
> -					     
soup_auth_get_realm(authData->auth),
> -					      uri->host,
> -					      NULL,
> -					      uri->scheme,
> -					     
soup_auth_get_scheme_name(authData->auth),
> -					      uri->port,
> -					      authData->password,
> -					     
(GnomeKeyringOperationGetIntCallback)set_password_callback,
> -					      NULL,
> -					      NULL);
> -    }
> +    if (msg->status_code >= 200 && msg->status_code < 300)
> +	   soup_auth_save_password(authData->auth, authData->username,
authData->password);
> +

Do we need to check the status code here still?


>  static void response_callback(GtkDialog* dialog, gint response_id,
WebKitAuthData* authData)
>  {
> +    gboolean freeAuthData = TRUE;
> +
>      switch(response_id) {
>      case GTK_RESPONSE_OK:
>	   authData->username =
g_strdup(gtk_entry_get_text(GTK_ENTRY(authData->loginEntry)));
>	   authData->password =
g_strdup(gtk_entry_get_text(GTK_ENTRY(authData->passwordEntry)));
> +
>	   soup_auth_authenticate(authData->auth, authData->username,
authData->password);
>  
> -#if USE(GNOMEKEYRING)
> -	   if
(gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(authData->checkButton)))
> +	   if
(gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(authData->checkButton))) {
>	       g_signal_connect(authData->msg, "got-headers",
G_CALLBACK(save_password_callback), authData);
> -#endif
> +	       freeAuthData = FALSE;
> +	   }
> +
>      default:
>	   break;
>      }

I don't think we need the switch here. I think we can just refactor this into a
conditional so we can ditch "freeAuthData".

> @@ -197,10 +176,8 @@ static void show_auth_dialog(WebKitAuthData* authData,
const char* login, const
>      GtkWidget* messageLabel;
>      char* message;
>      SoupURI* uri;
> -#if USE(GNOMEKEYRING)
>      GtkWidget* rememberBox;
>      GtkWidget* checkButton;
> -#endif

We seem to be introducing features that were not in non-gnomekeyring builds (I
think this is the source of my confusion). What are these for btw?

>  
>      /* From GTK+ gtkmountoperation.c, modified and simplified. LGPL 2
license */
>  
> @@ -279,7 +256,6 @@ static void show_auth_dialog(WebKitAuthData* authData,
const char* login, const
>  
>      gtk_entry_set_visibility(GTK_ENTRY(authData->passwordEntry), FALSE);
>  
> -#if USE(GNOMEKEYRING)
>      rememberBox = gtk_vbox_new (FALSE, 6);
>      gtk_box_pack_start (GTK_BOX (vbox), rememberBox,
>			   FALSE, FALSE, 0);
> @@ -290,36 +266,18 @@ static void show_auth_dialog(WebKitAuthData* authData,
const char* login, const
>     
gtk_label_set_line_wrap(GTK_LABEL(gtk_bin_get_child(GTK_BIN(checkButton))),
TRUE);
>      gtk_box_pack_start (GTK_BOX (rememberBox), checkButton, FALSE, FALSE,
0);
>      authData->checkButton = checkButton;
> -#endif

Ditto with introducing features in default non-gnomekeyring builds.

>  static void session_authenticate(SoupSession* session, SoupMessage* msg,
SoupAuth* auth, gboolean retrying, gpointer user_data)
>  {
>      SoupURI* uri;
>      WebKitAuthData* authData;
>      SoupSessionFeature* manager = (SoupSessionFeature*)user_data;
> +    GSList *users;

The asterisk should be in GSList's side.

> +    login = password = NULL;
> +
> +    users = soup_auth_get_saved_users(auth);

This returns one entry only as we don't seem to have a loop below?

> +    if (users) {
> +	   login = users->data;
> +	   password = soup_auth_get_saved_password(auth, login);
> +	   soup_auth_free_saved_users(auth, users);
> +    }

No big issue with the patch. Just some concerns about introducing new features
in default non-gnomekeyring builds, etc.. I'm going to say r- for now as patch
needs revision.