Only gather transparent regions inside their own view bounds in views that might have children exte…

Only gather transparent regions inside their own view bounds in views that might have children extending beyond their bounds.

BUG=604742
Committed: https://crrev.com/0ab0514557a8f2d7536acf6b2c13ab17e27446b6
Cr-Commit-Position: refs/heads/master@{#410701}

[Bernhard Bauer, 2016-08-05 16:44:22]

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

[commit-bot: I haz the power, 2016-08-05 16:44:42]

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

[Bernhard Bauer, 2016-08-05 16:55:58]

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

[Bernhard Bauer, 2016-08-05 16:55:58]

Please review (and see the bug for the longer explanation as far as I understand
it).

[Bernhard Bauer, 2016-08-05 16:56:42]

Description was changed from

==========
Only gather transparent regions inside our own view bounds in views that might
have children extending beyond their bounds.

BUG=604742
==========

to

==========
Only gather transparent regions inside their own view bounds in views that might
have children extending beyond their bounds.

BUG=604742
==========

[Ted C, 2016-08-05 17:59:25]

lgtm and awesome find!

Now, before you submit can you check that this doesn't mess with our surface
view optimizations?

Go to a non-NTP page and have the omnibox be visible.  Run "adb shell dumpsys
SurfaceFlinger".  Look at the list of hardware layers/surfaces (you should see
one for SurfaceView and one for ChromeTabbedActivity).  Now scroll the omnibox
off and re-run the command.  Now you should only see the SurfaceView.

