[chromium] Create sharedQuadState at same time as creating quads and give them to the quadSink
Created attachment 160027 [details] Patch
Comment on attachment 160027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160027&action=review > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:101 > +int CCQuadCuller::generateSharedQuadStateId() const > +{ > + return m_sharedQuadStateList.size(); > +} A potential foot gun is that you might think you could call generateSharedQuadStateId() n times and then add n states, which is wrong. As a potential suggestion, what about always having to appendSharedQuadState first prior to appending quads. That function sets the id internally on the shared quad state and returns a raw pointer to the modified shared quad state (for convenience, since you released the other pointer). You would use this pointer to pass to quads that you append (since you set the shared quad state id on quads and need it there). Maybe CCDrawQuad could assert if you gave it a shared quad state that hadn't been appended yet. At least that'd be a runtime check that you'd done the right thing.
Comment on attachment 160027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160027&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:101 >> +} > > A potential foot gun is that you might think you could call generateSharedQuadStateId() n times and then add n states, which is wrong. > > As a potential suggestion, what about always having to appendSharedQuadState first prior to appending quads. That function sets the id internally on the shared quad state and returns a raw pointer to the modified shared quad state (for convenience, since you released the other pointer). You would use this pointer to pass to quads that you append (since you set the shared quad state id on quads and need it there). Maybe CCDrawQuad could assert if you gave it a shared quad state that hadn't been appended yet. At least that'd be a runtime check that you'd done the right thing. Brilliant. I was not satisfied with this either but was not sure what to do with it, this is perfect. Thanks.
Created attachment 160206 [details] Patch
Done!
Comment on attachment 160206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160206&action=review > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:106 > + m_currentSharedQuadState = sharedQuadState.get(); Why useSharedQuadState + this stateful 'current' variable only used for an assert vs. just appendQuadState? If you're worried about writing an ASSERT, you could always just make sure that the passDrawQuad's quad state id in the quad state list has the same pointer as the passDrawQuad's quad state pointer?
Comment on attachment 160206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160206&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:106 >> + m_currentSharedQuadState = sharedQuadState.get(); > > Why useSharedQuadState + this stateful 'current' variable only used for an assert vs. just appendQuadState? If you're worried about writing an ASSERT, you could always just make sure that the passDrawQuad's quad state id in the quad state list has the same pointer as the passDrawQuad's quad state pointer? Actually I was thinking ahead to dropping the sharedQuadState if all quads for it were culled. So only allowing you to append quads when the sharedQuadState is active. Since this patch doesn't do that though, it doesn't need the m_current thing either, true.
(In reply to comment #7) > (From update of attachment 160206 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160206&action=review > > >> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:106 > >> + m_currentSharedQuadState = sharedQuadState.get(); > > > > Why useSharedQuadState + this stateful 'current' variable only used for an assert vs. just appendQuadState? If you're worried about writing an ASSERT, you could always just make sure that the passDrawQuad's quad state id in the quad state list has the same pointer as the passDrawQuad's quad state pointer? > > Actually I was thinking ahead to dropping the sharedQuadState if all quads for it were culled. So only allowing you to append quads when the sharedQuadState is active. Since this patch doesn't do that though, it doesn't need the m_current thing either, true. Ohhh, that makes a lot more sense.
Comment on attachment 160206 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160206&action=review R=me, with a few nits. > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:87 > + ASSERT(passDrawQuad->sharedQuadState() == m_currentSharedQuadState); Could you also assert here that m_sharedQuadStateList[passDrawQuad->sharedQuadStateId()] == m_currentSharedQuadState and that m_currentSharedQuadState->id() == passDrawQuad->sharedQuadStateId()? There's a lot of duplicate state running around (which I think is necessary), but I just want extra sanity checks that everything is as expected. > Source/WebCore/platform/graphics/chromium/cc/CCQuadSink.h:44 > + virtual CCSharedQuadState* useSharedQuadState(PassOwnPtr<CCSharedQuadState>) = 0; Add a comment here describing that this needs to be set prior to calling append with a quad using it?
Created attachment 160218 [details] Patch Ok I added some more asserts, thanks. I don't want to assert that id == array index, because it doesn't have to be. So instead I assert that it's the last thing in the array (it's current so it should be most recently added). I stuck the comment in CCQuadSink (and added the missing "CCQuadSink implementation" comment to CCQuadCuller).
Comment on attachment 160218 [details] Patch Clearing flags on attachment: 160218 Committed r126482: <http://trac.webkit.org/changeset/126482>
All reviewed patches have been landed. Closing bug.