[chromium] A general mechanism for passing data into and out of appendQuads and quadCuller via CCAppendQuadsData
Created attachment 160771 [details] Patch
Comment on attachment 160771 [details] Patch Attachment 160771 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13613303
Oops I missed the new header, I'll rebase and upload again when 94957 lands.
Created attachment 160796 [details] Patch
Comment on attachment 160796 [details] Patch The CCQuadSinkData hierarchy feels a little bit overengineered to me and I think it's more complex than its benefits. I understand the need to reduce plumbing churn by putting common function signature parameters into a struct. I'd prefer to see something more simple like: struct CCAppendQuadsData { bool hadMissingTiles; bool hadOcclusionFromOutsideTargetSurface; // FIXME: Hold the delegatedRenderPassId for the DelegatedRendererLayer. }; I don't find it problematic that CCQuadCuller has a boolean about whether it has occlusion from outside the target surface. The quad culler is stateful about the quads that have been appended to it, and this is extra information that it has calculated. It feels no different from asking about occlusion/unocclusion in a rect or overdraw. What do you think about having CCQuadSink::append just take a bool& hadExternalOcclusion? Alternatively, CCQuadSink::hasExternalOcclusion()?
(In reply to comment #5) > (From update of attachment 160796 [details]) > The CCQuadSinkData hierarchy feels a little bit overengineered to me and I think it's more complex than its benefits. I understand the need to reduce plumbing churn by putting common function signature parameters into a struct. I'd prefer to see something more simple like: > > struct CCAppendQuadsData > { > bool hadMissingTiles; > bool hadOcclusionFromOutsideTargetSurface; > // FIXME: Hold the delegatedRenderPassId for the DelegatedRendererLayer. > }; > > I don't find it problematic that CCQuadCuller has a boolean about whether it has occlusion from outside the target surface. The quad culler is stateful about the quads that have been appended to it, and this is extra information that it has calculated. It feels no different from asking about occlusion/unocclusion in a rect or overdraw. > > What do you think about having CCQuadSink::append just take a bool& hadExternalOcclusion? Alternatively, CCQuadSink::hasExternalOcclusion()? I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset. We could just use a CCAppendQuadsData struct and have QuadSink take that. I was just trying to keep it more abstract-y.. for better or worse. Would that feel better to you?
(In reply to comment #6) > I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset. ...but CCQuadSink won't need an additional parameter in the future, though. CCLayerImpl::appendQuads could continue taking CCAppendQuadsData& which could expand later, but the underlying CCQuadSink::append function could just take a bool&.
(In reply to comment #7) > (In reply to comment #6) > > > I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset. > > ...but CCQuadSink won't need an additional parameter in the future, though. CCLayerImpl::appendQuads could continue taking CCAppendQuadsData& which could expand later, but the underlying CCQuadSink::append function could just take a bool&. Oh I see. But then every layer has to unpack the bool& from the struct in order to call append, instead of just passing along the forward-declared struct.
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > > > I feel like the hadExternalOcclusion information is very similar to hadMissingTiles, but they are channeled through completely different paths. I like append() taking a bool& more, but we're just getting more and more parameters to CCLayerImpl::appendQuads(quadSink, bool&, bool&, CCRenderPassList&) and so on into the sunset. > > > > ...but CCQuadSink won't need an additional parameter in the future, though. CCLayerImpl::appendQuads could continue taking CCAppendQuadsData& which could expand later, but the underlying CCQuadSink::append function could just take a bool&. > > Oh I see. But then every layer has to unpack the bool& from the struct in order to call append, instead of just passing along the forward-declared struct. It's true. Although, that way only CCLayerImpl really has to know about CCAppendQuadsData? If you don't like that, I think I'm also ok with your other suggestion of CCQuadSink just taking a CCAppendQuadsData&. It seems about equivalent.
Created attachment 160831 [details] Patch simplified to a single struct
Comment on attachment 160831 [details] Patch R=me.
Comment on attachment 160831 [details] Patch Clearing flags on attachment: 160831 Committed r126819: <http://trac.webkit.org/changeset/126819>
All reviewed patches have been landed. Closing bug.