Bug 90031 - [chromium] Should schedule a commit when dropping contents textures
Summary: [chromium] Should schedule a commit when dropping contents textures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-26 17:56 PDT by James Robinson
Modified: 2012-06-28 12:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.12 KB, patch)
2012-06-26 17:57 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-06-26 17:56:16 PDT
[chromium] Should schedule a commit when dropping contents textures
Comment 1 James Robinson 2012-06-26 17:57:57 PDT
Created attachment 149649 [details]
Patch
Comment 2 Adrienne Walker 2012-06-28 09:36:01 PDT
Can you explain more why this commit needs to be here and why the proxies behave differently?
Comment 3 James Robinson 2012-06-28 10:39:35 PDT
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.
Comment 4 Dana Jansens 2012-06-28 10:43:01 PDT
FWIW CCSingleThreadProxy::doComposite does use canDraw as well.
Comment 5 James Robinson 2012-06-28 10:43:32 PDT
Oh! So it does...interesting
Comment 6 James Robinson 2012-06-28 10:58:40 PDT
(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.
Comment 7 Adrienne Walker 2012-06-28 11:27:22 PDT
(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.
Comment 8 James Robinson 2012-06-28 11:29:16 PDT
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.
Comment 9 Adrienne Walker 2012-06-28 12:12:26 PDT
(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 10 Adrienne Walker 2012-06-28 12:12:39 PDT
Comment on attachment 149649 [details]
Patch

R=me.
Comment 11 James Robinson 2012-06-28 12:20:27 PDT
Comment on attachment 149649 [details]
Patch

Thanks!
Comment 12 WebKit Review Bot 2012-06-28 12:28:02 PDT
Comment on attachment 149649 [details]
Patch

Clearing flags on attachment: 149649

Committed r121450: <http://trac.webkit.org/changeset/121450>
Comment 13 WebKit Review Bot 2012-06-28 12:28:07 PDT
All reviewed patches have been landed.  Closing bug.