Summary: | [chromium] Fix scrollbar layers holding onto invalid textures after lost context | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrienne Walker <enne> | ||||||
Component: | WebCore Misc. | Assignee: | Adrienne Walker <enne> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cc-bugs, enne, jamesr, nduca, trchen, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Adrienne Walker
2012-03-21 15:52:50 PDT
Created attachment 133148 [details]
Patch
W00t Comment on attachment 133148 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133148&action=review > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:98 > + m_texture.clear(); This texture's managed by the LRC::m_renderSurfaceTextureManager, and we rebuild the LRC after a lost context so this texture should go bogus. Do we just need a setTextureManager() call before the reserve() in willDraw(), mayhaps? didLostContext should only be for non-managed resources. (In reply to comment #3) > (From update of attachment 133148 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133148&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:98 > > + m_texture.clear(); > > This texture's managed by the LRC::m_renderSurfaceTextureManager, and we rebuild the LRC after a lost context so this texture should go bogus. Do we just need a setTextureManager() call before the reserve() in willDraw(), mayhaps? > > didLostContext should only be for non-managed resources. Ok, sure. It's the same difference from the scrollbar layer's perspective. Clearing the texture seemed slightly cleaner since there won't ever be an invalid texture manager, but I guess CCVideoLayerImpl does what you say above already. I can do that. Created attachment 133181 [details]
Use setTextureManager
Comment on attachment 133181 [details]
Use setTextureManager
Thanks. This isn't really much different, but I have wild dreams sometimes of making this a bit more automagic so you don't have to explicitly do this hookup before using the texture - maybe by having the reserve call be on some object that can take care of these details.
Comment on attachment 133181 [details]
Use setTextureManager
Gonna put this on cq so folks evaluating impl thread tomorrow can get this in their morning builds.
Comment on attachment 133181 [details] Use setTextureManager Rejecting attachment 133181 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: patching file Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp patching file Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp Hunk #1 FAILED at 25. Hunk #2 succeeded at 981 (offset -103 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12070863 Whommmmp Comment on attachment 133181 [details] Use setTextureManager View in context: https://bugs.webkit.org/attachment.cgi?id=133181&action=review > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:28 > +#include "cc/CCQuadCuller.h" was this on purpose? Comment on attachment 133181 [details] Use setTextureManager Clearing flags on attachment: 133181 Committed r111708: <http://trac.webkit.org/changeset/111708> All reviewed patches have been landed. Closing bug. (In reply to comment #10) > (From update of attachment 133181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=133181&action=review > > > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:28 > > +#include "cc/CCQuadCuller.h" > > was this on purpose? Oops, I had originally done this test without a CCLTHI, so that's just vestigial. Thanks for landing this. |