[webkit-reviews] review granted: [Bug 28173] [GTK] Remove keyring optional features : [Attachment 34643] removekeyringv3.patch

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has granted 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 34643: removekeyringv3.patch
https://bugs.webkit.org/attachment.cgi?id=34643&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> +static gboolean session_can_save_passwords(SoupSession* session)
> +{
> +    return soup_session_get_feature(session, SOUP_TYPE_PASSWORD_MANAGER) !=
NULL;
> +}
> +

Is the function necessary? It's only being used once.

>  static void show_auth_dialog(WebKitAuthData* authData, const char* login,
const char* password)
>  {
>      GtkWidget* toplevel;
> @@ -197,10 +179,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
>  
> -#if USE(GNOMEKEYRING)
> -    rememberBox = gtk_vbox_new (FALSE, 6);
> -    gtk_box_pack_start (GTK_BOX (vbox), rememberBox,
> -			   FALSE, FALSE, 0);
> +    rememberBox = gtk_vbox_new(FALSE, 6);
> +    gtk_box_pack_start(GTK_BOX(vbox), rememberBox,
> +			  FALSE, FALSE, 0);

Uhmm can't we pack the rememberBox along with the checkButton only if we can
save passwords?

>  
> -    checkButton = gtk_check_button_new_with_mnemonic(_("_Remember
password"));
> -    if (login && password)
> -	   gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(checkButton), TRUE);
> -   
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
> +    if (session_can_save_passwords(authData->session)) {
> +	   checkButton = gtk_check_button_new_with_mnemonic(_("_Remember
password"));
> +	   if (login && password)
> +	       gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(checkButton),
TRUE);
> +	  
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;
> +    }

Patch is fine otherwise. r=me but please don't commit this yet. We need to get
(1) libsoup updated, and (2) bot support for latest libsoup. 

Thanks!