Bug 59821 – [GTK] Untangle GtkAdjustments from WebCore

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Currently GTK+ adjustments are attached to three places in WebKit: the WebKitWebView, the main frame ScrollView and main frame Scrollbars. The lifetime of main frame ScrollViews and Scrollbars is different than that of the WebKitWebView. 

* Scrollbars, in particular, may be created and destroyed hundreds of times during window resizes (if opaque resizing is enabled in the window manager).
* In the past, we have had many bugs and crashes due to not updating the GTK+ adjustments properly when ScrollViews appear and disappear.

Both of these issues stem from the fact that we are attaching something with a very long lifetime (the GtkAdjustment) to something with a much shorter lifetime (Scrollbars/ScrollView). Instead of letting WebCore deal directly with the GtkAdjustments, I propose that we simply let WebKit worry about them and attach them to the ChromeClient.

[Martin Robinson]

Created attachment 91722 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 91722 [details]
Patch

Should this be a helper class for the ChromeClientGtk?  It seems like a relatively encapsulatable concept.

[Martin Robinson]

Created attachment 91959 [details]
Patch

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 91722 [details])
> Should this be a helper class for the ChromeClientGtk?  It seems like a relatively encapsulatable concept.

Thanks for the comment. I completely agree with you and have uploaded a new patch which moves the logic out of ChromeClientGtk into a helper class named GtkAdjustmentWatcher.

[Joone Hur]

https://bugs.webkit.org/show_bug.cgi?id=59276 is also able to be fixed through this patch.

Thanks!

[Gustavo Noronha (kov)]

Comment on attachment 91959 [details]
Patch

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

I love, love, love this change. ♥ Only a couple comments, but it looks correct to me otherwise.

> Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:43
> +        gtk_adjustment_configure(adjustment, 0, 0, 0, 0, 0, 0); // These are the settings which remove the scrollbar.

Hrm. Is this documented behaviour? I guess if it works in both gtk2 and gtk3 we can pretty much rely on it, but it makes me nervous.

> Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:75
> +    // The fact that this method was called means that we need to update the scrollbars, but at the
> +    // time of invocation they are not updated to reflect the scroll yet. We set a short timeout
> +    // here, which means that they will be updated as soon as WebKit returns to the main loop.
> +    g_timeout_add(0, reinterpret_cast<GSourceFunc>(updateAdjustmentCallback), 
> +                  const_cast<void*>(static_cast<const void*>(this)));

Although I don't foresee any issues with using the default priority here, I think it makes sense to make this timeout use GTK+'s relayout priority (which would be G_PRIORITY_HIGH_IDLE + 10). Also, do we have to worry here that the watcher may have disappeared in-between this call and the callback being called? Perhaps we should store the timeout id, and check for it in the destructor. Storing the ID would also make it possible to check here if an adjustment update has already been queued so you don't queue more than one unnecessarily.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:427
> +    if (Page* page = core(webView))

I think I'd prefer having the attribution in a line of its own before the if, as is usual. I was confused by a split second because the attribution is inside the if there heh.

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 91959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91959&action=review
> 
> I love, love, love this change. ♥ Only a couple comments, but it looks correct to me otherwise.
> 
> > Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:43
> > +        gtk_adjustment_configure(adjustment, 0, 0, 0, 0, 0, 0); // These are the settings which remove the scrollbar.
> 
> Hrm. Is this documented behaviour? I guess if it works in both gtk2 and gtk3 we can pretty much rely on it, but it makes me nervous.

You're right in that this is not documented explicitly. This was what we were doing before to force no scrollbars as well. Zeroes are the default values of GtkAdjustment as well.

> 
> > Source/WebKit/gtk/WebCoreSupport/GtkAdjustmentWatcher.cpp:75
> > +    // The fact that this method was called means that we need to update the scrollbars, but at the
> > +    // time of invocation they are not updated to reflect the scroll yet. We set a short timeout
> > +    // here, which means that they will be updated as soon as WebKit returns to the main loop.
> > +    g_timeout_add(0, reinterpret_cast<GSourceFunc>(updateAdjustmentCallback), 
> > +                  const_cast<void*>(static_cast<const void*>(this)));
> 
> Although I don't foresee any issues with using the default priority here, I think it makes sense to make this timeout use GTK+'s relayout priority (which would be G_PRIORITY_HIGH_IDLE + 10). Also, do we have to worry here that the watcher may have disappeared in-between this call and the callback being called? Perhaps we should store the timeout id, and check for it in the destructor. Storing the ID would also make it possible to check here if an adjustment update has already been queued so you don't queue more than one unnecessarily.

Nice catch. I've made the changes you requested (you can see them in the diff I uploaded).

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:427
> > +    if (Page* page = core(webView))
> 
> I think I'd prefer having the attribution in a line of its own before the if, as is usual. I was confused by a split second because the attribution is inside the if there heh.

Okay. I've split out the assignements from the if statements.

[Martin Robinson]

Created attachment 92878 [details]
Diff from patch for landing

[Martin Robinson]

Committed r86102: <http://trac.webkit.org/changeset/86102>

[WebKit Review Bot]

http://trac.webkit.org/changeset/86102 might have broken GTK Linux 32-bit Release
The following tests are not passing:
scrollbars/custom-scrollbar-with-incomplete-style.html