Plumb SetVisible from ui::Compositor to the DirectRenderer.

Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6f2861b9c957c7374a2a2c084c6cafba01b5e5d6
Cr-Commit-Position: refs/heads/master@{#412421}

[danakj, 2016-08-10 23:02:59]

Description was changed from

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
BUG=606056
==========

to

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-10 23:03:02]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-08-10 23:03:16]

danakj@chromium.org changed reviewers:
+ fsamuel@chromium.org

[danakj, 2016-08-10 23:03:16]

+fsamuel for mus

[danakj, 2016-08-10 23:03:18]

+fsamuel for mus

[danakj, 2016-08-10 23:04:07]

Description was changed from

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-10 23:04:07]

danakj@chromium.org changed reviewers:
+ sky@chromium.org
- fsamuel@chromium.org

[commit-bot: I haz the power, 2016-08-10 23:04:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-10 23:04:33]

On 2016/08/10 23:03:18, danakj wrote:
> +fsamuel for mus

Oh, sorry, no owners there. +sky then.

[danakj, 2016-08-10 23:05:26]

https://codereview.chromium.org/2238693002/diff/1/cc/output/renderer.h
File cc/output/renderer.h (right):

https://codereview.chromium.org/2238693002/diff/1/cc/output/renderer.h#newcode90
cc/output/renderer.h:90: : client_(client), settings_(settings), visible_(false)
{}
This is a change in the Renderer, I guess it defaulting true is what made things
work. LTH/Scheduler/etc all defaults to false so this should match. I might
break a million cc unit tests tho woops. Better fix.

[danakj, 2016-08-10 23:36:41]

This is a bit more complicated, arg. We need to restore visible when the output
surface is lost, and we can't set it on the Renderer when we make the Display
cuz it's not made until later so we end up caching the visible state in like 4
places.

[commit-bot: I haz the power, 2016-08-10 23:50:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 23:50:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-08-11 00:02:11]

danakj@chromium.org changed reviewers:
+ enne@chromium.org

[danakj, 2016-08-11 00:02:11]

So ok this is more like it. It's an exercise for the reader if this is worth or
I should delete all of the visible code in the Renderer instead.

[enne (OOO), 2016-08-11 00:04:19]

Here's an alternate thought that I'm not sure is ideal.  What about just
dropping the output surface (with display and backbuffer and anything the
command buffer is holding onto) when it goes invisible instead?

[danakj, 2016-08-11 00:08:37]

On 2016/08/11 00:04:19, enne wrote:
> Here's an alternate thought that I'm not sure is ideal.  What about just
> dropping the output surface (with display and backbuffer and anything the
> command buffer is holding onto) when it goes invisible instead?

Theres so many places that Hide (tho I don't think when we minimize a window is
one of them.. rip), so that seems like a cool idea. No point in holding a
context around for everything that has a ui::Compositor but isnt visible. Things
like app launchers, and toast notification windows and stuff.

[piman, 2016-08-11 04:01:34]

LGTM

re: discarding the OutputSurface (and I assume the context), it feels more
involved - (re)creating a context involves many round-trips, whereas we should
already be able to free most of the command buffer resources provided we call
SetAggressivelyFreeResources. I think we've also had issues on some drivers when
trying to recreate a GLSurface on a window that had one in the past.
I suggest starting with this CL, and maybe explore discarding the OutputSurface
separately?

[danakj, 2016-08-11 16:57:25]

Description was changed from

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-11 16:57:30]

Sounds good.

[danakj, 2016-08-11 16:57:35]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2016-08-11 16:57:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 17:12:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 17:12:55]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[danakj, 2016-08-11 22:15:04]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-08-11 22:15:05]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2238693002/#ps40001
(title: "setvisible-browser: typo")

[commit-bot: I haz the power, 2016-08-11 22:16:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 22:37:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 22:37:47]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[danakj, 2016-08-13 00:22:15]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 00:23:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-13 00:24:05]

