[chromium] Defer managed texture creation and destruction until updateCompositorResources
Created attachment 101255 [details] Patch
Created attachment 101947 [details] Patch
Comment on attachment 101947 [details] Patch I like the split but it worries me a bit that we have to pass a pointer to a GraphicsContext3D every time we need to do the real GL operation. This allows for inconsistent context usage, like for example creating a texture with one context and deleting it with another. Since we know that every manager is dealing with only one context, how about we keep the context as a member of the manager (doesn't have to be set by the constructor, just needs to be there before any actual GL operation takes place)?
The reason I didn't do that was that I want to start being more careful about when we use the compositor context so that it'll be easier to enforce the threading constraints on the context. The way I've been thinking to enforce this is to limit access to the GraphicsContext3D pointer itself and guard places that are passed the pointer with ASSERT()s, since I can't put ASSERT()s in the GraphicsContext3D interface itself. This does depend on the TextureManager always being passed in the same GraphicsContext3D*, but I'm not totally sure what to do about that. It could be that I'm being overly paranoid here about people misusing GraphicsContext3Ds that are member functions.
(In reply to comment #4) > It could be that I'm being overly paranoid here about people misusing GraphicsContext3Ds that are member functions. member variables that is
(In reply to comment #3) > (From update of attachment 101947 [details]) > I like the split but it worries me a bit that we have to pass a pointer to a GraphicsContext3D every time we need to do the real GL operation. This allows for inconsistent context usage, like for example creating a texture with one context and deleting it with another. On the other hand, in the previous world, we could be binding with the same GraphicsContext3D that we created with, but it would be the current one to draw with because some LayerTexture hadn't been updated to use a new one. I think the consistency issues aren't increased here, they've just been shifted around. We could always keep around the GraphicsContext3D pointer in debug builds and assert that we are always using a consistent one for each LayerTexture. That's kind of messy, but it would catch errors like this.
Created attachment 103550 [details] Patch
This adds a GraphicsContext3D* in debug only so we can ASSERT() that we aren't mixing up our contexts. I've also renamed LayerTexture since there are layer and non-layer users of it now (RenderSurfaces and the HUD use the class as well). Another thought I had as a potential follow-up was to delegate the creation/destruction logic out to a client interface so I don't have to make TextureManager aware of how the actual resources are made and destroyed. Then TextureManager would just be responsible for keeping track of resource use and deciding who gets evicted when under pressure. That would also make it easier to write some C++ unit tests for the class, which is something I should have done ages ago.
(In reply to comment #8) > This adds a GraphicsContext3D* in debug only so we can ASSERT() that we aren't mixing up our contexts. I've also renamed LayerTexture since there are layer and non-layer users of it now (RenderSurfaces and the HUD use the class as well). Awesome. This patch looks like a great refactoring. > Another thought I had as a potential follow-up was to delegate the creation/destruction logic out to a client interface so I don't have to make TextureManager aware of how the actual resources are made and destroyed. Then TextureManager would just be responsible for keeping track of resource use and deciding who gets evicted when under pressure. That would also make it easier to write some C++ unit tests for the class, which is something I should have done ages ago. This also sounds like a great idea.
Can I get an official review+?
Comment on attachment 103550 [details] Patch Looks good to me.
Comment on attachment 103550 [details] Patch Clearing flags on attachment: 103550 Committed r92900: <http://trac.webkit.org/changeset/92900>
All reviewed patches have been landed. Closing bug.