Bug 59821 - [GTK] Untangle GtkAdjustments from WebCore
Summary: [GTK] Untangle GtkAdjustments from WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
Depends on:
Blocks: 59276
  Show dependency treegraph
 
Reported: 2011-04-29 12:42 PDT by Martin Robinson
Modified: 2011-05-09 17:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (33.55 KB, patch)
2011-04-29 13:14 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (38.07 KB, patch)
2011-05-02 12:57 PDT, Martin Robinson
gns: review+
Details | Formatted Diff | Diff
Diff from patch for landing (6.00 KB, patch)
2011-05-09 16:17 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2011-04-29 12:42:32 PDT
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.
Comment 1 Martin Robinson 2011-04-29 13:14:36 PDT
Created attachment 91722 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-05-01 09:43:46 PDT
Comment on attachment 91722 [details]
Patch

Should this be a helper class for the ChromeClientGtk?  It seems like a relatively encapsulatable concept.
Comment 3 Martin Robinson 2011-05-02 12:57:45 PDT
Created attachment 91959 [details]
Patch
Comment 4 Martin Robinson 2011-05-02 13:00:30 PDT
(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.
Comment 5 Joone Hur 2011-05-08 07:54:43 PDT
https://bugs.webkit.org/show_bug.cgi?id=59276 is also able to be fixed through this patch.

Thanks!
Comment 6 Gustavo Noronha (kov) 2011-05-09 06:14:45 PDT
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.
Comment 7 Martin Robinson 2011-05-09 16:16:11 PDT
(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.
Comment 8 Martin Robinson 2011-05-09 16:17:05 PDT
Created attachment 92878 [details]
Diff from patch for landing
Comment 9 Martin Robinson 2011-05-09 16:25:03 PDT
Committed r86102: <http://trac.webkit.org/changeset/86102>
Comment 10 WebKit Review Bot 2011-05-09 17:34:50 PDT
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