Bug 82091

Summary: [chromium] Move recursive renderSurface clearing to CCLayerTreeHostImpl
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Shawn Singh 2012-03-23 14:11:08 PDT
There are a few "unclear" (pun blatantly intended) function names - clearRenderSurface() and clearRenderSurfacesOnCCLayerImplRecursive() and their corresponding functions on CCLayerImpl, too.

For readability, we should use more descriptive function names...
  - LayerRendererChromium::clearSurfaceContents()
  - LayerRendererChromium::clearRenderSurfacePointers()
  - CCLayerImpl::clearRenderSurfacePointer()
Comment 1 Shawn Singh 2012-04-03 11:41:44 PDT
Created attachment 135377 [details]
Patch
Comment 2 Shawn Singh 2012-04-03 11:42:18 PDT
Hi James, just one more clean-up for today, if it meets your approval =)  Thanks in advance
Comment 3 James Robinson 2012-04-03 14:45:51 PDT
Comment on attachment 135377 [details]
Patch

These functions have the same name, but they're in very different types so I'm not sure there is much risk of confusion.  I think clearFoo() is fairly well established for clearing an m_foo pointer.

I think a better way to disambiguate LayerRendererChromium would be to move this function up to CCLayerTreeHostImpl, where it would fit better
Comment 4 Shawn Singh 2012-04-03 17:46:56 PDT
OK sounds good.  That means that LRC::close() will also get removed.
Comment 5 James Robinson 2012-04-03 17:52:03 PDT
Yes, exactly.  LayerRendererChromium should be dealing only with GL calls, so 'clear' in that file should always mean something like "do a glClear()".  All layer tree lifetime / ownership issues for the impl thread should be dealt with up on CCLayerTreeHostImpl, and CCLayerTreeHostImpl shouldn't know anything about GL, so 'clear' up there should always be doing something to a smart pointer type.
Comment 6 Shawn Singh 2012-04-04 14:17:03 PDT
Created attachment 135679 [details]
Patch

Tested all layout tests and unit tests on linux, no obvious regressions
Comment 7 James Robinson 2012-04-04 14:42:56 PDT
Comment on attachment 135679 [details]
Patch

Nice
Comment 8 Shawn Singh 2012-04-04 14:43:46 PDT
Comment on attachment 135679 [details]
Patch

Thanks for reviewing!
Comment 9 WebKit Review Bot 2012-04-04 15:23:50 PDT
Comment on attachment 135679 [details]
Patch

Clearing flags on attachment: 135679

Committed r113248: <http://trac.webkit.org/changeset/113248>
Comment 10 WebKit Review Bot 2012-04-04 15:23:55 PDT
All reviewed patches have been landed.  Closing bug.