Bug 97202 – [GTK] Implement ViewState methods in PageClientImpl in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

PageClientImpl::isViewWindowActive(), PageClientImpl::isViewFocused(), PageClientImpl::isViewVisible() and PageClientImpl::isViewInWindow() are all unimplemented and always return true.

[Carlos Garcia Campos]

Created attachment 164898 [details]
Patch

[WebKit Review Bot]

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

[Martin Robinson]

Does this allow us to unskip some tests? I notice that it changes WebKitTestRunner.

[Martin Robinson]

Comment on attachment 164898 [details]
Patch

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

This looks good to me, though changes to the test harness suggest that this will allow us to unskip tests or change results somehow.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:100
> +    gulong toplevelResizeGripVisibilityID;
> +    gulong toplevelFocusInEventID;
> +    gulong toplevelFocusOutEventID;

Maybe just unsigned long here?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:120
> +static void webkitWebViewBaseNotifyResizerSizeForWindow(WebKitWebViewBase* webViewBase)

I guess you can remove the "ForWindow" from the name since you are no longer passing it.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:254
> +    if (widgetIsOnscreenToplevelWindow(toplevel))
> +        webkitWebViewBaseSetToplevelOnScreenWindow(webView, GTK_WINDOW(toplevel));

Are there situations where a window can be realized already inside a toplevel window, without set_parent being called?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:743
> +    if (widgetIsOnscreenToplevelWindow(toplevel))
> +        webkitWebViewBaseSetToplevelOnScreenWindow(webViewBase, GTK_WINDOW(toplevel));
> +    else
> +        webkitWebViewBaseSetToplevelOnScreenWindow(webViewBase, 0);

If you like, this could just be:

webkitWebViewBaseSetToplevelOnScreenWindow(webViewBase, widgetIsOnscreenToplevelWindow(toplevel) ? GTK_WINDOW(toplevel) :  0);

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 164898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164898&action=review
> 
> This looks good to me, though changes to the test harness suggest that this will allow us to unskip tests or change results somehow.

The changes in the tests is because this patch broke several tests. Current code always returns true for isFocused, isInwindow, etc. some tests move the focus to the view and then check that the view is focused calling isFocused() and of course it was always true. Others check isInActiveWindow() with the same situation, etc. We have PlatformWebView::focus() unimplemented in this moment because our view is always focused from the API POV. With the patch all view state flags are correctly updated and we need to implement PlatformWebView::focus() to focus the view programatically. This fixed all the tests that were failing because isFocused or isInActiveWindow returned false. I'll try again with the skipped to tests to check if any of them pass now.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:100
> > +    gulong toplevelResizeGripVisibilityID;
> > +    gulong toplevelFocusInEventID;
> > +    gulong toplevelFocusOutEventID;
> 
> Maybe just unsigned long here?

g_signal_connect returns a gulong.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:120
> > +static void webkitWebViewBaseNotifyResizerSizeForWindow(WebKitWebViewBase* webViewBase)
> 
> I guess you can remove the "ForWindow" from the name since you are no longer passing it.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:254
> > +    if (widgetIsOnscreenToplevelWindow(toplevel))
> > +        webkitWebViewBaseSetToplevelOnScreenWindow(webView, GTK_WINDOW(toplevel));
> 
> Are there situations where a window can be realized already inside a toplevel window, without set_parent being called?

A widget must be inside a toplevel to be realized, so parent_set has always been called when the widget is realized.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:743
> > +    if (widgetIsOnscreenToplevelWindow(toplevel))
> > +        webkitWebViewBaseSetToplevelOnScreenWindow(webViewBase, GTK_WINDOW(toplevel));
> > +    else
> > +        webkitWebViewBaseSetToplevelOnScreenWindow(webViewBase, 0);
> 
> If you like, this could just be:
> 
> webkitWebViewBaseSetToplevelOnScreenWindow(webViewBase, widgetIsOnscreenToplevelWindow(toplevel) ? GTK_WINDOW(toplevel) :  0);

I thought about it, but I thought the explicit if was clearer in this case.

[Carlos Garcia Campos]

Created attachment 165107 [details]
Updated patch according to review comments

[Martin Robinson]

(In reply to comment #5)
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:100
> > > +    gulong toplevelResizeGripVisibilityID;
> > > +    gulong toplevelFocusInEventID;
> > > +    gulong toplevelFocusOutEventID;
> > 
> > Maybe just unsigned long here?
>
> g_signal_connect returns a gulong.

gulong is always typedeffed to unsigned long, so we could avoid the use of simple GLib typedefs here.

> > Are there situations where a window can be realized already inside a toplevel window, without set_parent being called?
> 
> A widget must be inside a toplevel to be realized, so parent_set has always been called when the widget is realized.

I suppose you must call webkitWebViewBaseSetToplevelOnScreenWindow here because parent_set may be called before the widget has a toplevel window. Would it be possible to remove the call in parent_set and simply use the one here, in realize? It seems the only problem would be if a widget can change toplevel windows without being realized and unrealized. Surely that's not possible?

The reason I'm not sure about using parent_set is that it may not necessarily indicate that the toplevel window of the widget has changed, so perhaps it's not a good time to detect that change. Coupled with the check here, it seems redundant, but perhaps I'm missing a particular situation.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:100
> > > > +    gulong toplevelResizeGripVisibilityID;
> > > > +    gulong toplevelFocusInEventID;
> > > > +    gulong toplevelFocusOutEventID;
> > > 
> > > Maybe just unsigned long here?
> >
> > g_signal_connect returns a gulong.
> 
> gulong is always typedeffed to unsigned long, so we could avoid the use of simple GLib typedefs here.
> 
> > > Are there situations where a window can be realized already inside a toplevel window, without set_parent being called?
> > 
> > A widget must be inside a toplevel to be realized, so parent_set has always been called when the widget is realized.
> 
> I suppose you must call webkitWebViewBaseSetToplevelOnScreenWindow here because parent_set may be called before the widget has a toplevel window. Would it be possible to remove the call in parent_set and simply use the one here, in realize? It seems the only problem would be if a widget can change toplevel windows without being realized and unrealized. Surely that's not possible?

Yes, it's not possible.

> 
> The reason I'm not sure about using parent_set is that it may not necessarily indicate that the toplevel window of the widget has changed, so perhaps it's not a good time to detect that change. Coupled with the check here, it seems redundant, but perhaps I'm missing a particular situation.

No no, you are right, we can use parent-set only to detect when a widget is reparented to disconnect all the signals and clear the window, and set the new window only when the widget is realized to make sure we use the real toplevel window.

[Carlos Garcia Campos]

Created attachment 165143 [details]
Updated patch

[Carlos Garcia Campos]

Committed r129238: <http://trac.webkit.org/changeset/129238>