RESOLVED FIXED 84122
[Chromium] Clean up texture ids on the impl side when losing the context
https://bugs.webkit.org/show_bug.cgi?id=84122
Summary [Chromium] Clean up texture ids on the impl side when losing the context
Antoine Labour
Reported 2012-04-16 19:43:03 PDT
[Chromium] Clean up texture ids on the impl side when losing the context
Attachments
Patch (4.59 KB, patch)
2012-04-16 19:46 PDT, Antoine Labour
no flags
Patch (11.31 KB, patch)
2012-04-16 21:59 PDT, Antoine Labour
no flags
Patch for landing (11.31 KB, patch)
2012-04-16 22:06 PDT, Antoine Labour
no flags
Patch for landing (11.31 KB, patch)
2012-04-17 16:58 PDT, Antoine Labour
no flags
Antoine Labour
Comment 1 2012-04-16 19:46:54 PDT
James Robinson
Comment 2 2012-04-16 19:52:20 PDT
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
Antoine Labour
Comment 3 2012-04-16 19:59:25 PDT
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.
Adrienne Walker
Comment 4 2012-04-16 20:09:45 PDT
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.
James Robinson
Comment 5 2012-04-16 20:12:14 PDT
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)
Adrienne Walker
Comment 6 2012-04-16 20:14:01 PDT
(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.
Antoine Labour
Comment 7 2012-04-16 21:59:41 PDT
James Robinson
Comment 8 2012-04-16 22:02:08 PDT
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
Antoine Labour
Comment 9 2012-04-16 22:06:43 PDT
Created attachment 137478 [details] Patch for landing
WebKit Review Bot
Comment 10 2012-04-16 22:34:55 PDT
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
James Robinson
Comment 11 2012-04-17 10:28:56 PDT
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
Antoine Labour
Comment 12 2012-04-17 16:58:16 PDT
Created attachment 137639 [details] Patch for landing
WebKit Review Bot
Comment 13 2012-04-17 17:53:00 PDT
Comment on attachment 137639 [details] Patch for landing Clearing flags on attachment: 137639 Committed r114473: <http://trac.webkit.org/changeset/114473>
WebKit Review Bot
Comment 14 2012-04-17 17:53:12 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.