Bug 35146

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.

Comment on attachment 49067 [details] Preliminary patch > +void Frame::setTiledBackingStoreEnabled(bool enabled) > +{ > + if (!enabled) { > + m_tiledBackingStore.clear(); > + return; > + } > + if (m_tiledBackingStore) > + return; > + m_tiledBackingStore.set(new TiledBackingStore(this)); > +} Where are you freeing this? > +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? > + OwnPtr<TiledBackingStore> m_tiledBackingStore; > }; Ah, now I see :-) > + void viewportChanged(const IntRect& viewportRect); Frame uses setFrameRect, maybe setViewportRect? > + > + float scale() { return m_scale; } > + void setScale(float); scale, zoom? so this actually scales the widgets? > + > + void invalidate(const IntRect& dirtyRect); > + void paint(GraphicsContext*, const IntRect&); > + > + enum State { > + StateNormal, > + StateNoTileCreation > + }; 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 > + void dropOverhangingTiles(); What is overhanging? > + void dropTilesOutsideRect(const IntRect&); > + > + struct TileCoordinate { Why can't you use IntPoint? and maybe you should consider TilePoint? > + public: > + TileCoordinate(unsigned x, unsigned y) : m_x(x), m_y(y) { } > + > + PassRefPtr<Tile> tile(const TileCoordinate&) const; > + void setTile(const TileCoordinate& coordinate, PassRefPtr<Tile> tile); > + void removeTile(const TileCoordinate& coordinate); > + > + IntRect rectToDocument(const IntRect&) const; > + IntRect rectFromDocument(const IntRect&) const; > + > + IntRect contentRect() const; contentsRect ? More comments to follow, but I have a meeting now!

> > +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 49067 [details] Preliminary patch > + IntRect tileRectForCoordinate(const TileCoordinate&) const; or AtCoordinate? > + TiledBackingStore::TileCoordinate tileCoordinateForPoint(const IntPoint&) const; > + double tileDistance(const IntRect& viewport, const TiledBackingStore::TileCoordinate&); What kind of distance? to the closest point on the tile? distance to the tile from the view when the tile is outside the view? Maybe we could improve the name a bit to make this a bit more clear. > + void paintCheckerPattern(QPainter* painter, const IntRect& rect, TileCoordinate coordinate); You paint this per tile? QPainter here? that seems wrong. Shouldn't you use PaintContext? > +#if ENABLE(ENGINE_THREAD) > + Mutex m_tileMutex; > + Mutex m_paintMutex; > +#endif Shouldn't this be part of the next patch? > + // Don't use WebKit timers to avoid interfering with the engine. // Avoid using default WebKit timers ... paint engine > +static const int tileSize = 512; > +static const int checkerSize = 16; > +static const unsigned checkerColor1 = 0xff555555; > +static const unsigned checkerColor2 = 0xffaaaaaa; I think Darin once told me to prefix globals with g, like gTileSize. > +class TileTimer : public QObject { Maybe call it TileTimerQt and typedef it to TileTimer ? just wondering... > + Q_OBJECT > +public: > + TileTimer(TiledBackingStore*, void (TiledBackingStore::*f)()); > + void destroy(); > + void startOneShot(double time); singleShot? > +void TileTimer::timerEvent(QTimerEvent* ev) > +{ > +#if ENABLE(ENGINE_THREAD) > + bool needsLock = EngineThread::isCurrent(); > + if (needsLock) > + EngineThread::lock(); > +#endif > + m_timer.stop(); > + if (m_backingStore) > + (m_backingStore->*m_timerFunction)(); > +#if ENABLE(ENGINE_THREAD) > + if (needsLock) > + EngineThread::unlock(); > +#endif > +} This doesn't really seem that Qt specific. Could it be separated out so that others more easy can implement their TileTimer ? > +class Tile : public ThreadSafeShared<Tile> > +{ > +public: > + static PassRefPtr<Tile> create(TiledBackingStore* backingStore, const TiledBackingStore::TileCoordinate& tileCoordinate) { return adoptRef(new Tile(backingStore, tileCoordinate)); } > + ~Tile(); > + > + bool isDirty() const { return !m_dirtyRegion.isEmpty(); } > + const QRegion& dirtyRegion() const { return m_dirtyRegion; } Do you have a max for dirty regions? > + if (!m_backBuffer) { > + // Copy the current buffer if this is not a full tile update (or a new tile Missing ) > + if (!m_buffer) > + m_backBuffer = new QImage(m_backingStore->m_tileSize.width(), m_backingStore->m_tileSize.height(), QImage::Format_RGB16); Hardcoded 16 bit :-) > +void TiledBackingStore::updateTiles() dirty tiles, all tiles? sorry for nitpicking! :-) > + // FIXME: should not request system repaint for the full tile. > + fullDirtyRegion += it->second->rect(); > + //fullDirtyRegion += it->second->dirtyRegion(); Does it really make sense to update regions of the tiles and not just always taking the bounding rect of the dirty regions? > + paintedArea.append(rectToDocument(qrects[n])); I was told that in Qt .at(index) is faster than [index] > + QTransform nt(1., t.m12(), t.m13(), > + t.m21(), 1., t.m23(), > + t.m31(), t.m32(), t.m33()); > + painter->setWorldTransform(nt); > + nt? new transform? > + // Manhattan distance, biased so that vertical distances are shorter. > + const double horizontalBias = 1.3; > + return abs(centerCoordinate.y() - tileCoordinate.y()) + horizontalBias * abs(centerCoordinate.x() - tileCoordinate.x()); heh :-) > +void TiledBackingStore::createTiles() > +{ > + if (m_state != StateNormal) > + return; > + > + if (m_viewport.isEmpty()) > + return; > + > + dropOverhangingTiles(); > + > + // FIXME: Make adapt to memory. Make adaptable to memory constrains? or Adapt to ... > + IntRect keepRect = m_viewport; > + keepRect.inflateX(m_viewport.width()); > + keepRect.inflateY(3 * m_viewport.height()); Why 3? can you use a constant instead? > + keepRect.intersect(contentRect()); > + > + dropTilesOutsideRect(keepRect); Ah smart. > + // Paint the content of the newly created tiles > + if (tilesToCreateCount) newTilesRequiredCount? > + // Restart the timers. There might be pending invalidations that > + // were not painted or created because tiles are not created or > + // paintied when in StateNoTileCreation painted! when in the ... state > > +QRectF QGraphicsWebViewPrivate::visibleRect() const > +{ > + if (!q->scene()) > + return QRectF(); > + QList<QGraphicsView*> views = q->scene()->views(); > + if (views.size() > 1) { > + qDebug() << "QGraphicsWebView is in more than one graphics views, unable to compute the visible rect"; > + return QRectF(); > + } It still works when in more views? > + case ItemCursorHasChanged: { > + QEvent event(QEvent::CursorChange); > + QApplication::sendEvent(this, &event); > + return value; > + } Is the identation right here? > +float QGraphicsWebView::tiledBackingStoreScaleFactor() const Would it be better to just sue the zoomFactor() API. Noone are going to use both zoom factor and scale together, so we could just document that when using a tiled backing store, it will not scale the html forms + plugins as well. Simon? > + value = attributes.value(QWebSettings::TiledBackingStoreEnabled, > + global->attributes.value(QWebSettings::TiledBackingStoreEnabled)); > + settings->setTiledBackingStoreEnabled(value); Would be good with documentation explaining when it makes sense to not use this :-) otherwise it should probably just be default. How does this affect rotation of the qgraphicswebview? scrollbars (from the code I guess that doesn't work :-)) > Index: WebKit/qt/QGVLauncher/main.cpp QGVLauncher is going to die soonish, please add this support to QtLauncher :-) > + qDebug() << "Warning: at the moment --tiledBackingStore should always be used together with --resizesToContents"; > + QWebSettings::globalSettings()->setAttribute(QWebSettings::TiledBackingStoreEnabled, true); I guess that makes sense as well.

Created attachment 49491 [details] updated patch Split to platform independs/dependent files. Lots of cleanups.

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.

Created attachment 49578 [details] more updates - Updated to curent ToT - Moved the feature behind flag ENABLE(TILED_BACKING_STORE) - Removed zooming related Qt API. I wasn't happy with the API and that is anyway better done separately. - Removed QGVLauncher portion, QtLauncher patch coming separately. - Implemented some of Kenneths comments - Various fixes, cleanups

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.

Created attachment 49582 [details] added back some forgotten QGraphicsWebView code

> Is there a multi-threaded case? It seems that this patch implements a single > threaded tiled backingstore. Yes, but not in this patch :)

