[CoordinatedGraphics] Non-overlay scrollbar is not rendered

• Component: New Bugs
• Status: RESOLVED FIXED

[Gwang Yoon Hwang]

[CoordinatedGraphics] Non-overlay scrollbar is not rendered

[Gwang Yoon Hwang]

Created attachment 244256 [details]
Patch

[Gwang Yoon Hwang]

RenderLayerCompositor::mainFrameBackingIsTiled is checking the
RenderLayerBacking uses tiledbacking dynamically.  In the Coordinated
Graphics, the main frame backing is always uses tiled backing store. So
it should return true.

I think we should refactor TILED_BACKING_STORE to match with Apple's tiledBacking at some point, and remove the TILED_BACKING_STORE guard in separated task.

[Ryuan Choi]

I added gyuyoung to take a look.

Informally looks good to me.

[Ryuan Choi]

Comment on attachment 244256 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2927
> +#if USE(TILED_BACKING_STORE)

I removed WTF_USE_TILED_BACKING_STORE since r183529

Please change this to USE(COORDINATED_GRAPHICS)

[Gyuyoung Kim]

Comment on attachment 244256 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2927
>> +#if USE(TILED_BACKING_STORE)
> 
> I removed WTF_USE_TILED_BACKING_STORE since r183529
> 
> Please change this to USE(COORDINATED_GRAPHICS)

The mainFrameBackingIsTiled() was changed to documentUsesTiledBacking() in r183943. This patch needs to be updated.

[Simon Fraser (smfr)]

Comment on attachment 244256 [details]
Patch

r- since the patch is old. But are you sure the callers of documentUsesTiledBacking() are expecting this to return true for coordinated graphics?

[Gwang Yoon Hwang]

Created attachment 256346 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 256347 [details]
Patch

[Gwang Yoon Hwang]

ryuan, gyuyoung thanks for pointing out.
I just updated to match current codebase.

[Gwang Yoon Hwang]

(In reply to comment #6)
> Comment on attachment 244256 [details]
> Patch
> 
> r- since the patch is old. But are you sure the callers of
> documentUsesTiledBacking() are expecting this to return true for coordinated
> graphics?

Patch is updated. In Coordinated Graphics, we are just support overlay scrollbar at this point. To support non-overlay scrollbar, Coordinated Graphics should get true from documentUsesTiledBacking.
And I'm pretty sure about it. :)

[Gyuyoung Kim]

Comment on attachment 256347 [details]
Patch

LGTM for EFL port which uses the coordinated graphics. However I would delegate this review to Simon.

[Tim Horton]

Comment on attachment 256347 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2948
> +    return true;

This is pretty weird. Why does backing->usingTiledBacking() not return true? Why would you want this and that to return different things? Seems wrong.

[Gwang Yoon Hwang]

(In reply to comment #12)
> Comment on attachment 256347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256347&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2948
> > +    return true;
> 
> This is pretty weird. Why does backing->usingTiledBacking() not return true?
> Why would you want this and that to return different things? Seems wrong.

Thanks for review.

First of all, it is because CoordinatedGraphics uses TiledBackingStore, not a TiledBacking. Coordinated Graphics manages tiles in GraphicsLayer, but TiledBacking is used in RenderLayer.

There are several logics in RenderLayerBacking and RenderLayerCompositor which are specific TiledBacking implementation. 
So if we just return true in RenderLayerBacking::usingTiledBacking, it will introduce undesirable behavior in CoordinatedGraphics.

That's why I want to add it this way.

In a long-term, it is desirable to merge TiledBackingStore in to TiledBacking.
It is weird that we have two similar implementation with similar name.
And it will solve these weird stuffs.

I'll do this work after turning threaded-compositor on in GTK+ port.

[Michael Catanzaro]

Pretty sure this bug is obsolete now!