[webkit-reviews] review denied: [Bug 246852] [GTK] Crash on authentication dialog with GTK 4 : [Attachment 463425] fix-gtk4-auth-dialog-crashes.-take-one.patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has denied Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 246852: [GTK] Crash on authentication dialog with GTK 4
https://bugs.webkit.org/show_bug.cgi?id=246852

Attachment 463425: fix-gtk4-auth-dialog-crashes.-take-one.patch

https://bugs.webkit.org/attachment.cgi?id=463425&action=review




--- Comment #10 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 463425
  --> https://bugs.webkit.org/attachment.cgi?id=463425
fix-gtk4-auth-dialog-crashes.-take-one.patch

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

OK, you're on the right track here. Thanks for your patch. Besides fixing the
style bot and rewriting the commit message, there are some more things to
change....

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:69
> -    bool rememberPassword =
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(priv->rememberCheckButton));
> +    bool rememberPassword =
> +#if USE(GTK4)
> +	  
gtk_check_button_get_active(GTK_CHECK_BUTTON(priv->rememberCheckButton))
> +#else
> +	  
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(priv->rememberCheckButton))
> +#endif
> +	   ;

Good change, but style nit; let's avoid breaking statements or control with
preprocessor guards; instead, just duplicate a small amount of code.

#if USE(GTK4)
    bool rememberPassword =
gtk_check_button_get_active(GTK_CHECK_BUTTON(priv->rememberCheckButton));
#else
    bool rememberPassword =
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(priv->rememberCheckButton));
#endif

It's easier to read this way, and you're only duplicating the "bool
rememberPassword =" and the semicolon, which is hardly a big deal.

> Source/WebKit/UIProcess/API/gtk/WebKitAuthenticationDialog.cpp:194
> +    // ;madhu 221105 Gtk4 creates a checkbutton with a `label'
> +    // property. Don't try to set linewrap on a non-existent label in
> +    // that case.
> +    GtkWidget *tmplabel =
gtk_check_button_get_child(GTK_CHECK_BUTTON(priv->rememberCheckButton));
> +    if (tmplabel)
> +	   gtk_label_set_line_wrap(GTK_LABEL(tmplabel), TRUE);

So the problem here is gtk_check_button_get_child() returns nullptr? Presumably
it always returns nullptr, so the `if (tmplable)` is designed to catch a case
that will never be hit, correct? Looks like this code here just needs to be
removed or replaced with some other way of doing things.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1863
> +// ;madhu 221105 focus is not a valid signal for a Dialog Window in gtk4.
use move-focus instead

Please remove all your personal comments, as they don't belong in the final
version of the code.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewDialog.cpp:128
> +//;madhu 221105
> +    gtk_widget_set_can_focus(GTK_WIDGET(object), TRUE);

Style nit: add an extra blank line here.

BTW you know this is a GTK 3 block, right? Why are you changing this for GTK 3?