Bug 153745 – [GTK] Reduce IPC traffic due to view state changes

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Very often view state changes happen one after another in a very short period of time, even in the same run loop iteration. For example, when you switch to the web view window, the view is focused and the active window flag changes as well. In that case we are sending two messages to the web process and the page updates its status according to the new flags in two steps. So, we could group all state changes happening in the same run loop iteration and notify about them all in the next iteration. This also prevents unnecessary changes of state when we quickly go back to a previous state, for example in focus follows mouse configurations if you move the mouse outside the window and then inside the window again quickly.

[Carlos Garcia Campos]

Created attachment 270390 [details]
Patch

[WebKit Commit 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

[Sergio Villar Senin]

Comment on attachment 270390 [details]
Patch

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

Nice patch, although I think it doesn't really work as is (see my comment about the mask)

> Source/WebKit2/ChangeLog:27
> +        WebPageProxy::viewStateDidChange() and resret the flags that need

resret -> reset

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:316
> +        g_signal_connect(priv->toplevelOnScreenWindow, "window-state-event", G_CALLBACK(toplevelWindowStateEvent), webViewBase);

According to the docs you should enable GDK_STRUCTURE_MASK something that you are not doing. You should also remove the GDK_VISIBILITY_NOTIFY_MASK.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1385
> +    unsigned flagsToUpdate = ViewState::IsFocused;

We could use ViewState::Flags here for consistency with the rest of the code (even though it's an unsigned anyway).

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 270390 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270390&action=review
> 
> Nice patch, although I think it doesn't really work as is (see my comment
> about the mask)

Thanks for the review!

> > Source/WebKit2/ChangeLog:27
> > +        WebPageProxy::viewStateDidChange() and resret the flags that need
> 
> resret -> reset

Oops.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:316
> > +        g_signal_connect(priv->toplevelOnScreenWindow, "window-state-event", G_CALLBACK(toplevelWindowStateEvent), webViewBase);
> 
> According to the docs you should enable GDK_STRUCTURE_MASK something that
> you are not doing. You should also remove the GDK_VISIBILITY_NOTIFY_MASK.

But then it says "GDK will enable this mask automatically for all new windows." Also note that this is not a signal of the web view, but the toplevel window, and GtkWindos adds the GDK_STRUCTURE_MASK unconditionally in realize method.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:1385
> > +    unsigned flagsToUpdate = ViewState::IsFocused;
> 
> We could use ViewState::Flags here for consistency with the rest of the code
> (even though it's an unsigned anyway).

OK.

[Carlos Garcia Campos]

Committed r196062: <http://trac.webkit.org/changeset/196062>