Bug 101918 – [Qt] REGRESSION(134142): overscaled tiles in pixel test results and MiniBrowser

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Balazs Kelemen]

We have issues with that before (bug 76546), and we workarounded the problem by forcing the resize via QWindowSystemInterface. The assumption was that if we have an actual platform window it should work without the workaround but it seems like that's not the case. I'm not sure where is the bug but going through QWindowSystemInterface solves it.

[Balazs Kelemen]

Created attachment 173622 [details]
Patch

[Balazs Kelemen]

Comment on attachment 173622 [details]
Patch

Although it fixes some tests there are still too many failures.

[Balazs Kelemen]

History of pixel bot and bisecting shows it went wrong in http://trac.webkit.org/changeset/134142. Please, take a look. The visual effect is that the content is compressed by the x-axis (however it is ok in MiniBrowser).

[Balazs Kelemen]

It also shown in MiniBrowser, when loading some non-trivial page the tiles are overscaled at first. Scrolling fixes them.

[Noam Rosenthal]

I think that patch needs to be rolled out. It breaks some implicit behavior of tiled backing store.

[Dongseong Hwang]

I'm sorry for this regression.

Could you tell me what layout tests failed? and What sites show compressed effect?

I want to take a look at.

I think r134142 keeps the invariant between TiledBackingStore and CoordinatedTiledBacking, but there maybe invariant breakers related to WebView. IMHO, we can fix it instead of putting workaround.

[Dongseong Hwang]

I have looked at it a while, and I found TiledBackingStore::m_rect.size()) and GraphicsLayer::m_size can be different.
I think it is timing issue.
I keep finding the root of bug.

[Dongseong Hwang]

I found the reason of the bug.

I made a mistake.

r134142 sends the size of a layer via a CreateTile message from Web Process to UI Process. But even if the layer's size is changed, we do not need to create any tiles. So CoordinatedBackingStore can keep an outdated backing size.

And I found other small bugs too. I'll post some patches in separate bugs to fix them.

Finally, thanks for reporting this bug!

[Dongseong Hwang]

Created attachment 173795 [details]
Patch

[Noam Rosenthal]

Comment on attachment 173795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173795&action=review

Good catch!
Some suggestions for improvement inline.

> Source/WebKit2/ChangeLog:16
> +        This patch updates the size of CoordinatedBackingStore when receiving an
> +        UpdateTile or RemoveTile message also, because changing the size of
> +        GrahpicsLayer causes an UpdateTile or RemoveTile message too.

Some wording improvement:
This patch makes sure that we reset the backing store's size to the layer size when UpdateTile or RemoveTile are called.

> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:428
> +void LayerTreeRenderer::setBackingStoreSize(CoordinatedBackingStore* backingStore, WebLayerID layerID)

This seems somewhat inefficient and verbose, since you call layerByID twice (getBackingStore calls layerByID).
How about if we changed the function signature to getBackingStore(GraphicsLayer*) , and then you can have a function resetBackingStoreSizeToLayerSize(GraphicsLayer*)

[Dongseong Hwang]

Created attachment 173816 [details]
Patch

[Dongseong Hwang]

(In reply to comment #10)
> (From update of attachment 173795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173795&action=review
> 
> Good catch!
> Some suggestions for improvement inline.

Thank you for review!

> 
> > Source/WebKit2/ChangeLog:16
> > +        This patch updates the size of CoordinatedBackingStore when receiving an
> > +        UpdateTile or RemoveTile message also, because changing the size of
> > +        GrahpicsLayer causes an UpdateTile or RemoveTile message too.
> 
> Some wording improvement:
> This patch makes sure that we reset the backing store's size to the layer size when UpdateTile or RemoveTile are called.

Done.

> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:428
> > +void LayerTreeRenderer::setBackingStoreSize(CoordinatedBackingStore* backingStore, WebLayerID layerID)
> 
> This seems somewhat inefficient and verbose, since you call layerByID twice (getBackingStore calls layerByID).
> How about if we changed the function signature to getBackingStore(GraphicsLayer*) , and then you can have a function resetBackingStoreSizeToLayerSize(GraphicsLayer*)

Great idea! Done.
I changed removeBackingStoreIfNeeded() as well as getBackingStore() for the same purpose.

[Dongseong Hwang]

Comment on attachment 173816 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173816&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Oops, sorry. I'll change this next time.

[WebKit Review Bot]

Comment on attachment 173816 [details]
Patch

Clearing flags on attachment: 173816

Committed r134375: <http://trac.webkit.org/changeset/134375>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.