[Chromium] Clean up texture ids on the impl side when losing the context
Created attachment 137466 [details] Patch
Comment on attachment 137466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137466&action=review Looks like the correct code but needs tests to prevent regressions > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) this won't fly (will fail an SVN presubmit check). Seems like it shouldn't be too hard to construct a unit test for this - just magick up a CCLayerTreeHostImpl, stick an instance of each CCLayerImpl type touched here into the tree, synthesize a lost context, then recreate with a mock context that explodes if you try to bind a non-zero texture and do a draw pass
Yep, sounds good - I was starting to look into how to best add a test, your suggestion sounds excellent. Enne: could you give a look at the CCTiledLayerImpl to make sure I'm not doing anything incredibly stupid. In particular, my understanding is that we're ensured that m_tiler is != NULL when we get a lost context, but wanted to double-check on that.
Comment on attachment 137466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137466&action=review > Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:240 > + m_tiler->reset(); I think if you lose the context prior to the first commit, m_tiler would be null. We should probably just always create a tiler to not need these sorts of checks.
The CCTiledLayerImpl wouldn't exist in the impl tree until the first commit, and after the commit we always do a pushPropertiesTo() before trying to draw it. So I think it'll always be there by the time we can notice a lost context (although perhaps this is a fragile assumption)
(In reply to comment #5) > The CCTiledLayerImpl wouldn't exist in the impl tree until the first commit, and after the commit we always do a pushPropertiesTo() before trying to draw it. So I think it'll always be there by the time we can notice a lost context (although perhaps this is a fragile assumption) Hmm, I'll buy that argument. It still seems a little sketchy in general, but should be ok here.
Created attachment 137476 [details] Patch
Comment on attachment 137476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137476&action=review R=me > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1157 > +// ressource that wasn't created by it (resources created by typo ressource -> resource
Created attachment 137478 [details] Patch for landing
Comment on attachment 137478 [details] Patch for landing Attachment 137478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12419363
Comment on attachment 137478 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=137478&action=review > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1265 > +class FakeVideoFrameProvider: public VideoFrameProvider { Looks like you collided with http://trac.webkit.org/changeset/114335 - you want to implement WebKit::WebVideoFrameProvider
Created attachment 137639 [details] Patch for landing
Comment on attachment 137639 [details] Patch for landing Clearing flags on attachment: 137639 Committed r114473: <http://trac.webkit.org/changeset/114473>
All reviewed patches have been landed. Closing bug.