If you still see ChromeTabbedActivity then you're changes in
ToolbarControlContainer are probably confusing it.  n which case, I would add a
VISIBILITY check (which I'm assuming already happens at the ViewGroup level) and
then don't change the transparent region that case.

https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java
(right):

https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:81:
private static final int[] sLocationTmp = new int[2];
i think it's normal do put the variables at the top

https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:94:
return true;
I know this makes it a bit easier for embedders to use, but it's odd to return
always true here.  I'd probably leave this out and require embedders to return
it.

[Ted C, 2016-08-05 20:29:11]

On 2016/08/05 17:59:25, Ted C wrote:
> lgtm and awesome find!
> 
> Now, before you submit can you check that this doesn't mess with our surface
> view optimizations?
> 
> Go to a non-NTP page and have the omnibox be visible.  Run "adb shell dumpsys
> SurfaceFlinger".  Look at the list of hardware layers/surfaces (you should see
> one for SurfaceView and one for ChromeTabbedActivity).  Now scroll the omnibox
> off and re-run the command.  Now you should only see the SurfaceView.
> 
> If you still see ChromeTabbedActivity then you're changes in
> ToolbarControlContainer are probably confusing it.  n which case, I would add
a
> VISIBILITY check (which I'm assuming already happens at the ViewGroup level)
and
> then don't change the transparent region that case.
> 
>
https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
> File chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java
> (right):
> 
>
https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:81:
> private static final int[] sLocationTmp = new int[2];
> i think it's normal do put the variables at the top
> 
>
https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:94:
> return true;
> I know this makes it a bit easier for embedders to use, but it's odd to return
> always true here.  I'd probably leave this out and require embedders to return
> it.

Had a second thought here.  Why are we overriding gatherTransparentRegion in
these two places instead of just in NewTabPageView?  That draws underneath
the toolbar and contains the recycler view, so that seems like a better place
if it works.

[Bernhard Bauer, 2016-08-08 13:28:28]

On 2016/08/05 20:29:11, Ted C wrote:
> On 2016/08/05 17:59:25, Ted C wrote:
> > lgtm and awesome find!
> > 
> > Now, before you submit can you check that this doesn't mess with our surface
> > view optimizations?
> > 
> > Go to a non-NTP page and have the omnibox be visible.  Run "adb shell
dumpsys
> > SurfaceFlinger".  Look at the list of hardware layers/surfaces (you should
see
> > one for SurfaceView and one for ChromeTabbedActivity).  Now scroll the
omnibox
> > off and re-run the command.  Now you should only see the SurfaceView.
> > 
> > If you still see ChromeTabbedActivity then you're changes in
> > ToolbarControlContainer are probably confusing it.  n which case, I would
add
> a
> > VISIBILITY check (which I'm assuming already happens at the ViewGroup level)
> and
> > then don't change the transparent region that case.

So, I ran `dumpsys SurfaceFlinger`, and if the toolbar is hidden, the
ChromeTabbedActivity layer still shows up, but its visible region is empty. I
get the same dump on Canary (see
https://gist.github.com/sheepmaster/1ee37c675ad06cd635a11f667847855b), so I
don't think I'm breaking anything. Coming to think of it, even if the layer is
present, the SurfaceFlinger should know that the layer is fully transparent and
remove it from compositing -- that's exactly the optimization this is about :)

> Had a second thought here.  Why are we overriding gatherTransparentRegion in
> these two places instead of just in NewTabPageView?  That draws underneath
> the toolbar and contains the recycler view, so that seems like a better place
> if it works.

Hm, the toolbar is visited during the traversal *after* the NewTabPageView, so
if it then removes its bounds from the transparent region, we wouldn't be able
to fix it. I moved the override to the ToolbarViewResourceFrameLayout now, which
is the immediate parent of the toolbar.

https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java
(right):

https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:81:
private static final int[] sLocationTmp = new int[2];
On 2016/08/05 17:59:25, Ted C wrote:
> i think it's normal do put the variables at the top

Done.

https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:94:
return true;
On 2016/08/05 17:59:25, Ted C wrote:
> I know this makes it a bit easier for embedders to use, but it's odd to return
> always true here.  I'd probably leave this out and require embedders to return
> it.

Done.

[Bernhard Bauer, 2016-08-08 17:18:20]

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

[commit-bot: I haz the power, 2016-08-08 17:18:46]

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

[Ted C, 2016-08-08 17:32:00]

On 2016/08/08 13:28:28, Bernhard Bauer wrote:
> On 2016/08/05 20:29:11, Ted C wrote:
> > On 2016/08/05 17:59:25, Ted C wrote:
> > > lgtm and awesome find!
> > > 
> > > Now, before you submit can you check that this doesn't mess with our
surface
> > > view optimizations?
> > > 
> > > Go to a non-NTP page and have the omnibox be visible.  Run "adb shell
> dumpsys
> > > SurfaceFlinger".  Look at the list of hardware layers/surfaces (you should
> see
> > > one for SurfaceView and one for ChromeTabbedActivity).  Now scroll the
> omnibox
> > > off and re-run the command.  Now you should only see the SurfaceView.
> > > 
> > > If you still see ChromeTabbedActivity then you're changes in
> > > ToolbarControlContainer are probably confusing it.  n which case, I would
> add
> > a
> > > VISIBILITY check (which I'm assuming already happens at the ViewGroup
level)
> > and
> > > then don't change the transparent region that case.
> 
> So, I ran `dumpsys SurfaceFlinger`, and if the toolbar is hidden, the
> ChromeTabbedActivity layer still shows up, but its visible region is empty. I
> get the same dump on Canary (see
> https://gist.github.com/sheepmaster/1ee37c675ad06cd635a11f667847855b), so I
> don't think I'm breaking anything. Coming to think of it, even if the layer is
> present, the SurfaceFlinger should know that the layer is fully transparent
and
> remove it from compositing -- that's exactly the optimization this is about :)

I think later in the dumpsys, you'll actually see the allocated hardware surface
layers listed in a small table.  In that one, you should see
ChromeTabbedActivity
fall off.

  numHwLayers=5, flags=00000000
    type   |  handle  | hint | flag | tr | blnd |   format    |     source crop
(l,t,r,b)      |          frame         | name 
-----------+----------+------+------+----+------+-------------+--------------------------------+------------------------+------
       HWC | 733641bb40 | 0002 | 0000 | 00 | 0100 | RGBA_8888   |    0.0,   
0.0, 1356.0, 2392.0 |    0,    0, 1356, 2392 | SurfaceView -
com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity
       HWC | 7333ecd420 | 0002 | 0000 | 00 | 0105 | RGBA_8888   | 1125.0,   
0.0, 1440.0, 2392.0 | 1125,    0, 1440, 2392 |
com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity

Hmm...might not be the case on N-previews though.  I suspect you're fine, but
this
was more how I previously looked at this.

> 
> > Had a second thought here.  Why are we overriding gatherTransparentRegion in
> > these two places instead of just in NewTabPageView?  That draws underneath
> > the toolbar and contains the recycler view, so that seems like a better
place
> > if it works.
> 
> Hm, the toolbar is visited during the traversal *after* the NewTabPageView, so
> if it then removes its bounds from the transparent region, we wouldn't be able
> to fix it. I moved the override to the ToolbarViewResourceFrameLayout now,
which
> is the immediate parent of the toolbar.

Does it matter if it is visited after though?  I really don't know enough about
the API
to know, but it seems like siblings shouldn't be able to clobber the transparent
region
of other bits.  The toolbar and compositorview are siblings, so if the
compositor view
says that it is fully opaque, a truly transparent toolbar "shouldn't" affect it.
 But again,
the API might have a completely different behavior than what I'd expect.

> 
>
https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
> File chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java
> (right):
> 
>
https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:81:
> private static final int[] sLocationTmp = new int[2];
> On 2016/08/05 17:59:25, Ted C wrote:
> > i think it's normal do put the variables at the top
> 
> Done.
> 
>
https://codereview.chromium.org/2221483002/diff/20001/chrome/android/java/src...
> chrome/android/java/src/org/chromium/chrome/browser/util/ViewUtils.java:94:
> return true;
> On 2016/08/05 17:59:25, Ted C wrote:
> > I know this makes it a bit easier for embedders to use, but it's odd to
return
> > always true here.  I'd probably leave this out and require embedders to
return
> > it.
> 
> Done.

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

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

[commit-bot: I haz the power, 2016-08-08 18:07:21]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-09 12:49:52]

On 2016/08/08 17:32:00, Ted C (OOO till 8.15) wrote:
> On 2016/08/08 13:28:28, Bernhard Bauer wrote:
> > On 2016/08/05 20:29:11, Ted C wrote:
> > > On 2016/08/05 17:59:25, Ted C wrote:
> > > > lgtm and awesome find!
> > > > 
> > > > Now, before you submit can you check that this doesn't mess with our
> surface
> > > > view optimizations?
> > > > 
> > > > Go to a non-NTP page and have the omnibox be visible.  Run "adb shell
> > dumpsys
> > > > SurfaceFlinger".  Look at the list of hardware layers/surfaces (you
should
> > see
> > > > one for SurfaceView and one for ChromeTabbedActivity).  Now scroll the
> > omnibox
> > > > off and re-run the command.  Now you should only see the SurfaceView.
> > > > 
> > > > If you still see ChromeTabbedActivity then you're changes in
> > > > ToolbarControlContainer are probably confusing it.  n which case, I
would
> > add
> > > a
> > > > VISIBILITY check (which I'm assuming already happens at the ViewGroup
> level)
> > > and
> > > > then don't change the transparent region that case.
> > 
> > So, I ran `dumpsys SurfaceFlinger`, and if the toolbar is hidden, the
> > ChromeTabbedActivity layer still shows up, but its visible region is empty.
I
> > get the same dump on Canary (see
> > https://gist.github.com/sheepmaster/1ee37c675ad06cd635a11f667847855b), so I
> > don't think I'm breaking anything. Coming to think of it, even if the layer
is
> > present, the SurfaceFlinger should know that the layer is fully transparent
> and
> > remove it from compositing -- that's exactly the optimization this is about
:)
> 
> I think later in the dumpsys, you'll actually see the allocated hardware
surface
> layers listed in a small table.  In that one, you should see
> ChromeTabbedActivity
> fall off.
> 
>   numHwLayers=5, flags=00000000
>     type   |  handle  | hint | flag | tr | blnd |   format    |     source
crop
> (l,t,r,b)      |          frame         | name 
>
-----------+----------+------+------+----+------+-------------+--------------------------------+------------------------+------
>        HWC | 733641bb40 | 0002 | 0000 | 00 | 0100 | RGBA_8888   |    0.0,   
> 0.0, 1356.0, 2392.0 |    0,    0, 1356, 2392 | SurfaceView -
> com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity
>        HWC | 7333ecd420 | 0002 | 0000 | 00 | 0105 | RGBA_8888   | 1125.0,   
> 0.0, 1440.0, 2392.0 | 1125,    0, 1440, 2392 |
> com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity

