Bug 90573

Created attachment 150995 [details] Patch

Created attachment 151001 [details] Patch - move the contentsChanged flag into RenderPassDrawQuad instead of RenderPass.

Created attachment 151107 [details] Patch - rebase and remove unused drawingSurface variable

Comment on attachment 151107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151107&action=review A few random comments, but I like the way this looks in general. > Source/WebCore/ChangeLog:8 > + We Remove the managed textures from CCRenderSurface and store them in a s/We // > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298 > + // FIXME: Make this id global across all compositors (include a compositor id?) (Alternatively, maybe you won't need a compositor id here, since the host compositor will know which embedded compositor this pass/quad came from?) > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:56 > + static PassOwnPtr<CCRenderPass> create(CCRenderSurface*, unsigned id); Should id be an int here to match the type in CCLayerImpl::id()? > Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:60 > + bool m_contentsChangedSinceLastFrame; Can you just make this the full damage rect? I suspect that eventually we'll want that information so that if a surface texture is cached we can do a partial update of it. > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:130 > - OwnPtr<CCLayerImpl> root = CCLayerImpl::create(0); > + OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1); ...how were these all passing before?

Comment on attachment 151107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151107&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298 >> + // FIXME: Make this id global across all compositors (include a compositor id?) > > (Alternatively, maybe you won't need a compositor id here, since the host compositor will know which embedded compositor this pass/quad came from?) We could add the compositor id but I'd rather do it here I think. That said, I found a compositor id in CCProxy, so I can just do that now and no fixme. >> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:56 >> + static PassOwnPtr<CCRenderPass> create(CCRenderSurface*, unsigned id); > > Should id be an int here to match the type in CCLayerImpl::id()? ya, sure. matches compositor id too, which i will mashup with it. >> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:60 >> + bool m_contentsChangedSinceLastFrame; > > Can you just make this the full damage rect? I suspect that eventually we'll want that information so that if a surface texture is cached we can do a partial update of it. Hm ya, can do that. >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:130 >> + OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1); > > ...how were these all passing before? I think because none of them call drawLayers, they didn't hit that HashMap issue.

(In reply to comment #5) > (From update of attachment 151107 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151107&action=review > > >> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:130 > >> + OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1); > > > > ...how were these all passing before? > > I think because none of them call drawLayers, they didn't hit that HashMap issue. Ohhh. In that case, maybe we should assert that ids are non-zero in CCLayerImpl. Either that, or you need to use different hash traits on your maps that allow for zero ids.

Comment on attachment 151107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151107&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:298 >>> + // FIXME: Make this id global across all compositors (include a compositor id?) >> >> (Alternatively, maybe you won't need a compositor id here, since the host compositor will know which embedded compositor this pass/quad came from?) > > We could add the compositor id but I'd rather do it here I think. That said, I found a compositor id in CCProxy, so I can just do that now and no fixme. I'd like to punt on this for this patch. Its not needed for it strictly, and there's some work to be done. (SingleThreadProxy has compositorId == -1 always, and its not clear to me that's the correct id to use anyhow.)

Created attachment 151314 [details] Patch Changed the assert in CCLayerImpl from >= 0 to > 0. A bunch of tests had to be updated but it's simple changes. Addressed other comments also.

Comment on attachment 151314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151314&action=review R=me. > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:52 > + ASSERT(id); nit: id > 0

Created attachment 151320 [details] Patch for landing

Comment on attachment 151314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151314&action=review thanks! >> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:52 >> + ASSERT(id); > > nit: id > 0 oo right.

Comment on attachment 151320 [details] Patch for landing Clearing flags on attachment: 151320 Committed r122160: <http://trac.webkit.org/changeset/122160>

All reviewed patches have been landed. Closing bug.