[chromium] Should schedule a commit when dropping contents textures

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Should schedule a commit when dropping contents textures

[James Robinson]

Created attachment 149649 [details]
Patch

[Adrienne Walker]

Can you explain more why this commit needs to be here and why the proxies behave differently?

[James Robinson]

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.

[Dana Jansens]

FWIW CCSingleThreadProxy::doComposite does use canDraw as well.

[James Robinson]

Oh! So it does...interesting

[James Robinson]

(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.

[Adrienne Walker]

(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.

[James Robinson]

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.

[Adrienne Walker]

(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.

[Adrienne Walker]

Comment on attachment 149649 [details]
Patch

R=me.

[James Robinson]

Comment on attachment 149649 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 149649 [details]
Patch

Clearing flags on attachment: 149649

Committed r121450: <http://trac.webkit.org/changeset/121450>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.