[Webkit-unassigned] [Bug 45423] [Gtk] Port tiled backing store

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45423





--- Comment #10 from Julien Chaffraix <jchaffraix at webkit.org>  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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.