updateLayers processes the LayerChromium tree on the main thread, so it should live on CCLayerTreeHost rather than on the LayerRendererChromium.
Created attachment 106065 [details] Patch
Comment on attachment 106065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106065&action=review Sheep. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-104 > -void CCLayerTreeHost::preCommit(CCLayerTreeHostImpl* hostImpl) This is a wonderful deletion to wake up to. Thank you!!! > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:-239 > - m_layerTreeHostImpl->updateLayers(); <3
Created attachment 106153 [details] Patch
(In reply to comment #3) > Created an attachment (id=106153) [details] > Patch Rebaselined, applies to ToT. I'd love to get this into m15 to make merging less painful.
Comment on attachment 106153 [details] Patch R=me, excellent. The name "CCLayerTreeHostCommon.h" is a little worrying to me because it contains a friggin' gigantic inline templatized function. I don't want people to include that from another header file, or really from any file other than CCLayerTreeHost and LayerRendererChromium. One pattern I've seen elsewhere in the code is to suffix these sorts of headers with "InlineMethods.h", what would you think of doing that? Also, shouldn't the other caller be CCLayerTreeHostImpl, not LRC?
(In reply to comment #5) > (From update of attachment 106153 [details]) > R=me, excellent. The name "CCLayerTreeHostCommon.h" is a little worrying to me because it contains a friggin' gigantic inline templatized function. I don't want people to include that from another header file, or really from any file other than CCLayerTreeHost and LayerRendererChromium. One pattern I've seen elsewhere in the code is to suffix these sorts of headers with "InlineMethods.h", what would you think of doing that? Actually, how about I declare the two versions we export in the header, move the template to the source file and then define the two stub functions we care about? > Also, shouldn't the other caller be CCLayerTreeHostImpl, not LRC? Yes, eventually I think that should be the case. Nat has been making the excellent argument that LTH(I) should deal with layer trees and LRC is our gl implementation. Under that sort of organization, drawLayers should move to LTHI. This is the more important move though, because it's across the thread boundary. Moving drawLayers is more of a cleanup. One thing at a time.
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 106153 [details] [details]) > > R=me, excellent. The name "CCLayerTreeHostCommon.h" is a little worrying to me because it contains a friggin' gigantic inline templatized function. I don't want people to include that from another header file, or really from any file other than CCLayerTreeHost and LayerRendererChromium. One pattern I've seen elsewhere in the code is to suffix these sorts of headers with "InlineMethods.h", what would you think of doing that? > > Actually, how about I declare the two versions we export in the header, move the template to the source file and then define the two stub functions we care about? If you can make it blend that sounds great! > > > Also, shouldn't the other caller be CCLayerTreeHostImpl, not LRC? > > Yes, eventually I think that should be the case. Nat has been making the excellent argument that LTH(I) should deal with layer trees and LRC is our gl implementation. Under that sort of organization, drawLayers should move to LTHI. > > This is the more important move though, because it's across the thread boundary. Moving drawLayers is more of a cleanup. One thing at a time. Yes, definitely that's not a thing to do now - I was just asking to make sure I knew where we were going.
Created attachment 106185 [details] Patch
> > Actually, how about I declare the two versions we export in the header, move the template to the source file and then define the two stub functions we care about? > > If you can make it blend that sounds great! If I've learned anything from Will It Blend?, it's that the answer is always yes.
Comment on attachment 106185 [details] Patch Aha, I see what you did there. Clever. R=me
Committed r94474: <http://trac.webkit.org/changeset/94474>