So ok I fixed the InProcessContextFactory for tests, to track the visible state
and set it on the Display when it is created.

However I realize that the DisplayScheduler doesn't do anything about being
invisible... so if the renderer swapped a frame after ui::Compositor was set
invisible (which it surely can, it's a race at best), we'll try to draw it but
the renderer is not visible now, which we super don't expect. So..

[commit-bot: I haz the power, 2016-08-13 00:53:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 00:53:14]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[danakj, 2016-08-13 01:05:43]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 01:06:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-13 01:12:28]

Patchset #5 (id:80001) has been deleted

[danakj, 2016-08-13 01:12:45]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 01:13:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-13 01:37:03]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 01:37:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-13 01:38:57]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 01:39:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-13 01:39:43]

danakj@chromium.org changed reviewers:
+ sievers@chromium.org

[danakj, 2016-08-13 01:39:43]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-08-13 01:39:44]

OK So now the DisplayScheduler stops trying to draw when not visible. And we
dcheck visible in DirectRenderer::DrawFrame.

Idk how this will interact with android trying to draw when things are shutting
down tho. What if it's not visible? Should it become visible? Maybe it's ok if
not visible to not draw? Maybe we're always visible? +sievers

[danakj, 2016-08-13 01:40:08]

danakj@chromium.org changed reviewers:
+ jbauman@chromium.org, sunnyps@chromium.org

[danakj, 2016-08-13 01:40:09]

On 2016/08/13 01:39:44, danakj wrote:
> OK So now the DisplayScheduler stops trying to draw when not visible. And we
> dcheck visible in DirectRenderer::DrawFrame.

+sunnyps,jbauman also.

> Idk how this will interact with android trying to draw when things are
shutting
> down tho. What if it's not visible? Should it become visible? Maybe it's ok if
> not visible to not draw? Maybe we're always visible? +sievers

[danakj, 2016-08-13 02:21:04]

Patchset #7 (id:140001) has been deleted

[danakj, 2016-08-13 02:21:31]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 02:21:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-13 02:25:17]

I added/changed some DisplayScheduler tests for dealing with visibility.

[danakj, 2016-08-13 02:26:40]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 02:26:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 03:10:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 03:10:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[piman, 2016-08-15 20:33:17]

lgtm

[jbauman, 2016-08-15 21:37:25]

lgtm

On 2016/08/13 01:40:09, danakj wrote:
> On 2016/08/13 01:39:44, danakj wrote:
> > OK So now the DisplayScheduler stops trying to draw when not visible. And we
> > dcheck visible in DirectRenderer::DrawFrame.
> 
> +sunnyps,jbauman also.
> 
> > Idk how this will interact with android trying to draw when things are
> shutting
> > down tho. What if it's not visible? Should it become visible? Maybe it's ok
if
> > not visible to not draw? Maybe we're always visible? +sievers

Whenever the browser goes non-visible on android then it completely tears down
the output surface and cc::Display (in CompositorImpl::SetVisible). So this
doesn't really change the behavior there.

[sunnyps, 2016-08-15 23:20:03]

Left a few comments. All the SetVisible(true) calls make me think that visible =
true should be the default value.

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:336: missed_begin_frame_task_.Cancel();
nit: move the missed task cancellation to StopObservingBeginFrames.

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler_unittest.cc:285: EXPECT_EQ(1u,
fake_begin_frame_source_.num_observers());
Should the scheduler ask for begin frames if we have no damaged surfaces? This
seems like it would keep the frame source ticking forever which is bad for power
consumption.

https://codereview.chromium.org/2238693002/diff/200001/ui/compositor/test/in_...
File ui/compositor/test/in_process_context_factory.h (right):

https://codereview.chromium.org/2238693002/diff/200001/ui/compositor/test/in_...
ui/compositor/test/in_process_context_factory.h:99: bool visible;
I think you should initialize visible here? (and maybe even display)

