Bug 79787 – Do not iterate all tiles for resizing when the content didn't change

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 129243 [details]
Patch

[Jocelyn Turcotte]

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.

[Jocelyn Turcotte]

(In reply to comment #2)
Err, I mean the value of visibleContentsRect() instead of m_visibleRect.

[Kenneth Rohde Christiansen]

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

[Noam Rosenthal]

Comment on attachment 129243 [details]
Patch

I believe this code makes inappropriate use of m_previousVisibleRect.

[Kenneth Rohde Christiansen]

Comment on attachment 129243 [details]
Patch

I need coffee :-) This is wrong!

[Kenneth Rohde Christiansen]

Created attachment 129263 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 129265 [details]
Patch

[Jocelyn Turcotte]

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 :/

[Noam Rosenthal]

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.

[Kenneth Rohde Christiansen]

(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

[Kenneth Rohde Christiansen]

Created attachment 129414 [details]
Patch

[Simon Hausmann]

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 :)

[Kenneth Rohde Christiansen]

Landed in 109219.