Ah, I see. Yes, ChromeTabbedActivity doesn't show up there when the toolbar is
hidden.

> Hmm...might not be the case on N-previews though.  I suspect you're fine, but
> this
> was more how I previously looked at this.
> 
> > 
> > > Had a second thought here.  Why are we overriding gatherTransparentRegion
in
> > > these two places instead of just in NewTabPageView?  That draws underneath
> > > the toolbar and contains the recycler view, so that seems like a better
> place
> > > if it works.
> > 
> > Hm, the toolbar is visited during the traversal *after* the NewTabPageView,
so
> > if it then removes its bounds from the transparent region, we wouldn't be
able
> > to fix it. I moved the override to the ToolbarViewResourceFrameLayout now,
> which
> > is the immediate parent of the toolbar.
> 
> Does it matter if it is visited after though?  I really don't know enough
about
> the API
> to know, but it seems like siblings shouldn't be able to clobber the
transparent
> region
> of other bits.  The toolbar and compositorview are siblings, so if the
> compositor view
> says that it is fully opaque, a truly transparent toolbar "shouldn't" affect
it.
>  But again,
> the API might have a completely different behavior than what I'd expect.

Haha, yes, that is what one *might* think... What actually happens is that we
start out with everything transparent, and views remove their own bounds from
the transparent region that is passed through, but nothing prevents a view from
clobbering the region in any way it wants. More specifically in our case, there
were two places where we independently triggered the bug: one was with the
RecyclerView, the other one was when opening a new tab from an empty tab
switcher. I think in the latter case the toolbar still had some Y translation
applied to it when gatherTransparentRegion() was called, so it removed an area
outside of the activity bounds from the transparent region, and nothing would
fix that.

