[GTK4][Wayland] Add support for rendering web view contents

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

.

[Carlos Garcia Campos]

Created attachment 397561 [details]
Patch

[Carlos Garcia Campos]

Created attachment 397563 [details]
Patch

[Adrian Perez]

Comment on attachment 397563 [details]
Patch

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

Looks good overall, I have left a few comments but nothing that should prevent landing.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:606
> +    gtk_widget_set_vexpand(viewWidget, TRUE);

Wouldn't we also want a call to gtk_widget_set_hexpand(viewWidget, TRUE) here?
In general we want the web view to take as much space as possible inside its
continer, in both directions, unless I am missing something.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:754
> +    *minimumSize = 0;

Is it really useful to have zero as the minimum size? I know that using zero as
minimum is the current behaviour for GTK3 as well, but it strikes me as odd because
that can lead to situations like having a simple window without window controls
and only a web view inside having a 0×0 size if gtk_window_set_default_size() was
not called on the window—and that seems… bad?

This is more of a doubt I have than a request to change the minimum size, so feel
free to ignore this comment :]

> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:222
> +#endif

We could have a WebCore::gtkCreateGLContext() helper function in GtkUtilities.{h,cpp}
to avoid the #ifdefs here.

> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:362
>      glDeleteFramebuffers(1, &fb);

Unrelated to this patch, but maybe worth mentioning: There is no need to create
and delete the FBO every time this function is called, it could be created the
first time it's needed and reused aftwerwards (binding-unbinding is still neded,
of course).

[Claudio Saavedra]

(In reply to Adrian Perez from comment #3)
> Comment on attachment 397563 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397563&action=review
> 
> Looks good overall, I have left a few comments but nothing that should
> prevent landing.
> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:606
> > +    gtk_widget_set_vexpand(viewWidget, TRUE);
> 
> Wouldn't we also want a call to gtk_widget_set_hexpand(viewWidget, TRUE)
> here?
> In general we want the web view to take as much space as possible inside its
> continer, in both directions, unless I am missing something.

It's been a long time since I wrote any GTK+ widget, so take this with a grain of salt. But the idea is that for widgets there's a natural direction of expansion. In the case of the webview, that would be vertical (as that's how usually the content flow for webpages goes). Horizontal expansion is not necessarily wanted, that depends on what horizontal siblings the widget has and how the application wants to distribute the space between them.

This can be of course overriden by the application developers, just like with any widget.

> 
> > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:754
> > +    *minimumSize = 0;
> 
> Is it really useful to have zero as the minimum size? I know that using zero
> as
> minimum is the current behaviour for GTK3 as well, but it strikes me as odd
> because
> that can lead to situations like having a simple window without window
> controls
> and only a web view inside having a 0×0 size if
> gtk_window_set_default_size() was
> not called on the window—and that seems… bad?

Minimum size is the hard minimum. If you have anything other than 0x0 then apps that need a smaller size will not be able to do that. If an app is not setting a reasonable size for its windows, that's their bug.


Also with a grain of salt, as I said it's been a while since I wrote GTK+ widget layout code of any kind.

[Carlos Garcia Campos]

Comment on attachment 397563 [details]
Patch

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

Thanks!

>>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:606
>>> +    gtk_widget_set_vexpand(viewWidget, TRUE);
>> 
>> Wouldn't we also want a call to gtk_widget_set_hexpand(viewWidget, TRUE) here?
>> In general we want the web view to take as much space as possible inside its
>> continer, in both directions, unless I am missing something.
> 
> It's been a long time since I wrote any GTK+ widget, so take this with a grain of salt. But the idea is that for widgets there's a natural direction of expansion. In the case of the webview, that would be vertical (as that's how usually the content flow for webpages goes). Horizontal expansion is not necessarily wanted, that depends on what horizontal siblings the widget has and how the application wants to distribute the space between them.
> 
> This can be of course overriden by the application developers, just like with any widget.

I added this here because I was getting always a 0 height in size allocate. But it seems to be because there's no gtk_box_pack APIs in GTK4, that received the expand property. Now we use gtk_container_add() so we need to call set_vexpand to get the same as pack. So, I'll move this to the MiniBrowser instead.

>> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:222
>> +#endif
> 
> We could have a WebCore::gtkCreateGLContext() helper function in GtkUtilities.{h,cpp}
> to avoid the #ifdefs here.

It's just one line and only used here. We could move it to a helper, but here, not in WebCore.

>> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:362
>>      glDeleteFramebuffers(1, &fb);
> 
> Unrelated to this patch, but maybe worth mentioning: There is no need to create
> and delete the FBO every time this function is called, it could be created the
> first time it's needed and reused aftwerwards (binding-unbinding is still neded,
> of course).

This is fallback code that isn't used in most of the cases. In any case, it's existing code, so any optimization can be done in a separate bug.

[Carlos Garcia Campos]

Committed r260816: <https://trac.webkit.org/changeset/260816>