Bug 45423 - [Gtk] Port tiled backing store
: [Gtk] Port tiled backing store
Status: CLOSED LATER
: WebKit
Platform
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
: 35146 50373 56464 60687
: 52206
  Show dependency treegraph
 
Reported: 2010-09-08 16:58 PST by
Modified: 2012-01-11 05:58 PST (History)


Attachments
First try: Add a compile time option and implement the logic (16.19 KB, patch)
2010-09-20 19:44 PST, Julien Chaffraix
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
updated patch (15.56 KB, patch)
2010-09-22 07:04 PST, zaheer
no flags Review Patch | Details | Formatted Diff | Diff
Updated version: Takes all the comments into account (24.43 KB, patch)
2010-09-27 20:41 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
updated patch to work with the trunk (24.29 KB, patch)
2010-12-26 17:46 PST, Joone Hur
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
Proposed Patch (24.32 KB, patch)
2011-01-05 00:40 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
Fixed merge conflicts due to WeCore move (24.43 KB, patch)
2011-01-09 19:00 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
Fixed merge conflicts due to WebCore move (24.52 KB, patch)
2011-01-09 19:11 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
update issue example (video) (991.47 KB, video/ogg)
2011-01-13 01:19 PST, Joone Hur
no flags Details
Fixed the visual issue (25.29 KB, patch)
2011-01-13 01:31 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
Fixed merge conflict after source directory movement (25.38 KB, patch)
2011-01-17 00:28 PST, Joone Hur
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
Updated patch with Martin's changes and support for GTK+3.0 (28.09 KB, patch)
2011-01-25 02:34 PST, Joone Hur
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
Proposed Patch (28.35 KB, patch)
2011-01-29 05:00 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
TileCairo_Patch (34.32 KB, patch)
2011-02-09 06:54 PST, Joone Hur
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
TileCairo2 (32.55 KB, patch)
2011-03-13 21:12 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
TileCairo3 (32.55 KB, patch)
2011-03-14 00:57 PST, Joone Hur
gns: review-
Review Patch | Details | Formatted Diff | Diff
rebase of tiled_cairo3 patch on the latest trunk. (26.31 KB, patch)
2011-04-22 03:39 PST, Michael Trimarchi
no flags Review Patch | Details | Formatted Diff | Diff
Remove redraw problem in thin frame (30.50 KB, patch)
2011-05-04 08:17 PST, Michael Trimarchi
no flags Review Patch | Details | Formatted Diff | Diff
Programmatic scrolling support (54.48 KB, patch)
2011-05-07 23:27 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
GtkLauncher for programmatic scrolling (2.46 KB, patch)
2011-05-07 23:30 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
Programmatic scrolling support2 (54.59 KB, patch)
2011-05-07 23:40 PST, Joone Hur
gns: review-
Review Patch | Details | Formatted Diff | Diff
Programmatic scrolling support3 (44.28 KB, patch)
2011-05-20 08:46 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff
rebased on the trunk (44.69 KB, patch)
2011-06-21 02:40 PST, Joone Hur
joone.hur: review-
gyuyoung.kim: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-09-08 16:58:32 PST
Port tiled backing store to Gtk.
------- Comment #1 From 2010-09-08 17:01:25 PST -------
Working on the Gtk backing store. Patch coming soon!
------- Comment #2 From 2010-09-20 19:44:45 PST -------
Created an attachment (id=68178) [details]
First try: Add a compile time option and implement the logic
------- Comment #3 From 2010-09-20 23:34:56 PST -------
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.
------- Comment #4 From 2010-09-21 09:42:52 PST -------
(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.

> 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());
------- Comment #5 From 2010-09-22 06:59:45 PST -------
I work with julien, providing the updated patch and comments.

