Bug 45423 – [Gtk] Port tiled backing store

• Component: Platform
• Status: CLOSED LATER

[Enrico Ros]

Port tiled backing store to Gtk.

[Julien Chaffraix]

Working on the Gtk backing store. Patch coming soon!

[Julien Chaffraix]

Created attachment 68178 [details]
First try: Add a compile time option and implement the logic

[Alejandro G. Castro]

We did the initial port months ago you can find the patches in this branch:

http://git.igalia.com/cgi-bin/gitweb.cgi?p=webkit.git;a=shortlog;h=refs/heads/scrollgtk

We did not integrate them because the initial impressions regarding performance were not that good, probably it needs review, and WebKit2 seems the way to go regarding how to define the backing store.

Anyway, in case we want to add it I think we should add a feature define to activate it in compilation.

[Martin Robinson]

Comment on attachment 68178 [details]
First try: Add a compile time option and implement the logic

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

This patch looks good, in general, but I think I'd rather see this implemented in terms of the Cairo API as much as possible. We really want to avoid GdkPixmap/GdkPixbuf as much as possible and especially deprecated APIs like GdkRegion.

> WebCore/platform/graphics/Tile.h:82
> +    GdkPixmap* m_buffer;
> +    GdkRegion* m_dirtyRegion;

Instead of using GdkPixmap, I'd like to see a GRefPtr<cairo_surface_t> here. GdkRegion is deprecated in recent GDK releases (replaced by cairo regions), so you'll either have to figure out some version independent method or use #ifdefs.

> WebCore/platform/graphics/gtk/TileGtk.cpp:61
> +        Color color1(checkerColor1);
> +        Color color2(checkerColor2);
> +        float red1, green1, blue1, alpha1;
> +        float red2, green2, blue2, alpha2;
> +        color1.getRGBA(red1, green1, blue1, alpha1);
> +        color2.getRGBA(red2, green2, blue2, alpha2);
> +        for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +            bool alternate = y % checkerSize;
> +            for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +                cairo_rectangle(cr, x, y, checkerSize / 2, checkerSize /2);
> +                if (alternate)
> +                    cairo_set_source_rgb(cr, red1, green1, blue1);
> +                else
> +                    cairo_set_source_rgb(cr, red2, green2, blue2);
> +                cairo_fill(cr);
> +                alternate = !alternate;

I think it might be cleaner to use the stippling approach that Benjamin Otte oulined here: http://blogs.gnome.org/otte/2010/07/27/rendering-cleanup/

> WebCore/platform/graphics/gtk/TileGtk.cpp:119
> +        if (!m_buffer) {
> +            m_backBuffer = gdk_pixmap_new(0,
> +                               m_backingStore->m_tileSize.width(),
> +                               m_backingStore->m_tileSize.height(),
> +                               gdk_visual_get_depth(gdk_visual_get_system()));
> +        } else {

WebKit code style requires many block types with only one statement to omit the curly braces.

> WebCore/platform/graphics/gtk/TileGtk.cpp:154
> +    if (m_buffer)
> +        g_object_unref(m_buffer);

The use of GRefPtr would elminate the need to do this.

> WebCore/platform/graphics/gtk/TileGtk.cpp:170
> +                   target.height());
> +

Line lengths in the project can be ~120+ characters. So this should be:
    IntRect source((target.x() - m_rect.x()), (target.y() - m_rect.y()), target.height());

> WebCore/platform/graphics/gtk/TileGtk.cpp:178
> +    cairo_t* cairoContextSource = gdk_cairo_create(m_buffer);
> +    cairo_set_source_surface(
> +        cairoContext,
> +        cairo_get_target(cairoContextSource),
> +        target.x() - source.x(),
> +        target.y() - source.y());

Using a Cairo surface as the buffer will eliminate some of this and allow you do just do something like:
cairo_set_source_surface(cairoContxt, m_buffer.get(), target.x() - source.x(), target.y() - source.y());

[zaheer]

I work with julien, providing the updated patch and comments.

(In reply to comment #4)
> (From update of attachment 68178 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68178&action=review
> 
> This patch looks good, in general, but I think I'd rather see this implemented in terms of the Cairo API as much as possible. We really want to avoid GdkPixmap/GdkPixbuf as much as possible and especially deprecated APIs like GdkRegion.
> 
> > WebCore/platform/graphics/Tile.h:82
> > +    GdkPixmap* m_buffer;
> > +    GdkRegion* m_dirtyRegion;
> 
> Instead of using GdkPixmap, I'd like to see a GRefPtr<cairo_surface_t> here. GdkRegion is deprecated in recent GDK releases (replaced by cairo regions), so you'll either have to figure out some version independent method or use #ifdefs.
Done. used ifdefs for region (note cairo_region is not tested as its not available on my mc)
> 
> > WebCore/platform/graphics/gtk/TileGtk.cpp:61
> > +        Color color1(checkerColor1);
> > +        Color color2(checkerColor2);
> > +        float red1, green1, blue1, alpha1;
> > +        float red2, green2, blue2, alpha2;
> > +        color1.getRGBA(red1, green1, blue1, alpha1);
> > +        color2.getRGBA(red2, green2, blue2, alpha2);
> > +        for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> > +            bool alternate = y % checkerSize;
> > +            for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> > +                cairo_rectangle(cr, x, y, checkerSize / 2, checkerSize /2);
> > +                if (alternate)
> > +                    cairo_set_source_rgb(cr, red1, green1, blue1);
> > +                else
> > +                    cairo_set_source_rgb(cr, red2, green2, blue2);
> > +                cairo_fill(cr);
> > +                alternate = !alternate;
> 
> I think it might be cleaner to use the stippling approach that Benjamin Otte oulined here: http://blogs.gnome.org/otte/2010/07/27/rendering-cleanup/
Done. pixmap to cairo_pattern_t. iam not entirely sure if a client side image would be slower than the pixmap.
> 
> > WebCore/platform/graphics/gtk/TileGtk.cpp:119
> > +        if (!m_buffer) {
> > +            m_backBuffer = gdk_pixmap_new(0,
> > +                               m_backingStore->m_tileSize.width(),
> > +                               m_backingStore->m_tileSize.height(),
> > +                               gdk_visual_get_depth(gdk_visual_get_system()));
> > +        } else {
> 
> WebKit code style requires many block types with only one statement to omit the curly braces.
Removed the backbuffer as its not currently used.
> 
> > WebCore/platform/graphics/gtk/TileGtk.cpp:154
> > +    if (m_buffer)
> > +        g_object_unref(m_buffer);
> 
> The use of GRefPtr would elminate the need to do this.
Done
> 
> > WebCore/platform/graphics/gtk/TileGtk.cpp:170
> > +                   target.height());
> > +
> 
> Line lengths in the project can be ~120+ characters. So this should be:
>     IntRect source((target.x() - m_rect.x()), (target.y() - m_rect.y()), target.height());
Done
> 
> > WebCore/platform/graphics/gtk/TileGtk.cpp:178
> > +    cairo_t* cairoContextSource = gdk_cairo_create(m_buffer);
> > +    cairo_set_source_surface(
> > +        cairoContext,
> > +        cairo_get_target(cairoContextSource),
> > +        target.x() - source.x(),
> > +        target.y() - source.y());
> 
> Using a Cairo surface as the buffer will eliminate some of this and allow you do just do something like:
> cairo_set_source_surface(cairoContxt, m_buffer.get(), target.x() - source.x(), target.y() - source.y());
Done

[zaheer]

Created attachment 68365 [details]
updated patch

[zaheer]

Comment on attachment 68365 [details]
updated patch

doesnt apply on latest

[Martin Robinson]

Comment on attachment 68365 [details]
updated patch

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

Looks better. Eventually we need to do something about regions to get us out of this #ifdef jungle. That doesn't have to happen in this patch though.

> WebCore/platform/graphics/gtk/TileGtk.cpp:16
> +   Copyright (C) 2010 codeaurora.org
> +   All rights reserved.
> +
> +   Redistribution and use in source and binary forms, with or without
> +   modification, are permitted provided that the following conditions
> +   are met:
> +   1. Redistributions of source code must retain the above copyright
> +   notice, this list of conditions and the following disclaimer.
> +   2. Redistributions in binary form must reproduce the above copyright
> +   notice, this list of conditions and the following disclaimer in the
> +   documentation and/or other materials provided with the distribution.
> +   3. The name of the author may not be used to endorse or promote products
> +   derived from this software without specific prior written permission.
> +
> +   THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR

What license is this? It look like BSD, plus another clause. I feel uncomfortable r+ing a patch with a unique license. I believe you need to use one of the licenses found in the LICENSE* files in the WebCore directory. Also, the block is not formatted like blocks in other files.

> WebCore/platform/graphics/gtk/TileGtk.cpp:50
> +    if (!pattern){ 

Please use an early return here.

> WebCore/platform/graphics/gtk/TileGtk.cpp:69
> +        cairo_t* cr = cairo_create(surface.get());
> +        Color color1(checkerColor1);
> +        Color color2(checkerColor2);
> +        float red1, green1, blue1, alpha1;
> +        float red2, green2, blue2, alpha2;
> +        color1.getRGBA(red1, green1, blue1, alpha1);
> +        color2.getRGBA(red2, green2, blue2, alpha2);
> +        for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +            bool alternate = y % checkerSize;
> +            for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +                cairo_rectangle(cr, x, y, checkerSize / 2, checkerSize /2);
> +                cairo_set_source_rgb(cr, alternate ? red1 : red2, alternate ? green1 : green2, alternate ? blue1 : blue2);
> +                cairo_fill(cr);
> +                alternate = !alternate;
> +            }
> +        }
> +        pattern = cairo_pattern_create_for_surface (surface.get());
> +    }

