[webkit-reviews] review denied: [Bug 45423] [Gtk] Port tiled backing store : [Attachment 92731] Programmatic scrolling support2

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Joone Hur
<joone.hur at collabora.co.uk>'s request for review:
Bug 45423: [Gtk] Port tiled backing store
https://bugs.webkit.org/show_bug.cgi?id=45423

Attachment 92731: Programmatic scrolling support2
https://bugs.webkit.org/attachment.cgi?id=92731&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92731&action=review

r- mainly because of the offset handling that should use adjustments instead,
but we also need to fix the visible contents rect and contents size changed
things

> Source/JavaScriptCore/ChangeLog:8
> +	   cairo_rectangle_int_t doesn't need to be defined as the name of
GdkRectangle.

Can you make a separate patch for these cairo_rectangle_int_t changes? It can
go in before we land the tiled backing store.

> Source/WebCore/platform/gtk/ScrollViewGtk.cpp:197
> +#if ENABLE(TILED_BACKING_STORE)
> +    IntSize scrollOffset;
> +    if (delegatesScrolling())
> +	   scrollOffset = actualScrollOffset();
> +    else
> +	   scrollOffset = m_scrollOffset;
> +
> +    return IntRect(IntPoint(scrollOffset.width(), scrollOffset.height()),
> +		      IntSize(allocation.width, allocation.height));
> +#else

Like I said before, I don't think this is going to work. VisibleContentRect
should be the full contents size for the tiled backing store case, and messing
with it will quite likely cause rendering issues. What are you trying to do
here, exactly?

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:506
> +#if ENABLE(TILED_BACKING_STORE)
> +    int xOffset, yOffset;
> +    WebKitWebFrame* webFrame = kit(frame);
> +    webkit_web_frame_get_scroll_offset(webFrame, &xOffset, &yOffset);
> +
> +    if (frame && frame->tiledBackingStore())
> +	   frame->tiledBackingStore()->invalidate(IntRect(xOffset, yOffset,
size.width(), size.height()));
> +#endif

Why? We don't do this in the clutter port. The backing store itself should be
able to figure out the content size change and invalidate the appropriate
tiles. If we need something here it should be just an adjustVisibleRect(), but
I don't think we should need even that.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:996
> +static void queueScrollingUpdate(WebKitWebFrame* webFrame)

I wrote this like this for the clutter port, but I don't think this is
necessarily the best way to go for WebKitGTK+. Perhaps we should have a
WebKitTiledBackingStore object held by WebView that allows the user agent to
control it, so freezing, unfreezing, for instance, could be done by the browser
in a more flexible way. For now this is OK though, otherwise it's hard to test
that the backing store is doing its thing.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:1081
> +void webkit_web_frame_set_scroll_offset(WebKitWebFrame* webFrame,
> +					   gint x, gint y)
> +{

I think it may make sense to have scroll offsets defined in the frame, but we
can worry about that later. In the clutter port we are forced to keep track of
the offsets ourselves because we lack a toolkit-independant ajdustments object,
but in WebKitGTK+ we already have the GtkAdjustments, so we should use them
instead of tracking offsets here.