Created attachment 49067[details]
Preliminary patch
This patch adds WebCore level tiled backing store. Most of the implementation is in Qt specific file atm but parts of it could be generalized, depending on needs of other graphics systems. TiledBackingStore interface is platform independent.

> > +void Frame::tiledBackingStorePaintBegin()
> > +{
> > + if (!m_view)
> > + return;
> > + m_view->layoutIfNeededRecursive();
> > + m_view->flushDeferredRepaints();
> > +}
>
> I don't like these names very much, they don't seem to be only dealing with the
> tiled backing store.
>
> > +void Frame::tiledBackingStorePaintEnd(const Vector<IntRect>& paintedArea)
>
> So when you are finished painting, you have to inform where you painted? Maybe
> the API should reflect that?
>
>
> > +IntRect Frame::tiledBackingStoreContentRect()
>
> IS there a difference between the contents rect and the actual rect of the
> backing store?
These methods are part of the abstract TiledBackingStoreClient interface and are called by the TiledBackingStore. They need to be implemented by any class that wants to be backed by the tiled backing store. This is needed for proper layering as TiledBackingStore is a platform class can't know about FrameView. In this context I don't understand your comments.
(for example GraphicsLayers for accelerated compositing could use tiling too far large layers).
> > + void viewportChanged(const IntRect& viewportRect);
>
> Frame uses setFrameRect, maybe setViewportRect?
Yeah.
>
> > +
> > + float scale() { return m_scale; }
> > + void setScale(float);
>
> scale, zoom? so this actually scales the widgets?
Concept of zooming is already taken in WebKit. It is about zooming page by changing the element sizes and relayouting. This is a different concept than paint time transform based zooming here.
QPainter and graphics terminology in general uses "scale" for this.
> Shouldn't you put State as a suffix instead of as a prefix? Also, State doesn't
> say much, coudl we find a better name? What is StateNormal? Let's use a better
> name
Yep.
> > + void dropOverhangingTiles();
>
> What is overhanging?
Tiles that are at least partially outside the current document size (since document got smaller).
> Why can't you use IntPoint? and maybe you should consider TilePoint?
One could argue that it is not really a point. The original reason was to have a place to hang the hash function.
> More comments to follow, but I have a meeting now!
Have fun!

