[Webkit-unassigned] [Bug 246852] [GTK] Crash on authentication dialog with GTK 4

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=246852

Michael Catanzaro <mcatanzaro at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #463425|review?                     |review-
              Flags|                            |

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20221115/15cb5b6d/attachment-0001.htm>