Fix for fullscreen layout timing on Galaxy devices.

Fix for fullscreen layout timing on Galaxy devices.

Remove the requestLayout call in enterFullscreen

BUG=676634
Review-Url: https://codereview.chromium.org/2635563002
Cr-Original-Commit-Position: refs/heads/master@{#444352}
Committed: https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e9b3ed3aeb911
Review-Url: https://codereview.chromium.org/2635563002
Cr-Commit-Position: refs/heads/master@{#445070}
Committed: https://chromium.googlesource.com/chromium/src/+/6432d4b76477d8587270c0bc75689ae0f13aa989

[bokan, 2017-01-13 21:46:48]

Description was changed from

==========
Fix for fullscreen layout timing on Galaxy devices.

Move the requestLayout call to happen after SustemUiVisibility changes.

BUG=676634
==========

to

==========
Fix for fullscreen layout timing on Galaxy devices.

Move the requestLayout call to happen after SustemUiVisibility changes.

BUG=676634
==========

[bokan, 2017-01-13 21:46:48]

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

[bokan, 2017-01-13 22:07:17]

Ted, how does this look? It fixes the issue on my S7 and still works on N6.

I'm not familiar enough with Android or have the development env setup to know
for sure, but I think the issue is that because of where the requestLayout is,
we'd end up doing layout before we get the SystemUiVisiblityChanged message
(this was confirmed by logs, on an S7 we don't get a LayoutChanged after either
he FLAG_LAYOUT_FULLSCREEN set or FLAG_FULLSCREEN). I suspect the difference
comes from the S7 having hardware nav buttons so the only layout change comes
from the status bar. Perhaps LAYOUT_FULLSCREEN on Galaxy devices is buggy?
Interestingly, just calling requestLayout in addition inside the handleMessage
method (even with invalidate()) didn't help.

Do you know why we do this UI_FLAG_LAYOUT_FULLSCREEN -> UI_FLAG_FULLSCREEN
two-step at all? Why not just go straight to UI_FLAG_FULLSCREEN?

Also, no test since I'm not sure what the underlying difference causing this is
and have no idea how to simulate the conditions anyhow. Someone on Clank side
should take this over. In any case, 56 is getting close to stable so we should
land a fix ASAP.

[bokan, 2017-01-15 20:34:40]

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

[commit-bot: I haz the power, 2017-01-15 20:34:51]

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, 2017-01-15 21:05:25]

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

[commit-bot: I haz the power, 2017-01-15 21:05:26]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-01-17 19:30:10]

lgtm w/ adding back the other one to (especially for the 56 merge)

The reason we did it in two stages UI_FLAG_LAYOUT_FULLSCREEN ->
UI_FLAG_FULLSCREEN was that it give the renderer a chance to resize and start
drawing below the status bar (UI_FLAG_LAYOUT_FULLSCREEN) and then the transition
to fullscreen just scrolls off the status bar revealing content.

If we jumped right there (at least at the time, X years ago), we'd see garbage
content because the renderer hadn't pushed a frame of the new size yet.

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
(right):

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:143:
contentView.getRootView().requestLayout();
Hmm...I added the case below because we weren't getting the first layout change
callback.

To be safe, I would leave it below and add it here.

[bokan, 2017-01-17 21:43:11]

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
(right):

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:143:
contentView.getRootView().requestLayout();
On 2017/01/17 19:30:10, Ted C wrote:
> Hmm...I added the case below because we weren't getting the first layout
change
> callback.
> 
> To be safe, I would leave it below and add it here.

You mean leave the removed lines in? Having the requestLayout both here and in
enterFullscreen doesn't work. Perhaps there's a bug in Android? It seems as if
LAYOUT_FULLSCREEN doesn't include the status bar in the layout height (at least
if there's no navigation bar to hide), but LAYOUT_FULLSCREEN -> FULLSCREEN
transition doesn't invalidate the layout. I can try this out in a simple app but
it'll have to wait until I get home, I don't have an android dev environment
setup on my laptop.

