Refactor client visibility handling

Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/f6c4fb6b0b77f35f094cb73c49b0bfe22d3f4461
Cr-Commit-Position: refs/heads/master@{#415491}

[ericrk, 2016-08-26 00:51:56]

Description was changed from

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
==========

to

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ericrk, 2016-08-26 00:51:57]

Description was changed from

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ericrk, 2016-08-26 20:54:51]

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

[commit-bot: I haz the power, 2016-08-26 20:55:37]

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

[ericrk, 2016-08-26 20:57:03]

Description was changed from

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, SetClientVisible is now only accessible through the
ScopedContextClientVisibility object, which ensures that visible/
not-visible calls are paired.
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[ericrk, 2016-08-26 20:57:14]

Patchset #2 (id:20001) has been deleted

[ericrk, 2016-08-26 20:57:18]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-08-26 21:08:49]

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

[commit-bot: I haz the power, 2016-08-26 21:08:50]

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_...)

[ericrk, 2016-08-26 21:45:42]

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

[commit-bot: I haz the power, 2016-08-26 21:46:35]

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

[ericrk, 2016-08-26 21:47:34]

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

[ericrk, 2016-08-26 21:47:34]

Hey Daniel, what do you think of something like this?

[commit-bot: I haz the power, 2016-08-26 21:59:12]

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

[commit-bot: I haz the power, 2016-08-26 21:59:13]

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_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ericrk, 2016-08-26 22:49:18]

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

[commit-bot: I haz the power, 2016-08-26 22:50:21]

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

[ericrk, 2016-08-26 22:50:36]

Patchset #3 (id:80001) has been deleted

[ericrk, 2016-08-26 22:53:03]

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

[ericrk, 2016-08-26 22:53:03]

Hey Dana, here's the re-work of the visibility cleanup. Mostly pretty similar,
except that we move the complexity out of GLES2Implementation.

