[chromium] Ubercomp: clean up CCRenderer interface

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] Ubercomp: clean up CCRenderer interface

[Alexandre Elias]

Created attachment 152873 [details]
Patch

[Alexandre Elias]

This is an small cleanup to reduce boilerplate in the ubercomp implementation.  When CCHeadsUpDisplay is better integrated into LayerRendererChromium, we should make private finishDrawingFrame() as well.

[Dana Jansens]

Comment on attachment 152873 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:368
> +        renderPass->targetSurface()->damageTracker()->didDrawDamagedArea();

This won't be able to stay here obviously, once targetSurface() is gone. So if we are going to do this, we have to guarantee that every render pass given to drawFrame() is actually drawn. I'm not sure that this is going to make our lives easier.. I'm currently adding another piece to the code in CCLTHI::drawLayers() that would then go here. But it really belongs there. CCLTHI manages the RenderPasses, LRC just draws them.

So overall, I feel like this is not going to help us and isn't worth removing one method from the API. :/

[Alexandre Elias]

For ubercomp, I was going to have to stash away pointers to the renderpasses as they came in, reconstruct the render pass list, and then do the actual work in finishDrawingFrame().  It seems to me that the one-at-a-time model is an implementation detail of OpenGL.  Why shouldn't we make sure that the only renderpasses sent to CCRenderer will be actually drawn; that seems like a good idea anyway?

We can still do some work on the side in CCLTHI; I'm fine with moving the didDrawDamaged area back there in a loop over the render passes.

[Alexandre Elias]

Created attachment 153082 [details]
Patch

Move didDrawDamagedArea back to LTHI

[Adrienne Walker]

Comment on attachment 153082 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:555
> +    m_layerRenderer->drawFrame(frame.renderPasses, m_rootScissorRect);
> +    if (m_headsUpDisplay->enabled(settings()))
> +        m_headsUpDisplay->draw(this);
> +    m_layerRenderer->finishDrawingFrame();

It's a little weird to me to have this asymmetric finish function without an exposed begin, but I understand that you need to draw some extra stuff in the frame here.  The heads up display is a little bit of a wart, and I'm not sure if it should be generating quads or be completely contained within LRC or something else.  Refactoring for the future, I guess.

[WebKit Review Bot]

Comment on attachment 153082 [details]
Patch

Clearing flags on attachment: 153082

Committed r123033: <http://trac.webkit.org/changeset/123033>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Reverted r123036 for reason:

Triggers some DCHECKs in Chromium browser_tests

Committed r123120: <http://trac.webkit.org/changeset/123120>

[Dana Jansens]

Comment on attachment 153082 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:555
>> +    m_layerRenderer->finishDrawingFrame();
> 
> It's a little weird to me to have this asymmetric finish function without an exposed begin, but I understand that you need to draw some extra stuff in the frame here.  The heads up display is a little bit of a wart, and I'm not sure if it should be generating quads or be completely contained within LRC or something else.  Refactoring for the future, I guess.

Making quads I think so a child renderer can display a hud.

[Nate Chapin]

(In reply to comment #9)
> Reverted r123036 for reason:
> 
> Triggers some DCHECKs in Chromium browser_tests
> 
> Committed r123120: <http://trac.webkit.org/changeset/123120>

The ChangeLog got mangled, it wasn't this bug that got reverted.