Comment on attachment 49582 [details] added back some forgotten QGraphicsWebView code + const Tile::Coordinate coordinate() const { return m_coordinate; } Antti, would it be better to return const Tile::Coordinate& here?

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.

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.

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

(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. Huh? It is LGPL.

Comment on attachment 49582 [details] added back some forgotten QGraphicsWebView code >+public: >+ class Coordinate { >+ public: >+ Coordinate(unsigned x, unsigned y) : m_x(x), m_y(y) { } >+ bool operator==(const Coordinate& o) const { return m_x == o.m_x && m_y == o.m_y; } >+ unsigned x() const { return m_x; } >+ unsigned y() const { return m_y; } >+ unsigned hash() const { >+ unsigned hash = WTF::intHash(static_cast<uint64_t>(m_x) << 32 | m_y); >+ // avoid empty and deleted value >+ return hash | (!(hash + 1) << 31); >+ } >+ private: >+ unsigned m_x; >+ unsigned m_y; >+ }; 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. >+void TiledBackingStore::invalidate(const IntRect& contentsDirtyRect) >+{ >+ IntRect dirtyRect(mapFromContents(contentsDirtyRect)); >+ >+ Tile::Coordinate topLeft = tileCoordinateForPoint(dirtyRect.topLeft()); >+ Tile::Coordinate bottomRight = tileCoordinateForPoint(dirtyRect.bottomRight()); >+ >+ for (unsigned yCoordinate = topLeft.y(); yCoordinate <= bottomRight.y(); ++yCoordinate) { >+ for (unsigned xCoordinate = topLeft.x(); xCoordinate <= bottomRight.x(); ++xCoordinate) { >+ RefPtr<Tile> currentTile = tileAt(Tile::Coordinate(xCoordinate, yCoordinate)); >+ if (!currentTile) >+ continue; >+ currentTile->invalidate(dirtyRect); 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... >+ if (distance > shortestDistance) >+ continue; >+ if (distance < shortestDistance) { else if? Also I think a short comment about the rationale of tile creation and the use of distances would be useful. >+ unsigned tilesToCreateCount = tilesToCreate.size(); >+ if (tilesToCreateCount) { >+ for (unsigned n = 0; n < tilesToCreateCount; ++n) { >+ Tile::Coordinate coordinate = tilesToCreate[n]; >+ setTile(coordinate, Tile::create(this, coordinate)); >+ } >+ requiredTileCount -= tilesToCreateCount; >+ } nitpick, but you can just do the for loop and requiredTileCount-- on each iteration. >@@ -521,10 +556,11 @@ QVariant QGraphicsWebView::itemChange(Gr > // fire 'CursorChange'. > case ItemCursorChange: > return value; >- case ItemCursorHasChanged: >- QEvent event(QEvent::CursorChange); >- QApplication::sendEvent(this, &event); >- return value; >+ case ItemCursorHasChanged: { >+ QEvent event(QEvent::CursorChange); >+ QApplication::sendEvent(this, &event); >+ return value; >+ } Unrelated? And the indent is wrong.

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

Created attachment 49950 [details] updated patch - switched to IntPoint typedef for tile coordinates, IntPointHash - implemented bunch of review comments - fixed a checker painting bug - various cleanups

Created attachment 50031 [details] WebCore patch Separate WebCore and QtWebKit patches.

Created attachment 50032 [details] QtWebKit patch Now with QtLauncher support

Comment on attachment 50032 [details] QtWebKit patch > + int h = views[0]->horizontalScrollBar()->value(); > + int v = views[0]->verticalScrollBar()->value(); These return the size? or ? > +#if ENABLE(TILED_BACKING_STORE) > + if (!frame->tiledBackingStore()) > +#endif > + view->layoutIfNeededRecursive(); I prefer adding an #else in this case #if ENABLE(TILED_BACKING_STORE) if (!frame->tiledBackingStore()) view->layoutIfNeededRecursive(); #else view->layoutIfNeededRecursive(); #endif > +#if ENABLE(TILED_BACKING_STORE) > + if (frame->tiledBackingStore()) > + frame->tiledBackingStore()->paint(context, clipRect); > + else > +#endif > + view->paintContents(context, clipRect); Same here. > Index: WebKit/qt/QGVLauncher/main.cpp QGVLauncher was removed from trunk! > + toggleResizesToContents->setCheckable(true); > + toggleResizesToContents->setChecked(false); > + toggleResizesToContents->setEnabled(false); I guess here is should check the setting to find out if it is actually enabled or not. Such a change was done to the other toggles by Jesus. I was told that that change had gone in.

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

Dropping [Qt] from title since most of this is generic code.

Comment on attachment 50032 [details] QtWebKit patch > +QRectF QGraphicsWebViewPrivate::visibleRect() const > +{ > + if (!q->scene()) > + return QRectF(); > + QList<QGraphicsView*> views = q->scene()->views(); > + if (views.size() > 1) { > + qDebug() << "QGraphicsWebView is in more than one graphics views, unable to compute the visible rect"; > + return QRectF(); > + } > + if (views.size() < 1) > + return QRectF(); > + > + int h = views[0]->horizontalScrollBar()->value(); > + int v = views[0]->verticalScrollBar()->value(); I suggest to use views.at(0) instead of [] or make the views variable const. With the current code the compiler will choose the non-const [] operator. As the returned views is an implicitly shared copy of the list in QGraphicsScene, the code will end up detaching the entire list. > + TiledBackingStoreEnabled, It would be good to document this enum value in qwebsettings.cpp and mention the default. > @@ -724,6 +753,8 @@ void LauncherApplication::handleUserOpti > << "[-show-fps]" > << "[-r list]" > << "[-inspector-url location]" > + << "[-tiled-backing-store]" > + << "[-resizes-to-contents]" > << "URLs"; > appQuit(0); > } > @@ -746,6 +777,16 @@ void LauncherApplication::handleUserOpti > gCacheWebView = true; > } > > + if (args.contains("-tiled-backing-store")) { > + requiresGraphicsView("-tiled-backing-store"); > + gUseTiledBackingStore = true; > + } > + > + if (args.contains("-resizes-to-contents")) { > + requiresGraphicsView("-resizes-to-contents"); > + gResizesToContents = true; > + } > + > QString arg1("-viewport-update-mode"); > int modeIndex = args.indexOf(arg1); > if (modeIndex != -1) { 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 :) > +void WebViewGraphicsBased::setResizesToContents(bool b) > +{ > + m_resizesToContents = b; > + m_item->setResizesToContents(m_resizesToContents); > + if (m_resizesToContents) { > + setHorizontalScrollBarPolicy(Qt::ScrollBarAsNeeded); > + setVerticalScrollBarPolicy(Qt::ScrollBarAsNeeded); > + } else { > + setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); > + setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff); > + } 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. Why are the scrollbars turned _off_ if m_resizesToContents is false? Shouldn't it be the other way around?

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.

*** Bug 32122 has been marked as a duplicate of this bug. ***

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 :)

