Bug 226320 - [GTK] Try harder to find initial WebKitWebView size
Summary: [GTK] Try harder to find initial WebKitWebView size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 219202
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-27 02:48 PDT by Alexander Mikhaylenko
Modified: 2021-06-01 04:59 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2021-05-27 03:01 PDT, Alexander Mikhaylenko
mcatanzaro: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Patch (6.90 KB, patch)
2021-05-31 03:12 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Mikhaylenko 2021-05-27 02:48:01 PDT
See the patch.
Comment 1 Alexander Mikhaylenko 2021-05-27 03:01:44 PDT
Created attachment 429860 [details]
Patch
Comment 2 EWS Watchlist 2021-05-27 03:02:47 PDT
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
Comment 3 Alexander Mikhaylenko 2021-05-27 03:03:39 PDT
Since we eliminate a 0,0 -> proper size change at the beginning, the patch further regresses https://bugs.webkit.org/show_bug.cgi?id=219202 as the view will not redraw on its own, so it depends on the patch from that bug.
Comment 4 Adrian Perez 2021-05-27 03:06:16 PDT
Patch looks sensible to me, but I would like another pair of
eyes to take a look as well :)
Comment 5 Michael Catanzaro 2021-05-27 09:46:19 PDT
Comment on attachment 429860 [details]
Patch

Looks like there is some fallout:

Unexpected failures (2)
    /WebKit2Gtk/TestWebKitWebView
        /webkit/WebKitWebView/snapshot
    /WebKit2Gtk/TestUIClient
        /webkit/WebKitWebView/window-properties
Comment 6 Michael Catanzaro 2021-05-27 09:50:05 PDT
Comment on attachment 429860 [details]
Patch

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

This looks good to me.

I would add a comment though, to explain why we are doing this, why it's not going to return a perfect result but is better than (0, 0), and probably also link to this bug report so we don't remove it by mistake in the future.

BTW it's already mentioned in the patch changelog, but I'll link to it here as well: this is to fix https://gitlab.gnome.org/GNOME/epiphany/-/issues/1532.

> Source/WebKit/ChangeLog:10
> +        size, which will be (0, 0) because the drawing area ist still null

is

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2376
> +    // everything is fine and we'll just use that

Nit: that. with a period.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2383
> +    // through the hierarchy and try to find a parent with non-0 size

Nit: size. with a period.
Comment 7 Carlos Garcia Campos 2021-05-27 10:23:39 PDT
Comment on attachment 429860 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2373
> +{
> +#if USE(GTK4)
> +    int width = gtk_widget_get_width(GTK_WIDGET(webViewBase));
> +    int height = gtk_widget_get_height(GTK_WIDGET(webViewBase));
> +#else
> +    int width = gtk_widget_get_allocated_width(GTK_WIDGET(webViewBase));
> +    int height = gtk_widget_get_allocated_height(GTK_WIDGET(webViewBase));
> +#endif

I think we could add gtk_widget_get_width/height to GtkVersioning for gtk3 that returns allocated width/height. Assuming we always want the allocated size in gtk3 when width/height is queried in gtk4
Comment 8 Carlos Garcia Campos 2021-05-27 10:24:12 PDT
Comment on attachment 429860 [details]
Patch

Tests are not passing.
Comment 9 Alexander Mikhaylenko 2021-05-27 10:28:16 PDT
(In reply to Carlos Garcia Campos from comment #7)
> I think we could add gtk_widget_get_width/height to GtkVersioning for gtk3
> that returns allocated width/height. Assuming we always want the allocated
> size in gtk3 when width/height is queried in gtk4

I mean, really we want `get_width()` in both cases but it doesn't exist in GTK3, so `get_allocated_width()` is the closest we have.
Comment 10 Alexander Mikhaylenko 2021-05-28 02:53:13 PDT
(In reply to Carlos Garcia Campos from comment #8)
> Comment on attachment 429860 [details]
> Patch
> 
> Tests are not passing.

Great, they pass locally. I wonder what the difference is.
Comment 11 Alexander Mikhaylenko 2021-05-31 00:30:02 PDT
Nevermind, they pass for GTK4 and fail for GTK3. That makes more sense.
Comment 12 Alexander Mikhaylenko 2021-05-31 03:12:00 PDT
Created attachment 430194 [details]
Patch

Ok, one thing I hadn't known is that in GTK3 the default size is 1x1 and not 0x0. Another thing is I forgot about inspector which can take up a part of the web view size. Fixed both.
Comment 13 Alexander Mikhaylenko 2021-05-31 03:12:50 PDT
> I think we could add gtk_widget_get_width/height to GtkVersioning for gtk3 that returns allocated width/height. Assuming we always want the allocated size in gtk3 when width/height is queried in gtk4

I did this at first, but then reverted it - the latest version of the patch uses get_width()/height() only once and since the default size differs between GTK 3 and 4 I have an #if there anyway.
Comment 14 Michael Catanzaro 2021-06-01 04:55:52 PDT
Comment on attachment 430194 [details]
Patch

Thanks!
Comment 15 EWS 2021-06-01 04:59:28 PDT
Committed r278301 (238338@main): <https://commits.webkit.org/238338@main>

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