[danakj, 2016-08-15 23:47:46]

On Mon, Aug 15, 2016 at 4:20 PM, <sunnyps@chromium.org> wrote:

> Left a few comments. All the SetVisible(true) calls make me think that
> visible =
> true should be the default value.
>

Visible = false is the default in cc::Scheduler, and we don't start the
scheduler/request an output surface/etc until it becomes visible. This
replaced the SetLayerTreeHostClientReady signal we had previously. The
DirectRenderer was not matching that, but I prefer if it does, that seems
to make more sense to me. The DisplayScheduler could differ, but similar to
DirectRenderer it would make the first call to SetVisible(true) do
something in cc:Scheduler but noop/early out in DirectRenderer and
DisplayScheduler, which seems surprising. Thoughts?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-08-16 00:35:50]

Patchset #11 (id:240001) has been deleted

[danakj, 2016-08-16 00:36:00]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 00:36:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-16 00:37:23]

sunnyps: PTAL

piman: PTAL.
- I've made ui/compositor DEPS allow khronos/GLES2/gl2.h for the unit test fix
(using GL_TEXTURE_2D) in layer_unittest.cc.
- I've stopped caching the visible state in GPTF at all.
- I've moved the SetDisplayVisible call from 2 places that call
CreateOutputSurface to the singular SetOutputSurface, since it doesn't matter
until at least there.

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:336: missed_begin_frame_task_.Cancel();
On 2016/08/15 23:20:03, sunnyps wrote:
> nit: move the missed task cancellation to StopObservingBeginFrames.

Done.

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/200001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler_unittest.cc:285: EXPECT_EQ(1u,
fake_begin_frame_source_.num_observers());
On 2016/08/15 23:20:03, sunnyps wrote:
> Should the scheduler ask for begin frames if we have no damaged surfaces? This
> seems like it would keep the frame source ticking forever which is bad for
power
> consumption.

I think it would only do one, and then in the deadline stop. But there's no
reason to even tick that once right? We can just wait for some damage. So I've
done that and made tests for it.

https://codereview.chromium.org/2238693002/diff/200001/ui/compositor/test/in_...
File ui/compositor/test/in_process_context_factory.h (right):

https://codereview.chromium.org/2238693002/diff/200001/ui/compositor/test/in_...
ui/compositor/test/in_process_context_factory.h:99: bool visible;
On 2016/08/15 23:20:03, sunnyps wrote:
> I think you should initialize visible here? (and maybe even display)

Oh, yes I should! But actually I looked at this a bit more cuz I was like wait
how are tests passing.

The cacheing in GPTF and here in IPCF was redundant, because we are setting
visibility on the Display when we recreate it from ui::Compositor already, since
the GPTF may not even know about the compositor at times. So we can just use
that as the single place that resets visibility and drop cacheing in the
ContextFactory impls. With that knowledge I've reverted most of the changes in
this class.

[piman, 2016-08-16 00:45:28]

lgtm

[commit-bot: I haz the power, 2016-08-16 01:17:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 01:17:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-08-16 18:05:00]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 18:05:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 19:26:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 19:26:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2016-08-16 19:53:03]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 19:53:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-16 19:55:14]

On 2016/08/15 23:47:46, danakj wrote:
> On Mon, Aug 15, 2016 at 4:20 PM, <mailto:sunnyps@chromium.org> wrote:
> 
> > Left a few comments. All the SetVisible(true) calls make me think that
> > visible =
> > true should be the default value.
> >
> 
> Visible = false is the default in cc::Scheduler, and we don't start the
> scheduler/request an output surface/etc until it becomes visible. This
> replaced the SetLayerTreeHostClientReady signal we had previously. The
> DirectRenderer was not matching that, but I prefer if it does, that seems
> to make more sense to me. The DisplayScheduler could differ, but similar to
> DirectRenderer it would make the first call to SetVisible(true) do
> something in cc:Scheduler but noop/early out in DirectRenderer and
> DisplayScheduler, which seems surprising. Thoughts?

