Android: Second attempt at freeing GLHelper ashmem

Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

But it only does this if there are no more readback
requests in flight.

BUG=641962, 636630
Committed: https://crrev.com/c9afe77bc5411c3bf0760c9acc37941e6e24540a
Cr-Commit-Position: refs/heads/master@{#423045}

[no sievers, 2016-09-30 23:15:28]

Description was changed from

==========
Android: Second attempt at freeing GLHelper ashmem

This mostly reverts https://codereview.chromium.org/2336043004/.
And instead drops the GLHelper and context when the app
goes to the background and no readbacks are in flight.

BUG=641962,636630
==========

to

==========
Android: Second attempt at freeing GLHelper ashmem

This mostly reverts https://codereview.chromium.org/2336043004/.
And instead drops the GLHelper and context when the app
goes to the background and no readbacks are in flight.

BUG=641962,636630
==========

[no sievers, 2016-09-30 23:15:28]

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

[no sievers, 2016-09-30 23:36:25]

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

[commit-bot: I haz the power, 2016-09-30 23:37:11]

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-10-01 00:02:58]

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

[commit-bot: I haz the power, 2016-10-01 00:02:59]

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

[danakj, 2016-10-03 20:47:43]

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

[danakj, 2016-10-03 20:47:44]

LGTM

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:109: class
PendingReadbackLock : public base::RefCounted<PendingReadbackLock> {
This is fine, but another way to do this would be

int g_num_readbacks_pending = 0;

class PendingReadbackLock() {
  PendingReadbackLock() { ++g_num_readbacks_pending; }
  ~PendingReadbackLock() { --g_num_readbacks_pending; }
};

Then we don't need any refcounted objects, the global is simply the refcount. Up
to you.

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:116:
~PendingReadbackLock() {
Destructor should be private with friend of RefCounted.

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:303:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
I'd like a comment explaining why this is posting the task instead of doing it
immediately, cuz that's not clear to me

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1872: namespace
{
bonus points if this method can move up to the anon namespace at the top of the
file.

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1930: }  //
anonymous namespace
just "// namespace"

[no sievers, 2016-10-05 00:26:09]

Patchset #3 (id:40001) has been deleted

[no sievers, 2016-10-05 00:27:42]

Patchset #3 (id:60001) has been deleted

[no sievers, 2016-10-05 00:28:09]

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

[commit-bot: I haz the power, 2016-10-05 00:28:31]

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

[no sievers, 2016-10-05 00:28:38]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-10-05 00:30:55]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_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_...)

[no sievers, 2016-10-05 00:31:08]

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

[commit-bot: I haz the power, 2016-10-05 00:31:50]

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

[no sievers, 2016-10-05 00:38:40]

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:116:
~PendingReadbackLock() {
On 2016/10/03 20:47:43, danakj wrote:
> Destructor should be private with friend of RefCounted.

Done.

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:303:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2016/10/03 20:47:44, danakj wrote:
> I'd like a comment explaining why this is posting the task instead of doing it
> immediately, cuz that's not clear to me

Done.

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1872: namespace
{
On 2016/10/03 20:47:44, danakj wrote:
> bonus points if this method can move up to the anon namespace at the top of
the
> file.

Done.

https://codereview.chromium.org/2383613005/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1930: }  //
anonymous namespace
On 2016/10/03 20:47:44, danakj wrote:
> just "// namespace"

Done.

[no sievers, 2016-10-05 00:39:09]

Patchset #4 (id:120001) has been deleted

[boliu, 2016-10-05 00:49:50]

lgtm

remember to update CL description to not mention the revert part

[no sievers, 2016-10-05 00:57:07]

Description was changed from

==========
Android: Second attempt at freeing GLHelper ashmem

This mostly reverts https://codereview.chromium.org/2336043004/.
And instead drops the GLHelper and context when the app
goes to the background and no readbacks are in flight.

BUG=641962,636630
==========

to

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

BUG=641962,636630
==========

[no sievers, 2016-10-05 00:57:14]

On 2016/10/05 00:49:50, boliu wrote:
> lgtm
> 
> remember to update CL description to not mention the revert part

Done

[no sievers, 2016-10-05 00:57:52]

Description was changed from

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

BUG=641962,636630
==========

to

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

But it only does this if there are no more readback
requests in flight.

BUG=641962,636630
==========

[no sievers, 2016-10-05 01:00:00]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2016-10-05 01:00:00]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2383613005/#ps160001
(title: "add comment and dcheck()")

[commit-bot: I haz the power, 2016-10-05 01:00: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-10-05 01:43:25]

Description was changed from

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

But it only does this if there are no more readback
requests in flight.

BUG=641962,636630
==========

to

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

But it only does this if there are no more readback
requests in flight.

BUG=641962,636630
==========

[commit-bot: I haz the power, 2016-10-05 01:43:27]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-10-05 01:47:24]

Description was changed from

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

But it only does this if there are no more readback
requests in flight.

BUG=641962,636630
==========

to

==========
Android: Second attempt at freeing GLHelper ashmem

This tears down the GLHelperHolder in RenderWidgetHostViewAndroid
when all activities get stopped.

But it only does this if there are no more readback
requests in flight.

BUG=641962,636630

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

[commit-bot: I haz the power, 2016-10-05 01:47:25]

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