Isn't it possible to switch to the stippling approach from the link I gave above?

> WebCore/platform/graphics/gtk/TileGtk.cpp:136
> +    gdk_region_get_rectangles (m_dirtyRegion, &rects.outPtr(), &rectCount);

Watch the spacing here.

> WebCore/platform/graphics/gtk/TileGtk.cpp:179
> +    cairo_save(cr);
> +    cairo_set_source_surface(cr, m_buffer.get(), target.x() - source.x(), target.y() - source.y());
> +    cairo_rectangle(cr, target.x(), target.y(), target.width(), target.height());
> +    cairo_fill(cr);
> +    cairo_restore(cr);

save/restore are pretty expensive, is it possible to just save the previous source and restore it after the operation?

> WebKit/gtk/webkit/webkitwebview.cpp:535
> +    context->save();
> +    context->translate(-scrollX, -scrollY);
> +    rect.move(scrollX, scrollY);
> +    frame->tiledBackingStore()->paint(context, rect);
> +    context->restore();

Save and restore are actually pretty expensive. Is it possible to translate the context back after the operation?

> WebKit/gtk/webkit/webkitwebview.cpp:572
> +            if (frame->tiledBackingStore()){ 

Watch the spacing around your parenthesis.

> WebKit/gtk/webkit/webkitwebview.cpp:573
> +                // FIXME:  We should set the backing store viewport earlier than in paint

Is it possible to fix this now?

> WebKit/gtk/webkit/webkitwebview.cpp:588
> +                if (frame->tiledBackingStore()){ 

Ditto.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:145
> +#if ENABLE(TILED_BACKING_STORE)
> +        virtual WebCore::IntRect visibleRectForTiledBackingStore() const;
> +#endif
> +

It seems this could just be a helper method in webkitwebview.cpp. The only member your using in ChromeClientGtk is m_webView, which is a local variable in the context you use it.

[Martin Robinson]

(In reply to comment #5)

> Done. pixmap to cairo_pattern_t. iam not entirely sure if a client side image would be slower than the pixmap.

Good point. It is possible to create pixmap surfaces in Cairo using the method Julien used in the original patch. That might be something to consider if you find that it is more performant.

[Julien Chaffraix]

> Looks better. Eventually we need to do something about regions to get us out 
> of this #ifdef jungle. That doesn't have to happen in this patch though.

Noted.

> What license is this? It look like BSD, plus another clause. 
> I feel uncomfortable r+ing a patch with a unique license.

The license used is a 3-clause BSD license. It is not an uncommon license inside WebKit (see a lot of WebCore's files).

> I believe you need to use one of the licenses found in the LICENSE* files in 
> the WebCore directory.

I disagree. Those are provided as examples but there is no agreement on the form of the BSD or LGPL-2.1 licenses used inside the project.

> Also, the block is not formatted like blocks in other files.

Noted, I will see what I can do.

> Please use an early return here.

This form underlines the singleton pattern more IMHO. I am too set up on that so if you really want an early return I am also fine with the early return.

> Isn't it possible to switch to the stippling approach from the link I gave 
> above?

We could, do we want to draw a dotted pattern as opposed to a bigger checker box (8pixels wide) in other port?

> > WebCore/platform/graphics/gtk/TileGtk.cpp:179
> > +    cairo_save(cr);
> > +    cairo_set_source_surface(cr, m_buffer.get(), target.x() - source.x(), target.y() - source.y());
> > +    cairo_rectangle(cr, target.x(), target.y(), target.width(), target.height());
> > +    cairo_fill(cr);
> > +    cairo_restore(cr);
> 
> save/restore are pretty expensive, is it possible to just save the previous source and restore it after the operation?
> 
> > WebKit/gtk/webkit/webkitwebview.cpp:535
> > +    context->save();
> > +    context->translate(-scrollX, -scrollY);
> > +    rect.move(scrollX, scrollY);
> > +    frame->tiledBackingStore()->paint(context, rect);
> > +    context->restore();
> 
> Save and restore are actually pretty expensive. Is it possible to translate > the context back after the operation?

Good catch, the above two save/restore are not required as the caller (expose) already does that.

> > WebKit/gtk/webkit/webkitwebview.cpp:573
> > +                // FIXME:  We should set the backing store viewport earlier than in paint
> 
> Is it possible to fix this now?

Not sure, since we need to hook on all places where the visible rect could change (e.g resize)

> It seems this could just be a helper method in webkitwebview.cpp. The only 
> member your using in ChromeClientGtk is m_webView, which is a local variable 
> in the context you use it.

OK, this will be changed.

The style issues will be taken care of. Thanks for the review!

[Martin Robinson]

(In reply to comment #10)
> The license used is a 3-clause BSD license. It is not an uncommon license inside WebKit (see a lot of WebCore's files).

You're right. Sorry about the misunderstanding!

> > Isn't it possible to switch to the stippling approach from the link I gave 
> > above?
> We could, do we want to draw a dotted pattern as opposed to a bigger checker box (8pixels wide) in other port?

I was thinking it might be possible to scale the pattern, which would simplify this code section. The approach you have is fine though, if that approach will not work.

> > > WebKit/gtk/webkit/webkitwebview.cpp:573
> > > +                // FIXME:  We should set the backing store viewport earlier than in paint
> > Is it possible to fix this now?
> Not sure, since we need to hook on all places where the visible rect could change (e.g resize)

Hrm. Perhaps ChromeClient::contentsSizeChanged and ChromeClient::scroll?

[Julien Chaffraix]

Created attachment 69014 [details]
Updated version: Takes all the comments into account

Just a caveat, I say I would move the ChromeClientGtk method into the webview but actually visibleRectForTiledBackingStore is a virtual function expected to be implemented by ChromeClient ports. It’s a one liner so not sure how it can be split. Also a call from ChromeClient -> webview api layer doesn’t seem right.

[Martin Robinson]

Comment on attachment 69014 [details]
Updated version: Takes all the comments into account

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

Looks good. I think we should avoid adding the option to GtkLauncher, as we try to keep it very minimal and it isn't strictly necessary there. Since this is an API change, it will need the approval of another GTK+ reviewer as well.

> WebCore/platform/graphics/gtk/TileGtk.cpp:27
> +   Copyright (C) 2010 codeaurora.org
> +   All rights reserved.
> +
> +   Redistribution and use in source and binary forms, with or without
> +   modification, are permitted provided that the following conditions
> +   are met:
> +   1. Redistributions of source code must retain the above copyright
> +   notice, this list of conditions and the following disclaimer.
> +   2. Redistributions in binary form must reproduce the above copyright
> +   notice, this list of conditions and the following disclaimer in the
> +   documentation and/or other materials provided with the distribution.
> +   3. The name of the author may not be used to endorse or promote products
> +   derived from this software without specific prior written permission.
> +
> +   THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> +   IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> +   OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> +   IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> +   INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> +   NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> +   THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +

Do you mind using the same license formatting as other files in the WebCore/graphics/gtk/ directory? There's an example of a BSD license in FontGtk.cpp. We should be as consistent as possible.

> WebCore/platform/graphics/gtk/TileGtk.cpp:45
> +static const unsigned checkerSize = 16;
> +static const unsigned checkerColor1 = 0xff555555;
> +static const unsigned checkerColor2 = 0xffaaaaaa;

If these are only used in checkeredPixmap, shouldn't they be defined there?

> WebCore/platform/graphics/gtk/TileGtk.cpp:50
> +    if (!pixmap) {

In WebKit, early returns are preferred, because it makes for shallower blocks.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:381
> +#if ENABLE(TILED_BACKING_STORE)
> +    Frame* frame = core(m_webView)->mainFrame();
> +    if (frame && frame->tiledBackingStore())
> +        frame->tiledBackingStore()->adjustVisibleRect();
> +#endif
> +

Can you confirm that this is needed here or if doing it in ::contentsSizeChanged is enough. I just took a guess on where this was necessary, so I'm just surprised that I was right. :)

> WebKit/gtk/webkit/webkitwebview.cpp:519
> +static void renderFromTiledBackingStore(Frame *frame, GraphicsContext* context, IntRect rect)

Frame *frame should be Frame* frame.

[Antonio Gomes]

Comment on attachment 69014 [details]
Updated version: Takes all the comments into account

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

>> WebKit/gtk/webkit/webkitwebview.cpp:519
>> +static void renderFromTiledBackingStore(Frame *frame, GraphicsContext* context, IntRect rect)
> 
> Frame *frame should be Frame* frame.

const IntRect& ?

[Julien Chaffraix]

> Looks good. I think we should avoid adding the option to GtkLauncher, as we try to keep it very minimal and it isn't strictly necessary there. Since this is an API change, it will need the approval of another GTK+ reviewer as well.

I don't mind avoiding another option is that is what the GTK reviewers prefer.

> Do you mind using the same license formatting as other files in the WebCore/graphics/gtk/ directory? There's an example of a BSD license in FontGtk.cpp. We should be as consistent as possible.

This will be an issue. There is a couple of non-trivial differences in wording with respect to the other licenses which means that I will need to run it through our legal department (which usually take a least 3 weeks to reply :-/).

> Can you confirm that this is needed here or if doing it in ::contentsSizeChanged is enough. I just took a guess on where this was necessary, so I'm just surprised that I was right. :)

I will double check and update the patch accordingly.

The other comments will be taken care in the next version. Thanks!

[Martin Robinson]

(In reply to comment #15)
> > Do you mind using the same license formatting as other files in the WebCore/graphics/gtk/ directory? There's an example of a BSD license in FontGtk.cpp. We should be as consistent as possible.

> This will be an issue. There is a couple of non-trivial differences in wording with respect to the other licenses which means that I will need to run it through our legal department (which usually take a least 3 weeks to reply :-/).

Sorry. I don't mean to change the wording, but to ensure that it is has the same formatting (for instance, in other files, every line starts with a space then an asterisk and then a space).

> The other comments will be taken care in the next version. Thanks!

Great! Thank you.

[Joone Hur]

Created attachment 77469 [details]
updated patch to work with the trunk

I cleaned up the code to work with the trunk and updated ChangeLog files.

[Martin Robinson]

Comment on attachment 77469 [details]
updated patch to work with the trunk

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

Looks good. There are a few issues that I can see though.

> WebCore/page/Frame.cpp:865
> -    unsigned size = paintedArea.size();
> +    int size = paintedArea.size();

Is this change related?

> WebCore/platform/graphics/gtk/TileGtk.cpp:7
> +   Redistribution and use in source and binary forms, with or without
> +   modification, are permitted provided that the following conditions
> +   are met:

Nit: do you mind adding the asterisks here so that this license block looks like others in this directory?

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:437
> +    notImplemented();

Is this really notImplemented() or is it an empty implementation? There's a subtle difference.

> WebKit/gtk/webkit/webkitwebsettings.cpp:920
> +    /**
> +     * WebKitWebSettings: enable-tiled-backing-store:
> +     *
> +     * Enable or disable tiled backing store

This documentation should be expanded greatly. What is the tiled backing store and why, as an embedder, would I want to turn it on?

> WebKit/gtk/webkit/webkitwebview.cpp:721
> +        if (frame->tiledBackingStore())
> +                renderFromTiledBackingStore(frame, &context, clipRect);
> +        else
> +#endif
>              frame->view()->paint(&context, rect);

The indentation seems incorrect here.

[Joone Hur]

(In reply to comment #18)
> (From update of attachment 77469 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77469&action=review
> 
> Looks good. There are a few issues that I can see though.
> 
> > WebCore/page/Frame.cpp:865
> > -    unsigned size = paintedArea.size();
> > +    int size = paintedArea.size();
> 
> Is this change related?

No, I just fixed a compiler warning:
../../WebCore/page/Frame.cpp:867: warning: comparison between signed and unsigned integer expressions

There are more compiler warnings in tiled backing store code, so it would be better to file another bug to fix them.

> > WebCore/platform/graphics/gtk/TileGtk.cpp:7
> > +   Redistribution and use in source and binary forms, with or without
> > +   modification, are permitted provided that the following conditions
> > +   are met:
> 
> Nit: do you mind adding the asterisks here so that this license block looks like others in this directory?

Of course not.
 
> > WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:437
> > +    notImplemented();
> 
> Is this really notImplemented() or is it an empty implementation? There's a subtle difference.

QWebKit uses this method to emit the scrollRequested signal. http://trac.webkit.org/changeset/71352
I don't see any case that this callback is called during scrolling in WebKtGtk+, so we need more investigation why this callback is required.
> 
> > WebKit/gtk/webkit/webkitwebsettings.cpp:920
> > +    /**
> > +     * WebKitWebSettings: enable-tiled-backing-store:
> > +     *
> > +     * Enable or disable tiled backing store
> 
> This documentation should be expanded greatly. What is the tiled backing store and why, as an embedder, would I want to turn it on?

Okay, I will add more details.
 
> > WebKit/gtk/webkit/webkitwebview.cpp:721
> > +        if (frame->tiledBackingStore())
> > +                renderFromTiledBackingStore(frame, &context, clipRect);
> > +        else
> > +#endif
> >              frame->view()->paint(&context, rect);
> 
> The indentation seems incorrect here.

I'll fix it.

Thank you for the review.

[Joone Hur]

Created attachment 77979 [details]
Proposed Patch

WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:437
> > +    notImplemented();
> 
> Is this really notImplemented() or is it an empty implementation? There's a subtle difference.

This callback can be called when m_deletegatesScrolling of the ScrollView is true, but this value is false by default.
m_deletegatesScrolling can be set to true value in QWebKit, so we don't need to implement this function in WebKitGtk+.

[Joone Hur]

Created attachment 78364 [details]
Fixed merge conflicts due to WeCore move

[Joone Hur]

Created attachment 78366 [details]
Fixed merge conflicts due to WebCore move

[zaheer]

(In reply to comment #22)
> Created an attachment (id=78366) [details]
> Fixed merge conflicts due to WebCore move

hi Joone,
would you mind adding my name ([e-mail]) to the author list for this change as i was involved in coding most part of it along with julien. I didnt have the relevant legal account (codeaurora) before.

If you wish i can submit the modified changeset.

Thanks,
Zaheer

[Joone Hur]

(In reply to comment #23)
> (In reply to comment #22)
> > Created an attachment (id=78366) [details] [details]
> > Fixed merge conflicts due to WebCore move
> 
> hi Joone,
> would you mind adding my name ([e-mail]) to the author list for this change as i was involved in coding most part of it along with julien. I didnt have the relevant legal account (codeaurora) before.
> 
> If you wish i can submit the modified changeset.
> 
> Thanks,
> Zaheer

Hi Zaheer,

Sure, you can submit the modified changeset with your name.

:-)

[Alejandro G. Castro]

Comment on attachment 78366 [details]
Fixed merge conflicts due to WebCore move

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

I would try to remove the trailing whitespaces in the files.

I've tested it and seen some visual issues: the left upper tile is not cleaned in some situation when loading a new page (search in google, open a webpage), scrolling has some issues in slashdot with the floating bar in the bottom.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:160
> +void Tile::swapBackBufferToFront()
> +{
> +}

Why swapping the buffers is not implemented?

[Joone Hur]

(In reply to comment #25)
> (From update of attachment 78366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78366&action=review
> 
> I would try to remove the trailing whitespaces in the files.
> 
> I've tested it and seen some visual issues: the left upper tile is not cleaned in some situation when loading a new page (search in google, open a webpage), scrolling has some issues in slashdot with the floating bar in the bottom.

Yes, the left or right side of tiles are not updated immediately in some case. 
Slashdot site seems to use CSS fixed positioning, but it doesn't work with the tiled backing store. I already commented this issue in the API document.

> > Source/WebCore/platform/graphics/gtk/TileGtk.cpp:160
> > +void Tile::swapBackBufferToFront()
> > +{
> > +}
> 
> Why swapping the buffers is not implemented?

Currently, the back buffer doesn't need to be implemented. In the case of QWebKit, it has the front & back buffer, but all buffers are updated synchronously at the same time so there is no real need to have separate back and front buffers.

[Joone Hur]

Created attachment 78783 [details]
update issue example (video)

this video shows the update issue while loading a new page.

[Joone Hur]

Created attachment 78786 [details]
Fixed the visual issue

Added code to invalidate the tiles in ChromeClient::invalidateContentsAndWindow.

[Joone Hur]

Created attachment 79136 [details]
Fixed merge conflict after source directory movement

[Martin Robinson]

Comment on attachment 79136 [details]
Fixed merge conflict after source directory movement

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

Looks good. r- because we can no longer use GdkPixmap.

> Source/WebCore/platform/graphics/Tile.h:44
> +typedef struct _GdkRegion ScreenRegion;

If you instead used

typedef GOwnPtr<GdkRegion> ScreenRegion;
and
typedef OwnPtr<cairo_region_t> ScreenRegion;

and then implemented the necessary specializations in RefPtrCairo.h and GOwnPtrGtk.h, you might be able to avoid manually calling *_region_destroy in TileGtk.cpp.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:46
>  {
> +    setTileSize(m_tileSize);
>  }

You don't mention in the ChangeLog why this is necessary.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:38
> +#include <GOwnPtr.h>
> +#include <GRefPtr.h>

It's safer to include "GOwnPtrGtk.h" and "GRefPtrGtk.h" here. I think we should enforce this for all GTK+ specific code.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:47
> +static const unsigned checkerColor1 = 0xff555555;
> +static const unsigned checkerColor2 = 0xffaaaaaa;

Foreground and background, instead of 1 and 2 maybe? I'm not sure here.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:54
> +    static GdkPixmap* pixmap = 0;
> +
> +    if (pixmap) 
> +        return pixmap;

Super nit: I'd rather see this bit in one clump and then the work below separated out.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:55
> +    pixmap = gdk_pixmap_new(0, checkerSize, checkerSize, gdk_visual_get_depth(gdk_visual_get_system()));

I'm pretty sure that gdk_pixmap_new has been removed from GDK 3.x. You should just use a Cairo surface here.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:57
> +    GraphicsContext context(cr);

I'd rather see this done manually with Cairo than bringing GraphicsContext into the mix.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:179
> +    cairo_t* cr = context->platformContext();
> +    gdk_cairo_set_source_pixmap(cr, checkeredPixmap(), 0, 0);

If you wish you use a GdkPixmap here you would want to do cairo_surface_create_similar, I believe.

> Source/WebKit/gtk/ChangeLog:28
> +        (renderFromTiledBackingStore): Added to calculate the document dirty rect taking the scroll offsets and invoke the tile paint api.
> +        (paintWebView): Use the tile store if enabled when painting.
> +        (webkit_web_view_update_settings): Exposed a new property enable-tiled-backing-store on the webkitwebsettings object.
> +        (webkit_web_view_settings_notify):
> +
>  2011-01-16  Adam Barth  <[e-mail]>

Please break ChangeLog lines similarly to other entries in the file.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:459
> +#if ENABLE(TILED_BACKING_STORE)
> +void ChromeClient::delegatedScrollRequested(const IntSize& delta)
> +{
> +}
> +#endif

Please put this with ChromeClient::visibleRectForTiledBackingStore() to reduce the number of #ifdefs.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:155
> +        virtual WebCore::IntRect visibleRectForTiledBackingStore() const;

Is it necessary to specify WebCore:: here?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:926
> +     * Whether to enable the the tiled backing store feature.
> +     * With the tiled backing store enabled, the content of the web page are cached to bitmap tiles.
> +     * These bitmap tiles are created and deleted on-demand as the viewport moves over the web page.
> +     * Enabling tiling can significantly speed up painting heavy operations like scrolling, but it 

Very nice! Thank you.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:936
> +                                                         _("Whether Tiled Backing Store should be enabled"),

Should Tiled Backing Store be capitalized?

[Joone Hur]

(In reply to comment #30)
> (From update of attachment 79136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79136&action=review

Thanks, Martin for the review.
I updated the patch according to Martin's suggestions except the following:

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:47
> > +static const unsigned checkerColor1 = 0xff555555;
> > +static const unsigned checkerColor2 = 0xffaaaaaa; 
> Foreground and background, instead of 1 and 2 maybe? I'm not sure here.

I'm not sure which color is background color.

> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:155
> > +        virtual WebCore::IntRect visibleRectForTiledBackingStore() const;
> 
> Is it necessary to specify WebCore:: here?

Yes, using namespace statement is not allowed in header files
 
> 
> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:936
> > +                                                         _("Whether Tiled Backing Store should be enabled"),
> 
> Should Tiled Backing Store be capitalized?

Other setting names are also capitalized.

[Joone Hur]

Created attachment 80038 [details]
Updated patch with Martin's changes and support for GTK+3.0

[Martin Robinson]

Comment on attachment 80038 [details]
Updated patch with Martin's changes and support for GTK+3.0

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

Looking good! I think this should go one more round, but it's very close. We'll need another r+ before it can land though (since it includes an API change).

> Source/WebCore/ChangeLog:1
> [phone]  Julien Chaffraix <[e-mail]>, Zaheer Ahmad <[e-mail]>, and Joone Hur <[e-mail]>

I think usually this is FirstName LastName<space><space><email>. I'm not sure if this is necessary, but I wouldn't be surprised if the tools don't handle this format properly.

> Source/WebCore/ChangeLog:8
> +        This is the GTK port of tiling mechanism implemented in 35146

This sentence is missing a period. Also, it might be better to rephrase this. "Add the GTK+ port of the tiling..."

> Source/WebCore/ChangeLog:13
> +        (WebCore::TiledBackingStore::TiledBackingStore): Call setTileSize to apply the default tile size.

I'm still not sure why this is necessary for GTK+ and not for other ports. This file is platform-independent. What is different to require this?

> Source/WebCore/platform/graphics/Tile.h:90
> +    ScreenRegion m_dirtyRegion;

Since you only use ScreenRegion in one place, I think it makes sense to pull the #ifdefs down here like so:

#if PLATFORM(GTK)
    GOwnPtr<GdkRegion> m_dirtyRegion;
#ifdef GTK_API_VERSION_2
    GOwnPtr<GdkRegion> m_dirtyRegion;
#else
    OwnPtr<cairo_region_t> m_dirtyRegion;
#endif
#endif

> Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:56
> +template <> void deleteOwnedPtr<cairo_region_t>(cairo_region_t* ptr)
> +{
> +    if (ptr)
> +        cairo_region_destroy(ptr);
> +}

Since cairo_region_t is new in 1.10 this needs to be guarded by #if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 10, 0). You may need to provide dummy struct implementation to fulfill the typedef for older Cairo versions. Maybe not, because the template is never instantiated for old versions. I can test that if you need me to.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:52
> +static cairo_surface_t* checkeredPixmap()

You should change the name of this method now that it no longer returns a pixmap.

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:149
> +#ifdef GTK_API_VERSION_2
> +    int rectCount;
> +    GOwnPtr<GdkRectangle> rects;
> +    gdk_region_get_rectangles(m_dirtyRegion.get(), &rects.outPtr(), &rectCount);
> +    m_dirtyRegion.clear();
> +    m_dirtyRegion.set(gdk_region_new());
> +#else
> +    int rectCount = cairo_region_num_rectangles(m_dirtyRegion.get());
> +    GOwnPtr<GdkRectangle> rects(g_new(GdkRectangle, rectCount));
> +    for (int i = 0; i < rectCount; i++)
> +        cairo_region_get_rectangle(m_dirtyRegion.get(), i, rects.get()+i);
> +    m_dirtyRegion.clear();
> +    m_dirtyRegion.set(cairo_region_create());
> +#endif
> +
> +    RefPtr<cairo_t> cr = adoptRef(cairo_create(m_buffer.get()));
> +    GraphicsContext context(cr.get());
> +    context.translate(-m_rect.x(), -m_rect.y());

I think it would be better to use a vector of IntRects here. See how it's done in webkit_web_view_expose_event and webkit_web_view_draw. We probably want to isolate the code in the #ifdefs into a static function that just returns a Vector of IntRects.

[Collabora GTK+ EWS bot]

FWIW, the API addition looks OK to me. Sorry if that was discusssed earlier, but since we're adding a runtime option, do we get any benefit in making this a compile-time option? Shouldn't we just enable it in build time?

[Gustavo Noronha (kov)]

Oops, the post bellow is mine, I just forgot I was logged in with the new EWS bot login (doh):

(In reply to comment #34)
> FWIW, the API addition looks OK to me. Sorry if that was discusssed earlier, but since we're adding a runtime option, do we get any benefit in making this a compile-time option? Shouldn't we just enable it in build time?

[Martin Robinson]

(In reply to comment #34)
> FWIW, the API addition looks OK to me. Sorry if that was discusssed earlier, but since we're adding a runtime option, do we get any benefit in making this a compile-time option? Shouldn't we just enable it in build time?

I think it makes sense to keep the build time option. This will keep our build options in sync with the ENABLE_FOO flags. If other ports have this feature disabled at compile time, they won't detect any performance or layout regressions in their testing infrastructure. We should probably only remove the build option if ports generally have a feature turned on by default.

[Joone Hur]

(In reply to comment #33)
> (From update of attachment 80038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80038&action=review
>
> Looking good! I think this should go one more round, but it's very close. We'll need another r+ before it can land though (since it includes an API change).

Thanks, Martin for the review. I am applying your suggestions to the patch.
>
> > Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:56
> > +template <> void deleteOwnedPtr<cairo_region_t>(cairo_region_t* ptr)
> > +{
> > +    if (ptr)
> > +        cairo_region_destroy(ptr);
> > +}
>
> Since cairo_region_t is new in 1.10 this needs to be guarded by #if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 10, 0). You may need to provide dummy struct implementation to fulfill the typedef for older Cairo versions. Maybe not, because the template is never instantiated for old versions. I can test that if you need me to.
> 

Right, people can try to build WebKitGtk+ with old cairo and Gtk+2, so it would be great to test this patch with them.

How about checking GTK+ version instead of Cairo version? 
Because cairo_region_t is used for Gtk+3, which is dependent on Cairo 1.10.
http://library.gnome.org/devel/gtk/2.99/ch25s02.html

> > Source/WebCore/platform/graphics/gtk/TileGtk.cpp:52
> > +static cairo_surface_t* checkeredPixmap()
>
> You should change the name of this method now that it no longer returns a pixmap.
>
> > Source/WebCore/platform/graphics/gtk/TileGtk.cpp:149
> > +#ifdef GTK_API_VERSION_2
> > +    int rectCount;
> > +    GOwnPtr<GdkRectangle> rects;
> > +    gdk_region_get_rectangles(m_dirtyRegion.get(), &rects.outPtr(), &rectCount);
> > +    m_dirtyRegion.clear();
> > +    m_dirtyRegion.set(gdk_region_new());
> > +#else
> > +    int rectCount = cairo_region_num_rectangles(m_dirtyRegion.get());
> > +    GOwnPtr<GdkRectangle> rects(g_new(GdkRectangle, rectCount));
> > +    for (int i = 0; i < rectCount; i++)
> > +        cairo_region_get_rectangle(m_dirtyRegion.get(), i, rects.get()+i);
> > +    m_dirtyRegion.clear();
> > +    m_dirtyRegion.set(cairo_region_create());
> > +#endif
> > +
> > +    RefPtr<cairo_t> cr = adoptRef(cairo_create(m_buffer.get()));
> > +    GraphicsContext context(cr.get());
> > +    context.translate(-m_rect.x(), -m_rect.y());
>
> I think it would be better to use a vector of IntRects here. See how it's done in webkit_web_view_expose_event and webkit_web_view_draw. We probably want to isolate the code in the #ifdefs into a static function that just returns a Vector of IntRects.

I am updating the patch as you suggested. This makes the Tile::updateBackBuffer() method simple, but I'm not sure if isolating the code is efficient.

[Joone Hur]

(In reply to comment #33)
> (From update of attachment 80038 [details])

> > Source/WebCore/ChangeLog:13
> > +        (WebCore::TiledBackingStore::TiledBackingStore): Call setTileSize to apply the default tile size.
> 
> I'm still not sure why this is necessary for GTK+ and not for other ports. This file is platform-independent. What is different to require this?
> 

QtWebKit calls the setTileSize() method from the port layer to set a user defined tile size, but WebKitGtk+ doesn't provide such functionality yet.

By the way, the default tile size is already defined in TiledBackingStore.cpp, but nobody uses it. So, WebKitGkt+ takes the default tile size to set the tile size, this seems not to be a burden for QtWebKit.

[Joone Hur]

Created attachment 80561 [details]
Proposed Patch

[Kenneth Rohde Christiansen]

Comment on attachment 80561 [details]
Proposed Patch

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

> Source/WebCore/platform/graphics/gtk/TileGtk.cpp:75
> +static cairo_surface_t* checkeredSurface()
> +{
> +    static cairo_surface_t* surface = 0;
> +    if (surface)
> +        return surface;
> +
> +    surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, checkerSize, checkerSize);
> +    RefPtr<cairo_t> cr = adoptRef(cairo_create(surface));
> + 
> +    for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +        bool alternate = y % checkerSize;
> +        for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +            if (alternate)
> +                setSourceRGBAFromColor(cr.get(), checkerColor1);
> +            else
> +                setSourceRGBAFromColor(cr.get(), checkerColor2);
> +            cairo_rectangle(cr.get(), x, y, checkerSize / 2, checkerSize / 2); 
> +            cairo_fill(cr.get());
> +            alternate = !alternate;
> +        }
> +    }
> +
> +    return surface;
> +}

I believe that we should let the embedding app paint the checker boards. Especially because sometimes you want the checkerboards area to be bigger than the contents size (think on rotation, look how it is done on the iPad - try using google.com).

[Joone Hur]

(In reply to comment #40)
> (From update of attachment 80561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80561&action=review
>
> 
> I believe that we should let the embedding app paint the checker boards. Especially because sometimes you want the checkerboards area to be bigger than the contents size (think on rotation, look how it is done on the iPad - try using google.com).

Do you mean that we need to expose an API to allow the embedding apps to change the tile size?

[Gustavo Noronha (kov)]

(In reply to comment #36)
> I think it makes sense to keep the build time option. This will keep our build options in sync with the ENABLE_FOO flags. If other ports have this feature disabled at compile time, they won't detect any performance or layout regressions in their testing infrastructure. We should probably only remove the build option if ports generally have a feature turned on by default.

Works for me =) I suggest we improve the documentation for the enable-tiled-backing-store property to say it's going to be ignored if the build time option is disabled, then.

[Joone Hur]

Created attachment 81801 [details]
TileCairo_Patch

TileGtk has been renamed to TileCairo and Gtk+ dependency has been removed in TileCairo, but if CAIRO_VERSION is older than 1.10.0, the exiting gtk+ code would be used. 

This change allows the tiled backing store feature to be shared among other ports like WinCairo, Efl, and Clutter.

[Martin Robinson]

Comment on attachment 81801 [details]
TileCairo_Patch

Using cairo_rectangle_int_t unconditionally means that this will break for any version of cairo < 1.10. That seems like it could be an issue. Perhaps that's the direction we should go as a project, but I think it should be a consious decision and not a side-effect.

[Gustavo Noronha (kov)]

Comment on attachment 81801 [details]
TileCairo_Patch

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

I agree with Martin. My personal opinion on the matter is that upgrading cairo is not difficult, I think it's safe to assume that any distribution building GTK+3 and WebKitGTK+ 1.3.x is going to have that version available, and since it would make our lives less complex, I like it. FWIW, I've gone out of my way to add cairo_rectangle_int_t support to WebKitClutter even with cairo versions < 1.10 because I didn't want to ask people to upgrade right at that point in time, but the plan is to require 1.10.

> Source/WebCore/platform/graphics/IntRect.h:53
>  #endif

PLATFORM(CAIRO) is windows cairo? I think we should have USE(CAIRO) instead?

[Martin Robinson]

(In reply to comment #45)
> (From update of attachment 81801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81801&action=review
> 
>  FWIW, I've gone out of my way to add cairo_rectangle_int_t support to WebKitClutter even with cairo versions < 1.10 because I didn't want to ask people to upgrade right at that point in time, but the plan is to require 1.10.

If this is an option for us, I much prefer it to adding one more dependency that no stable distribution ships. :/

[Gustavo Noronha (kov)]

(In reply to comment #46)
> If this is an option for us, I much prefer it to adding one more dependency that no stable distribution ships. :/

Here's the hack:

http://gitorious.org/webkit-clutter/webkit-clutter/blobs/master/Source/WebCore/platform/graphics/IntRect.h#line57

[Joone Hur]

(In reply to comment #45)
> (From update of attachment 81801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81801&action=review
> 
> I agree with Martin. My personal opinion on the matter is that upgrading cairo is not difficult, I think it's safe to assume that any distribution building GTK+3 and WebKitGTK+ 1.3.x is going to have that version available, and since it would make our lives less complex, I like it. FWIW, I've gone out of my way to add cairo_rectangle_int_t support to WebKitClutter even with cairo versions < 1.10 because I didn't want to ask people to upgrade right at that point in time, but the plan is to require 1.10.
> 
> > Source/WebCore/platform/graphics/IntRect.h:53
> >  #endif
> 
> PLATFORM(CAIRO) is windows cairo? I think we should have USE(CAIRO) instead?

No, there is WIN_CAIRO preprocessor definition.
if WIN_CARIO is defined, WTF_PLATFORM_CAIRO is also defined.

[Martin Robinson]

Comment on attachment 81801 [details]
TileCairo_Patch

Please try to find a way not to depend on 1.10.0.  Alternatively, perhaps we can just have the tiled backing store depend on Cairo 1.10.0 (and properly work it into configure.ac). If we do decide to make the tiled backing store depend on Cairo 1.10.0 we need to remove the check for the Cairo version in the code specific to the tiled backing store.

[Gustavo Noronha (kov)]

Comment on attachment 81801 [details]
TileCairo_Patch

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

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:389
> +#if ENABLE(TILED_BACKING_STORE)
> +        Frame* frame = core(m_webView)->mainFrame();
> +        if (frame && frame->tiledBackingStore())
> +            frame->tiledBackingStore()->invalidate(updateRect);
> +#endif

While trying integrating this work on the clutter port I found out that this is wrong and makes babies cry. This is precisly the method the backing store uses to let WebKit know it should update the view from the backing store contents when it's done painting, so invalidating the backing store here causes us to enter an infinite loop. It does not lock up in a tight loop simply because the backing store uses a timer to process updates, and delays them for a few ms, but it's easy to see the problem given the CPU usage being high and responsiveness decreasing over time.

[Joone Hur]

(In reply to comment #50)
> (From update of attachment 81801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81801&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:389
> > +#if ENABLE(TILED_BACKING_STORE)
> > +        Frame* frame = core(m_webView)->mainFrame();
> > +        if (frame && frame->tiledBackingStore())
> > +            frame->tiledBackingStore()->invalidate(updateRect);
> > +#endif
> 
> While trying integrating this work on the clutter port I found out that this is wrong and makes babies cry. This is precisly the method the backing store uses to let WebKit know it should update the view from the backing store contents when it's done painting, so invalidating the backing store here causes us to enter an infinite loop. It does not lock up in a tight loop simply because the backing store uses a timer to process updates, and delays them for a few ms, but it's easy to see the problem given the CPU usage being high and responsiveness decreasing over time.

I added this code to fix the following issue:

https://bugs.webkit.org/show_bug.cgi?id=45423#c25
> I've tested it and seen some visual issues: the left upper tile is not cleaned in some situation when loading a new page (search in google, open a webpage), scrolling has some issues in slashdot with the floating bar in the bottom.

However, it has the critical performance issue as Kov has commented, so I will remove it. Thanks Kov for reporting this issue.

Anyway, the above visual issues seems to be appeared because of erasing the content during loading a new page on WebKitGtk+. On the other hand, other ports don't erase the content before painting a new page. 
Otherwise, we have to stop painting the backing store when loading a new page.

[Joone Hur]

(In reply to comment #49)
> (From update of attachment 81801 [details])
> Please try to find a way not to depend on 1.10.0.  Alternatively, perhaps we can just have the tiled backing store depend on Cairo 1.10.0 (and properly work it into configure.ac). If we do decide to make the tiled backing store depend on Cairo 1.10.0 we need to remove the check for the Cairo version in the code specific to the tiled backing store.

That's a good idea, I will update configure.ac.

[Joone Hur]

Created attachment 85642 [details]
TileCairo2

I have updated this patch with the changes:
- Check Cairo 1.10.0 when configure instead of checking it in the code. 
- Remove the code for invalidating the tiled backing store in ChromeClient::invalidateContentsAndWindow

Thank you.

[WebKit Review Bot]

Attachment 85642 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Source/WebCore/platform/graphics/cairo/TileCairo.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 85656 [details]
TileCairo3

Fixed style error

[Gustavo Noronha (kov)]

Comment on attachment 85656 [details]
TileCairo3

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/1192ac5b3f92bf9c1e16c0e457d26ede3a4f5787

> 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/73db7b5a8b64705bc7669d9ff757bdd38ad18c97. 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/f78e8bb091f41e20f40533db0ee33348759576d2

> 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.

[Michael Trimarchi]

Created attachment 90686 [details]
rebase of tiled_cairo3 patch on the latest trunk.

Hi,

I have tried to rebase the tiled patch on the new trunk. I'm looking at the code to understand how to implement the
missed part. Right now there is not work as expected. There are repaint bugs and performance issue compared with
the Qt in an arm based machine (arm926 400Mhz core)

[Joone Hur]

(In reply to comment #57)
> Created an attachment (id=90686) [details]
> rebase of tiled_cairo3 patch on the latest trunk.
> 
> Hi,
> 
> I have tried to rebase the tiled patch on the new trunk. I'm looking at the code to understand how to implement the
> missed part. Right now there is not work as expected. There are repaint bugs and performance issue compared with
> the Qt in an arm based machine (arm926 400Mhz core)

Thanks for the patch, I am preparing a new patch to fix the issues.

[Michael Trimarchi]

(In reply to comment #56)
> (From update of attachment 85656 [details])
> 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/1192ac5b3f92bf9c1e16c0e457d26ede3a4f5787
> 
> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:765
> > +    return frame->view()->visibleContentRect();
> 

I fix in this way 
Frame* frame = core(m_webView)->mainFrame();
    IntSize ofs;

    if (!frame || !frame->view())
        return IntRect();

    ofs = frame->view()->scrollOffset();
    IntSize size = frame->view()->frameRect().size();

    return IntRect(ofs.width(), ofs.height(), size.width(), size.height());

I think that qt code do the same.

> 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/73db7b5a8b64705bc7669d9ff757bdd38ad18c97. 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/f78e8bb091f41e20f40533db0ee33348759576d2
> 
> > 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.

What is here the difference with the qt implementation?

[Michael Trimarchi]

Created attachment 92242 [details]
Remove redraw problem in thin frame

Hi all,

I have done some change above the previus patch. This is just an help to people that are working on rewrite a new patch. The main differences from
the previous patch are:

* take the scroll position from the scrollbar (dirty hack)
* change the paint using cairo
* invalidate region in the scroll ChomeClientGtk.cpp

The scrolling present artifacts but the final result now it's ok. 
I don't know if I'm using in the correct way the tiled storage

Michael

[Joone Hur]

Created attachment 92728 [details]
Programmatic scrolling support

I applied the programmatic scrolling to this patch like webkit-clutter, so scrolling doesn’t work in GtkLauncher. If you want to test this patch, you need to update GtkLauncher with the next patch.

[WebKit Review Bot]

Attachment 92728 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebframe.h:180:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:181:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:184:  Extra space between gint and *x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:185:  Extra space between gint and *y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:188:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:189:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebCore/platform/ScrollView.h:193:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebframe.cpp:1016:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/gtk/webkit/webkitwebframe.cpp:1043:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebframe.cpp:1083:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebframe.cpp:1113:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 11 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 92729 [details]
GtkLauncher for programmatic scrolling

[Joone Hur]

Created attachment 92731 [details]
Programmatic scrolling support2

Fixed the style error.

[WebKit Review Bot]

Attachment 92731 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebframe.h:180:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:181:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:184:  Extra space between gint and *x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:185:  Extra space between gint and *y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:188:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:189:  Extra space between gint and y  [whitespace/declaration] [3]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Trimarchi]

(In reply to comment #63)
> Created an attachment (id=92729) [details]
> GtkLauncher for programmatic scrolling

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

> Tools/GtkLauncher/main.c:132
> +       webkit_web_frame_scroll(webFrame, 0, step_y);

You need to put a return TRUE here

> Tools/GtkLauncher/main.c:135
> +    return TRUE;

Change this to return FALSE because you need to know if you are over something

[Joone Hur]

(In reply to comment #66)
> (In reply to comment #63)
> > Created an attachment (id=92729) [details] [details]
> > GtkLauncher for programmatic scrolling
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=92729&action=review
> 
> > Tools/GtkLauncher/main.c:132
> > +       webkit_web_frame_scroll(webFrame, 0, step_y);
> 
> You need to put a return TRUE here
> 
> > Tools/GtkLauncher/main.c:135
> > +    return TRUE;
> 
> Change this to return FALSE because you need to know if you are over something

The tiled backing store feature is required for touch devices like the iPhone, so I think that we don't need to send all mouse move events to WebCore.

[Gustavo Noronha (kov)]

> > Change this to return FALSE because you need to know if you are over something
> 
> The tiled backing store feature is required for touch devices like the iPhone, so I think that we don't need to send all mouse move events to WebCore.

I disagree, but as a simple test it doesn't hurt =)

[Gustavo Noronha (kov)]

Comment on attachment 92731 [details]
Programmatic scrolling support2

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.

[Kenneth Rohde Christiansen]

> 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?

Well doing that will make other things break, line innerWidth, scrollBy etc. which is why we are using teh actualVisibleContentsRect on our branch on gitorious, similarily with what iOS does.

[Gustavo Noronha (kov)]

(In reply to comment #70)
> 
> > 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?
> 
> Well doing that will make other things break, line innerWidth, scrollBy etc. which is why we are using teh actualVisibleContentsRect on our branch on gitorious, similarily with what iOS does.

Yep, we did the same on the clutter port, and it seems to be the way to go for webkitgtk+ as well.

[Kenneth Rohde Christiansen]

(In reply to comment #71)
> (In reply to comment #70)
> > 
> > > 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?
> > 
> > Well doing that will make other things break, line innerWidth, scrollBy etc. which is why we are using teh actualVisibleContentsRect on our branch on gitorious, similarily with what iOS does.
> 
> Yep, we did the same on the clutter port, and it seems to be the way to go for webkitgtk+ as well.

Feel free to upstream patches from our branch regarding this (it will take some time before I get around to that myself). Mostly look for patches by me or Zalan. Also, please cc me to any such bug reports :-)

[Joone Hur]

Thanks, Kov and Kenneth for the review.
I am updating the patch according to your suggestions and making a separate patch for these cairo_rectangle_int_t changes.

[Joone Hur]

Created attachment 94226 [details]
Programmatic scrolling support3

This patch depends on another patch:
https://bugs.webkit.org/show_bug.cgi?id=60687

[WebKit Review Bot]

Attachment 94226 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebframe.cpp:1006:  Declaration has space between type name and * in WebKitWebView *view  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.cpp:1058:  Declaration has space between type name and * in WebKitWebView *view  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:180:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:181:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:184:  Extra space between gint and *x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:185:  Extra space between gint and *y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:189:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:190:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebview.cpp:647:  Declaration has space between type name and * in WebKitWebView *view  [whitespace/declaration] [3]
Total errors found: 9 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joone Hur]

Created attachment 97953 [details]
rebased on the trunk

I rebased this patch on the trunk because #60697 landed.

[WebKit Review Bot]

Attachment 97953 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebframe.h:188:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:189:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:192:  Extra space between gint and *x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:193:  Extra space between gint and *y  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:197:  Extra space between gint and x  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebframe.h:198:  Extra space between gint and y  [whitespace/declaration] [3]
Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1416:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 7 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 97953 [details]
rebased on the trunk

Attachment 97953 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8977949

[Tomasz Morawski]

(In reply to comment #78)
> (From update of attachment 97953 [details])
> Attachment 97953 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/8977949
Build error due to that the efl buildbot server doesn't has the last release version of cairo library.

[Tomasz Morawski]

Hello,
I am working on EFL port of Tiled Backing Store that reuses your patch in Cairo 
related parts. I am preparing a patch that introduces TileCairo and 
TiledBackingStoreBackendCairo code. This classes are used in current version
of WebCore's backing store implementation. If you don't have anything against I 
would like to open a new bug and show there the patch. I would like also include
you and me as authors of new patch.

[Joone Hur]

(In reply to comment #80)
> Hello,
> I am working on EFL port of Tiled Backing Store that reuses your patch in Cairo 
> related parts. I am preparing a patch that introduces TileCairo and 
> TiledBackingStoreBackendCairo code. This classes are used in current version
> of WebCore's backing store implementation. If you don't have anything against I 
> would like to open a new bug and show there the patch. I would like also include
> you and me as authors of new patch.

Hi Tomasz,

You can include the TileCairo patch in your patch for the EFL port.
When your patch is landed, I will apply it to the Gtk+ port.

Thank you.

[Eric Seidel (no email)]

This patch looks abandoned.  It's very difficult to work with bugs once they go epic like this.  I would suggest closing this one, and opening a new one if you can't get this patch reviewed soon.

[Joone Hur]

Comment on attachment 97953 [details]
rebased on the trunk

The part of this patch already landed: https://bugs.webkit.org/show_bug.cgi?id=71350
So I will close this bug and create a new one.