I came across a new example of why I think default visible false would be bad.
We now have code in GLRenderer that when becoming visible we tell the context we
became visible. If we default to visible, we would early out and not tell the
context, unless we remembered to tell it in the constructor (or basically call
SetVisible(true) there. It feels more complicated to me, to have to remember to
do things like that. And once we turned on shared compositor contexts we'd end
up with the context thinking it wasn't visible and doing the wrong thing with
caches sometimes.

[sunnyps, 2016-08-16 20:18:13]

LGTM with nits

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2220: EXPECT_CALL(*(context_support_ptr_),
SetClientVisible(0, true));
nit: brackets around context_support_ptr_ are unnecessary

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2225:
Mock::VerifyAndClearExpectations(context_support_ptr_);
nit: TBH these mock expectations seem excessive. Not sure what we're trying to
test here.

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:327: if (needs_draw_ && !output_surface_lost_
&& visible_) {
nit: Can we extract needs_draw_ && !output_surface_lost_ && visible_ into a
separate method? (maybe ShouldDraw?)

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler_unittest.cc:500: EXPECT_EQ(count,
scheduler_.scheduler_begin_frame_deadline_count());
This isn't related to your CL but I think this test isn't useful. This only
tests the number of times ScheduleBeginFrameDeadline is called but NOT the
number of times a deadline was actually scheduled.

https://codereview.chromium.org/2238693002/diff/280001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/ui/compositor/composit...
ui/compositor/compositor.cc:511: if (widget_valid_) {
nit: the extra braces are unnecessary

[danakj, 2016-08-16 20:33:06]

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2220: EXPECT_CALL(*(context_support_ptr_),
SetClientVisible(0, true));
On 2016/08/16 20:18:13, sunnyps wrote:
> nit: brackets around context_support_ptr_ are unnecessary

Done.

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2225:
Mock::VerifyAndClearExpectations(context_support_ptr_);
On 2016/08/16 20:18:13, sunnyps wrote:
> nit: TBH these mock expectations seem excessive. Not sure what we're trying to
> test here.

The mock will yell about unexpected calls otherwise.

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler.cc:327: if (needs_draw_ && !output_surface_lost_
&& visible_) {
On 2016/08/16 20:18:13, sunnyps wrote:
> nit: Can we extract needs_draw_ && !output_surface_lost_ && visible_ into a
> separate method? (maybe ShouldDraw?)

Done.

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/surfaces/display_sc...
cc/surfaces/display_scheduler_unittest.cc:500: EXPECT_EQ(count,
scheduler_.scheduler_begin_frame_deadline_count());
On 2016/08/16 20:18:13, sunnyps wrote:
> This isn't related to your CL but I think this test isn't useful. This only
> tests the number of times ScheduleBeginFrameDeadline is called but NOT the
> number of times a deadline was actually scheduled.

Ya it's just making sure each function calls ScheduleBeginFrameDeadline,
basically a mock test. I think it's okay? I'm not sure what I'd replace it with.

[danakj, 2016-08-16 20:33:39]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-08-16 20:33:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, piman@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2238693002/#ps300001
(title: "setvisible-browser: nits")

[commit-bot: I haz the power, 2016-08-16 20:33:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-16 20:34:22]

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/2238693002/diff/280001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2225:
Mock::VerifyAndClearExpectations(context_support_ptr_);
On 2016/08/16 20:33:05, danakj wrote:
> On 2016/08/16 20:18:13, sunnyps wrote:
> > nit: TBH these mock expectations seem excessive. Not sure what we're trying
to
> > test here.
> 
> The mock will yell about unexpected calls otherwise.

Oh and the expected calls here are actually very good to be testing. When
becoming visible the renderer had better tell the context about it, and disable
cache destruction.

[danakj, 2016-08-16 21:57:05]

danakj@chromium.org changed reviewers:
+ boliu@chromium.org

[danakj, 2016-08-16 21:57:06]

Ah, I broke webview somehow.

[ RUN      ] ActiveCompositorSwitchAfterConstructionTest.RunTest
[FATAL:direct_renderer.cc(204)] Check failed: visible().

[danakj, 2016-08-16 22:02:43]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 22:03:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-16 22:04:28]

I think the change in android_webview/browser/surfaces_instance.cc will fix it,
setting visible on the Display when we initialize it.

[boliu, 2016-08-16 22:05:37]

On 2016/08/16 22:04:28, danakj wrote:
> I think the change in android_webview/browser/surfaces_instance.cc will fix
it,
> setting visible on the Display when we initialize it.

lgtm

[danakj, 2016-08-16 22:11:54]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-08-16 22:11:59]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-08-16 22:12:00]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
jbauman@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2238693002/#ps320001
(title: "setvisible-browser: webview")

[commit-bot: I haz the power, 2016-08-16 22:12:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-16 23:20:29]

Need one more call to SetVisible(true) in CompositorImpl::InitializeDisplay too.

[danakj, 2016-08-16 23:20:42]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 23:21:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-16 23:43:02]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-08-16 23:43:02]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
piman@chromium.org, jbauman@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2238693002/#ps340001
(title: "setvisible-browser: and-compositor-impl")

