[webkit-reviews] review granted: [Bug 59821] [GTK] Untangle GtkAdjustments from WebCore : [Attachment 91959] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 59821: [GTK] Untangle GtkAdjustments from WebCore
https://bugs.webkit.org/show_bug.cgi?id=59821

Attachment 91959: Patch
https://bugs.webkit.org/attachment.cgi?id=91959&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.