[Webkit-unassigned] [Bug 35146] [Qt] Support tiled backing store

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Xan Lopez <xan.lopez at gmail.com>  2010-03-03 02:36:54 PST ---
(From update of attachment 49582)

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

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