Anyway, if I keep requestLayout in enterFullscreen, we never get an
onLayoutChange, neither on the listener added there nor the one added here.

[Ted C, 2017-01-17 22:09:45]

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
(right):

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:143:
contentView.getRootView().requestLayout();
On 2017/01/17 21:43:11, bokan wrote:
> On 2017/01/17 19:30:10, Ted C wrote:
> > Hmm...I added the case below because we weren't getting the first layout
> change
> > callback.
> > 
> > To be safe, I would leave it below and add it here.
> 
> You mean leave the removed lines in? Having the requestLayout both here and in
> enterFullscreen doesn't work. Perhaps there's a bug in Android? It seems as if
> LAYOUT_FULLSCREEN doesn't include the status bar in the layout height (at
least
> if there's no navigation bar to hide), but LAYOUT_FULLSCREEN -> FULLSCREEN
> transition doesn't invalidate the layout. I can try this out in a simple app
but
> it'll have to wait until I get home, I don't have an android dev environment
> setup on my laptop.
> 
> Anyway, if I keep requestLayout in enterFullscreen, we never get an
> onLayoutChange, neither on the listener added there nor the one added here.

Oh, that is rather unfortunate (and likely what your comment was referring to in
your original patch).  I had to add the request layout below otherwise it would
never get here in my local testing (probably a Nexus 5 of some incantation).

I'm afraid removing that below will break some device and the same here.

Oooh, is this only for clearing the fullscreen layout flag layout change
listener above?  If so, I think we can actually just remove it from the layout
change thing altogether and increase the timeout to something like 200MS instead
of 20MS.  Not exactly clean, but again, should be a clean merge and safer IMO.

[bokan, 2017-01-17 23:35:24]

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
(right):

https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:143:
contentView.getRootView().requestLayout();
On 2017/01/17 22:09:45, Ted C wrote:
> On 2017/01/17 21:43:11, bokan wrote:
> > On 2017/01/17 19:30:10, Ted C wrote:
> > > Hmm...I added the case below because we weren't getting the first layout
> > change
> > > callback.
> > > 
> > > To be safe, I would leave it below and add it here.
> > 
> > You mean leave the removed lines in? Having the requestLayout both here and
in
> > enterFullscreen doesn't work. Perhaps there's a bug in Android? It seems as
if
> > LAYOUT_FULLSCREEN doesn't include the status bar in the layout height (at
> least
> > if there's no navigation bar to hide), but LAYOUT_FULLSCREEN -> FULLSCREEN
> > transition doesn't invalidate the layout. I can try this out in a simple app
> but
> > it'll have to wait until I get home, I don't have an android dev environment
> > setup on my laptop.
> > 
> > Anyway, if I keep requestLayout in enterFullscreen, we never get an
> > onLayoutChange, neither on the listener added there nor the one added here.
> 
> Oh, that is rather unfortunate (and likely what your comment was referring to
in
> your original patch).  I had to add the request layout below otherwise it
would
> never get here in my local testing (probably a Nexus 5 of some incantation).

Presumably because we send the MSG_ID_SET_FULLSCREEN_SYSTEM_UI_FLAGS from the
onLayoutChange, right? Shouldn't we get an onContentViewSystemUiVisibilityChange
when we go into LAYOUT_FULLSCREEN which will send the
MSG_ID_SET_FULLSCREEN_SYSTEM_UI_FLAGS message and lead us here?

> 
> I'm afraid removing that below will break some device and the same here.
> 
> Oooh, is this only for clearing the fullscreen layout flag layout change
> listener above?  If so, I think we can actually just remove it from the layout
> change thing altogether and increase the timeout to something like 200MS
instead
> of 20MS.  Not exactly clean, but again, should be a clean merge and safer IMO.

