Bug 67438 - [chromium] Move updateLayers from LayerRendererChromium to CCLayerTreeHost
Summary: [chromium] Move updateLayers from LayerRendererChromium to CCLayerTreeHost
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on: 66430
Blocks: 67440
  Show dependency treegraph
 
Reported: 2011-09-01 14:50 PDT by Adrienne Walker
Modified: 2011-09-02 18:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (67.78 KB, patch)
2011-09-01 16:51 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (67.69 KB, patch)
2011-09-02 10:53 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (68.71 KB, patch)
2011-09-02 13:40 PDT, Adrienne Walker
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-09-01 14:50:56 PDT
updateLayers processes the LayerChromium tree on the main thread, so it should live on CCLayerTreeHost rather than on the LayerRendererChromium.
Comment 1 Adrienne Walker 2011-09-01 16:51:05 PDT
Created attachment 106065 [details]
Patch
Comment 2 Nat Duca 2011-09-02 09:44:52 PDT
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
Comment 3 Adrienne Walker 2011-09-02 10:53:55 PDT
Created attachment 106153 [details]
Patch
Comment 4 Adrienne Walker 2011-09-02 10:55:17 PDT
(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 5 James Robinson 2011-09-02 11:21:56 PDT
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?
Comment 6 Adrienne Walker 2011-09-02 11:29:16 PDT
(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.
Comment 7 James Robinson 2011-09-02 11:39:52 PDT
(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.
Comment 8 Adrienne Walker 2011-09-02 13:40:09 PDT
Created attachment 106185 [details]
Patch
Comment 9 Adrienne Walker 2011-09-02 13:40:42 PDT
> > 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 10 James Robinson 2011-09-02 13:47:17 PDT
Comment on attachment 106185 [details]
Patch

Aha, I see what you did there.  Clever.

R=me
Comment 11 Adrienne Walker 2011-09-02 18:05:33 PDT
Committed r94474: <http://trac.webkit.org/changeset/94474>