[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