No, the listener attached here doesn't seem to matter. In fact, this
requestLayout is a red herring, removing both makes it work on both the S7 and
Nexus 6. I'm seeing the onLayoutChange listener attached in enterFullscreen
called even without requestLayout. Perhaps the call to requestLayout makes us do
a layout before the status bar is hidden on S7 and since there's no navigation
bar, there's no geometry change?

Why did we need to explicitly requestLayout? Shouldn't changing the
systemUiVisibility automatically schedule a layout?

[Ted C, 2017-01-18 05:29:54]

On 2017/01/17 23:35:24, bokan wrote:
>
https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
> (right):
> 
>
https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:143:
> contentView.getRootView().requestLayout();
> On 2017/01/17 22:09:45, Ted C wrote:
> > On 2017/01/17 21:43:11, bokan wrote:
> > > On 2017/01/17 19:30:10, Ted C wrote:
> > > > Hmm...I added the case below because we weren't getting the first layout
> > > change
> > > > callback.
> > > > 
> > > > To be safe, I would leave it below and add it here.
> > > 
> > > You mean leave the removed lines in? Having the requestLayout both here
and
> in
> > > enterFullscreen doesn't work. Perhaps there's a bug in Android? It seems
as
> if
> > > LAYOUT_FULLSCREEN doesn't include the status bar in the layout height (at
> > least
> > > if there's no navigation bar to hide), but LAYOUT_FULLSCREEN -> FULLSCREEN
> > > transition doesn't invalidate the layout. I can try this out in a simple
app
> > but
> > > it'll have to wait until I get home, I don't have an android dev
environment
> > > setup on my laptop.
> > > 
> > > Anyway, if I keep requestLayout in enterFullscreen, we never get an
> > > onLayoutChange, neither on the listener added there nor the one added
here.
> > 
> > Oh, that is rather unfortunate (and likely what your comment was referring
to
> in
> > your original patch).  I had to add the request layout below otherwise it
> would
> > never get here in my local testing (probably a Nexus 5 of some incantation).
> 
> Presumably because we send the MSG_ID_SET_FULLSCREEN_SYSTEM_UI_FLAGS from the
> onLayoutChange, right? Shouldn't we get an
onContentViewSystemUiVisibilityChange
> when we go into LAYOUT_FULLSCREEN which will send the
> MSG_ID_SET_FULLSCREEN_SYSTEM_UI_FLAGS message and lead us here?
> 
> > 
> > I'm afraid removing that below will break some device and the same here.
> > 
> > Oooh, is this only for clearing the fullscreen layout flag layout change
> > listener above?  If so, I think we can actually just remove it from the
layout
> > change thing altogether and increase the timeout to something like 200MS
> instead
> > of 20MS.  Not exactly clean, but again, should be a clean merge and safer
IMO.
> 
> No, the listener attached here doesn't seem to matter. In fact, this
> requestLayout is a red herring, removing both makes it work on both the S7 and
> Nexus 6. I'm seeing the onLayoutChange listener attached in enterFullscreen
> called even without requestLayout. Perhaps the call to requestLayout makes us
do
> a layout before the status bar is hidden on S7 and since there's no navigation
> bar, there's no geometry change?
> 
> Why did we need to explicitly requestLayout? Shouldn't changing the
> systemUiVisibility automatically schedule a layout?

In your test, was the omnibox hidden or showing?  I "believe" I might have added
it because in the hidden state, going to fullscreen wasn't triggering the layout
change.

Sadly, I did a terrible job of annotating the code or the code review in this
case, so  I don't have a strong foot to stand on.  I was fighting the various
fullscreen tests on that patch, and I had to have added that for one of them.

If we can delete it and all tests pass, then I think we can just go with that
for now.  If it becomes an issue, I'll tweak the class to add layout change
listeners but also force the code based on timeouts if we can't find
a predictable signal.

[bokan, 2017-01-18 12:10:54]

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

[commit-bot: I haz the power, 2017-01-18 12:11:14]

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, 2017-01-18 12:42:06]

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

