Bug 82091 - [chromium] Move recursive renderSurface clearing to CCLayerTreeHostImpl
Summary: [chromium] Move recursive renderSurface clearing to CCLayerTreeHostImpl
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
Depends on:
Reported: 2012-03-23 14:11 PDT by Shawn Singh
Modified: 2012-04-04 15:23 PDT (History)
3 users (show)

See Also:

Patch (10.39 KB, patch)
2012-04-03 11:41 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (9.22 KB, patch)
2012-04-04 14:17 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
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]

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]

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]

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

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

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.