[commit-bot: I haz the power, 2016-08-16 23:43:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 00:48:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 00:48:28]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2016-08-17 00:49:52]

Hm hm. android_webview_test_apk is timing out or failing somehow. Running it
locally it seems to be working okay, but maybe it fails a long way in.

[danakj, 2016-08-17 01:00:38]

On 2016/08/17 00:49:52, danakj wrote:
> Hm hm. android_webview_test_apk is timing out or failing somehow. Running it
> locally it seems to be working okay, but maybe it fails a long way in.

OK still getting this for some tests. testLoadWithOverviewModeWithTwoViews is
one of them.

[FATAL:direct_renderer.cc(217)] Check failed: visible_.

[danakj, 2016-08-17 01:16:29]

On 2016/08/17 01:00:38, danakj wrote:
> On 2016/08/17 00:49:52, danakj wrote:
> > Hm hm. android_webview_test_apk is timing out or failing somehow. Running it
> > locally it seems to be working okay, but maybe it fails a long way in.
> 
> OK still getting this for some tests. testLoadWithOverviewModeWithTwoViews is
> one of them.
> 
> [FATAL:direct_renderer.cc(217)] Check failed: visible_.

Software draws, right. They have their own Display in the
SynchronousCompositorOutputSurface. Now it is made visible on Init also.

[danakj, 2016-08-17 01:16:34]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-08-17 01:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
piman@chromium.org, jbauman@chromium.org, sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2238693002/#ps360001
(title: "setvisible-browser: onemore")

[commit-bot: I haz the power, 2016-08-17 01:17:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 02:10:37]

Description was changed from

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-17 02:10:40]

Committed patchset #16 (id:360001)

[commit-bot: I haz the power, 2016-08-17 02:12:51]

Description was changed from

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Plumb SetVisible from ui::Compositor to the DirectRenderer.

This connection got lost when ui::Compositor was switched to using
Display and Surfaces. Restore the connection so that when the root
compositor becomes invisible we drop the backbuffer memory in the
DirectRenderer.

R=piman@chromium.org
TBR=sky
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/6f2861b9c957c7374a2a2c084c6cafba01b5e5d6
Cr-Commit-Position: refs/heads/master@{#412421}
==========

[commit-bot: I haz the power, 2016-08-17 02:12:53]

Patchset 16 (id:??) landed as
https://crrev.com/6f2861b9c957c7374a2a2c084c6cafba01b5e5d6
Cr-Commit-Position: refs/heads/master@{#412421}