> Those are implicitly shared, any particular reason to use pointers? See above. > Why redefine IntPoint? It is regularly used as such for coordinates. See above.

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?

Sending WebCore/ChangeLog Sending WebCore/WebCore.pri Sending WebCore/WebCore.pro Sending WebCore/page/Frame.cpp Sending WebCore/page/Frame.h Sending WebCore/page/FrameView.cpp Sending WebCore/page/Settings.cpp Sending WebCore/page/Settings.h Adding WebCore/platform/graphics/Tile.h Adding WebCore/platform/graphics/TiledBackingStore.cpp Adding WebCore/platform/graphics/TiledBackingStore.h Adding WebCore/platform/graphics/TiledBackingStoreClient.h Adding WebCore/platform/graphics/qt/TileQt.cpp Transmitting file data ............. Committed revision 55976.

Sending WebKit/qt/Api/qgraphicswebview.cpp Sending WebKit/qt/Api/qwebframe.cpp Sending WebKit/qt/Api/qwebframe.h Sending WebKit/qt/Api/qwebsettings.cpp Sending WebKit/qt/Api/qwebsettings.h Sending WebKit/qt/ChangeLog Transmitting file data ...... Committed revision 55978.

Sending WebKitTools/ChangeLog Sending WebKitTools/QtLauncher/main.cpp Sending WebKitTools/QtLauncher/webview.cpp Sending WebKitTools/QtLauncher/webview.h Sending WebKitTools/Scripts/build-webkit Transmitting file data ..... Committed revision 55979.

Followup for Qt scaling control API in bug 36102

Followup for making WebKit scrollbars work with tiling in bug 36121

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