Currently, some quad types draw their entire contentBounds instead of only their visibleRect, and they rely on scissoring to correctly clip the layers. This is proposed to be refactored in https://bugs.webkit.org/show_bug.cgi?id=94049, but in the meantime, for correctness we need to do per-quad scissoring so that all layer types are correctly clipped. Before fixing this, after a recent refactor, scissoring is done only per-surface and results in incorrect unclipped layers for layers such as canvas.
The fix is quite straightforward, but i'm not sure if we have a good way to unit test this. So I'll see if a layout test can cover the issue instead.
Created attachment 158586 [details] Patch passes all unit tests and layout tests on osx. In this patch I opted for the always-scissor approach. I felt like adding more logic to turn off scissoring was over-engineering the solution without enough evidence that it's worth the savings. If you want I can make a different solution that adds a bit more logic passed into drawQuad, so that the quad can decide for itself whether to turn off scissoring or not. But, especially if we do intend to refactor quad types to draw only their clipped/visible bounds, then maybe this solution is better to keep things simple.
Comment on attachment 158586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 > - else > - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't.
(In reply to comment #3) > (From update of attachment 158586 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 > > - else > > - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); > > This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. Thanks - good catch =) But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach.
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 158586 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 > > > - else > > > - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); > > > > This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. > > Thanks - good catch =) > > But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect.
Comment on attachment 158586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review >>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 >>>> - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); >>> >>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. >> >> Thanks - good catch =) >> >> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. > > to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect. Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that.
Comment on attachment 158586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review >>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 >>>>> - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); >>>> >>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. >>> >>> Thanks - good catch =) >>> >>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. >> >> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect. > > Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that. Why are you setting the scissor in two places?
Comment on attachment 158586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review >>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 >>>>>> - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); >>>>> >>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. >>>> >>>> Thanks - good catch =) >>>> >>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. >>> >>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect. >> >> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that. > > Why are you setting the scissor in two places? One's for the whole render surface for clear.
(In reply to comment #7) > (From update of attachment 158586 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review > > >>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 > >>>>> - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); > >>>> > >>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. > >>> > >>> Thanks - good catch =) > >>> > >>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. > >> > >> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect. > > > > Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that. > > Why are you setting the scissor in two places? So that clearing the renderSurface gets correctly scissored, too. I think we need that for when partial swap is enabled, we can't avoid scissoring there.
Comment on attachment 158586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review >>>>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 >>>>>>>> - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); >>>>>>> >>>>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. >>>>>> >>>>>> Thanks - good catch =) >>>>>> >>>>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. >>>>> >>>>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect. >>>> >>>> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that. >>> >>> Why are you setting the scissor in two places? >> >> One's for the whole render surface for clear. > > So that clearing the renderSurface gets correctly scissored, too. I think we need that for when partial swap is enabled, we can't avoid scissoring there. Ok, sure. Re: original if statement. I'd prefer to leave it as is. Maybe unconditionally setting the scissor is also correct, but why change it?
(In reply to comment #10) > (From update of attachment 158586 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158586&action=review > > >>>>>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-425 > >>>>>>>> - GLC(m_context, m_context->disable(GraphicsContext3D::SCISSOR_TEST)); > >>>>>>> > >>>>>>> This shouldn't go away. Else the last layer's scissor could apply to the clear here when it shouldn't. > >>>>>> > >>>>>> Thanks - good catch =) > >>>>>> > >>>>>> But, if you're ok with it, I think it's better to remove the if statement instead, to keep the always-scissor approach. > >>>>> > >>>>> to be clear, I meant, literally remove the if (...), and always set the scissor, even if it's the same as the surface's rect. > >>>> > >>>> Is that better? It differs from how the code was before I touched any of this, so I defer to enne on that. > >>> > >>> Why are you setting the scissor in two places? > >> > >> One's for the whole render surface for clear. > > > > So that clearing the renderSurface gets correctly scissored, too. I think we need that for when partial swap is enabled, we can't avoid scissoring there. > > Ok, sure. > > Re: original if statement. I'd prefer to leave it as is. Maybe unconditionally setting the scissor is also correct, but why change it? The logic is as follows: 1. if scissor is different than contentRect, set it accordingly 2. if scissor is not different, then disable it 3. clear the framebuffer, only in the scissored region 4. (new in this patch) enable scissor and set it to the correct rect so that quads are correctly clipped. It feels a bit unclean to keep doing step 2 if we are always doing step 4. I felt it's better to keep the code simple rather than keeping the patch simple just because the patch works...
Created attachment 158623 [details] Addressed Dana's comment, still uses always-scissor approach I had this patch just before your questions, so feel free to r- it if you still want that if-statement back =)
Comment on attachment 158623 [details] Addressed Dana's comment, still uses always-scissor approach R=me.
Comment on attachment 158623 [details] Addressed Dana's comment, still uses always-scissor approach Attachment 158623 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13512262 New failing tests: compositing/overflow/overflow-hidden-canvas-layer.html
Created attachment 158665 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 158748 [details] Patch for landing
Committed r125758: <http://trac.webkit.org/changeset/125758>
Committed r125824: <http://trac.webkit.org/changeset/125824>