[webkit-reviews] review denied: [Bug 53894] [Qt] Very jerky scrolling because of blocking in tiled backing store. : [Attachment 96335] Patch 3: Add tile reuse.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 8 02:13:16 PDT 2011


Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Viatcheslav
Ostapenko <ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 53894: [Qt] Very jerky scrolling because of blocking in tiled backing
store.
https://bugs.webkit.org/show_bug.cgi?id=53894

Attachment 96335: Patch 3: Add tile reuse.
https://bugs.webkit.org/attachment.cgi?id=96335&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96335&action=review

> Source/WebCore/ChangeLog:9
> +	   Part3 of tiles painting redesign.
> +	   Add tile reuse.

You don't explain the reasoning, why this is a good idea, any cons?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:-238
> -    // Remove tiles that extend outside the current contents rect.

Why remove the explanation of what overhanging tiles are... I think it is quite
confusing.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:245
> +    getOverhangingTiles(keepRect, visibleRect, overhangingTiles);

getTilesExtendingOutsideContentsRect ? or similar. You use visibleRect here and
in the implementation you use viewport.... confusing

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:286
> +	   // all tiles in place, can drop remainting overhanging tiles

Please use proper sentences for comments. Start with capital and end with dot.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:394
> +	   RefPtr<Tile> ret = tileAt(oldCoordinate);

Please use something more descriptive than ret

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:398
> +	   setTile(newCoordinate, ret);
> +	   return ret;

duplicated code

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:402
> +	   setTile(newCoordinate, ret);
> +	   return ret;

same code here

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:410
> +    return (int)((va->distance() - vb->distance()) * 1000);

why not static_cast<int>() ?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:413
> +void TiledBackingStore::getOverhangingTiles(const IntRect& rect, const
IntRect& viewport, CoordinateDistanceVector& overhangingTiles)

Why not return CoordinateDistanceVector& ? would make the api a lot more clear

> Source/WebCore/platform/graphics/TiledBackingStore.h:100
> +	   CoordinateDistancePair(const Tile::Coordinate& coordinate, double
distance) :
> +		   m_coordinate(coordinate), m_distance(distance) {}

This is not valid style

> Source/WebCore/platform/graphics/TiledBackingStore.h:106
> +	   Tile::Coordinate    m_coordinate;
> +	   double	       m_distance;
> +    };

Remove the spacing between Coordinate and m_coordinate etc


More information about the webkit-reviews mailing list