android: Fix drawing content layer identification for tab.

android: Fix drawing content layer identification for tab.

DoesLeafDrawContents in content_layer.cc traverses the tree to check if
the content layer for a tab will draw content. Currently if a layer
hides itself and its subtree, but has a descendent which draws content
and does not have a background color set, it would assume that the
layer will draw. This is wrong since if the layer hides it subtree then
its children will never draw.

How did this manage to cause the peculiar bug in the tab switcher?
Before http://crrev.com/2133873004, the RenderWidgetHostViewAndroid
would hold a SurfaceLayer with the frame from the renderer. When the
view is hidden, it would SetHideLayerAndSubtree on this layer, and
since the layer was the leaf node, DoesLeafDrawContents would correctly
identify that the tab's layer does not draw content. This is important
since in ContentLayer while we attach both the live and static layer
for a tab, the static layer's opacity is left 0 if the content layer
draws.

After this change, the DelegatedFrameHostAndroid holds a layer which is
a container for the SurfaceLayer. When the RenderWidgetHostViewAndroid
is now hidden, it hides this container layer instead. This causes
DoesLeafDrawContents to incorrectly assume that the tab's content layer
will draw.

The reason why we see a momentory black flash is that when a tab is
hidden, we enter the state above. But in the meanwhile, the now
visible tab receives its first frame from the renderer. This will cause
the RendererFrameManager to ask the invisible tab to evict its frame.
When this happens the DelegatedFrameHostAndroid will destroy the
SurfaceLayer and DoesLeafDrawContents in ContentLayer will now know
that the tab's layer will not draw.

BUG=644159
Committed: https://crrev.com/a15106c4b0abdb45b8d18ba255d6ff49f0e516a9
Cr-Commit-Position: refs/heads/master@{#419580}

[Khushal, 2016-09-19 17:39:01]

khushalsagar@chromium.org changed reviewers:
+ sievers@chromium.org, tedchoc@chromium.org

[Khushal, 2016-09-19 17:39:01]

I was thinking if there is a better way to check whether the tab's layer will
draw rather than inspecting the layer returned from ContentViewCore. Here is an
idea:

Right now in TabContentManager we save the layer for the tab directly. Could we
store an interface, TabContentProvider, implemented by TabAndroid instead? This
way when someone asks for the layer, tab can check that if its backed by
WebContents then the current RenderWidgetHostView has (IsShowing() &&
HasSurfaceAvailableForCopy()). There is some quirkiness with when we actually
hide the layer there for activity transitions stuff that I wasn't sure about.

[Khushal, 2016-09-19 17:59:48]

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

[David Trainor- moved to gerrit, 2016-09-19 18:01:05]

lgtm nice catch!

[David Trainor- moved to gerrit, 2016-09-19 18:02:17]

On 2016/09/19 17:39:01, Khushal wrote:
> I was thinking if there is a better way to check whether the tab's layer will
> draw rather than inspecting the layer returned from ContentViewCore. Here is
an
> idea:
> 
> Right now in TabContentManager we save the layer for the tab directly. Could
we
> store an interface, TabContentProvider, implemented by TabAndroid instead?
This
> way when someone asks for the layer, tab can check that if its backed by
> WebContents then the current RenderWidgetHostView has (IsShowing() &&
> HasSurfaceAvailableForCopy()). There is some quirkiness with when we actually
> hide the layer there for activity transitions stuff that I wasn't sure about.

That sounds fine.  I'd be fine with checking the RWHV or just passing that
information through the updateFrameInfo() and storing it or something.

[Ted C, 2016-09-19 18:48:26]

tedchoc@chromium.org changed reviewers:
+ mdjones@chromium.org

[Ted C, 2016-09-19 18:48:26]

+mdjones

lgtm - Is this something we could add a test for?  It would be good to make sure
this doesn't regress in the future.

[Khushal, 2016-09-19 18:56:56]

On 2016/09/19 18:48:26, Ted C wrote:
> +mdjones
> 
> lgtm - Is this something we could add a test for?  It would be good to make
sure
> this doesn't regress in the future.

I thought about adding a test but to some extent it seemed like we're bleeding
stuff that content does with the layer here (background color and all), so I
wasn't sure what the best way would be to validate that and I figured the better
thing would be to remove this logic here instead.
In the meanwhile should I add a test to validate what DoesLeafDrawContents is
doing right now?

[Ted C, 2016-09-19 20:02:28]

On 2016/09/19 18:56:56, Khushal wrote:
> On 2016/09/19 18:48:26, Ted C wrote:
> > +mdjones
> > 
> > lgtm - Is this something we could add a test for?  It would be good to make
> sure
> > this doesn't regress in the future.
> 
> I thought about adding a test but to some extent it seemed like we're bleeding
> stuff that content does with the layer here (background color and all), so I
> wasn't sure what the best way would be to validate that and I figured the
better
> thing would be to remove this logic here instead.
> In the meanwhile should I add a test to validate what DoesLeafDrawContents is
> doing right now?

It'd be nice to know that our transitions from visible tab <-> backgrounded tab
ensure that a valid layer is returned at all times.  I suspect our current setup
makes that quite difficult to do, but just posing the question of whether it was
feasible at all.

If testing DoesLeafDrawContents buys us anything, that is a good thing, but if
the
values that we are testing are set by an outside source and we wouldn't actually
be doing any testing what represents reality then it is probably not worth
investing
in.

[no sievers, 2016-09-19 20:17:37]

lgtm.

But as discussed there is probably still a race between Hide() and the readback
having finished *incl. compression* so that the static UIResourceLayer is
usable.

[Khushal, 2016-09-19 20:20:24]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-09-19 20:21:05]

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

