Update WebView visibility during attach/detach

Update WebView visibility during attach/detach

Whether WebView is attached to window is factored into the page
visibility. So when the condition changes, WebView needs to update the
visibility.

BUG=550532
Committed: https://crrev.com/c983c0d13e74c4c1c4f90e276e8c0bd0dc8d5716
Cr-Commit-Position: refs/heads/master@{#357723}

[hush (inactive), 2015-11-03 02:47:15]

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

[hush (inactive), 2015-11-03 02:47:16]

Hi Bo,
PTAL.
So during detachFromWindow, the window visibility first becomes invisible,
setting the CVC visibility to false. Then webview gets detached from window,
setting the CVC visibility to true. I don't know how WebView would get rid of
this quick flip of visibility because the window visibility can be changed
separately, not necessarily during detach from window. Basically webview does
not have pre-knowledge of what the next visibility changing event is.

[boliu, 2015-11-03 02:49:10]

lgtm

On 2015/11/03 02:47:16, hush wrote:
> Hi Bo,
> PTAL.
> So during detachFromWindow, the window visibility first becomes invisible,
> setting the CVC visibility to false. Then webview gets detached from window,
> setting the CVC visibility to true. I don't know how WebView would get rid of
> this quick flip of visibility because the window visibility can be changed
> separately, not necessarily during detach from window. Basically webview does
> not have pre-knowledge of what the next visibility changing event is.

You could post the toggle.

[hush (inactive), 2015-11-03 19:55:41]

On 2015/11/03 02:49:10, boliu wrote:
> lgtm
> 
> On 2015/11/03 02:47:16, hush wrote:
> > Hi Bo,
> > PTAL.
> > So during detachFromWindow, the window visibility first becomes invisible,
> > setting the CVC visibility to false. Then webview gets detached from window,
> > setting the CVC visibility to true. I don't know how WebView would get rid
of
> > this quick flip of visibility because the window visibility can be changed
> > separately, not necessarily during detach from window. Basically webview
does
> > not have pre-knowledge of what the next visibility changing event is.
> 
> You could post the toggle.

okay. Let me submit this now, and deal with the flip later

[hush (inactive), 2015-11-03 19:55:45]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2015-11-03 19:58:17]

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

[commit-bot: I haz the power, 2015-11-03 21:13:41]

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

[commit-bot: I haz the power, 2015-11-03 21:13:42]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[hush (inactive), 2015-11-03 21:29:45]

On 2015/11/03 21:13:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

The test failures look real. I think we have to address the visibility flip
issue in this CL (which will be merged back to m47)

[boliu, 2015-11-03 22:02:39]

On 2015/11/03 21:29:45, hush wrote:
> On 2015/11/03 21:13:42, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
> 
> The test failures look real. I think we have to address the visibility flip
> issue in this CL (which will be merged back to m47)

Can't tell much from the bot, so I guess run test locally and see.

But this made me think of something:
WasHidden will pause any playing video I think
And due to the bit flip, we will always cause a WasHidden when entering full
screen. So I think now whenever we enter full screen, the video will pause. Is
this true?

[hush (inactive), 2015-11-03 22:03:10]

