Allow ui::Compositor to disable commits during tab-switch

Allow ui::Compositor to defer commits during tab-switch

Do this by using a ui::CompositorLock. For Mac, we don't care about making
sure that the compositor doesn't hang waiting for renderer frames (because
the browser compositor doesn't draw the UI), so disable the lock timeout.

Also, re-create the browser compositor immediately after the tab is made
visible. This way, if the DelegatedFrameHost has an un-evicted old frame,
then that frame will be displayed immediately (and if not, the compositor
will remain locked until a new frame is swapped in).

Background:

When we switched the ui::Compositor to use the scheduler (done
in https://codereview.chromium.org/638653003), we started hitting
issues where unexpected frames would flash on Mac during tab-switch.

The reason for this is that, prior to the change, calling
SetRootLayer(null) on the ui::Compositor would prevent it from drawing
any new frames.

The Mac was relying on this behavior to prevent transient frames from
appearing. The transient frames appear during tab-switch on Mac because
the compositor takes damage (as its root layer is changed) and also
because the compositor is resized (among potentially other things).

These frames do not actually appear on-screen (there are mechanisms
to prevent this), but they waste valuable time during tab-switch (the
browser allows only 50 msec for a new frame to appear on-screen before
it flashes white, and this extra unused frame causes us to miss the
deadline).

With this patch, the frequency of the white flash during tab-switch is
reduced substantially.

BUG=463988
Committed: https://crrev.com/00e438cd22c5e2f26d003c15c0a1621aa24acd0c
Cr-Commit-Position: refs/heads/master@{#320241}

[ccameron, 2015-03-09 19:56:12]

ccameron@chromium.org changed reviewers:
+ danakj@chromium.org, weiliangc@chromium.org

[ccameron, 2015-03-09 20:20:03]

LMK if this is even a reasonable thing to consider doing (in the context of
ui::Compositor & LTH world). If it is, I can beef it up with tests, etc.

Of note is that it appears that the threaded proxy can use the
ui::CompositorLock for purposes similar to this (it claims not to work for
single thread, and has also a timeout, which is undesirable for our purposes
here).

This patch is targeted for merge to M42 (cause tab switch performance is pretty
bad there).

https://codereview.chromium.org/996453002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/996453002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/compositor_impl_android.cc:612: bool
CompositorImpl::ScheduleComposite() {
It may be that this is a place where returning false would be desirable as well?

[danakj, 2015-03-10 17:23:58]

On 2015/03/09 20:20:03, ccameron wrote:
> LMK if this is even a reasonable thing to consider doing (in the context of
> ui::Compositor & LTH world). If it is, I can beef it up with tests, etc.
> 
> Of note is that it appears that the threaded proxy can use the
> ui::CompositorLock for purposes similar to this (it claims not to work for
> single thread, and has also a timeout, which is undesirable for our purposes
> here).

FWIW ThreadProxy is only used in the renderer compositor.

[danakj, 2015-03-10 17:25:43]

Why not use DeferCommits?

[ccameron, 2015-03-11 00:28:49]

On 2015/03/10 17:25:43, danakj wrote:
> Why not use DeferCommits?

That's the path that said it only worked in threaded mode (it's what the lock
calls). But... it turns out that comment is a lie (or obsolete) -- updated the
patch to just use DeferCommits instead (and fix the comment).

[danakj, 2015-03-11 17:22:55]

https://codereview.chromium.org/996453002/diff/20001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/996453002/diff/20001/cc/trees/proxy.h#newcode84
cc/trees/proxy.h:84: // scheduler. It's an error to make a sync call like
CompositeAndReadback
Ahaha CompositeAndReadback is no more, missed this comment. Can you just remove
the whole "It's an error" bit?

https://codereview.chromium.org/996453002/diff/20001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/996453002/diff/20001/ui/compositor/compositor...
ui/compositor/compositor.cc:329: host_->SetDeferCommits(defer_commits);
You'll now have a state where commits are deferred but IsLocked() will return
false. Is that going to be weird?

I was expecting that you used the lock but just let your code remove the
timeout. But maybe this is okay. Can you give me some confidence in this
potential pitfall?

[danakj, 2015-03-11 17:23:12]

https://codereview.chromium.org/996453002/diff/20001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/996453002/diff/20001/cc/BUILD.gn#newcode516
cc/BUILD.gn:516: "trees/layer_tree_host_single_thread_client.h",
Looks like these buildfile changes aren't needed anymore

[ccameron, 2015-03-11 21:46:43]

Update to use the ui::CompositorLock.

I added a mode where you can tell the compositor "don't make my locks time out"
-- we only have them time out so that the browser UI doesn't hang, but Mac
doesn't draw the browser UI with the compositor, so that's not an issue. Added
some tests cause none exist (gulp).

The other really really nice thing about using the compositor lock is that the
locks taken by the browser compositor view play nice with the locks taken by the
DelegatedFrameHost. In particular, the DFH will keep the compositor locked until
a frame (new or cached) is available, so the RWHVMac can safely un-lock the
compositor once DFH::WasShown is called.

https://codereview.chromium.org/996453002/diff/20001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/996453002/diff/20001/cc/BUILD.gn#newcode516
cc/BUILD.gn:516: "trees/layer_tree_host_single_thread_client.h",
On 2015/03/11 17:23:12, danakj wrote:
> Looks like these buildfile changes aren't needed anymore

True, not needed, but IIUC these .h files should have been here to begin with
(so might as well take the opportunity to add them).

https://codereview.chromium.org/996453002/diff/20001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/996453002/diff/20001/cc/trees/proxy.h#newcode84
cc/trees/proxy.h:84: // scheduler. It's an error to make a sync call like
CompositeAndReadback
On 2015/03/11 17:22:55, danakj wrote:
> Ahaha CompositeAndReadback is no more, missed this comment. Can you just
remove
> the whole "It's an error" bit?

Done.

https://codereview.chromium.org/996453002/diff/20001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/996453002/diff/20001/ui/compositor/compositor...
ui/compositor/compositor.cc:329: host_->SetDeferCommits(defer_commits);
On 2015/03/11 17:22:55, danakj wrote:
> You'll now have a state where commits are deferred but IsLocked() will return
> false. Is that going to be weird?
> 
> I was expecting that you used the lock but just let your code remove the
> timeout. But maybe this is okay. Can you give me some confidence in this
> potential pitfall?

The timeout for the lock is only so that the UI doesn't hang ... that's not a
concern for Mac (ui::Compositor doesn't draw the UI), so I added something a
flag to disable the timeout and just use a CompositorLock.

[danakj, 2015-03-12 00:07:15]

Cool! I'm glad this is working nicely

https://codereview.chromium.org/996453002/diff/60001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/996453002/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:50:
compositor_suspended_lock_ = compositor_.GetCompositorLock();
nit: Call Suspend() ?

https://codereview.chromium.org/996453002/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:60:
compositor_suspended_lock_ = NULL;
nullptr

https://codereview.chromium.org/996453002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/996453002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:848:
EnsureBrowserCompositorView();
How come this is needed?

https://codereview.chromium.org/996453002/diff/60001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/996453002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:356: scoped_refptr<CompositorLock>
Compositor::GetCompositorLock() {
Rather than adding state to ui::Compositor class, how about
GetCompositorLockWithoutTimeout() and just pass false to the CompositorLock?

[ccameron, 2015-03-12 01:01:35]

Thanks -- updated. Let me know what you'd like to do about the lock behavior 
ui::Compositor state (I have a preference for it to be ui::Compositor state,
though how and where that is specified is something I can see either way).

https://codereview.chromium.org/996453002/diff/60001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/996453002/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:50:
compositor_suspended_lock_ = compositor_.GetCompositorLock();
On 2015/03/12 00:07:15, danakj wrote:
> nit: Call Suspend() ?

Done.

https://codereview.chromium.org/996453002/diff/60001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:60:
compositor_suspended_lock_ = NULL;
On 2015/03/12 00:07:15, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/996453002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/996453002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:848:
EnsureBrowserCompositorView();
On 2015/03/12 00:07:15, danakj wrote:
> How come this is needed?

Oh -- I added the explanation to the CL description, but to any comments:

  Also, re-create the browser compositor immediately after the tab is made
  visible. This way, if the DelegatedFrameHost has an un-evicted old frame,
  then that frame will be displayed immediately (and if not, the compositor
  will remain locked until a new frame is swapped in).

Also added a comment to this effect.

https://codereview.chromium.org/996453002/diff/60001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/996453002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:356: scoped_refptr<CompositorLock>
Compositor::GetCompositorLock() {
On 2015/03/12 00:07:15, danakj wrote:
> Rather than adding state to ui::Compositor class, how about
> GetCompositorLockWithoutTimeout() and just pass false to the CompositorLock?

This was the approach that I first went with, but then I realized that I want
"whether or not locks to time out" to be a property of the ui::Compositor class.
The reason is that things like the DelegatedFrameHost will create locks, and I
will want them to be WithoutTimeout on Mac and WithTimeout on Aura.

I considered adding a LockBehavior property to the constructor of the
ui::Compositor (because there's really no reason for it to be mutable state),
but went against that because I wasn't sure if this smallish property warranted
that esteemed a position. Do you think that having it as a ctor-arg would be
better?

[danakj, 2015-03-12 01:08:50]

LGTM

https://codereview.chromium.org/996453002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/996453002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:848:
EnsureBrowserCompositorView();
On 2015/03/12 01:01:34, ccameron wrote:
> On 2015/03/12 00:07:15, danakj wrote:
> > How come this is needed?
> 
> Oh -- I added the explanation to the CL description, but to any comments:
> 
>   Also, re-create the browser compositor immediately after the tab is made
>   visible. This way, if the DelegatedFrameHost has an un-evicted old frame,
>   then that frame will be displayed immediately (and if not, the compositor
>   will remain locked until a new frame is swapped in).
> 
> Also added a comment to this effect.

Thanks :)

[ccameron, 2015-03-12 05:41:35]

Thanks!

[ccameron, 2015-03-12 05:41:40]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-03-12 05:41:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/996453002/80001

[commit-bot: I haz the power, 2015-03-12 06:18:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-12 06:18:47]

Patchset 5 (id:??) landed as
https://crrev.com/00e438cd22c5e2f26d003c15c0a1621aa24acd0c
Cr-Commit-Position: refs/heads/master@{#320241}