[android_webview] Do not declare WebView visible on detaching from window.

[android_webview] Do not declare WebView visible on detaching from window.

We have been pretending that WebView is visible for the Blink API
(OnShown/OnHidden) if WebView is not attached to a window. This makes
some operations work in background.

This CL eliminates the case when the WebView is declared visible
upon detach from window. Now the WebView is considered visible if it
has never been attached, but once it is attached, every subsequent
detach sets it invisible.

BUG=675950
Review-Url: https://codereview.chromium.org/2637273002
Cr-Commit-Position: refs/heads/master@{#444588}
Committed: https://chromium.googlesource.com/chromium/src/+/9288679e3eb12c219b520c6327ac5ab9a7741cb5

[Tima Vaisburd, 2017-01-17 21:55:31]

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

[commit-bot: I haz the power, 2017-01-17 21:55:57]

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

[Tima Vaisburd, 2017-01-17 22:04:54]

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

[Tima Vaisburd, 2017-01-17 22:04:55]

PTAL.

https://codereview.chromium.org/2637273002/diff/1/android_webview/browser/bro...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/1/android_webview/browser/bro...
android_webview/browser/browser_view_renderer.cc:452: is_client_visible_ =
!is_paused_ && window_visible_;
This code follows the current behavior and it is easy to see that we can turn
WebView invisible on attaching to window.
Seems counter-intuitive to me, shall we say
  if (!is_client_visible_)
    is_client_visible_ = !is_paused_ && window_visible_;   ?

[commit-bot: I haz the power, 2017-01-17 22:22:51]

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

[commit-bot: I haz the power, 2017-01-17 22:22:52]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-01-18 01:01:23]

Also add a test

