[webkit-reviews] review denied: [Bug 45423] [Gtk] Port tiled backing store : [Attachment 85656] TileCairo3

[bugzilla-daemon at webkit.org]

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

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

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

I learned a few things while poking this code for the clutter backend, so I
have a few more comments now =D

> Source/WebCore/platform/graphics/cairo/TileCairo.cpp:117
> +void Tile::updateBackBuffer()
> +{
> +    if (m_buffer && !isDirty())

After https://bugs.webkit.org/show_bug.cgi?id=56464 lands (I'm going to try to
land it right now) this function should return the updateRects when it
succeeds, and return an empty Vector<IntRect> if this condition fails. See:
http://gitorious.org/webkit-clutter/webkit-clutter/commit/1192ac5b3f92bf9c1e16c
0e457d26ede3a4f5787

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:765
> +    return frame->view()->visibleContentRect();

This is not correct. This should return the area that the backing store should
consider visible (to figure out which tiles to paint). Take a look here:
http://gitorious.org/webkit-clutter/webkit-clutter/commit/73db7b5a8b64705bc7669
d9ff757bdd38ad18c97. So this function should return a rect with the offset
values obtained from the adjustments as its location, and the WebView's
allocation as its size.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:770
> +void ChromeClient::delegatedScrollRequested(const IntPoint& delta)
> +{
> +}

I believe we should tell webcore to delegate scrolling and implement this. We
should rely and act exclusively on the webview's adjustments when tiled backing
store is turned on. For this to work properly for programmatic scrolls we'll
need a fix to webcore first, though:
http://gitorious.org/webkit-clutter/webkit-clutter/commit/f78e8bb091f41e20f4053
3db0ee33348759576d2

> Source/WebKit/gtk/webkit/webkitwebview.cpp:691
> +    int scrollX = view->scrollX();
> +    int scrollY = view->scrollY();
> +
> +    context->translate(-scrollX, -scrollY);
> +    rect.move(scrollX, scrollY);

Which means we should use the adjustment values here, not what is reported by
ScrollView, because it will not match the real scroll offset.