Summary: | Do not iterate all tiles for resizing when the content didn't change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||
Component: | WebKit Qt | Assignee: | Kenneth Rohde Christiansen <kenneth> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | hausmann, jturcotte, noam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Kenneth Rohde Christiansen
2012-02-28 07:12:14 PST
Created attachment 129243 [details]
Patch
Comment on attachment 129243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129243&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:241 > + if (visibleRect != m_previousVisibleRect) > + didResizeTiles = resizeEdgeTiles(); m_visibleRect and m_previousVisibleRect are both set in coverWithTilesIfNeeded, and then the creation timer is started. So in most case they will be equal and this code will never run, unless the visible rect changed between the timer start and fire which is unlikely. (In reply to comment #2) Err, I mean the value of visibleContentsRect() instead of m_visibleRect. (In reply to comment #2) > (From update of attachment 129243 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129243&action=review > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:241 > > + if (visibleRect != m_previousVisibleRect) > > + didResizeTiles = resizeEdgeTiles(); > > m_visibleRect and m_previousVisibleRect are both set in coverWithTilesIfNeeded, and then the creation timer is started. > So in most case they will be equal and this code will never run, unless the visible rect changed between the timer start and fire which is unlikely. This method (createTiles) is also called as a result of viewport change, which means that they are different. Comment on attachment 129243 [details]
Patch
I believe this code makes inappropriate use of m_previousVisibleRect.
Comment on attachment 129243 [details]
Patch
I need coffee :-) This is wrong!
Created attachment 129263 [details]
Patch
Created attachment 129265 [details]
Patch
Comment on attachment 129265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129265&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:236 > + const IntRect& visibleRect = visibleContentsRect(); Is it OK to use a reference here? > Source/WebCore/platform/graphics/TiledBackingStore.cpp:246 > + if (contentsSize != m_previousContentsSize) m_previousContentsSize is never assigned, so this patch doesn't do much, unless I'm missing something :/ Comment on attachment 129265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129265&action=review r- based on jocelyn's comments. >> Source/WebCore/platform/graphics/TiledBackingStore.cpp:236 >> + const IntRect& visibleRect = visibleContentsRect(); > > Is it OK to use a reference here? I think it doesn't add much to use references. Let's use regular assignment. (In reply to comment #9) > (From update of attachment 129265 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129265&action=review > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:236 > > + const IntRect& visibleRect = visibleContentsRect(); > > Is it OK to use a reference here? > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:246 > > + if (contentsSize != m_previousContentsSize) > > m_previousContentsSize is never assigned, so this patch doesn't do much, unless I'm missing something :/ I guess my brain wasn't working correctly yesterday. I ofcourse meant to assign it just as we do with the visible rect Created attachment 129414 [details]
Patch
Comment on attachment 129414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129414&action=review > Source/WebCore/ChangeLog:3 > + Do not interate all tiles for resizing when the content didn't change Interate looks like a typo :) Landed in 109219. |