[Ted C, 2016-08-09 16:27:15]

On 2016/08/09 12:49:52, Bernhard Bauer wrote:
> On 2016/08/08 17:32:00, Ted C (OOO till 8.15) wrote:
> > On 2016/08/08 13:28:28, Bernhard Bauer wrote:
> > > On 2016/08/05 20:29:11, Ted C wrote:
> > > > On 2016/08/05 17:59:25, Ted C wrote:
> > > > > lgtm and awesome find!
> > > > > 
> > > > > Now, before you submit can you check that this doesn't mess with our
> > surface
> > > > > view optimizations?
> > > > > 
> > > > > Go to a non-NTP page and have the omnibox be visible.  Run "adb shell
> > > dumpsys
> > > > > SurfaceFlinger".  Look at the list of hardware layers/surfaces (you
> should
> > > see
> > > > > one for SurfaceView and one for ChromeTabbedActivity).  Now scroll the
> > > omnibox
> > > > > off and re-run the command.  Now you should only see the SurfaceView.
> > > > > 
> > > > > If you still see ChromeTabbedActivity then you're changes in
> > > > > ToolbarControlContainer are probably confusing it.  n which case, I
> would
> > > add
> > > > a
> > > > > VISIBILITY check (which I'm assuming already happens at the ViewGroup
> > level)
> > > > and
> > > > > then don't change the transparent region that case.
> > > 
> > > So, I ran `dumpsys SurfaceFlinger`, and if the toolbar is hidden, the
> > > ChromeTabbedActivity layer still shows up, but its visible region is
empty.
> I
> > > get the same dump on Canary (see
> > > https://gist.github.com/sheepmaster/1ee37c675ad06cd635a11f667847855b), so
I
> > > don't think I'm breaking anything. Coming to think of it, even if the
layer
> is
> > > present, the SurfaceFlinger should know that the layer is fully
transparent
> > and
> > > remove it from compositing -- that's exactly the optimization this is
about
> :)
> > 
> > I think later in the dumpsys, you'll actually see the allocated hardware
> surface
> > layers listed in a small table.  In that one, you should see
> > ChromeTabbedActivity
> > fall off.
> > 
> >   numHwLayers=5, flags=00000000
> >     type   |  handle  | hint | flag | tr | blnd |   format    |     source
> crop
> > (l,t,r,b)      |          frame         | name 
> >
>
-----------+----------+------+------+----+------+-------------+--------------------------------+------------------------+------
> >        HWC | 733641bb40 | 0002 | 0000 | 00 | 0100 | RGBA_8888   |    0.0,   
> > 0.0, 1356.0, 2392.0 |    0,    0, 1356, 2392 | SurfaceView -
> > com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity
> >        HWC | 7333ecd420 | 0002 | 0000 | 00 | 0105 | RGBA_8888   | 1125.0,   
> > 0.0, 1440.0, 2392.0 | 1125,    0, 1440, 2392 |
> > com.chrome.dev/org.chromium.chrome.browser.ChromeTabbedActivity
> 
> Ah, I see. Yes, ChromeTabbedActivity doesn't show up there when the toolbar is
> hidden.
> 
> > Hmm...might not be the case on N-previews though.  I suspect you're fine,
but
> > this
> > was more how I previously looked at this.
> > 
> > > 
> > > > Had a second thought here.  Why are we overriding
gatherTransparentRegion
> in
> > > > these two places instead of just in NewTabPageView?  That draws
underneath
> > > > the toolbar and contains the recycler view, so that seems like a better
> > place
> > > > if it works.
> > > 
> > > Hm, the toolbar is visited during the traversal *after* the
NewTabPageView,
> so
> > > if it then removes its bounds from the transparent region, we wouldn't be
> able
> > > to fix it. I moved the override to the ToolbarViewResourceFrameLayout now,
> > which
> > > is the immediate parent of the toolbar.
> > 
> > Does it matter if it is visited after though?  I really don't know enough
> about
> > the API
> > to know, but it seems like siblings shouldn't be able to clobber the
> transparent
> > region
> > of other bits.  The toolbar and compositorview are siblings, so if the
> > compositor view
> > says that it is fully opaque, a truly transparent toolbar "shouldn't" affect
> it.
> >  But again,
> > the API might have a completely different behavior than what I'd expect.
> 
> Haha, yes, that is what one *might* think...