[mdjones, 2016-09-19 20:23:42]

lgtm, but it's a bit frustrating that this code relies on properties of the tree
rather than whether or not content can be shown.

[commit-bot: I haz the power, 2016-09-19 21:53:23]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-19 21:55:24]

Description was changed from

==========
android: Fix drawing content layer identification for tab.

DoesLeafDrawContents in content_layer.cc traverses the tree to check if
the content layer for a tab will draw content. Currently if a layer
hides itself and its subtree, but has a descendent which draws content
and does not have a background color set, it would assume that the
layer will draw. This is wrong since if the layer hides it subtree then
its children will never draw.

How did this manage to cause the peculiar bug in the tab switcher?
Before http://crrev.com/2133873004, the RenderWidgetHostViewAndroid
would hold a SurfaceLayer with the frame from the renderer. When the
view is hidden, it would SetHideLayerAndSubtree on this layer, and
since the layer was the leaf node, DoesLeafDrawContents would correctly
identify that the tab's layer does not draw content. This is important
since in ContentLayer while we attach both the live and static layer
for a tab, the static layer's opacity is left 0 if the content layer
draws.

After this change, the DelegatedFrameHostAndroid holds a layer which is
a container for the SurfaceLayer. When the RenderWidgetHostViewAndroid
is now hidden, it hides this container layer instead. This causes
DoesLeafDrawContents to incorrectly assume that the tab's content layer
will draw.

The reason why we see a momentory black flash is that when a tab is
hidden, we enter the state above. But in the meanwhile, the now
visible tab receives its first frame from the renderer. This will cause
the RendererFrameManager to ask the invisible tab to evict its frame.
When this happens the DelegatedFrameHostAndroid will destroy the
SurfaceLayer and DoesLeafDrawContents in ContentLayer will now know
that the tab's layer will not draw.

BUG=644159
==========

to

==========
android: Fix drawing content layer identification for tab.

DoesLeafDrawContents in content_layer.cc traverses the tree to check if
the content layer for a tab will draw content. Currently if a layer
hides itself and its subtree, but has a descendent which draws content
and does not have a background color set, it would assume that the
layer will draw. This is wrong since if the layer hides it subtree then
its children will never draw.

How did this manage to cause the peculiar bug in the tab switcher?
Before http://crrev.com/2133873004, the RenderWidgetHostViewAndroid
would hold a SurfaceLayer with the frame from the renderer. When the
view is hidden, it would SetHideLayerAndSubtree on this layer, and
since the layer was the leaf node, DoesLeafDrawContents would correctly
identify that the tab's layer does not draw content. This is important
since in ContentLayer while we attach both the live and static layer
for a tab, the static layer's opacity is left 0 if the content layer
draws.

After this change, the DelegatedFrameHostAndroid holds a layer which is
a container for the SurfaceLayer. When the RenderWidgetHostViewAndroid
is now hidden, it hides this container layer instead. This causes
DoesLeafDrawContents to incorrectly assume that the tab's content layer
will draw.

The reason why we see a momentory black flash is that when a tab is
hidden, we enter the state above. But in the meanwhile, the now
visible tab receives its first frame from the renderer. This will cause
the RendererFrameManager to ask the invisible tab to evict its frame.
When this happens the DelegatedFrameHostAndroid will destroy the
SurfaceLayer and DoesLeafDrawContents in ContentLayer will now know
that the tab's layer will not draw.

BUG=644159

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

[commit-bot: I haz the power, 2016-09-19 21:55:25]

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