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

Shawn Singh
Reported 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()
Attachments
Patch (10.39 KB, patch)
2012-04-03 11:41 PDT, Shawn Singh 		no flags
DetailsFormatted DiffDiff
Patch (9.22 KB, patch)
2012-04-04 14:17 PDT, Shawn Singh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Shawn Singh
Comment 1 2012-04-03 11:41:44 PDT
Created attachment 135377 [details] Patch
Shawn Singh
Comment 2 2012-04-03 11:42:18 PDT
Hi James, just one more clean-up for today, if it meets your approval =) Thanks in advance
James Robinson
Comment 3 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
Shawn Singh
Comment 4 2012-04-03 17:46:56 PDT
OK sounds good. That means that LRC::close() will also get removed.
James Robinson
Comment 5 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.
Shawn Singh
Comment 6 2012-04-04 14:17:03 PDT
Created attachment 135679 [details] Patch Tested all layout tests and unit tests on linux, no obvious regressions
James Robinson
Comment 7 2012-04-04 14:42:56 PDT
Comment on attachment 135679 [details] Patch Nice
Shawn Singh
Comment 8 2012-04-04 14:43:46 PDT
Comment on attachment 135679 [details] Patch Thanks for reviewing!
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2012-04-04 15:23:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.