https://codereview.chromium.org/2637273002/diff/1/android_webview/browser/bro...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/1/android_webview/browser/bro...
android_webview/browser/browser_view_renderer.cc:499: bool
BrowserViewRenderer::IsClientVisible() const {
please keep the the logic unified here like before

you should only need to add a new "bool was_attached_" state

[Tima Vaisburd, 2017-01-18 02:33:30]

Description was changed from

==========
[android_webview] Never make WebView visible on detaching from window.

We have been pretending that WebView is visible for the Blink API
(OnShown/OnHidden) if WebView is not attached to a window. This makes
some operations work in background.

Before this CL the visibility depended solely of the state of the
|is_paused_|, |attached_to_window_| and |window_visible_| properties.
This CL enhances the heuristics by adding dependency on the history
of operations. Namely, it prevents WebView to become visible when a
previously attached window is detached.

BUG=675950
==========

to

==========
[android_webview] Do not declare WebView visible on detaching from window.

We have been pretending that WebView is visible for the Blink API
(OnShown/OnHidden) if WebView is not attached to a window. This makes
some operations work in background.

This CL eliminates the case when the WebView is declared visible
upon detach from window. Now the WebView is considered visible if it
has never been attached, but once it is attached, every subsequent
detach sets it invisible.

BUG=675950
==========

[Tima Vaisburd, 2017-01-18 02:36:37]

https://codereview.chromium.org/2637273002/diff/1/android_webview/browser/bro...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/1/android_webview/browser/bro...
android_webview/browser/browser_view_renderer.cc:499: bool
BrowserViewRenderer::IsClientVisible() const {
On 2017/01/18 01:01:23, boliu wrote:
> please keep the the logic unified here like before
> 
> you should only need to add a new "bool was_attached_" state

Simplified, do you like it better now?

The test is not added yet, should it be in browser_view_renderer_unittest.cc ?

[boliu, 2017-01-18 03:07:50]

yeah, bvr_unittests is fine

https://codereview.chromium.org/2637273002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:492: return !is_paused_ &&
(!was_ever_attached_ || window_visible_);
I don't think logic is right.., unless I'm missing some implication?

It should be:

if (!was_attached)
  return !paused; (same condition as before, but !was_attached implies
!attached_to_window_)
else
  return !paused && attached && window_visible; (new condition)


That simplifies to:
!paused && (!was_attached || (!attached && visible))

Maybe should keep the if statements with comments stead

[Tima Vaisburd, 2017-01-18 03:22:45]

https://codereview.chromium.org/2637273002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:492: return !is_paused_ &&
(!was_ever_attached_ || window_visible_);
On 2017/01/18 03:07:50, boliu wrote:
> That simplifies to:
> !paused && (!was_attached || (!attached && visible))

I think you mean (!was_attached || (attached && visible))
 
> Maybe should keep the if statements with comments stead

Yes!

[Tima Vaisburd, 2017-01-19 00:18:40]

Added tests, please take a look at them.

https://codereview.chromium.org/2637273002/diff/20001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:492: return !is_paused_ &&
(!was_ever_attached_ || window_visible_);
On 2017/01/18 03:22:45, Tima Vaisburd wrote:
> On 2017/01/18 03:07:50, boliu wrote:
> > That simplifies to:
> > !paused && (!was_attached || (!attached && visible))
> 
> I think you mean (!was_attached || (attached && visible))
>  
> > Maybe should keep the if statements with comments stead
> 
> Yes!

Done.

[Tima Vaisburd, 2017-01-19 00:18:50]

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

[commit-bot: I haz the power, 2017-01-19 00:19:28]

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

[boliu, 2017-01-19 00:27:03]

lgtm after comment changes

https://codereview.chromium.org/2637273002/diff/40001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:492: // When WebView is not
paused, we decrare it visible even before it is
declare typo

https://codereview.chromium.org/2637273002/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:494: // attached, however, the
visibility is determined in the regular way, i.e.
drop the "regular way" bit, there's nothing regular about this..

[Tima Vaisburd, 2017-01-19 00:45:52]

https://codereview.chromium.org/2637273002/diff/40001/android_webview/browser...
File android_webview/browser/browser_view_renderer.cc (right):

https://codereview.chromium.org/2637273002/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:492: // When WebView is not
paused, we decrare it visible even before it is
On 2017/01/19 00:27:03, boliu wrote:
> declare typo

Done.

https://codereview.chromium.org/2637273002/diff/40001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:494: // attached, however, the
visibility is determined in the regular way, i.e.
On 2017/01/19 00:27:03, boliu wrote:
> drop the "regular way" bit, there's nothing regular about this..

Rewrote the last sentence to
"If it ever gets attached though, the WebView is visible as long as it
is attached to a window and the window is visible."

[Tima Vaisburd, 2017-01-19 00:46:20]

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

[commit-bot: I haz the power, 2017-01-19 00:46: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-19 01:15:42]

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

[commit-bot: I haz the power, 2017-01-19 01:15:43]

Dry run: This issue passed the CQ dry run.

[Tima Vaisburd, 2017-01-19 01:18:49]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2017-01-19 01:18:50]

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/2637273002/#ps60001
(title: "Fixed comment")

[commit-bot: I haz the power, 2017-01-19 01:19:32]

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-19 01:29:16]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484788729956320,
"parent_rev": "2742a8b3bdc414e596a9382a3b6959c0c4e95f93", "commit_rev":
"9288679e3eb12c219b520c6327ac5ab9a7741cb5"}

[commit-bot: I haz the power, 2017-01-19 01:29:55]

Description was changed from

==========
[android_webview] Do not declare WebView visible on detaching from window.

We have been pretending that WebView is visible for the Blink API
(OnShown/OnHidden) if WebView is not attached to a window. This makes
some operations work in background.

This CL eliminates the case when the WebView is declared visible
upon detach from window. Now the WebView is considered visible if it
has never been attached, but once it is attached, every subsequent
detach sets it invisible.

BUG=675950
==========

to

==========
[android_webview] Do not declare WebView visible on detaching from window.

We have been pretending that WebView is visible for the Blink API
(OnShown/OnHidden) if WebView is not attached to a window. This makes
some operations work in background.

This CL eliminates the case when the WebView is declared visible
upon detach from window. Now the WebView is considered visible if it
has never been attached, but once it is attached, every subsequent
detach sets it invisible.

BUG=675950

Review-Url: https://codereview.chromium.org/2637273002
Cr-Commit-Position: refs/heads/master@{#444588}
Committed:
https://chromium.googlesource.com/chromium/src/+/9288679e3eb12c219b520c6327ac...
==========

[commit-bot: I haz the power, 2017-01-19 01:29:56]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9288679e3eb12c219b520c6327ac...