Bug 88953

Summary: [chromium] Refactor CCLayerTreeHostCommon: remove root layer special case initialization
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, danakj, enne, jamesr, webkit.review.bot, zlieber
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 80622    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch enne: review+

Shawn Singh
Reported 2012-06-12 21:09:58 PDT
In CCLayerTreeHostCommonTest some of the test cases don't actually initialize renderSurfaceLayerList the same way that the code actually does. This means the tested scenarios are ever-so-subtly different than practical scenarios. Furthermore, its been a sore point against code elegance of CCLTH and CCLTHi that the root layer has to initialize junk outside of calcDrawTransforms. It would be very nice to remove this special casing, so that we can blindly call calculateDrawTransforms for a root layer that may or may not have an existing renderSurface, and receives an empty renderSurfaceLayerList instead of it already being populated with one awkward item.
Attachments
Patch (67.02 KB, patch)
2012-07-24 16:22 PDT, Shawn Singh 		no flags
DetailsFormatted DiffDiff
Patch (68.99 KB, patch)
2012-07-25 11:39 PDT, Shawn Singh 		enne: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dana Jansens
Comment 1 2012-06-13 08:59:06 PDT
/upvote
Adrienne Walker
Comment 2 2012-06-13 09:42:09 PDT
I think that CCLTH(i) has this special root layer code because only CCLTH(i) knows about the viewport size. Were you thinking about just making a different calcDraw entry point where you could pass in the viewport and then abstracting all that surface initialization into CCLTHCommon? Cleaning up this code would be great. :)
Shawn Singh
Comment 3 2012-06-13 10:12:32 PDT
(In reply to comment #2) > I think that CCLTH(i) has this special root layer code because only CCLTH(i) knows about the viewport size. > > Were you thinking about just making a different calcDraw entry point where you could pass in the viewport and then abstracting all that surface initialization into CCLTHCommon? > > Cleaning up this code would be great. :) As a first pass, yeah thats what I had in mind. In my own words, it would be moving the special case stuff into CCLayerTreeHostCommon. In the best case, it eventually becomes clear how to generalize without any "if layer==rootLayer" clauses, but that's probably not possible.
Shawn Singh
Comment 4 2012-07-24 16:22:19 PDT
Created attachment 154166 [details] Patch unit tests and manual tests seem OK. I will do more thorough tests of debug mode and release mode after 91417 lands and this patch gets rebased to it.
Shawn Singh
Comment 5 2012-07-24 16:24:13 PDT
Please note that there might be a part 2 for this, if it works out, which will try to reduce the root layer special-ness from inside of calcDrawTransforms. This current patch just removes the special-ness from the outside perspective.
Adrienne Walker
Comment 6 2012-07-25 09:27:52 PDT
Comment on attachment 154166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154166&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:470 > + CCLayerTreeHostCommon::calculateDrawTransforms(rootLayer, deviceViewportSize(), m_deviceScaleFactor, updateList, layerRendererCapabilities().maxTextureSize); style nit: output parameters last, please. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:956 > + // FIXME: creating a renderSurface on renderSurface1 might not have the intended effect here. do we mean to setForceRenderSurface(true) ?? > + // (same FIXME exists in 1 more location.) Can you investigate both of these tests?
Shawn Singh
Comment 7 2012-07-25 11:39:38 PDT
Created attachment 154403 [details] Patch
Shawn Singh
Comment 8 2012-07-25 12:52:43 PDT
(In reply to comment #6) > (From update of attachment 154166 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154166&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:470 > > + CCLayerTreeHostCommon::calculateDrawTransforms(rootLayer, deviceViewportSize(), m_deviceScaleFactor, updateList, layerRendererCapabilities().maxTextureSize); > > style nit: output parameters last, please. done > > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:956 > > + // FIXME: creating a renderSurface on renderSurface1 might not have the intended effect here. do we mean to setForceRenderSurface(true) ?? > > + // (same FIXME exists in 1 more location.) > > Can you investigate both of these tests? done!
Adrienne Walker
Comment 9 2012-07-25 12:58:49 PDT
Comment on attachment 154403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154403&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394 > +// Should be called just before the recursive calculateDrawTransformsInternal(). > +template<typename LayerType, typename LayerList> > +void setupRootLayerAndSurfaceForRecursion(LayerType* rootLayer, LayerList& renderSurfaceLayerList, const IntSize& deviceViewportSize) I had one last thought. Rather than commenting that this needs to be called at a certain time, why don't you just make it call calculateDrawTransformsInternal directly. Then, it could also do all the setup that you duplicate in the calculateDrawTransforms functions.
Shawn Singh
Comment 10 2012-07-25 13:15:35 PDT
(In reply to comment #9) > (From update of attachment 154403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154403&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:394 > > +// Should be called just before the recursive calculateDrawTransformsInternal(). > > +template<typename LayerType, typename LayerList> > > +void setupRootLayerAndSurfaceForRecursion(LayerType* rootLayer, LayerList& renderSurfaceLayerList, const IntSize& deviceViewportSize) > > I had one last thought. Rather than commenting that this needs to be called at a certain time, why don't you just make it call calculateDrawTransformsInternal directly. Then, it could also do all the setup that you duplicate in the calculateDrawTransforms functions. In part 2 of this, I'm expecting this function to go away. So, would it be OK to keep it as-is for this patch? If it turns out not to be possible to totally remove the root layer setup, then I'll change it at that time to this idea you're proposing - are you OK with that?
Adrienne Walker
Comment 11 2012-07-25 13:16:43 PDT
Comment on attachment 154403 [details] Patch R=me. Ok, sounds good.
Shawn Singh
Comment 12 2012-07-25 13:47:58 PDT
Committed r123654: <http://trac.webkit.org/changeset/123654>
Note You need to log in before you can comment on or make changes to this bug.