:-/ sigh

 What actually happens is that we
> start out with everything transparent, and views remove their own bounds from
> the transparent region that is passed through, but nothing prevents a view
from
> clobbering the region in any way it wants. More specifically in our case,
there
> were two places where we independently triggered the bug: one was with the
> RecyclerView, the other one was when opening a new tab from an empty tab
> switcher. I think in the latter case the toolbar still had some Y translation
> applied to it when gatherTransparentRegion() was called, so it removed an area
> outside of the activity bounds from the transparent region, and nothing would
> fix that.

Thanks for the explanation and investigation on your end!  Still lgtm :-P

[Bernhard Bauer, 2016-08-09 16:34:36]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2016-08-09 16:35:02]

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-09 16:40:15]

Description was changed from

==========
Only gather transparent regions inside their own view bounds in views that might
have children extending beyond their bounds.

BUG=604742
==========

to

==========
Only gather transparent regions inside their own view bounds in views that might
have children extending beyond their bounds.

BUG=604742
==========

[commit-bot: I haz the power, 2016-08-09 16:40:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-09 16:41:54]

Description was changed from

==========
Only gather transparent regions inside their own view bounds in views that might
have children extending beyond their bounds.

BUG=604742
==========

to

==========
Only gather transparent regions inside their own view bounds in views that might
have children extending beyond their bounds.

BUG=604742

Committed: https://crrev.com/0ab0514557a8f2d7536acf6b2c13ab17e27446b6
Cr-Commit-Position: refs/heads/master@{#410701}
==========

[commit-bot: I haz the power, 2016-08-09 16:41:55]

Patchset 3 (id:??) landed as
https://crrev.com/0ab0514557a8f2d7536acf6b2c13ab17e27446b6
Cr-Commit-Position: refs/heads/master@{#410701}