Bug 67438 – [chromium] Move updateLayers from LayerRendererChromium to CCLayerTreeHost

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

updateLayers processes the LayerChromium tree on the main thread, so it should live on CCLayerTreeHost rather than on the LayerRendererChromium.

[Adrienne Walker]

Created attachment 106065 [details]
Patch

[Nat Duca]

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

[Adrienne Walker]

Created attachment 106153 [details]
Patch

[Adrienne Walker]

(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.

[James Robinson]

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?

[Adrienne Walker]

(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.

[James Robinson]

(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.

[Adrienne Walker]

Created attachment 106185 [details]
Patch

[Adrienne Walker]

> > 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.

[James Robinson]

Comment on attachment 106185 [details]
Patch

Aha, I see what you did there.  Clever.

R=me

[Adrienne Walker]

Committed r94474: <http://trac.webkit.org/changeset/94474>