Android: Don't destroy LayerTreeHost when Surface goes away

Android: Don't destroy LayerTreeHost when Surface goes away

Instead just release the OutputSurface. Also remove some
unnecessary complexity and indirections from the UI resource management,
esp. now that UI resource IDs stays valid.
And fix Thumbnail bitmap reloading on context lost.

TBR=danakj@chromium.org

NOTRY=True

BUG=374906
Committed: https://crrev.com/148fedf0c1f445294866dc812169dcf0ac083e8c
Cr-Commit-Position: refs/heads/master@{#351918}

[no sievers, 2015-09-24 23:26:07]

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

[no sievers, 2015-09-24 23:27:43]

dana, dtrain, how does this look?

https://codereview.chromium.org/1371523003/diff/1/chrome/browser/android/thum...
File chrome/browser/android/thumbnail/thumbnail.cc (right):

https://codereview.chromium.org/1371523003/diff/1/chrome/browser/android/thum...
chrome/browser/android/thumbnail/thumbnail.cc:94: // Return a place holder for
all other calls to GetBitmap.
This is basically what is making it hard to simplify the UI resource management
further without making other changes.

The invalidation below also actually causes it to delete |this|, so this needs a
bite more rework.

[danakj, 2015-09-28 22:45:43]

Looks ok couple comments/questions

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

https://codereview.chromium.org/1371523003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:230:
CompositorImpl::CompositorImpl(CompositorClient* client,
Style-guidey comment: This class should probably have an Init method. This is a
very heavy constructor.

https://codereview.chromium.org/1371523003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:255:
ui_resource_provider_.reset(new ui::UIResourceProvider(host_.get()));
I'm wondering if this class had an Init, and maybe the ui_resource_provider_ had
an init, if you wouldn't need to make these 2 things into scoped_ptrs and they
could be inline members still.

https://codereview.chromium.org/1371523003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:505:
pending_swapbuffers_ = 0;
I don't really know what these are about, but these were in CreateLTH which set
visible true. Now they are in a set visible false path. Why is that right?

[no sievers, 2015-09-29 19:15:28]

sievers@chromium.org changed reviewers:
+ jdduke@chromium.org
- dtrainor@chromium.org

[no sievers, 2015-09-30 02:32:40]

+Jared 

Ok I've removed UIResourceClientAndroid, because it's the same as
cc::ScopedUIResource at this point.

I've also removed the UIResourceProvider implementation, since it's just an
interface to create and delete resources, so CompositorImpl can implement it.

The only thing I had to change was for the Thumbnails now to be explicitly
notified about eviction (from CompositorView).

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

https://codereview.chromium.org/1371523003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:230:
CompositorImpl::CompositorImpl(CompositorClient* client,
On 2015/09/28 22:45:43, danakj wrote:
> Style-guidey comment: This class should probably have an Init method. This is
a
> very heavy constructor.

ui/compositor/compositor.cc is even heavier though :)
(not that that's a good argument)

https://codereview.chromium.org/1371523003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:255:
ui_resource_provider_.reset(new ui::UIResourceProvider(host_.get()));
On 2015/09/28 22:45:43, danakj wrote:
> I'm wondering if this class had an Init, and maybe the ui_resource_provider_
had
> an init, if you wouldn't need to make these 2 things into scoped_ptrs and they
> could be inline members still.

Done.

https://codereview.chromium.org/1371523003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/compositor_impl_android.cc:505:
pending_swapbuffers_ = 0;
On 2015/09/28 22:45:43, danakj wrote:
> I don't really know what these are about, but these were in CreateLTH which
set
> visible true. Now they are in a set visible false path. Why is that right?

We just have to make sure the accounting doesn't break. If the output surface
gets deleted we don't get anymore pending acks. Now one corner case is that
|pending_swapbuffers_ = kMaxSwapBuffers| and we destroy the OS (or previously
LTH). When we become visible again, we then might bail out of
CompositorImpl::Composite() forever below (line 339) before ever getting a
RequestNewOutputSurface() if we don't reset the pending acks (before
Composite()) somewhere.

I don't think it matters whether we reset it on becoming invisible or visible
again, since it shouldn't be able to change if there is no OutputSurface,
because DidPostSwapBuffers() wouldn't get called even if we tried to force a
composite. Thinking more about it, we probably also want to avoid any failed
attempt to create an OutputSurface while we are invisible and don't have an
underlying surface. (Also there is a DCHECK() for the surface id in
CreateOutputSurface()). Since we cancel |current_composite_task_| here, I can
actually put a DCHECK(!host_->visible()) inside Composite().

Does that add up? (Thanks for forcing me to think it through thoroughly.)

[jdduke (slow), 2015-09-30 17:19:44]

ui/android lgtm, with the tests fleshed out.

https://codereview.chromium.org/1371523003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/1371523003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:69: cc::UIResourceId
CreateUIResource(cc::UIResourceClient* client) final;
Should this be "final override"? Do we have any guidance on preferring
"override" vs "final override"? Seems like most of our codebase intends the
latter semantics, but just uses "override".

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:23:
ResourceManagerImpl::ResourceManagerImpl() {
: host_(nullptr)

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl_unittest.cc (right):

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl_unittest.cc:56:
MOCK_METHOD1(CreateUIResource, cc::UIResourceId(cc::UIResourceClient*));
Hopefully this is much cleaner =/

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
File ui/android/resources/ui_resource_provider.h (right):

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
ui/android/resources/ui_resource_provider.h:8: #include
"base/containers/hash_tables.h"
Nit: Looks like we don't need the hash include.

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
ui/android/resources/ui_resource_provider.h:13: class LayerTreeHost;
Nit: Looks like we can lose this decl.

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
ui/android/resources/ui_resource_provider.h:25: UIResourceProvider() {}
Do we need the constructor?

[no sievers, 2015-09-30 19:07:16]

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

[David Trainor- moved to gerrit, 2015-09-30 19:09:25]

chrome/ lgtm.

[no sievers, 2015-09-30 21:58:48]

Dana, mind taking another look?

https://codereview.chromium.org/1371523003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.h (right):

https://codereview.chromium.org/1371523003/diff/20001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.h:69: cc::UIResourceId
CreateUIResource(cc::UIResourceClient* client) final;
On 2015/09/30 17:19:43, jdduke wrote:
> Should this be "final override"? Do we have any guidance on preferring
> "override" vs "final override"? Seems like most of our codebase intends the
> latter semantics, but just uses "override".

Done.

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/re...
File ui/android/resources/resource_manager_impl.cc (right):

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/re...
ui/android/resources/resource_manager_impl.cc:23:
ResourceManagerImpl::ResourceManagerImpl() {
On 2015/09/30 17:19:43, jdduke wrote:
> : host_(nullptr)

Done.

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
File ui/android/resources/ui_resource_provider.h (right):

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
ui/android/resources/ui_resource_provider.h:8: #include
"base/containers/hash_tables.h"
On 2015/09/30 17:19:44, jdduke wrote:
> Nit: Looks like we don't need the hash include.

Done.

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
ui/android/resources/ui_resource_provider.h:13: class LayerTreeHost;
On 2015/09/30 17:19:44, jdduke wrote:
> Nit: Looks like we can lose this decl.

Done.

https://codereview.chromium.org/1371523003/diff/20001/ui/android/resources/ui...
ui/android/resources/ui_resource_provider.h:25: UIResourceProvider() {}
On 2015/09/30 17:19:44, jdduke wrote:
> Do we need the constructor?

Done.

[no sievers, 2015-09-30 22:13:35]

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

[commit-bot: I haz the power, 2015-09-30 22:14:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371523003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371523003/80001

[commit-bot: I haz the power, 2015-09-30 22:27:16]

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

[commit-bot: I haz the power, 2015-09-30 22:27:17]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[no sievers, 2015-10-01 20:12:20]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-10-01 20:12:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1371523003/#ps120001
(title: "fix RequestNewOS/Hide/SetOS race")

[commit-bot: I haz the power, 2015-10-01 20:13:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371523003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371523003/120001

[no sievers, 2015-10-01 20:16:25]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2015-10-01 21:19:32]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-10-01 21:19:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1371523003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-10-01 21:21:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371523003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371523003/160001

[commit-bot: I haz the power, 2015-10-01 21:33:08]

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

[commit-bot: I haz the power, 2015-10-01 21:33:09]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[no sievers, 2015-10-01 21:33:38]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2015-10-01 21:34:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371523003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371523003/160001

[no sievers, 2015-10-02 00:03:27]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2015-10-02 00:04:30]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2015-10-02 00:05:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371523003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371523003/160001

[commit-bot: I haz the power, 2015-10-02 00:12:17]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-02 00:13:29]

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