Created attachment 400703 [details] Patch

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

Comment on attachment 400703 [details] Patch Patch looks good, there's just one small wrinkle (please read below) and a nit, otherwise I would have approved it already :] View in context: https://bugs.webkit.org/attachment.cgi?id=400703&action=review > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1715 > + if (isActive && !gtk_widget_get_visible(GTK_WIDGET(window))) This will return early if the window is not visible, regardless of its activity status. I think the code would be clearer if it checked the window visibility first and returned early in that case. Something like this: if (!gtk_widget_get_visible(GTK_WIDGET(window)) return; const bool isActive = gtk_window_is_active(window); // ... > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1735 > + bool visible = !(state & GDK_SURFACE_STATE_MINIMIZED); This should check also for GDK_SURFACE_STATE_WITHDRAWN, which indicates that the surface is not being shown.

Comment on attachment 400703 [details] Patch Carlos and me looked into GDK_SURFACE_STATE_WITHDRAWN and it's basically equivalent to “the surface/window is not mapped”, which is handled elsewhere by the widget implementation — we do not need to add handling for it also in this patch. Therefore, changed to r+

(In reply to Adrian Perez from comment #3) > Comment on attachment 400703 [details] > Patch > > Patch looks good, there's just one small wrinkle (please read below) > and a nit, otherwise I would have approved it already :] > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400703&action=review > > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1715 > > + if (isActive && !gtk_widget_get_visible(GTK_WIDGET(window))) > > This will return early if the window is not visible, regardless of > its activity status. I think the code would be clearer if it checked > the window visibility first and returned early in that case. Something > like this: > > if (!gtk_widget_get_visible(GTK_WIDGET(window)) > return; > > const bool isActive = gtk_window_is_active(window); > // ... This comes from GTK3 code, where this early return only happens in the focus-in, and includes this comment: // Spurious focus in events can occur when the window is hidden. I don't even know if that's the case with GTK4, so I think I'm going to assume a hidden toplevel can't get the focus. > > Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1735 > > + bool visible = !(state & GDK_SURFACE_STATE_MINIMIZED); > > This should check also for GDK_SURFACE_STATE_WITHDRAWN, > which indicates that the surface is not being shown.

Committed r262373: <https://trac.webkit.org/changeset/262373>