Bug 79787 - Do not iterate all tiles for resizing when the content didn't change
Summary: Do not iterate all tiles for resizing when the content didn't change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-28 07:12 PST by Kenneth Rohde Christiansen
Modified: 2012-02-29 09:02 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.76 KB, patch)
2012-02-28 07:13 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2012-02-28 08:43 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (2.29 KB, patch)
2012-02-28 08:44 PST, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (2.71 KB, patch)
2012-02-29 01:33 PST, Kenneth Rohde Christiansen
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-02-28 07:12:14 PST
SSIA
Comment 1 Kenneth Rohde Christiansen 2012-02-28 07:13:28 PST
Created attachment 129243 [details]
Patch
Comment 2 Jocelyn Turcotte 2012-02-28 07:34:48 PST
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.
Comment 3 Jocelyn Turcotte 2012-02-28 07:37:30 PST
(In reply to comment #2)
Err, I mean the value of visibleContentsRect() instead of m_visibleRect.
Comment 4 Kenneth Rohde Christiansen 2012-02-28 07:39:50 PST
(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 5 Noam Rosenthal 2012-02-28 07:41:41 PST
Comment on attachment 129243 [details]
Patch

I believe this code makes inappropriate use of m_previousVisibleRect.
Comment 6 Kenneth Rohde Christiansen 2012-02-28 07:45:14 PST
Comment on attachment 129243 [details]
Patch

I need coffee :-) This is wrong!
Comment 7 Kenneth Rohde Christiansen 2012-02-28 08:43:00 PST
Created attachment 129263 [details]
Patch
Comment 8 Kenneth Rohde Christiansen 2012-02-28 08:44:15 PST
Created attachment 129265 [details]
Patch
Comment 9 Jocelyn Turcotte 2012-02-28 10:00:08 PST
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 10 Noam Rosenthal 2012-02-28 10:02:07 PST
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.
Comment 11 Kenneth Rohde Christiansen 2012-02-29 01:07:45 PST
(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
Comment 12 Kenneth Rohde Christiansen 2012-02-29 01:33:30 PST
Created attachment 129414 [details]
Patch
Comment 13 Simon Hausmann 2012-02-29 03:05:29 PST
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 :)
Comment 14 Kenneth Rohde Christiansen 2012-02-29 09:02:11 PST
Landed in 109219.