Bug 97202 - [GTK] Implement ViewState methods in PageClientImpl in WebKit2
Summary: [GTK] Implement ViewState methods in PageClientImpl in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-09-20 05:32 PDT by Carlos Garcia Campos
Modified: 2012-09-21 09:50 PDT (History)
4 users (show)

See Also:


Attachments
Patch (24.09 KB, patch)
2012-09-20 05:47 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch according to review comments (23.86 KB, patch)
2012-09-21 03:26 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (23.71 KB, patch)
2012-09-21 09:03 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-09-20 05:32:53 PDT
PageClientImpl::isViewWindowActive(), PageClientImpl::isViewFocused(), PageClientImpl::isViewVisible() and PageClientImpl::isViewInWindow() are all unimplemented and always return true.
Comment 1 Carlos Garcia Campos 2012-09-20 05:47:33 PDT
Created attachment 164898 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-20 05:50:25 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 http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Martin Robinson 2012-09-20 14:35:16 PDT
Does this allow us to unskip some tests? I notice that it changes WebKitTestRunner.
Comment 4 Martin Robinson 2012-09-20 14:54:56 PDT
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);
Comment 5 Carlos Garcia Campos 2012-09-20 23:49:01 PDT
(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.
Comment 6 Carlos Garcia Campos 2012-09-21 03:26:35 PDT
Created attachment 165107 [details]
Updated patch according to review comments
Comment 7 Martin Robinson 2012-09-21 08:18:11 PDT
(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.
Comment 8 Carlos Garcia Campos 2012-09-21 08:56:41 PDT
(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.
Comment 9 Carlos Garcia Campos 2012-09-21 09:03:51 PDT
Created attachment 165143 [details]
Updated patch
Comment 10 Carlos Garcia Campos 2012-09-21 09:50:27 PDT
Committed r129238: <http://trac.webkit.org/changeset/129238>