Refactor BrowserViewRenderer to support page visibility

Refactor BrowserViewRenderer to support page visibility

The page visiblity will be calculated in BVR::IsVisible and AwContents
will not calculate it again. It reads from BVR via JNI instead.

BUG=520089
Committed: https://crrev.com/bcf491b6751a352bda8d801907c05a9215f47007
Cr-Commit-Position: refs/heads/master@{#351253}

[hush (inactive), 2015-09-28 21:25:36]

Patchset #1 (id:1) has been deleted

[hush (inactive), 2015-09-28 21:26:11]

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

[hush (inactive), 2015-09-28 21:26:12]

PTAL

[boliu, 2015-09-28 21:31:31]

This is worse than before. Now native side makes no sense either. Also you are
spreading the "is page visibility enabled" among more files, which is the
opposite of what the refactor was for

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

https://codereview.chromium.org/1377593003/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:107:
base::CommandLine::ForCurrentProcess()
nit: Do the CommandLine check in AwContents, and pass the bool in as a
parameter.

https://codereview.chromium.org/1377593003/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:448: return
!attached_to_window_ || window_visible_;
This... makes zero sense. This is IsWindowVisible.

[boliu, 2015-09-28 21:39:39]

There are at least two or three different concepts that need to be computed:

inputs: attached, window visible, view visible, paused

window visible = (!attached || window visible)
content view core visible <- this one depends on the command line switch
maybe also devtools visible. Maybe this should match CVC visible, not sure

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

https://codereview.chromium.org/1377593003/diff/20001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:660: bool
throttle_fallback_tick = (is_paused_ && !clear_view_) || !IsVisible();
Not everything need to be refactor. This was checking for "WindowVisible", not
generally visible, and that's intentional.

[hush (inactive), 2015-09-28 23:27:02]

Let me just summarize a little:

1. CVC isVisible: !isPaused when page visibility is disabled, and !isPausled &&
window visible && view visible otherwise

2. DevTools visible: window visible && view visible regardless of page
visibility toggle. !isPaused *could be* taken into consideration too for
DevTools visible.

3. SychronousCompositor isActive: !isPaused && window visible when page
visibility is disabled, and !isPausled && window visible && view visible
otherwise

The three values are the same when page visibility is enabled, but different
from one another when it is disabled.

[boliu, 2015-09-28 23:41:17]

Yep all correct.

And maybe not bother changing fallback tick logic since it's going away soon.

On 2015/09/28 23:27:02, hush wrote:
> Let me just summarize a little:
> 
> 1. CVC isVisible: !isPaused when page visibility is disabled, and !isPausled
&&
> window visible && view visible otherwise
> 
> 2. DevTools visible: window visible && view visible regardless of page
> visibility toggle. !isPaused *could be* taken into consideration too for
> DevTools visible.

Yeah I'm on the fence for the isPaused bit. Maybe ask Mikhail for opinion?

> 
> 3. SychronousCompositor isActive: !isPaused && window visible when page
> visibility is disabled, and !isPausled && window visible && view visible
> otherwise
> 
> The three values are the same when page visibility is enabled, but different
> from one another when it is disabled.

[hush (inactive), 2015-09-29 00:53:10]

PTAL PS2.
So BVR will know the page visibility commandline flag and decides the visibility
for CVC, the synchronous compositor and Dev Tools.

[hush (inactive), 2015-09-29 00:54:11]

Patchset #2 (id:40001) has been deleted

[hush (inactive), 2015-09-29 00:55:09]

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:450: bool
BrowserViewRenderer::IsClientVisible() const {
this is for CVC's visibility. I didn't think of any better name.

[boliu, 2015-09-29 01:00:42]

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:107:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
again, do this in aw_contents.cc and pass it in, don't have BVR depend on
CommandLine

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:450: bool
BrowserViewRenderer::IsClientVisible() const {
On 2015/09/29 00:55:09, hush wrote:
> this is for CVC's visibility. I didn't think of any better name.

Shouldn't paused be handled here as well then?

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:109:
private static final String ENABLE_PAGE_VISIBILITY = "enable-page-visibility";
remove this

[hush (inactive), 2015-09-29 01:38:59]

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:107:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/09/29 01:00:42, boliu wrote:
> again, do this in aw_contents.cc and pass it in, don't have BVR depend on
> CommandLine

BVR already depends on base::commandline in
BrowserViewRenderer::CalculateTileMemoryPolicy?

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:450: bool
BrowserViewRenderer::IsClientVisible() const {
On 2015/09/29 01:00:42, boliu wrote:
> On 2015/09/29 00:55:09, hush wrote:
> > this is for CVC's visibility. I didn't think of any better name.
> 
> Shouldn't paused be handled here as well then?

Done.

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:109:
private static final String ENABLE_PAGE_VISIBILITY = "enable-page-visibility";
On 2015/09/29 01:00:42, boliu wrote:
> remove this

oh... my bad. Removed.

[boliu, 2015-09-29 01:40:23]

new patch set?

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:107:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/09/29 01:38:59, hush wrote:
> On 2015/09/29 01:00:42, boliu wrote:
> > again, do this in aw_contents.cc and pass it in, don't have BVR depend on
> > CommandLine
> 
> BVR already depends on base::commandline in
> BrowserViewRenderer::CalculateTileMemoryPolicy?

Maybe that should be fixed too

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

new patch set is just uploaded

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

https://codereview.chromium.org/1377593003/diff/60001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:107:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/09/29 01:40:23, boliu wrote:
> On 2015/09/29 01:38:59, hush wrote:
> > On 2015/09/29 01:00:42, boliu wrote:
> > > again, do this in aw_contents.cc and pass it in, don't have BVR depend on
> > > CommandLine
> > 
> > BVR already depends on base::commandline in
> > BrowserViewRenderer::CalculateTileMemoryPolicy?
> 
> Maybe that should be fixed too

let's not change CalculateTileMemoryPolicy here since it is quite unrelated to
page visiblity.

[boliu, 2015-09-29 01:47:28]

lgtm % last comment

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

https://codereview.chromium.org/1377593003/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:732:
compositor_->SetIsActive(!is_paused_ && IsVisible());
IsClientVisible here

[hush (inactive), 2015-09-29 01:56:52]

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

https://codereview.chromium.org/1377593003/diff/80001/android_webview/browser...
android_webview/browser/browser_view_renderer.cc:732:
compositor_->SetIsActive(!is_paused_ && IsVisible());
On 2015/09/29 01:47:27, boliu wrote:
> IsClientVisible here

Done.

[hush (inactive), 2015-09-29 01:59:59]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-09-29 01:59:59]

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/1377593003/#ps100001
(title: "comments")

[hush (inactive), 2015-09-29 02:00:05]

The CQ bit was checked by hush@chromium.org

[commit-bot: I haz the power, 2015-09-29 02:00:58]

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

[commit-bot: I haz the power, 2015-09-29 02:15:21]

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

[commit-bot: I haz the power, 2015-09-29 02:15:22]

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

[hush (inactive), 2015-09-29 03:02:00]

The CQ bit was checked by hush@chromium.org

[hush (inactive), 2015-09-29 03:02:01]

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/1377593003/#ps140001
(title: "fix test compilation")

[commit-bot: I haz the power, 2015-09-29 03:02:46]

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

[commit-bot: I haz the power, 2015-09-29 03:38:18]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-09-29 03:39:00]

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