[ericrk, 2016-08-26 22:55:47]

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:27: class CC_EXPORT ContextCacheController
{
This name is a bit silly - I'm thinking of a better one :P

[danakj, 2016-08-26 23:49:09]

https://codereview.chromium.org/2278283003/diff/100001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.h (right):

https://codereview.chromium.org/2278283003/diff/100001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.h:62:
std::unique_ptr<cc::ContextCacheController> cache_controller_;
This has a pointer to the gr_context_ so it should be below it.

https://codereview.chromium.org/2278283003/diff/100001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:94:
cache_controller_->SetGrContext(gr_context_->get());
nit: prefer this grouped with the gr_context_reset more than then return (or
drop the whitespace cuz why not both?)

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:22: // ContextCacheController manages
clearing cached data on ContextProvider when
My one thought is that it's a bit weird that the impl of this is in cc, cuz you
have to dep on cc/ to use GPU things without cc things.. It's equally weird that
the context provider API is in cc tho, I guess they can both move to gpu at the
same time one day

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:23: // appropriate. Currently, cache
clearing happens when the Context provider
when the ContextProvider

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:24: // transitions from Visible to Not
Visible. As a ContextProvider may have
visible to not visible

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:27: class CC_EXPORT ContextCacheController
{
On 2016/08/26 22:55:47, ericrk wrote:
> This name is a bit silly - I'm thinking of a better one :P

lies it's perfect

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:42: void SetGrContext(GrContext*
gr_context);
whitespace after constructors/destructor plz

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
File cc/output/context_cache_controller_unittest.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller_unittest.cc:31: auto visibility =
cache_controller.ClientBecameVisible();
the type of |visiblity| is not actually mentioned anywhere, and we're not simply
using it as a temp var. so maybe you should spell out the name of it

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:423: output_surface_->context_provider()
nit: maybe

auto* cache_controller = outsur->conpro->CacheController();
cache_controller->ClientBecameNotVisible(..)

would be less lines and easier to read formattingwise?

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:449: if (visible_ && !context_visibility_) {
Can you actually become visible and have a context_visibility? Sounds like a bug
if so. How about

if (visible_) {
  DCHECK(!c_v);
  ..
} else {
  DCHECK(c_v);
  ..
}
?

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:450: context_visibility_ =
output_surface_->context_provider()
similars for formatting?

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (left):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2137: EXPECT_CALL(*context_support_ptr_,
SetAggressivelyFreeResources(false));
This is testing mocks 2 levels in now, hm. I wonder about making the cc::CCC a
virtual interface so you could mock that but idk. I kinda liked it not being
virtual for no reason. Eh.. thots?

https://codereview.chromium.org/2278283003/diff/100001/cc/raster/raster_buffe...
File cc/raster/raster_buffer_provider_perftest.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/raster/raster_buffe...
cc/raster/raster_buffer_provider_perftest.cc:104: return gr_context_.get();
setgrcontext?

https://codereview.chromium.org/2278283003/diff/100001/cc/test/test_context_p...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/test/test_context_p...
cc/test/test_context_provider.h:88: std::unique_ptr<ContextCacheController>
cache_controller_;
Hm, kinda weird that gr_context_ and this are way at the bottom, can they be up
with context_gl_?

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1270: SetWorkerContextVisibility(true /*
is_visible */);
i think the name of this bool is in the function name already

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1360: SetWorkerContextVisibility(false /*
is_visible */);
dittos

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2314: SetMainContextVisibility(false /*
is_visible */);
dittos

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2341: output_surface_ = output_surface;
Should we set visibility on the compositor context here?

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4097: if (is_visible ==
!!main_context_visibility_)
Is it actually possible to have this early out happen and its not a bug?

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4104: 
nit: remove whitespace

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4118: if (is_visible ==
!!worker_context_visibility_)
Same Q

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4125: 
nit: remove whitespace

fwiw I try to think of empty lines separating paragraphs of code, not
sentences.. aka for topic changes

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4130: 
nit: remove whitespace

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:731: //
ContextCacheController::ClientBecameNotVisible
you could say "ContextCacheController's ClientBecameNotVisible()" to allow
better wrapping

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:495: class
LayerTreeHostFreeWorkerContextResourcesOnInvisible
Should these test the compositor context too?

.. and we do this in SetVisible for the compositor not the worker context.. wait
what is happening in this test then?

https://codereview.chromium.org/2278283003/diff/100001/services/ui/public/cpp...
File services/ui/public/cpp/context_provider.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/services/ui/public/cpp...
services/ui/public/cpp/context_provider.cc:24: return !!context_;
This suggests context_ can be null. Either your code will crash or this should
just return true.

https://codereview.chromium.org/2278283003/diff/100001/services/ui/surfaces/s...
File services/ui/surfaces/surfaces_context_provider.h (right):

https://codereview.chromium.org/2278283003/diff/100001/services/ui/surfaces/s...
services/ui/surfaces/surfaces_context_provider.h:82: 
nit: no whitespace

[ericrk, 2016-08-29 22:28:34]

Patchset #4 (id:120001) has been deleted

[ericrk, 2016-08-29 22:41:12]

Patchset #4 (id:140001) has been deleted

[ericrk, 2016-08-29 22:43:23]

Thanks for the feedback!

https://codereview.chromium.org/2278283003/diff/100001/android_webview/browse...
File android_webview/browser/aw_render_thread_context_provider.h (right):

https://codereview.chromium.org/2278283003/diff/100001/android_webview/browse...
android_webview/browser/aw_render_thread_context_provider.h:62:
std::unique_ptr<cc::ContextCacheController> cache_controller_;
On 2016/08/26 23:49:08, danakj wrote:
> This has a pointer to the gr_context_ so it should be below it.

Done.

https://codereview.chromium.org/2278283003/diff/100001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_context_provider.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_context_provider.cc:94:
cache_controller_->SetGrContext(gr_context_->get());
On 2016/08/26 23:49:08, danakj wrote:
> nit: prefer this grouped with the gr_context_reset more than then return (or
> drop the whitespace cuz why not both?)

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:22: // ContextCacheController manages
clearing cached data on ContextProvider when
On 2016/08/26 23:49:08, danakj wrote:
> My one thought is that it's a bit weird that the impl of this is in cc, cuz
you
> have to dep on cc/ to use GPU things without cc things.. It's equally weird
that
> the context provider API is in cc tho, I guess they can both move to gpu at
the
> same time one day

Yeah, I had a similar thought, but I assumed there was a reason for
ContextProvider to be there :P, if not, then yeah, this can really go anywhere
(probably GPU is best)...

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:23: // appropriate. Currently, cache
clearing happens when the Context provider
On 2016/08/26 23:49:08, danakj wrote:
> when the ContextProvider

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:24: // transitions from Visible to Not
Visible. As a ContextProvider may have
On 2016/08/26 23:49:08, danakj wrote:
> visible to not visible

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller.h:42: void SetGrContext(GrContext*
gr_context);
On 2016/08/26 23:49:08, danakj wrote:
> whitespace after constructors/destructor plz

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
File cc/output/context_cache_controller_unittest.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/context_cach...
cc/output/context_cache_controller_unittest.cc:31: auto visibility =
cache_controller.ClientBecameVisible();
On 2016/08/26 23:49:08, danakj wrote:
> the type of |visiblity| is not actually mentioned anywhere, and we're not
simply
> using it as a temp var. so maybe you should spell out the name of it

sure - but it's sooo long :P

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:423: output_surface_->context_provider()
On 2016/08/26 23:49:08, danakj wrote:
> nit: maybe
> 
> auto* cache_controller = outsur->conpro->CacheController();
> cache_controller->ClientBecameNotVisible(..)
> 
> would be less lines and easier to read formattingwise?

Sadly, that first line is more than 80 chars... but splitting it into 3 lines is
maybe clearer?

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:449: if (visible_ && !context_visibility_) {
On 2016/08/26 23:49:08, danakj wrote:
> Can you actually become visible and have a context_visibility? Sounds like a
bug
> if so. How about
> 
> if (visible_) {
>   DCHECK(!c_v);
>   ..
> } else {
>   DCHECK(c_v);
>   ..
> }
> ?

Yeah, this seems reasonable.

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:450: context_visibility_ =
output_surface_->context_provider()
On 2016/08/26 23:49:08, danakj wrote:
> similars for formatting?

Refactored.

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer_...
File cc/output/gl_renderer_unittest.cc (left):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2137: EXPECT_CALL(*context_support_ptr_,
SetAggressivelyFreeResources(false));
On 2016/08/26 23:49:08, danakj wrote:
> This is testing mocks 2 levels in now, hm. I wonder about making the cc::CCC a
> virtual interface so you could mock that but idk. I kinda liked it not being
> virtual for no reason. Eh.. thots?

Yeah... I thought of this as well - it felt like making it virtual for no reason
wasn't great, but I guess it would make it easier to test...

https://codereview.chromium.org/2278283003/diff/100001/cc/raster/raster_buffe...
File cc/raster/raster_buffer_provider_perftest.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/raster/raster_buffe...
cc/raster/raster_buffer_provider_perftest.cc:104: return gr_context_.get();
On 2016/08/26 23:49:08, danakj wrote:
> setgrcontext?

yeah

https://codereview.chromium.org/2278283003/diff/100001/cc/test/test_context_p...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/test/test_context_p...
cc/test/test_context_provider.h:88: std::unique_ptr<ContextCacheController>
cache_controller_;
On 2016/08/26 23:49:08, danakj wrote:
> Hm, kinda weird that gr_context_ and this are way at the bottom, can they be
up
> with context_gl_?

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1270: SetWorkerContextVisibility(true /*
is_visible */);
On 2016/08/26 23:49:09, danakj wrote:
> i think the name of this bool is in the function name already

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1360: SetWorkerContextVisibility(false /*
is_visible */);
On 2016/08/26 23:49:09, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2314: SetMainContextVisibility(false /*
is_visible */);
On 2016/08/26 23:49:09, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2341: output_surface_ = output_surface;
On 2016/08/26 23:49:08, danakj wrote:
> Should we set visibility on the compositor context here?

Yeah, if we are visible_, and only for the main context - worker context is set
via memory policy change below, added a comment.

Added a unit test.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4097: if (is_visible ==
!!main_context_visibility_)
On 2016/08/26 23:49:09, danakj wrote:
> Is it actually possible to have this early out happen and its not a bug?

Sure, only change I may need to make is not call it with false/true directly, so
I need to check at the call site... but this would help us catch any bugs, so
probably best to remove this and handle exceptions at the call site.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4104: 
On 2016/08/26 23:49:08, danakj wrote:
> nit: remove whitespace

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4118: if (is_visible ==
!!worker_context_visibility_)
On 2016/08/26 23:49:08, danakj wrote:
> Same Q

Worker context visibility is handled through both direct visibility changes and
changes to the managed memory policy (if memory limit is zeroed we set to
invisible)... I'm not too confident that the memory policy changes are as
controlled as the visibility changes...

We should look into this more - it seems pretty messy to conflate these two...
and I'm not sure when we'd need to use memory policy rather than direct
visibility.  I think Android webview may do this. Let's look at this as a
follow-up, and leave this check for now (in the worker context case).

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4125: 
On 2016/08/26 23:49:09, danakj wrote:
> nit: remove whitespace
> 
> fwiw I try to think of empty lines separating paragraphs of code, not
> sentences.. aka for topic changes

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:4130: 
On 2016/08/26 23:49:09, danakj wrote:
> nit: remove whitespace

Done.

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.h:731: //
ContextCacheController::ClientBecameNotVisible
On 2016/08/26 23:49:09, danakj wrote:
> you could say "ContextCacheController's ClientBecameNotVisible()" to allow
> better wrapping

yup

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:495: class
LayerTreeHostFreeWorkerContextResourcesOnInvisible
On 2016/08/26 23:49:09, danakj wrote:
> Should these test the compositor context too?
> 
> .. and we do this in SetVisible for the compositor not the worker context..
wait
> what is happening in this test then?

SetVisible also modifies the memory policy, zeroing it when we go invisible.
That eventually causes the worker context to free as well... But we should test
the main context here as well.

https://codereview.chromium.org/2278283003/diff/100001/services/ui/public/cpp...
File services/ui/public/cpp/context_provider.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/services/ui/public/cpp...
services/ui/public/cpp/context_provider.cc:24: return !!context_;
On 2016/08/26 23:49:09, danakj wrote:
> This suggests context_ can be null. Either your code will crash or this should
> just return true.

Yeah, it can be null, good point.

https://codereview.chromium.org/2278283003/diff/100001/services/ui/surfaces/s...
File services/ui/surfaces/surfaces_context_provider.h (right):

https://codereview.chromium.org/2278283003/diff/100001/services/ui/surfaces/s...
services/ui/surfaces/surfaces_context_provider.h:82: 
On 2016/08/26 23:49:09, danakj wrote:
> nit: no whitespace

Done.

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

LGTM, few more thoughts

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2278283003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:423: output_surface_->context_provider()
On 2016/08/29 22:43:22, ericrk wrote:
> On 2016/08/26 23:49:08, danakj wrote:
> > nit: maybe
> > 
> > auto* cache_controller = outsur->conpro->CacheController();
> > cache_controller->ClientBecameNotVisible(..)
> > 
> > would be less lines and easier to read formattingwise?
> 
> Sadly, that first line is more than 80 chars... but splitting it into 3 lines
is
> maybe clearer?

Eh, 3 lines for 3 lines with more text I guess.

https://codereview.chromium.org/2278283003/diff/160001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2278283003/diff/160001/cc/output/context_cach...
cc/output/context_cache_controller.h:52: virtual
std::unique_ptr<ScopedVisibility> ClientBecameVisible();
If you are going to make these virtual I think the impl should probably move to
a separate class ContextCacheControllerImpl or somethingsomething, which can be
declared as |final| as well.

Maybe then you don't need the fortesting methods. The mock class can subclass
the interface, and own the impl, and forward calls between them.

https://codereview.chromium.org/2278283003/diff/160001/cc/output/context_cach...
cc/output/context_cache_controller.h:61: std::unique_ptr<ScopedVisibility>
CreateScopedVisibilityForTesting() const;
Can you TODO to remove these when c++11 gmock support happens?

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

https://codereview.chromium.org/2278283003/diff/160001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2095: return
CreateScopedVisibilityForTesting();
this could just call ContextCacheController::OnClientBecameVisible() i guess to
not need ForTesting too? but spliting the interface/impl probably helps more.

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1276: if (image_decode_controller_) {
nit: no {} needed now

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1355: if (image_decode_controller_) {
nitto

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2312: if (main_context_visibility_)
can you use if (visible_) for the main context, and mention why the worker needs
to check the unique_ptr? (fwiw other code calls it the "compositor context")

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:479: // Ensure all expectations are
satisfied.
I don't think these are needed.. gmock will do this when they are destroyed?

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:555:
host_impl->SetMemoryPolicy(zero_policy);
You could just VerifyAndClear/EXPECT_CALL/EndTest() after this call, there's
nothing async going on here right? That's easier to follow than WillOnce()

[ericrk, 2016-08-30 18:10:31]

Patchset #6 (id:200001) has been deleted

[ericrk, 2016-08-30 18:18:58]

Thanks for the feedback! De-virtualized to simplify things.

https://codereview.chromium.org/2278283003/diff/160001/cc/output/context_cach...
File cc/output/context_cache_controller.h (right):

https://codereview.chromium.org/2278283003/diff/160001/cc/output/context_cach...
cc/output/context_cache_controller.h:52: virtual
std::unique_ptr<ScopedVisibility> ClientBecameVisible();
On 2016/08/29 23:56:20, danakj wrote:
> If you are going to make these virtual I think the impl should probably move
to
> a separate class ContextCacheControllerImpl or somethingsomething, which can
be
> declared as |final| as well.
> 
> Maybe then you don't need the fortesting methods. The mock class can subclass
> the interface, and own the impl, and forward calls between them.

Made these un-virtual - seemed to be adding too much extra code.

https://codereview.chromium.org/2278283003/diff/160001/cc/output/context_cach...
cc/output/context_cache_controller.h:61: std::unique_ptr<ScopedVisibility>
CreateScopedVisibilityForTesting() const;
On 2016/08/29 23:56:20, danakj wrote:
> Can you TODO to remove these when c++11 gmock support happens?

made un-virtual

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

https://codereview.chromium.org/2278283003/diff/160001/cc/output/gl_renderer_...
cc/output/gl_renderer_unittest.cc:2095: return
CreateScopedVisibilityForTesting();
On 2016/08/29 23:56:20, danakj wrote:
> this could just call ContextCacheController::OnClientBecameVisible() i guess
to
> not need ForTesting too? but spliting the interface/impl probably helps more.

Made un-virtual, so no longer an issue.

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1276: if (image_decode_controller_) {
On 2016/08/29 23:56:20, danakj wrote:
> nit: no {} needed now

Done.

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1355: if (image_decode_controller_) {
On 2016/08/29 23:56:20, danakj wrote:
> nitto

Done.

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2312: if (main_context_visibility_)
On 2016/08/29 23:56:20, danakj wrote:
> can you use if (visible_) for the main context, and mention why the worker
needs
> to check the unique_ptr? (fwiw other code calls it the "compositor context")

Done.

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:479: // Ensure all expectations are
satisfied.
On 2016/08/29 23:56:20, danakj wrote:
> I don't think these are needed.. gmock will do this when they are destroyed?

Done.

https://codereview.chromium.org/2278283003/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:555:
host_impl->SetMemoryPolicy(zero_policy);
On 2016/08/29 23:56:20, danakj wrote:
> You could just VerifyAndClear/EXPECT_CALL/EndTest() after this call, there's
> nothing async going on here right? That's easier to follow than WillOnce()

Sadly, because we don't want to interrupt in-process work, we actually do a
round-trip of scheduling tiles / AllTileTasksCompleted to hit this, so it is
actually async.

[ericrk, 2016-08-30 18:22:03]

ericrk@chromium.org changed reviewers:
+ boliu@chromium.org, msw@chromium.org, wez@chromium.org

[ericrk, 2016-08-30 18:22:04]

Adding some reviewers for ContextProvider changes - ContextProviders now have a
ContextCacheController - this manages visibility and cleanup (and Idle in a
future patch), letting us remove DeleteCachedResources.

+boliu for android_webview/
+wez for blimp/
+msw for services/

Thanks!

[ericrk, 2016-08-30 18:22:24]

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

[commit-bot: I haz the power, 2016-08-30 18:23:26]

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

[boliu, 2016-08-30 18:27:12]

rs lgtm

[commit-bot: I haz the power, 2016-08-30 18:27:27]

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

[commit-bot: I haz the power, 2016-08-30 18:27:28]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ericrk, 2016-08-30 18:58:45]

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

[commit-bot: I haz the power, 2016-08-30 18:59:33]

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-30 19:29:52]

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

[commit-bot: I haz the power, 2016-08-30 19:29:53]

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

[no sievers, 2016-08-30 20:09:52]

content + gpu lgtm

[ericrk, 2016-08-30 20:10:49]

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

[commit-bot: I haz the power, 2016-08-30 20:11:48]

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-30 20:30:08]

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

[commit-bot: I haz the power, 2016-08-30 20:30:09]

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

[Wez, 2016-08-30 20:48:49]

blimp/ LGTM

It looks like the impl in the BlimpContextProvider is pretty standard - is there
value in providing a ContextProviderBase for specific implementations to inherit
from and extend, that would take care of stuff like this?

[ericrk, 2016-08-30 20:50:10]

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

[commit-bot: I haz the power, 2016-08-30 20:50:40]

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-30 21:58:01]

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

[commit-bot: I haz the power, 2016-08-30 21:58:03]

Dry run: This issue passed the CQ dry run.

[msw, 2016-08-30 23:11:50]

services/ui rubber-stamp lgtm (+CC fsamuel and rjkroege)

[ericrk, 2016-08-30 23:20:26]

On 2016/08/30 20:48:49, Wez wrote:
> blimp/ LGTM
> 
> It looks like the impl in the BlimpContextProvider is pretty standard - is
there
> value in providing a ContextProviderBase for specific implementations to
inherit
> from and extend, that would take care of stuff like this?

Hmm... that might be a good idea - there are a few pieces that can probably be
factored out. I'll take a look.

[ericrk, 2016-08-30 23:20:34]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2016-08-30 23:20:35]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
boliu@chromium.org, sievers@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2278283003/#ps280001
(title: "more fixes")

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

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-30 23:27:53]

Description was changed from

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-30 23:27:55]

Committed patchset #9 (id:280001)

[commit-bot: I haz the power, 2016-08-30 23:29:41]

Description was changed from

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Refactor client visibility handling

Currently, ContextSupport::SetClientVisibile relies on the caller
remembering to pair visible/not-visible calls. Failure to do so will
result in a "leak", where the context support will always think there
are visible clients.

To avoid this, visibility handling now uses ScopedVisibility objects
which will dcheck if visible > not visible updates are not paired.

This change also introduces a new ContextCacheController object which
handles trimming context caches based on visibility. This simplifies
handling in clients of the ContextProvider and removes this logic
from ContextSupport.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/f6c4fb6b0b77f35f094cb73c49b0bfe22d3f4461
Cr-Commit-Position: refs/heads/master@{#415491}
==========

[commit-bot: I haz the power, 2016-08-30 23:29:43]

Patchset 9 (id:??) landed as
https://crrev.com/f6c4fb6b0b77f35f094cb73c49b0bfe22d3f4461
Cr-Commit-Position: refs/heads/master@{#415491}