PTAL ps2.
I think the hidden assumption is that onAttachedToWindow and
onWindowVisibilityChanged are called in the same function call in android
frameworks (which is true right now. View#dispatchAttachedToWindow) If that is
not the case, then the post task in onAttachedToWindow may still be run before
onWindowVisibilityChanged.

[boliu, 2015-11-03 22:05:46]

https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2369:
mHandler.post(new Runnable() {
Can you not post more than you need to?

Also don't re-allocate new runnable every time. Save it in an instance variable,
and maybe allocate it lazily. (To avoid gc churn)

[hush (inactive), 2015-11-03 22:07:54]

On 2015/11/03 22:02:39, boliu wrote:
> On 2015/11/03 21:29:45, hush wrote:
> > On 2015/11/03 21:13:42, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
> > 
> > The test failures look real. I think we have to address the visibility flip
> > issue in this CL (which will be merged back to m47)
> 
> Can't tell much from the bot, so I guess run test locally and see.
> 
> But this made me think of something:
> WasHidden will pause any playing video I think
> And due to the bit flip, we will always cause a WasHidden when entering full
> screen. So I think now whenever we enter full screen, the video will pause. Is
> this true?

Yes. With the patch set 1, CVC will send washidden immediately after
onAttachedToWindow, which I think it deletes the video hole punching surface
that was just created. Before this CL, CVC's visibility only changes from false
to true once, so it does not send WasHidden.

[hush (inactive), 2015-11-03 22:09:59]

On 2015/11/03 22:05:46, boliu wrote:
>
https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:2369:
> mHandler.post(new Runnable() {
> Can you not post more than you need to?
> 
> Also don't re-allocate new runnable every time. Save it in an instance
variable,
> and maybe allocate it lazily. (To avoid gc churn)

Yeah. I was thinking of doing that only for trunk but not for m47. Not sure how
much complexity it is going to add.
I was planning on saving the runnable as a member in AwContents and cancel it
with Handler#removeCallbacks.

[boliu, 2015-11-03 22:13:03]

On 2015/11/03 22:09:59, hush wrote:
> On 2015/11/03 22:05:46, boliu wrote:
> >
>
https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
> > File android_webview/java/src/org/chromium/android_webview/AwContents.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
> > android_webview/java/src/org/chromium/android_webview/AwContents.java:2369:
> > mHandler.post(new Runnable() {
> > Can you not post more than you need to?
> > 
> > Also don't re-allocate new runnable every time. Save it in an instance
> variable,
> > and maybe allocate it lazily. (To avoid gc churn)
> 
> Yeah. I was thinking of doing that only for trunk but not for m47. Not sure
how
> much complexity it is going to add.
> I was planning on saving the runnable as a member in AwContents and cancel it
> with Handler#removeCallbacks.

You don't need to cancel. Just don't post if there is already one pending, in
which case you might end up posting like 5 of them; and you can do that with a
bool. Then saving the callback in a member to avoid allocation is a separate
issue.

[boliu, 2015-11-03 22:14:28]

On 2015/11/03 22:13:03, boliu wrote:
> On 2015/11/03 22:09:59, hush wrote:
> > On 2015/11/03 22:05:46, boliu wrote:
> > >
> >
>
https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
> > > File android_webview/java/src/org/chromium/android_webview/AwContents.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1410333012/diff/20001/android_webview/java/sr...
> > >
android_webview/java/src/org/chromium/android_webview/AwContents.java:2369:
> > > mHandler.post(new Runnable() {
> > > Can you not post more than you need to?
> > > 
> > > Also don't re-allocate new runnable every time. Save it in an instance
> > variable,
> > > and maybe allocate it lazily. (To avoid gc churn)
> > 
> > Yeah. I was thinking of doing that only for trunk but not for m47. Not sure
> how
> > much complexity it is going to add.
> > I was planning on saving the runnable as a member in AwContents and cancel
it
> > with Handler#removeCallbacks.
> 
> You don't need to cancel. Just don't post if there is already one pending, in
> which case you might end up posting like 5 of them; and you can do that with a
> bool. Then saving the callback in a member to avoid allocation is a separate
> issue.

s/in which case/otherwise/

[boliu, 2015-11-03 23:15:24]

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:327:
private class UpdateVisibilityRunnable implements Runnable {
Don't really need to declare a whole new class for this imo

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2380:
mHandler.post(mUpdateVisibilityRunnable);
set pending to true here

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2384:
mIsUpdateVisibilityTaskPending = true;
set pending to false here

[hush (inactive), 2015-11-03 23:32:47]

okay.. so we run into a pretty grey area here:
you can't always post task to update the CVC visibility here.
As you can see from the test bot run in patch set 2, 2 popup related tests
failed.

I found that the popup webview is not shown at all. The problem is that during
"receivePopupContents", there is a setNewAwContents call which updates the CVC
visibility. If the visibility is not updated immediately, the popup window is
not shown at all. (I still need to check the code for reason of this)

Anyhow, the failure in popup tests can be fixed by calling
updateContentViewCoreVisibility() directly in setNewAwContents.

This leads me to think that post task should be done only when necessary.
Otherwise, use the direct call.

[boliu, 2015-11-03 23:35:02]

On 2015/11/03 23:32:47, hush wrote:
> This leads me to think that post task should be done only when necessary.
> Otherwise, use the direct call.

That sounds ok. Just leave a comment explaining why that case should be posted

[hush (inactive), 2015-11-04 00:44:04]

On 2015/11/03 23:35:02, boliu wrote:
> On 2015/11/03 23:32:47, hush wrote:
> > This leads me to think that post task should be done only when necessary.
> > Otherwise, use the direct call.
> 
> That sounds ok. Just leave a comment explaining why that case should be posted

okay. To answer myself the question: why does the setNewAwContents for the pop
up window needs to call updateContentViewCoreVisibility immediately?
That's because the popup awcontents is visible before and after
"supplyContentsForPopup". (not attached to window before
"supplyContentsForPopup", so visible. And attached to a visible window after
"supplyContentsForPopup", so visible again)
But we do need a flip here, so that ContentViewCore#onShow will be called after
the popup awcontents received content. That is point of these cleaning up calls:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

In short, CVC#onshow has to be called after the popup contents is received, and
post tasks will actually make the popup contents always visible, so CVC#onshow
is not called.

[hush (inactive), 2015-11-04 00:44:13]

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:327:
private class UpdateVisibilityRunnable implements Runnable {
On 2015/11/03 23:15:24, boliu wrote:
> Don't really need to declare a whole new class for this imo

ok. I just deleted it, and create new Runnable when needed.

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2380:
mHandler.post(mUpdateVisibilityRunnable);
On 2015/11/03 23:15:24, boliu wrote:
> set pending to true here

Done.

https://codereview.chromium.org/1410333012/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2384:
mIsUpdateVisibilityTaskPending = true;
On 2015/11/03 23:15:23, boliu wrote:
> set pending to false here

Done.

[boliu, 2015-11-04 00:49:03]

lgtm if bot agrees, so glad we have tests that actually catch things :)

https://codereview.chromium.org/1410333012/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1410333012/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2376:
mHandler.post(new Runnable() {
I mean you don't need to declare a new runnable class, but do save the runnable
in an instance var to avoid gc churn. But honestly it doesn't matter that much,
so up to you.

[hush (inactive), 2015-11-04 01:00:35]

https://codereview.chromium.org/1410333012/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1410333012/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2376:
mHandler.post(new Runnable() {
On 2015/11/04 00:49:02, boliu wrote:
> I mean you don't need to declare a new runnable class, but do save the
runnable
> in an instance var to avoid gc churn. But honestly it doesn't matter that
much,
> so up to you.

okay. I just did it

[hush (inactive), 2015-11-04 01:01:11]

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

[commit-bot: I haz the power, 2015-11-04 01:08:45]

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

[commit-bot: I haz the power, 2015-11-04 01:38:30]

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

[commit-bot: I haz the power, 2015-11-04 01:38:31]

Dry run: This issue passed the CQ dry run.

[hush (inactive), 2015-11-04 01:45:08]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-11-04 01:45:09]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1410333012/#ps80001
(title: "reuse runnable object")

[commit-bot: I haz the power, 2015-11-04 01:46:37]

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

[commit-bot: I haz the power, 2015-11-04 01:53:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-04 01:54:55]

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