[chromium] Should schedule a commit when dropping contents textures
Created attachment 149649 [details] Patch
Can you explain more why this commit needs to be here and why the proxies behave differently?
Without this commit we have a bit of stale state in CCLayerTreeHostImpl::canDraw(), which is used only by the thread scheduler. In single threaded mode, render_widget forces us to make a frame no matter what our state is so we do a commit regardless.
FWIW CCSingleThreadProxy::doComposite does use canDraw as well.
Oh! So it does...interesting
(In reply to comment #4) > FWIW CCSingleThreadProxy::doComposite does use canDraw as well. In CCSingleThreadProxy commitAndComposite() does an unconditional doCommit(), then calls doComposite() which checks canDraw. Actually though I was wrong that this is why the proxies behave differently for this patch. If we did a commit in threaded mode we would reset the canDraw state as well. In single threaded mode, we get a call to commitAndComposite() from render_widget whenever we become visible. In threaded mode, the scheduler won't schedule a commit unless something tells it to. That's what this patch does.
(In reply to comment #6) > (In reply to comment #4) > > FWIW CCSingleThreadProxy::doComposite does use canDraw as well. > > In CCSingleThreadProxy commitAndComposite() does an unconditional doCommit(), then calls doComposite() which checks canDraw. > > Actually though I was wrong that this is why the proxies behave differently for this patch. If we did a commit in threaded mode we would reset the canDraw state as well. In single threaded mode, we get a call to commitAndComposite() from render_widget whenever we become visible. In threaded mode, the scheduler won't schedule a commit unless something tells it to. That's what this patch does. Should we do something parallel here, like calling setNeedsCommit() when we become visible? Maybe I just want more symmetry, and it seems a little weird to have this extra commit to notify the main thread that the contents textures earlier so we can clear a flag.
It's not quite just that, we actually do need to do a commit and paint, upload, etc if we've dropped contents textures. I don't think we want to do setNeedsCommit() unconditionally when becoming visible - if we become visible but we haven't evicted anything (say you're switching back and forth rapidly between two tabs), then we should just put a frame up. There's no need for a commit in that case, unless something else has changed.
(In reply to comment #8) > It's not quite just that, we actually do need to do a commit and paint, upload, etc if we've dropped contents textures. > > I don't think we want to do setNeedsCommit() unconditionally when becoming visible - if we become visible but we haven't evicted anything (say you're switching back and forth rapidly between two tabs), then we should just put a frame up. There's no need for a commit in that case, unless something else has changed. Ah, ok. The piece I was missing was that we also don't commit when not visible, so this commit gets saved for when we need it and come visible again. Sounds good.
Comment on attachment 149649 [details] Patch R=me.
Comment on attachment 149649 [details] Patch Thanks!
Comment on attachment 149649 [details] Patch Clearing flags on attachment: 149649 Committed r121450: <http://trac.webkit.org/changeset/121450>
All reviewed patches have been landed. Closing bug.