Bug 114496 – [Texmap] Remove unnecessary TiledBackingStore code.

• Status: NEW
• Component: Layout and Rendering

[JungJik Lee]

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.

[JungJik Lee]

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

[JungJik Lee]

Created attachment 197729 [details]
ProposedPatch

[Build Bot]

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

[Build Bot]

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

[JungJik Lee]

Created attachment 197921 [details]
Patch

[JungJik Lee]

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

[Jocelyn Turcotte]

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.

[JungJik Lee]

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.