[Webkit-unassigned] [Bug 35146] [Qt] Support tiled backing store
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Feb 19 09:58:42 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=35146
--- Comment #4 from Kenneth Rohde Christiansen <kenneth at webkit.org> 2010-02-19 09:58:42 PST ---
(From update of attachment 49067)
> + 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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list