Bug 101656

Created attachment 173135 [details] Patch

Comment on attachment 173135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review > Source/WebCore/ChangeLog:31 > + This representation is fine on terminal using a monospace font

Comment on attachment 173135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review Cool you found this! > Source/WebCore/ChangeLog:40 > + Changing cache policy is not testable in layout tests. Could we find another way of testing it? unit tests? > Source/WebCore/platform/graphics/TiledBackingStore.cpp:278 > + * We must create or keep the tiles on the HERE region. s/on/in/ > Source/WebCore/platform/graphics/TiledBackingStore.cpp:312 > - double distance = tileDistance(visibleRect, currentCoordinate); > + double distance = tileDistance(intersectedVisibleRect, currentCoordinate); why not just use m_visibleRect here? ie. why create the separate intersectedVisibleRect ? Where is it intersected? > Source/WebCore/platform/graphics/TiledBackingStore.cpp:345 > + // If candidateSize is bigger than bounds (i.e. TBS is used as a backing store of GraphicsLayer), we skip the below adjusting logic which expects bounds covers the given rect. bounds TO cover the ...

Created attachment 173248 [details] Patch

Comment on attachment 173248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173248&action=review > Source/WebCore/platform/graphics/TiledBackingStore.h:107 > - IntRect visibleContentsRect() const; > + IntRect visibleContentsRectOnBacking() const; > + IntRect visibleRectOnBacking() const; > IntRect visibleRect() const; This is becoming too confusing, between what they represents and in what coordinate system.

Comment on attachment 173248 [details] Patch I've tested various sites and checked tiles are removed properly. After this patch, Coordinated Graphics uses pretty less memory. I explain in detail below. View in context: https://bugs.webkit.org/attachment.cgi?id=173248&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:244 > + return coverageRatio(visibleContentsRectOnBacking()) == 1.0f; The logic is not changed because visibleContentsRectOnBacking is renamed from visibleContentsRect. > Source/WebCore/platform/graphics/TiledBackingStore.cpp:284 > m_visibleRect = visibleRect; m_visibleRect keeps visibleRect(), not visibleRectOnBacking(), because we must remove invisible tiles. void TiledBackingStore::coverWithTilesIfNeeded(const FloatPoint& trajectoryVector) { IntRect visibleRect = this->visibleRect(); ... if (m_trajectoryVector == normalizedVector && m_visibleRect == visibleRect) return; ... m_visibleRect = visibleRect; createTiles(); } visibleRectOnBacking() returns IntRect() when we scroll the given TBS of GraphicsLayer off the viewport, and then coverWithTilesIfNeeded() early returns. It means we can have the change to remove tiles. So we must keep visibleRect(). > Source/WebCore/platform/graphics/TiledBackingStore.cpp:295 > + return; If we don't return here, createTile() creates one tile because tileCoordinateForPoint(IntRect()) == Tile::Coordinate(0, 0). It means we execute following "for loop statement" one time. Tile::Coordinate topLeft = tileCoordinateForPoint(coverRect.location()); for (int yCoordinate = topLeft.y(); yCoordinate <= bottomRight.y(); ++yCoordinate) { So we must early return to save memory.

(In reply to comment #3) > (From update of attachment 173135 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review > > Cool you found this! > > Could we find another way of testing it? unit tests? I think unit test is possible. After adding API to query the count of tile, we can check the count of tiles during scrolling. > > s/on/in/ > > why not just use m_visibleRect here? ie. why create the separate intersectedVisibleRect ? Where is it intersected? The names are changed in the second patch and I explained. > > bounds TO cover the ... Thanks for review! We posted simultaneously. I'll update :) (In reply to comment #5) > (From update of attachment 173248 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173248&action=review > > > Source/WebCore/platform/graphics/TiledBackingStore.h:107 > > - IntRect visibleContentsRect() const; > > + IntRect visibleContentsRectOnBacking() const; > > + IntRect visibleRectOnBacking() const; > > IntRect visibleRect() const; > > This is becoming too confusing, between what they represents and in what coordinate system. I agree. If name contains "Contents", it is on the contents coordinate system. I want to express which to be intersected with m_rect or not. Could you suggest?

Created attachment 173252 [details] Patch

Comment on attachment 173252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173252&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:283 > const IntRect visibleRect = this->visibleRect(); mapFromContents(m_client->tiledBackingStoreVisibleRect()); and get rid of visibleRect?

Comment on attachment 173252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173252&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:283 > const IntRect visibleRect = this->visibleRect(); mapFromContents(m_client->tiledBackingStoreVisibleRect()); and get rid of visibleRect?

Created attachment 173254 [details] Patch

Comment on attachment 173254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173254&action=review > Source/WebCore/platform/graphics/TiledBackingStore.h:107 > IntRect visibleRect() const; How about these names? Is it confused yet? (In reply to comment #3) > (From update of attachment 173135 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173135&action=review > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:312 > > - double distance = tileDistance(visibleRect, currentCoordinate); > > + double distance = tileDistance(intersectedVisibleRect, currentCoordinate); > > why not just use m_visibleRect here? ie. why create the separate intersectedVisibleRect ? Where is it intersected? you are right! intersectedVisibleRect is more reasonable. now intersectedVisibleRect is boundedVisibleRect.

(In reply to comment #10) > (From update of attachment 173252 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173252&action=review > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:283 > > const IntRect visibleRect = this->visibleRect(); > > mapFromContents(m_client->tiledBackingStoreVisibleRect()); > and get rid of visibleRect? Ok, I'll get rid of it!

yeah fine!

Created attachment 173258 [details] Patch

Comment on attachment 173258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173258&action=review > Source/WebCore/platform/graphics/TiledBackingStore.h:105 > IntRect visibleRect() const; I removed boundedSomthings instead of visibleRect.

Comment on attachment 173258 [details] Patch Clearing flags on attachment: 173258 Committed r134048: <http://trac.webkit.org/changeset/134048>

All reviewed patches have been landed. Closing bug.

(In reply to comment #18) > All reviewed patches have been landed. Closing bug. Thank you for review!