[commit-bot: I haz the power, 2017-01-18 12:42:07]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-01-18 14:43:41]

On 2017/01/18 05:29:54, Ted C wrote:
> On 2017/01/17 23:35:24, bokan wrote:
> >
>
https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2635563002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:143:
> > contentView.getRootView().requestLayout();
> > On 2017/01/17 22:09:45, Ted C wrote:
> > > On 2017/01/17 21:43:11, bokan wrote:
> > > > On 2017/01/17 19:30:10, Ted C wrote:
> > > > > Hmm...I added the case below because we weren't getting the first
layout
> > > > change
> > > > > callback.
> > > > > 
> > > > > To be safe, I would leave it below and add it here.
> > > > 
> > > > You mean leave the removed lines in? Having the requestLayout both here
> and
> > in
> > > > enterFullscreen doesn't work. Perhaps there's a bug in Android? It seems
> as
> > if
> > > > LAYOUT_FULLSCREEN doesn't include the status bar in the layout height
(at
> > > least
> > > > if there's no navigation bar to hide), but LAYOUT_FULLSCREEN ->
FULLSCREEN
> > > > transition doesn't invalidate the layout. I can try this out in a simple
> app
> > > but
> > > > it'll have to wait until I get home, I don't have an android dev
> environment
> > > > setup on my laptop.
> > > > 
> > > > Anyway, if I keep requestLayout in enterFullscreen, we never get an
> > > > onLayoutChange, neither on the listener added there nor the one added
> here.
> > > 
> > > Oh, that is rather unfortunate (and likely what your comment was referring
> to
> > in
> > > your original patch).  I had to add the request layout below otherwise it
> > would
> > > never get here in my local testing (probably a Nexus 5 of some
incantation).
> > 
> > Presumably because we send the MSG_ID_SET_FULLSCREEN_SYSTEM_UI_FLAGS from
the
> > onLayoutChange, right? Shouldn't we get an
> onContentViewSystemUiVisibilityChange
> > when we go into LAYOUT_FULLSCREEN which will send the
> > MSG_ID_SET_FULLSCREEN_SYSTEM_UI_FLAGS message and lead us here?
> > 
> > > 
> > > I'm afraid removing that below will break some device and the same here.
> > > 
> > > Oooh, is this only for clearing the fullscreen layout flag layout change
> > > listener above?  If so, I think we can actually just remove it from the
> layout
> > > change thing altogether and increase the timeout to something like 200MS
> > instead
> > > of 20MS.  Not exactly clean, but again, should be a clean merge and safer
> IMO.
> > 
> > No, the listener attached here doesn't seem to matter. In fact, this
> > requestLayout is a red herring, removing both makes it work on both the S7
and
> > Nexus 6. I'm seeing the onLayoutChange listener attached in enterFullscreen
> > called even without requestLayout. Perhaps the call to requestLayout makes
us
> do
> > a layout before the status bar is hidden on S7 and since there's no
navigation
> > bar, there's no geometry change?
> > 
> > Why did we need to explicitly requestLayout? Shouldn't changing the
> > systemUiVisibility automatically schedule a layout?
> 
> In your test, was the omnibox hidden or showing?  I "believe" I might have
added
> it because in the hidden state, going to fullscreen wasn't triggering the
layout
> change.

I was testing with the Omnibox showing, but I've now tried it hidden and it
still works without the requestLayout.

> 
> Sadly, I did a terrible job of annotating the code or the code review in this
> case, so  I don't have a strong foot to stand on.  I was fighting the various
> fullscreen tests on that patch, and I had to have added that for one of them.
> 
> If we can delete it and all tests pass, then I think we can just go with that
> for now.  If it becomes an issue, I'll tweak the class to add layout change
> listeners but also force the code based on timeouts if we can't find
> a predictable signal.

Ok, looks like the tests are passing. I've tried it with Omnibox hidden and
shown on:

Samsung Galaxy S7 running Marshmallow 
Nexus 5X running Nougat
Nexus 6 Running Marshmallow
Nexus 7v2 running Lollipop

Worked in all cases so I'm going to land this and request merge to 56.

[bokan, 2017-01-18 14:44:07]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-01-18 14:44:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2635563002/#ps20001
(title: "Remove the requestLayout")

[commit-bot: I haz the power, 2017-01-18 14:44: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, 2017-01-18 14:48:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484750647787290,
"parent_rev": "5dd1b512e8210a8f1ffe395b293c0f6cee3a61f2", "commit_rev":
"8e1c144ac1b517fb8406a3a3d17e9b3ed3aeb911"}

[commit-bot: I haz the power, 2017-01-18 14:48:44]

Description was changed from

==========
Fix for fullscreen layout timing on Galaxy devices.

Move the requestLayout call to happen after SustemUiVisibility changes.

BUG=676634
==========

to

==========
Fix for fullscreen layout timing on Galaxy devices.

Move the requestLayout call to happen after SustemUiVisibility changes.

BUG=676634

Review-Url: https://codereview.chromium.org/2635563002
Cr-Commit-Position: refs/heads/master@{#444352}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
==========

[commit-bot: I haz the power, 2017-01-18 14:48:45]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...

[bokan, 2017-01-18 14:50:57]

Description was changed from

==========
Fix for fullscreen layout timing on Galaxy devices.

Move the requestLayout call to happen after SustemUiVisibility changes.

BUG=676634

Review-Url: https://codereview.chromium.org/2635563002
Cr-Commit-Position: refs/heads/master@{#444352}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
==========

to

==========
Fix for fullscreen layout timing on Galaxy devices.

Remove the requestLayout call in enterFullscreen

BUG=676634

Review-Url: https://codereview.chromium.org/2635563002
Cr-Commit-Position: refs/heads/master@{#444352}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
==========

[Ted C, 2017-01-18 19:27:50]

On 2017/01/18 14:48:45, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:20001) as
>
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...

Alas, it does seem to have broken tests:
https://uberchromegw.corp.google.com/i/chromium.android/builders/Lollipop%20P...

[bokan, 2017-01-18 19:29:11]

On 2017/01/18 19:27:50, Ted C wrote:
> On 2017/01/18 14:48:45, commit-bot: I haz the power wrote:
> > Committed patchset #2 (id:20001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
> 
> Alas, it does seem to have broken tests:
>
https://uberchromegw.corp.google.com/i/chromium.android/builders/Lollipop%20P...

Ok, let me see if I can repro locally. Do we have those tests on a trybot I
could push a CL on?

[Ted C, 2017-01-18 19:38:14]

On 2017/01/18 19:29:11, bokan wrote:
> On 2017/01/18 19:27:50, Ted C wrote:
> > On 2017/01/18 14:48:45, commit-bot: I haz the power wrote:
> > > Committed patchset #2 (id:20001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
> > 
> > Alas, it does seem to have broken tests:
> >
>
https://uberchromegw.corp.google.com/i/chromium.android/builders/Lollipop%20P...
> 
> Ok, let me see if I can repro locally. Do we have those tests on a trybot I
> could push a CL on?

I don't see one sadly.  I might have just been testing on a Nexus 5 running L
at the time.

[bokan, 2017-01-19 03:27:58]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2645793002/ by bokan@chromium.org.

The reason for reverting is: Fullscreen tests breaking on Clank builders.
BUG=682353.

[bokan, 2017-01-20 00:17:29]

Ok, this passes all gtest_filter=*Fullscreen* tests and works manually on the
S7, N5 and N6. Strangely, the test failure on N5 didn't translate into any
issues when using fullscreen manually (I tried with both hidden and shown top
controls).

It seems that calling requestLayout on the root view in enterFullscreen prevents
the requestLayout after *really* going fullscreen from effecting any kind of
change on the S7. Doing a layout on the content view itself seems to do the
trick but I'm not really sure what's going on. WDYT?

Incidentally, if I just comment out the handleMessage handler that goes to
UI_FULLSCREEN (so that we layout to UI_LAYOUT_FULLSCREEN), the content appears
behind the status bar - as expected - but the height doesn't reflect the full
screen height. It's shorter than the screen by the navigation bar and status
bar. Adding the LAYOUT_HIDE_NAVIGATION increases the height by the navigation
bar but we still have a status bar sized gap. It seems LAYOUT_FULLSCREEN isn't
doing what we expect it to.

[Ted C, 2017-01-20 05:39:21]

On 2017/01/20 00:17:29, bokan wrote:
> Ok, this passes all gtest_filter=*Fullscreen* tests and works manually on the
> S7, N5 and N6. Strangely, the test failure on N5 didn't translate into any
> issues when using fullscreen manually (I tried with both hidden and shown top
> controls).
> 
> It seems that calling requestLayout on the root view in enterFullscreen
prevents
> the requestLayout after *really* going fullscreen from effecting any kind of
> change on the S7. Doing a layout on the content view itself seems to do the
> trick but I'm not really sure what's going on. WDYT?
> 
> Incidentally, if I just comment out the handleMessage handler that goes to
> UI_FULLSCREEN (so that we layout to UI_LAYOUT_FULLSCREEN), the content appears
> behind the status bar - as expected - but the height doesn't reflect the full
> screen height. It's shorter than the screen by the navigation bar and status
> bar. Adding the LAYOUT_HIDE_NAVIGATION increases the height by the navigation
> bar but we still have a status bar sized gap. It seems LAYOUT_FULLSCREEN isn't
> doing what we expect it to.

lgtm

Yeah, that is surprising that LAYOUT_FULLSCEEN is not telling us the height
increased
as that is definitely what the API implies (you're given the full viewport but
overdrawn
by the android UI controls).

Also, I thought I tried request layout on the contentview itself, but I again
maybe not.

Something indeed is acting very strange here.  I wouldn't be surprised if we do
need to make these layout listeners but also timeout based for unreliable
Android
APIs/versions.

[bokan, 2017-01-20 15:14:31]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2017-01-20 15:14:44]

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

[bokan, 2017-01-20 15:42:57]

On 2017/01/20 05:39:21, Ted C wrote:
> On 2017/01/20 00:17:29, bokan wrote:
> > Ok, this passes all gtest_filter=*Fullscreen* tests and works manually on
the
> > S7, N5 and N6. Strangely, the test failure on N5 didn't translate into any
> > issues when using fullscreen manually (I tried with both hidden and shown
top
> > controls).
> > 
> > It seems that calling requestLayout on the root view in enterFullscreen
> prevents
> > the requestLayout after *really* going fullscreen from effecting any kind of
> > change on the S7. Doing a layout on the content view itself seems to do the
> > trick but I'm not really sure what's going on. WDYT?
> > 
> > Incidentally, if I just comment out the handleMessage handler that goes to
> > UI_FULLSCREEN (so that we layout to UI_LAYOUT_FULLSCREEN), the content
appears
> > behind the status bar - as expected - but the height doesn't reflect the
full
> > screen height. It's shorter than the screen by the navigation bar and status
> > bar. Adding the LAYOUT_HIDE_NAVIGATION increases the height by the
navigation
> > bar but we still have a status bar sized gap. It seems LAYOUT_FULLSCREEN
isn't
> > doing what we expect it to.
> 
> lgtm
> 
> Yeah, that is surprising that LAYOUT_FULLSCEEN is not telling us the height
> increased
> as that is definitely what the API implies (you're given the full viewport but
> overdrawn
> by the android UI controls).
> 
> Also, I thought I tried request layout on the contentview itself, but I again
> maybe not.
> 
> Something indeed is acting very strange here.  I wouldn't be surprised if we
do
> need to make these layout listeners but also timeout based for unreliable
> Android
> APIs/versions.

I think we could just ditch the FULLSCREEN_LAYOUT intermediate step - I can't
see a difference on any devices back to Lollipop and I think that's what's
causing all the headache. If nothing else, we could do it just on Lollipop+.

[commit-bot: I haz the power, 2017-01-20 16:28:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484925271256270,
"parent_rev": "7de29d98ff473630649264e9837bfc4235d0921d", "commit_rev":
"6432d4b76477d8587270c0bc75689ae0f13aa989"}

[commit-bot: I haz the power, 2017-01-20 16:29:11]

Description was changed from

==========
Fix for fullscreen layout timing on Galaxy devices.

Remove the requestLayout call in enterFullscreen

BUG=676634

Review-Url: https://codereview.chromium.org/2635563002
Cr-Commit-Position: refs/heads/master@{#444352}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
==========

to

==========
Fix for fullscreen layout timing on Galaxy devices.

Remove the requestLayout call in enterFullscreen

BUG=676634

Review-Url: https://codereview.chromium.org/2635563002
Cr-Original-Commit-Position: refs/heads/master@{#444352}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e1c144ac1b517fb8406a3a3d17e...
Review-Url: https://codereview.chromium.org/2635563002
Cr-Commit-Position: refs/heads/master@{#445070}
Committed:
https://chromium.googlesource.com/chromium/src/+/6432d4b76477d8587270c0bc7568...
==========

[commit-bot: I haz the power, 2017-01-20 16:29:12]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6432d4b76477d8587270c0bc7568...

[Ted C, 2017-01-20 16:42:20]

On 2017/01/20 15:42:57, bokan wrote:
> On 2017/01/20 05:39:21, Ted C wrote:
> > On 2017/01/20 00:17:29, bokan wrote:
> > > Ok, this passes all gtest_filter=*Fullscreen* tests and works manually on
> the
> > > S7, N5 and N6. Strangely, the test failure on N5 didn't translate into any
> > > issues when using fullscreen manually (I tried with both hidden and shown
> top
> > > controls).
> > > 
> > > It seems that calling requestLayout on the root view in enterFullscreen
> > prevents
> > > the requestLayout after *really* going fullscreen from effecting any kind
of
> > > change on the S7. Doing a layout on the content view itself seems to do
the
> > > trick but I'm not really sure what's going on. WDYT?
> > > 
> > > Incidentally, if I just comment out the handleMessage handler that goes to
> > > UI_FULLSCREEN (so that we layout to UI_LAYOUT_FULLSCREEN), the content
> appears
> > > behind the status bar - as expected - but the height doesn't reflect the
> full
> > > screen height. It's shorter than the screen by the navigation bar and
status
> > > bar. Adding the LAYOUT_HIDE_NAVIGATION increases the height by the
> navigation
> > > bar but we still have a status bar sized gap. It seems LAYOUT_FULLSCREEN
> isn't
> > > doing what we expect it to.
> > 
> > lgtm
> > 
> > Yeah, that is surprising that LAYOUT_FULLSCEEN is not telling us the height
> > increased
> > as that is definitely what the API implies (you're given the full viewport
but
> > overdrawn
> > by the android UI controls).
> > 
> > Also, I thought I tried request layout on the contentview itself, but I
again
> > maybe not.
> > 
> > Something indeed is acting very strange here.  I wouldn't be surprised if we
> do
> > need to make these layout listeners but also timeout based for unreliable
> > Android
> > APIs/versions.
> 
> I think we could just ditch the FULLSCREEN_LAYOUT intermediate step - I can't
> see a difference on any devices back to Lollipop and I think that's what's
> causing all the headache. If nothing else, we could do it just on Lollipop+.

Yeah, that code predates lollipop by a lot, but if we see that it gains us
nothing on L+ then dropping it sgtm