Bug 58840

I'm taking a look into this now. My rough plan is as follows: - land https://bugs.webkit.org/show_bug.cgi?id=58830 so the property pushing aspect of LRC::updatePropertiesAndRenderSurfaces() is separate from the transform portion - make LayerRendererChromium::updatePropertiesAndRenderSurfaces() a generic function to calculate the draw transforms, visible regions, and z-order for a set of layers templated for LayerChromium+??? and CCLayerImpl+RenderSurfaceChromium so we can use the same codepath for LayerChromiums and CCLayerImpl. This will also involve abstracting the "holds on to a z-order list of layers, transform and bounds" portion of RenderSurfaceChromium out from the "is usable as a render target and can draw itself" of RenderSurfaceChromium. We'll run the algorithm to calculate draw transforms, visible regions, and z-order in the main thread portion using the LayerChromium tree and aggregate results into some class name TBD. We'll then run the same code on the CCLayerImpl tree in the compositor thread and aggregate into real RenderSurfaceChromiums.

(In reply to comment #1) > - make LayerRendererChromium::updatePropertiesAndRenderSurfaces() a generic function to calculate the draw transforms, visible regions, and z-order for a set of layers templated for LayerChromium+??? and CCLayerImpl+RenderSurfaceChromium so we can use the same codepath for LayerChromiums and CCLayerImpl. This will also involve abstracting the "holds on to a z-order list of layers, transform and bounds" portion of RenderSurfaceChromium out from the "is usable as a render target and can draw itself" of RenderSurfaceChromium. I was looking at some of this same code yesterday to try to better fix https://bugs.webkit.org/show_bug.cgi?id=59020. The scissor rect / visible region calculation is wrong in one particular case, but I think there also needs to be some refactoring about where we calculate and store that scissor rect. However, this sounds like exactly the same patch of code that you're about to uproot. Would it make sense for you to try to address both at the same time? Otherwise, maybe I should postpone that fix until you're done reorganizing this?

Created attachment 102323 [details] Patch

This patch might be a touch large to review (hah!). I can think of at least one preliminary patch that I can land ahead of time to make the diff easier to read, which I'll file as a dependent bug on this one, but for the most part these changes have to land atomically, I'm afraid.

Comment on attachment 102323 [details] Patch Attachment 102323 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9269278

Comment on attachment 102323 [details] Patch Attachment 102323 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9266366

Created attachment 102372 [details] compile fixes

Created attachment 102417 [details] Patch

With http://trac.webkit.org/changeset/92037 landed this patch is still large, but a bit more manageable.

Comment on attachment 102417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102417&action=review Looks good! I suppose additional work will need to be done to deal with the texture manager used by CCRenderSurface? > Source/WebCore/ChangeLog:408 > +2011-07-29 James Robinson <jamesr@chromium.org> Needs to be removed > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:410 > +bool LayerChromium::descendantsDrawsContent() nit: descendantDrawsContent() ? > Source/WebCore/platform/graphics/chromium/LayerChromium.h:-42 > -#include "RenderSurfaceChromium.h" Why change the order here? Headers were alphabetically sorted. If the need to be rearranged then there are hidden dependencies that should be eliminated. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:2 > + * Copyright (C) 2010 Google Inc. All rights reserved. 2011 > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:30 > +#include "CCRenderSurface.h" cc/CCRenderSurface.h ?

(In reply to comment #10) > (From update of attachment 102417 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102417&action=review > > Looks good! I suppose additional work will need to be done to deal with the texture manager used by CCRenderSurface? Yes, there's some more work needed here - https://bugs.webkit.org/show_bug.cgi?id=64772 is the next piece of that puzzle. > > > Source/WebCore/ChangeLog:408 > > +2011-07-29 James Robinson <jamesr@chromium.org> > > Needs to be removed fixed > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:410 > > +bool LayerChromium::descendantsDrawsContent() > > nit: descendantDrawsContent() ? good idea, fixed > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:-42 > > -#include "RenderSurfaceChromium.h" > > Why change the order here? Headers were alphabetically sorted. If the need to be rearranged then there are hidden dependencies that should be eliminated. resorted > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:2 > > + * Copyright (C) 2010 Google Inc. All rights reserved. > > 2011 done > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:30 > > +#include "CCRenderSurface.h" > > cc/CCRenderSurface.h ? done

Created attachment 102579 [details] Patch

Comment on attachment 102579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102579&action=review Couple more small things but otherwise and unofficial LGTM from me. > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:66 > + , m_debugBorderWidth(0) I don't see m_debugBorderWidth used here. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:2 > + * Copyright (C) 2010 Google Inc. All rights reserved. 2011

Created attachment 102592 [details] fix copyright year

(In reply to comment #13) > (From update of attachment 102579 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102579&action=review > > Couple more small things but otherwise and unofficial LGTM from me. > > > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:66 > > + , m_debugBorderWidth(0) > > I don't see m_debugBorderWidth used here. > It's used in pushPropertiesTo() and later used on the CC side of life when drawing layers.

Comment on attachment 102592 [details] fix copyright year View in context: https://bugs.webkit.org/attachment.cgi?id=102592&action=review Overall this looks like good progress. A couple of minor comments. r=me > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:245 > OwnPtr<CCCanvasLayerImpl::Program> m_canvasLayerProgram; > + OwnPtr<CCPluginLayerImpl::Program> m_pluginLayerProgram; > + OwnPtr<CCRenderSurface::MaskProgram> m_renderSurfaceMaskProgram; > + OwnPtr<CCRenderSurface::Program> m_renderSurfaceProgram; > OwnPtr<CCVideoLayerImpl::RGBAProgram> m_videoLayerRGBAProgram; > OwnPtr<CCVideoLayerImpl::YUVProgram> m_videoLayerYUVProgram; It seems to me that these references are abstraction violations. The LayerRendererChromium shouldn't reference these programs directly. Is there a plan to move these entirely into the cc/ package? If so, is it worth a FIXME? > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:107 > + OwnPtr<LayerTexture> m_contentsTexture; Is this reference an abstraction violation? It seems to me that the compositor's classes should use those in cc/ but not the other way around.

You are correct on both points. Nat is working on splitting up LayerRendererChromium into (at least) two halves, part of that will involve moving the program management into a cc/ class. For now, though, there isn't a good cc/ class to hold the programs on. The LayerTexture dependency is also a bit wonky because we are using the TextureManager concept for both content textures (which aren't currently a cc/ concept) and for render surfaces (which are). It's going to take a bit more work over the next few weeks to fully clean this up.

Comment on attachment 102592 [details] fix copyright year Clearing flags on attachment: 102592 Committed r92245: <http://trac.webkit.org/changeset/92245>

All reviewed patches have been landed. Closing bug.