TBS removes tiles outside m_rect in resizeEdgeTiles function. However before calling resizeEdgeTiles, setKeepRect does same thing. In setKeepRect, TBS remove tiles outside keepRect. And m_rect includes m_keepRect. So TBS does same thing twice. I think that it is better to remove the unnecessary code in resizeEdgeTiles.
(In reply to comment #0) > TBS removes tiles outside m_rect in resizeEdgeTiles function. > However before calling resizeEdgeTiles, setKeepRect does same thing. > In setKeepRect, TBS remove tiles outside keepRect. And m_rect includes m_keepRect. So TBS does same thing twice. I think that it is better to remove the unnecessary code in resizeEdgeTiles. I understood wrongly. Removing tiles in resizeEdgeTiles is not working currently, not doing same thing twice. I'll file a removing patch.
Created attachment 197729 [details] ProposedPatch
Comment on attachment 197729 [details] ProposedPatch Attachment 197729 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/131023 New failing tests: fast/repaint/japanese-rl-selection-repaint-in-regions.html
Created attachment 197750 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Created attachment 197921 [details] Patch
(In reply to comment #5) > Created an attachment (id=197921) [details] > Patch I think it's not the failure of the layout test owing of this patch. I filled the patch again.
Comment on attachment 197921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197921&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:447 > IntRect expectedTileRect = tileRectForCoordinate(tileCoordinate); The return value of tileRectForCoordinate is intersected with m_rect while, as best as I can read the code, m_keepRect isn't. This means that if the page's content size is shrunk, your patch wouldn't remove tiles that fall outside the new content rect anymore since setKeepRect doesn't handle this case. I have to admit that TiledBackingStore is very badly tested and for this reason, unless there is great gain in doing this kind of lossy refactoring, the risk is usually not worth it. r- assuming that what I'm saying is correct. Let me know if you think I'm missing something.
Comment on attachment 197921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197921&action=review >> Source/WebCore/platform/graphics/TiledBackingStore.cpp:447 >> IntRect expectedTileRect = tileRectForCoordinate(tileCoordinate); > > The return value of tileRectForCoordinate is intersected with m_rect while, as best as I can read the code, m_keepRect isn't. > This means that if the page's content size is shrunk, your patch wouldn't remove tiles that fall outside the new content rect anymore since setKeepRect doesn't handle this case. > > I have to admit that TiledBackingStore is very badly tested and for this reason, unless there is great gain in doing this kind of lossy refactoring, the risk is usually not worth it. > > r- assuming that what I'm saying is correct. Let me know if you think I'm missing something. if the page's content size is shrunk, m_keepRect will also be shrunk, because keepRect is intersected with m_rect at last in computeCoverAndKeepRect. so I think setKeepRect will handle the case before calling resizeEdgeTiles. And I understood it's not easy to test TiledBackingStore. However In mobile platform, we can get lot's of gain by adjusting the TBS drawing area. Maybe this refactoring does not help to gain the performance. I'm still think the redundant code is unnecessary.