(In reply to comment #4)
> (From update of attachment 68178 [details] [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
------- Comment #6 From 2010-09-22 07:04:19 PST -------
Created an attachment (id=68365) [details]
updated patch
------- Comment #7 From 2010-09-22 07:48:23 PST -------
(From update of attachment 68365 [details])
doesnt apply on latest
------- Comment #8 From 2010-09-22 07:51:52 PST -------
(From update of attachment 68365 [details])
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.
------- Comment #9 From 2010-09-22 07:57:35 PST -------
(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.
------- Comment #10 From 2010-09-22 19:43:22 PST -------
> 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!
------- Comment #11 From 2010-09-24 08:31:08 PST -------
(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?
------- Comment #12 From 2010-09-27 20:41:49 PST -------
Created an attachment (id=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.
------- Comment #13 From 2010-09-27 21:10:03 PST -------
(From update of attachment 69014 [details])
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.
------- Comment #14 From 2010-09-27 21:27:29 PST -------
(From update of attachment 69014 [details])
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& ?
------- Comment #15 From 2010-10-06 19:36:37 PST -------
> 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!
------- Comment #16 From 2010-10-06 19:42:25 PST -------
(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.
------- Comment #17 From 2010-12-26 17:46:42 PST -------
Created an attachment (id=77469) [details]
updated patch to work with the trunk

I cleaned up the code to work with the trunk and updated ChangeLog files.
------- Comment #18 From 2010-12-28 16:47:10 PST -------
(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?

> 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.
------- Comment #19 From 2010-12-28 19:56:29 PST -------
(In reply to comment #18)
> (From update of attachment 77469 [details] [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.
------- Comment #20 From 2011-01-05 00:40:07 PST -------
Created an attachment (id=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+.
------- Comment #21 From 2011-01-09 19:00:11 PST -------
Created an attachment (id=78364) [details]
Fixed merge conflicts due to WeCore move
------- Comment #22 From 2011-01-09 19:11:04 PST -------
Created an attachment (id=78366) [details]
Fixed merge conflicts due to WebCore move
------- Comment #23 From 2011-01-11 07:49:45 PST -------
(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 (zahmad@codeaurora.org) 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
------- Comment #24 From 2011-01-11 08:05:58 PST -------
(In reply to comment #23)
> (In reply to comment #22)
> > Created an attachment (id=78366) [details] [details] [details]
> > Fixed merge conflicts due to WebCore move
> 
> hi Joone,
> would you mind adding my name (zahmad@codeaurora.org) 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.

:-)
------- Comment #25 From 2011-01-12 05:58:11 PST -------
(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.

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

Why swapping the buffers is not implemented?
------- Comment #26 From 2011-01-12 07:57:46 PST -------
(In reply to comment #25)
> (From update of attachment 78366 [details] [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.
------- Comment #27 From 2011-01-13 01:19:58 PST -------
Created an attachment (id=78783) [details]
update issue example (video)

this video shows the update issue while loading a new page.
------- Comment #28 From 2011-01-13 01:31:43 PST -------
Created an attachment (id=78786) [details]
Fixed the visual issue

Added code to invalidate the tiles in ChromeClient::invalidateContentsAndWindow.
------- Comment #29 From 2011-01-17 00:28:41 PST -------
Created an attachment (id=79136) [details]
Fixed merge conflict after source directory movement
------- Comment #30 From 2011-01-19 15:17:13 PST -------
(From update of attachment 79136 [details])
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  <abarth@webkit.org>

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?
------- Comment #31 From 2011-01-25 02:28:25 PST -------
(In reply to comment #30)
> (From update of attachment 79136 [details] [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.
------- Comment #32 From 2011-01-25 02:34:33 PST -------
Created an attachment (id=80038) [details]
Updated patch with Martin's changes and support for GTK+3.0
------- Comment #33 From 2011-01-25 14:51:10 PST -------
(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).

> Source/WebCore/ChangeLog:1
> +2011-01-25  Julien Chaffraix <jchaffraix@codeaurora.org>, Zaheer Ahmad <zahmad@codeaurora.org>, and Joone Hur <joone.hur@collabora.co.uk>

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.
------- Comment #34 From 2011-01-26 06:38:02 PST -------
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?
------- Comment #35 From 2011-01-26 06:39:19 PST -------
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?
------- Comment #36 From 2011-01-26 09:23:26 PST -------
(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.
------- Comment #37 From 2011-01-29 00:15:12 PST -------
(In reply to comment #33)
> (From update of attachment 80038 [details] [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.
------- Comment #38 From 2011-01-29 04:52:05 PST -------
(In reply to comment #33)
> (From update of attachment 80038 [details] [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.
------- Comment #39 From 2011-01-29 05:00:34 PST -------
Created an attachment (id=80561) [details]
Proposed Patch
------- Comment #40 From 2011-01-29 14:51:39 PST -------
(From update of attachment 80561 [details])
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).
------- Comment #41 From 2011-02-06 06:32:35 PST -------
(In reply to comment #40)
> (From update of attachment 80561 [details] [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?
------- Comment #42 From 2011-02-07 06:22:34 PST -------
(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.
------- Comment #43 From 2011-02-09 06:54:56 PST -------
Created an attachment (id=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.
------- Comment #44 From 2011-02-10 09:32:26 PST -------
(From update of attachment 81801 [details])
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.
------- Comment #45 From 2011-02-10 09:48:54 PST -------
(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?
------- Comment #46 From 2011-02-10 09:52:41 PST -------
(In reply to comment #45)
> (From update of attachment 81801 [details] [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. :/
------- Comment #47 From 2011-02-10 10:04:59 PST -------
(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
------- Comment #48 From 2011-02-14 05:58:40 PST -------
(In reply to comment #45)
> (From update of attachment 81801 [details] [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.
------- Comment #49 From 2011-02-24 09:51:14 PST -------
(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.
------- Comment #50 From 2011-03-03 07:13:27 PST -------
(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.
------- Comment #51 From 2011-03-06 23:03:42 PST -------
(In reply to comment #50)
> (From update of attachment 81801 [details] [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.
------- Comment #52 From 2011-03-06 23:06:36 PST -------
(In reply to comment #49)
> (From update of attachment 81801 [details] [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.
------- Comment #53 From 2011-03-13 21:12:15 PST -------
Created an attachment (id=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.
------- Comment #54 From 2011-03-13 21:14:04 PST -------
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.
------- Comment #55 From 2011-03-14 00:57:32 PST -------
Created an attachment (id=85656) [details]
TileCairo3

Fixed style error
------- Comment #56 From 2011-03-17 04:56:36 PST -------
(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();

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.
------- Comment #57 From 2011-04-22 03:39:55 PST -------
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)
------- Comment #58 From 2011-04-22 06:13:12 PST -------
(In reply to comment #57)
> Created an attachment (id=90686) [details] [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.
------- Comment #59 From 2011-05-02 06:38:13 PST -------
(In reply to comment #56)
> (From update of attachment 85656 [details] [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?
------- Comment #60 From 2011-05-04 08:17:42 PST -------
Created an attachment (id=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
------- Comment #61 From 2011-05-07 23:27:56 PST -------
Created an attachment (id=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.
------- Comment #62 From 2011-05-07 23:30:16 PST -------
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.
------- Comment #63 From 2011-05-07 23:30:25 PST -------
Created an attachment (id=92729) [details]
GtkLauncher for programmatic scrolling
------- Comment #64 From 2011-05-07 23:40:00 PST -------
Created an attachment (id=92731) [details]
Programmatic scrolling support2

Fixed the style error.
------- Comment #65 From 2011-05-07 23:42:43 PST -------
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.
------- Comment #66 From 2011-05-08 02:34:40 PST -------
(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
------- Comment #67 From 2011-05-08 09:48:30 PST -------
(In reply to comment #66)
> (In reply to comment #63)
> > Created an attachment (id=92729) [details] [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.
------- Comment #68 From 2011-05-10 05:17:49 PST -------
> > 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 =)
------- Comment #69 From 2011-05-10 05:40:21 PST -------
(From update of attachment 92731 [details])
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.
------- Comment #70 From 2011-05-10 05:51:46 PST -------

> 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.
------- Comment #71 From 2011-05-10 10:04:16 PST -------
(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.
------- Comment #72 From 2011-05-10 14:01:26 PST -------
(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 :-)
------- Comment #73 From 2011-05-11 05:39:01 PST -------
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.
------- Comment #74 From 2011-05-20 08:46:06 PST -------
Created an attachment (id=94226) [details]
Programmatic scrolling support3

This patch depends on another patch:
https://bugs.webkit.org/show_bug.cgi?id=60687
------- Comment #75 From 2011-05-20 08:48:13 PST -------
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.
------- Comment #76 From 2011-06-21 02:40:18 PST -------
Created an attachment (id=97953) [details]
rebased on the trunk

I rebased this patch on the trunk because #60697 landed.
------- Comment #77 From 2011-06-21 02:43:38 PST -------
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.
------- Comment #78 From 2011-07-04 07:18:58 PST -------
(From update of attachment 97953 [details])
Attachment 97953 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8977949
------- Comment #79 From 2011-08-02 01:58:08 PST -------
(In reply to comment #78)
> (From update of attachment 97953 [details] [details])
> Attachment 97953 [details] [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.
------- Comment #80 From 2011-10-24 07:30:01 PST -------
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.
------- Comment #81 From 2011-10-26 07:50:57 PST -------
(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.
------- Comment #82 From 2012-01-10 18:47:10 PST -------
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.
------- Comment #83 From 2012-01-11 05:50:09 PST -------
(From update of attachment 97953 [details])
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.