Bug 234629 – [GTK] Implement form validation with gtk3 widgets in the UI process

• Component: WebKitGTK
• Status: RESOLVED FIXED

[ChangSeok Oh]

SSIA.

[ChangSeok Oh]

Created attachment 447863 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 447863 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:249
> +#include "WebKitWebViewBasePrivate.h"

Why do we need this here?

> Source/WebKit/UIProcess/WebValidationBubble.h:37
> +class WebValidationBubble : public RefCounted<WebValidationBubble>, public CanMakeWeakPtr<WebValidationBubble> {

Why is this refcounted and can make weak at the same time?

> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:48
> +    , m_webView(webView)

We don't need this member since we are using gtk_popover_get_relative_to

> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:50
> +    webkitWebViewBaseSetShouldNotifyFocusEvents(WEBKIT_WEB_VIEW_BASE(webView), false);

Ok, I guess this is the reason why we want this in the WebKit layer. I think we could simply this by calling this from the caller, right before creating the validation bubble. Then we add a callback to WebCore::ValidationBubble to be called when the popup is dismissed and we use it to set this to false again.

> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:56
> +    const char* format = "<span font='%f'>%s</span>";
> +    GUniquePtr<gchar> markup(g_markup_printf_escaped(format, m_fontSize, message.utf8().data()));

I don't think we need the local variable for format, just use the literal in the g_markup_printf_escaped

> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:75
> +    m_popover = gtk_popover_new(webView);
> +    gtk_popover_set_modal(GTK_POPOVER(m_popover), FALSE);
> +    gtk_popover_set_position(GTK_POPOVER(m_popover), GTK_POS_TOP);
> +    gtk_popover_set_constrain_to(GTK_POPOVER(m_popover), GTK_POPOVER_CONSTRAINT_NONE);

What's the plan for GTK4? Will you reuse this file just adding ifdefs? You should add ifdefs now in any case to not break the gtk4 build.

[ChangSeok Oh]

Comment on attachment 447863 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:249
>> +#include "WebKitWebViewBasePrivate.h"
> 
> Why do we need this here?

Removed.

>> Source/WebKit/UIProcess/WebValidationBubble.h:37
>> +class WebValidationBubble : public RefCounted<WebValidationBubble>, public CanMakeWeakPtr<WebValidationBubble> {
> 
> Why is this refcounted and can make weak at the same time?

This class is removed. We reuse WebCore::ValidationBubble as the mac port does.

>> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:48
>> +    , m_webView(webView)
> 
> We don't need this member since we are using gtk_popover_get_relative_to

We use WebCore::ValidationBubble now so we initialize m_view of ValidationBubble instead.

>> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:50
>> +    webkitWebViewBaseSetShouldNotifyFocusEvents(WEBKIT_WEB_VIEW_BASE(webView), false);
> 
> Ok, I guess this is the reason why we want this in the WebKit layer. I think we could simply this by calling this from the caller, right before creating the validation bubble. Then we add a callback to WebCore::ValidationBubble to be called when the popup is dismissed and we use it to set this to false again.

If we add a callback to WebCore::ValidationBubble, we don't need the call from the caller in my opinion. We can simply call the callback here instead. Please let me know if you don't like it.

>> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:56
>> +    GUniquePtr<gchar> markup(g_markup_printf_escaped(format, m_fontSize, message.utf8().data()));
> 
> I don't think we need the local variable for format, just use the literal in the g_markup_printf_escaped

Done.

>> Source/WebKit/UIProcess/gtk/WebValidationBubbleGtk.cpp:75
>> +    gtk_popover_set_constrain_to(GTK_POPOVER(m_popover), GTK_POPOVER_CONSTRAINT_NONE);
> 
> What's the plan for GTK4? Will you reuse this file just adding ifdefs? You should add ifdefs now in any case to not break the gtk4 build.

I hide this code with ifdefs for gtk4 temporarily and install pageConfiguration.validationMessageClient for gtk3 only. So, gtk4 uses the shadow dom validation bubble until we land a patch for gtk4.

[ChangSeok Oh]

Created attachment 448085 [details]
Patch

[ChangSeok Oh]

Created attachment 448086 [details]
Patch

[ChangSeok Oh]

Would anyone review this?

[Carlos Garcia Campos]

Comment on attachment 448086 [details]
Patch

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

> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:39
> +static const float horizontalMargin = 5;
> +static const float verticalMargin = 5;
> +static const float maxLabelWidthChars = 40;

I think it's ok to use the numbers directly for these, the same way you do for  minimum font size or label lines.

> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:94
> +    GtkWidget* webView = gtk_popover_get_relative_to(GTK_POPOVER(m_popover));

We keep the web view as m_webView. So either don't keep it and get it always with gtk_popover_get_relative_to(), or just use the member, no?

> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:98
> +    if (m_shouldNotifyFocusEventsCallback)

This should not be optional.

> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:106
> +    if (m_shouldNotifyFocusEventsCallback)

Ditto.

[ChangSeok Oh]

Comment on attachment 448086 [details]
Patch

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

Thanks for your review. I will come up with the gtk4 support soon.

>> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:39
>> +static const float maxLabelWidthChars = 40;
> 
> I think it's ok to use the numbers directly for these, the same way you do for  minimum font size or label lines.

O.K. "static const float minFontSize = 11" is appended here.

>> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:94
>> +    GtkWidget* webView = gtk_popover_get_relative_to(GTK_POPOVER(m_popover));
> 
> We keep the web view as m_webView. So either don't keep it and get it always with gtk_popover_get_relative_to(), or just use the member, no?

This line is removed. We use the member cached, instead.

>> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:98
>> +    if (m_shouldNotifyFocusEventsCallback)
> 
> This should not be optional.

Done.

>> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:106
>> +    if (m_shouldNotifyFocusEventsCallback)
> 
> Ditto.

Done.

[ChangSeok Oh]

Created attachment 449350 [details]
Patch

[ChangSeok Oh]

Created attachment 449352 [details]
Patch

[EWS]

Committed r288104 (246118@main): <https://commits.webkit.org/246118@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449352 [details].