Comment on attachment 49491[details]
updated patch
Ah nice, it is getting there! So what is missing before you are ready to put this up for review?
Some quick comments.
> + const IntRect rectToContents() const;
Is this a mapping function? what about mapToContents?
> +
> + // FIXME: In single threaded case, tile backbuffers could be updated asynchrnously
Spelling issue :-)
> +PassRefPtr<Tile> TiledBackingStore::tile(const Tile::Coordinate& coordinate) const
> +{
tileAt?
> +IntRect TiledBackingStore::rectToContents(const IntRect& rect) const
mapRectToContents? mapToContents?
> +bool QGraphicsWebView::tiledBackingStoreFrozen() const
Could you add documentation with \since 4.6 to these public methods?
Also, add a 'is' in front. isTiledBackingStoreFrozen()
> Index: WebKit/qt/QGVLauncher/main.cpp
QGVLauncher was removed as far as I know. You should add the code to QtLauncher instead.

Attachment 49578[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "WebKit/qt/Api/qwebsettings.cpp": this file is exempt from the style guide.
WebCore/platform/graphics/TiledBackingStoreClient.h:27: This { should be at the end of the previous line [whitespace/braces] [4]
WebCore/platform/graphics/TiledBackingStore.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/TiledBackingStore.h:28: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/TiledBackingStore.h:40: This { should be at the end of the previous line [whitespace/braces] [4]
Ignoring "WebKit/qt/Api/qwebsettings.h": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebframe.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebframe.h": this file is exempt from the style guide.
WebCore/platform/graphics/Tile.h:41: This { should be at the end of the previous line [whitespace/braces] [4]
WebCore/platform/graphics/Tile.h:49: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 6 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.

// FIXME: In single threaded case, tile back buffers could be updated asynchronously
// one by one and then swapped to front in one go. This would minimize the time spent
// blocking on tile updates.
Is there a multi-threaded case? It seems that this patch implements a single threaded tiled backingstore.

Attachment 49582[details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Ignoring "WebKit/qt/Api/qwebsettings.cpp": this file is exempt from the style guide.
WebCore/platform/graphics/TiledBackingStoreClient.h:27: This { should be at the end of the previous line [whitespace/braces] [4]
Ignoring "WebKit/qt/Api/qwebsettings.h": this file is exempt from the style guide.
WebCore/platform/graphics/TiledBackingStore.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/TiledBackingStore.h:28: Alphabetical sorting problem. [build/include_order] [4]
WebCore/platform/graphics/TiledBackingStore.h:40: This { should be at the end of the previous line [whitespace/braces] [4]
Ignoring "WebKit/qt/Api/qgraphicswebview.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebframe.cpp": this file is exempt from the style guide.
Ignoring "WebKit/qt/Api/qwebframe.h": this file is exempt from the style guide.
WebCore/platform/graphics/Tile.h:41: This { should be at the end of the previous line [whitespace/braces] [4]
WebCore/platform/graphics/Tile.h:49: Place brace on its own line for function definitions. [whitespace/braces] [4]
Total errors found: 6 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #14)
> Antti, are you sure you want to use a GPL license here? Do you have a written
> permission from Apple? I think only LGPL 2 and BSD is allowed to commit.
Sorry. I see the Library now. Forget about this comment.

> As commented elsewhere I think it makes sense to use IntPoint here and make it
> hashable. IIRC you were worried there could be confusion about the coordinate
> space, but we could make a typedef to a better name instead of creating a new
> class.
Yep. I added hashing support for IntPoint in bug 35586.
> Seems like it would be faster to just iterate through the tiles (which should
> be a lot less than the total amount of individual coordinates) and just check
> if they intersect dirtyRect? or is tileAt + hashing faster than that? :) This
> also applies to a couple other places fwiw...
This is most efficient for small updates (like say animated gifs). What you suggest would be more efficient for large updates. Anyway, this can be optimized later if needed.
> >+ if (distance > shortestDistance)
> >+ continue;
> >+ if (distance < shortestDistance) {
>
> else if?
Unnecessary after continue (and against coding style too).
> nitpick, but you can just do the for loop and requiredTileCount-- on each
> iteration.
Sure but why? It would be slower. Please limit nitpicking to stuff that makes sense. :)

(In reply to comment #18)
> Unnecessary after continue (and against coding style too).
The style guide only mentions return, FWIW, but I guess continue adheres to the spirit of it.
>
> > nitpick, but you can just do the for loop and requiredTileCount-- on each
> > iteration.
>
> Sure but why? It would be slower. Please limit nitpicking to stuff that makes
> sense. :)
The point was that protecting the for loop with an if seems redundant since the code works exactly the same way without it. I suggested subtracting one on each iteration to get rid of the variable declaration and avoid the weird looking requiredTileCount -= tilesToCreateCount; when tilesToCreateCount is 0, and surely whatever difference in performance is caused by this is totally irrelevant.
But it was just a detail, so shrug.

(In reply to comment #23)
> (From update of attachment 50032[details])
> > + int h = views[0]->horizontalScrollBar()->value();
> > + int v = views[0]->verticalScrollBar()->value();
> These return the size? or ?
They return the current scroll position, in pixels. Yeah, it is a bad API but it is documented for QAbstractScollArea.
> QGVLauncher was removed from trunk!
Seems I have a local copy in conflict.

Comment on attachment 50031[details]
WebCore patch
> +#if PLATFORM(QT)
> + QPixmap* m_buffer;
> + QPixmap* m_backBuffer;
> + QRegion* m_dirtyRegion;
> +#endif
I think it would be more efficient (less mallocs, less indirections) to store the pixmaps and regions by value. QPixmap itself holds only a pointer to the private as member variable. The same applies to QRegion.

(In reply to comment #27)
> (From update of attachment 50031[details])
>
> > +#if PLATFORM(QT)
> > + QPixmap* m_buffer;
> > + QPixmap* m_backBuffer;
> > + QRegion* m_dirtyRegion;
> > +#endif
>
> I think it would be more efficient (less mallocs, less indirections) to store
> the pixmaps and regions by value. QPixmap itself holds only a pointer to the
> private as member variable. The same applies to QRegion.
I prefer doing it this way since I can forward declare the classes and avoid Qt-specific includes. The overhead from the extra allocations and indirection is minimal.

(In reply to comment #26)
> Are the command line options really worth it? The launcher code might be
> simplier if we support the options only in the menu. Just a thought though :)
Perhaps we might want to have non-interactive functionality in QtLauncher in the future (for performance testing for example)? But I'll remove these, they don't currently even work correctly.
> Hmm, why is this needed? Should setResizesToContents automatically do this? Or
> perhaps the docs of the resizesToContents property should explain that this may
> also be a good thing to do.
Without this the view content is not pannable in resizesToContent mode. Easiest way to make it scrollable again is to enable QGraphicsView scrollbars. This is not the only or necessarily the best way to do this (see yberbrowser which implements panning by manipulating graphics items and does not enable QGraphicsView scrollbars).
> Why are the scrollbars turned _off_ if m_resizesToContents is false? Shouldn't
> it be the other way around?
When resizesToContents is false (the normal behavior) QWebGraphicsView is responsible of drawing the scrollbars itself. QGraphicsView scrollbars are redundant in this case.

From a quick look of the webcore patch:
> #include <QDebug>, #include <QThread>
I guess you can skip those, they probably come from an implementation with threads.
> #include <QImage>
Shouldn't that be <QPixmap>? QImage is not used in this patch, the tiles are stored as Pixmaps.
> #if PLATFORM(QT)
> QPixmap* m_buffer;
> QPixmap* m_backBuffer;
> QRegion* m_dirtyRegion;
> #endif
Those are implicitly shared, any particular reason to use pointers?
> typedef IntPoint Coordinate;
Why redefine IntPoint? It is regularly used as such for coordinates.
Good luck to the reviewer, this patch is not trivial :)

Comment on attachment 50032[details]
QtWebKit patch
> + WebCore::TiledBackingStore* backingStore = QWebFramePrivate::core(page()->mainFrame())->tiledBackingStore();;
A catastrophic error: Two trailing semicolons :)
The rest of the patch looks good to me, only minor things that we can fix after landing (for example docs).

(In reply to comment #29)
> > Why are the scrollbars turned _off_ if m_resizesToContents is false? Shouldn't
> > it be the other way around?
>
> When resizesToContents is false (the normal behavior) QWebGraphicsView is
> responsible of drawing the scrollbars itself. QGraphicsView scrollbars are
> redundant in this case.
Ohh, right, I confused the two scrollbars, thought you were disabling WebKit's scrollbars.

Comment on attachment 50031[details]
WebCore patch
After going through this patch I believe it is a great start and ready for initial landing.
The previous rounds of review resulted in good improvements and I think the remaining issues can be fixed incrementally on top.

I do not think this going into WebCore is a good idea. I know that all the existing clients that have a tiled backingstore do it in completely different (potentially incompatible) ways. I think it would make more sense for this to be implemented in the Qt specific WebKit layer rather than WebCore unless/until other ports decided to use the same.

(In reply to comment #36)
> I do not think this going into WebCore is a good idea. I know that all the
> existing clients that have a tiled backingstore do it in completely different
> (potentially incompatible) ways. I think it would make more sense for this to
> be implemented in the Qt specific WebKit layer rather than WebCore unless/until
> other ports decided to use the same.
To be clear, I'm not speaking up to override Simon's r+. Just making my opinion known.

(In reply to comment #36)
> I do not think this going into WebCore is a good idea. I know that all the
> existing clients that have a tiled backingstore do it in completely different
> (potentially incompatible) ways. I think it would make more sense for this to
> be implemented in the Qt specific WebKit layer rather than WebCore unless/until
> other ports decided to use the same.
Do you have technical arguments why this should not be shared code? What do you mean by "doing it completely different ways"? If you have a better way if doing this, why have you chosen not to contribute it?

(In reply to comment #38)
> (In reply to comment #36)
> > I do not think this going into WebCore is a good idea. I know that all the
> > existing clients that have a tiled backingstore do it in completely different
> > (potentially incompatible) ways. I think it would make more sense for this to
> > be implemented in the Qt specific WebKit layer rather than WebCore unless/until
> > other ports decided to use the same.
>
> Do you have technical arguments why this should not be shared code? What do you
> mean by "doing it completely different ways"? If you have a better way if doing
> this, why have you chosen not to contribute it?
FWIW, the argument is that this has been handled by WebKit clients and not WebKit code in all existing cases that I am aware of. Moreover, WebCore/platform has not ever contained a backingstore class. I don't see why this should be implemented in WebCore if only one port is using it. That's all. Kind of moot at this point though.

> FWIW, the argument is that this has been handled by WebKit clients and not
> WebKit code in all existing cases that I am aware of. Moreover,
> WebCore/platform has not ever contained a backingstore class. I don't see why
> this should be implemented in WebCore if only one port is using it. That's
> all. Kind of moot at this point though.
Some more interesting tiling features will only be possible if tiling is done inside WebKit. Examples include tiled compositing layers, tiled subframes and tile painting from a secondary thread. Putting more of the code to the Qt layer (with hooks) would of course be possible but generally the idea is to avoid that in the project. Hopefully this will be useful for others as well in the future.

(In reply to comment #45)
> > FWIW, the argument is that this has been handled by WebKit clients and not
> > WebKit code in all existing cases that I am aware of. Moreover,
> > WebCore/platform has not ever contained a backingstore class. I don't see why
> > this should be implemented in WebCore if only one port is using it. That's
> > all. Kind of moot at this point though.
>
> Some more interesting tiling features will only be possible if tiling is done
> inside WebKit. Examples include tiled compositing layers, tiled subframes and
> tile painting from a secondary thread. Putting more of the code to the Qt layer
> (with hooks) would of course be possible but generally the idea is to avoid
> that in the project. Hopefully this will be useful for others as well in the
> future.
Interesting. Can you elaborate on why tiled subframes and tile painting from another thread would require this class in WebCore vs another layer? I've thought about the former case and came to a different conclusion. I have no idea about the latter and would be very interested to hear your thoughts on why that is necessary.