Remove java ViewAndroid class

Remove java ViewAndroid class

The java ViewAndroid class was only surving two purposes:

1) Implement content::PowerSaveBlocker with anchor views.
This is only used from content, and is mostly implemented
in content already, so just move the entire implementation
to content PowerSaveBlocker without going through a ui
layer interface.

2) Hold the ViewAndroidDelegate. Just have the native class
hold this object directly instead.

Other required changes:
* Updated to one anchor view per PowerSaveBlocker instead
  one per ContentViewCore and keeping a count.
* PowerSaveBlockerImpl::Delegate is made a
  WebContentsObserver to observer WebContents destruction.
* Removed an instrumentation test that's no longer
  relevant.

BUG=469803
Committed: https://crrev.com/65a9d1d2258c3068196b8a25261b9ad7fa0b107c
Cr-Commit-Position: refs/heads/master@{#324357}

[boliu, 2015-03-31 22:19:22]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org, tedchoc@chromium.org

[boliu, 2015-03-31 22:19:23]

ptal

I chose to put the PowerSaveBlocker implementation in ContentViewCore.

The other option is put it in ContentViewAndroidDelegate (factored to separate
file). Then PowerSaveBlocker can retrieve ViewAndroidDelegate from native
ViewAndroid, and cast it to ContentViewAndroidDelegate. I didn't like this
because it unnecessarily involves a ui layer interface.

Open to other suggestions since I know we don't want to shove more things into
CVC.

ViewAndroidDelegate can probably be renamed to something like
AnchorViewDelegate. Waiting for a follow up to do that.

[boliu, 2015-04-06 16:15:50]

On 2015/03/31 22:19:23, boliu wrote:
> ptal
> 
> I chose to put the PowerSaveBlocker implementation in ContentViewCore.
> 
> The other option is put it in ContentViewAndroidDelegate (factored to separate
> file). Then PowerSaveBlocker can retrieve ViewAndroidDelegate from native
> ViewAndroid, and cast it to ContentViewAndroidDelegate. I didn't like this
> because it unnecessarily involves a ui layer interface.
> 
> Open to other suggestions since I know we don't want to shove more things into
> CVC.
> 
> ViewAndroidDelegate can probably be renamed to something like
> AnchorViewDelegate. Waiting for a follow up to do that.

after branch point ping!

[Ted C, 2015-04-06 20:56:11]

https://codereview.chromium.org/1041823004/diff/20001/content/browser/power_s...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/1041823004/diff/20001/content/browser/power_s...
content/browser/power_save_blocker_android.cc:59: if (!web_contents())
it took me a second to see why this needed to be a WebContentsObserver but it is
because of web_contents() doing the right thing.

I guess the screen on count dies when a CVC dies right?  Since the view we use
for the power check is added via the view delegate.

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
(left):

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
private static void applyBlock(ViewAndroid view) {
I honestly would prefer this class remain and all the logic in CVC could live in
here (making this a 1:1 relationship with the native delegate).

[boliu, 2015-04-06 21:04:07]

https://codereview.chromium.org/1041823004/diff/20001/content/browser/power_s...
File content/browser/power_save_blocker_android.cc (right):

https://codereview.chromium.org/1041823004/diff/20001/content/browser/power_s...
content/browser/power_save_blocker_android.cc:59: if (!web_contents())
On 2015/04/06 20:56:11, Ted C wrote:
> it took me a second to see why this needed to be a WebContentsObserver but it
is
> because of web_contents() doing the right thing.
> 
> I guess the screen on count dies when a CVC dies right?

No in general, but yes in practice on android.

PowerSaveBlocker is a content/public api that's not tied to the life time of any
other content object (and it doesn't work on android, since android needs the
special InitDisplaySleepBlocker method).

The existing code is still dangerous because it used to hold a raw pointer to
the ViewAndroid across a PostTask. If CVC.destroy is called after the task is
posted but before it runs, then it leads to a UAF.

> Since the view we use
> for the power check is added via the view delegate.

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
(left):

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
private static void applyBlock(ViewAndroid view) {
On 2015/04/06 20:56:11, Ted C wrote:
> I honestly would prefer this class remain and all the logic in CVC could live
in
> here (making this a 1:1 relationship with the native delegate).

Sounds fine. Will do.

[boliu, 2015-04-06 22:47:00]

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
(left):

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
private static void applyBlock(ViewAndroid view) {
On 2015/04/06 21:04:06, boliu wrote:
> On 2015/04/06 20:56:11, Ted C wrote:
> > I honestly would prefer this class remain and all the logic in CVC could
live
> in
> > here (making this a 1:1 relationship with the native delegate).
> 
> Sounds fine. Will do.

Oh, now I remember why I didn't do this..

Current implementation keeps a per-ViewAndroid KeepScreenOn count. So 1:1 with
native PowerSaveBlocker is wrong, and a global count is not correct either.

[boliu, 2015-04-06 22:50:20]

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
(left):

https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
private static void applyBlock(ViewAndroid view) {
On 2015/04/06 22:47:00, boliu wrote:
> On 2015/04/06 21:04:06, boliu wrote:
> > On 2015/04/06 20:56:11, Ted C wrote:
> > > I honestly would prefer this class remain and all the logic in CVC could
> live
> > in
> > > here (making this a 1:1 relationship with the native delegate).
> > 
> > Sounds fine. Will do.
> 
> Oh, now I remember why I didn't do this..
> 
> Current implementation keeps a per-ViewAndroid KeepScreenOn count. So 1:1 with
> native PowerSaveBlocker is wrong, and a global count is not correct either.

Could totally do one anchor view per PowerSaveBlocker (instead the current
behavior of one anchor view per ViewAndroid, and then keep a count). WDYT?

[Ted C, 2015-04-06 22:56:51]

On 2015/04/06 22:50:20, boliu wrote:
>
https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
> (left):
> 
>
https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
> private static void applyBlock(ViewAndroid view) {
> On 2015/04/06 22:47:00, boliu wrote:
> > On 2015/04/06 21:04:06, boliu wrote:
> > > On 2015/04/06 20:56:11, Ted C wrote:
> > > > I honestly would prefer this class remain and all the logic in CVC could
> > live
> > > in
> > > > here (making this a 1:1 relationship with the native delegate).
> > > 
> > > Sounds fine. Will do.
> > 
> > Oh, now I remember why I didn't do this..
> > 
> > Current implementation keeps a per-ViewAndroid KeepScreenOn count. So 1:1
with
> > native PowerSaveBlocker is wrong, and a global count is not correct either.
> 
> Could totally do one anchor view per PowerSaveBlocker (instead the current
> behavior of one anchor view per ViewAndroid, and then keep a count). WDYT?

Do you know what the relationship with PowerSaveBlocker is to webcontents?

Are they shared across multiple?  Or would one web contents have multiple (i.e.
one per frame or something)?

I think having one view per powersaver seems reasonable to me.

[boliu, 2015-04-06 23:02:12]

On 2015/04/06 22:56:51, Ted C wrote:
> On 2015/04/06 22:50:20, boliu wrote:
> >
>
https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
> > (left):
> > 
> >
>
https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
> >
>
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
> > private static void applyBlock(ViewAndroid view) {
> > On 2015/04/06 22:47:00, boliu wrote:
> > > On 2015/04/06 21:04:06, boliu wrote:
> > > > On 2015/04/06 20:56:11, Ted C wrote:
> > > > > I honestly would prefer this class remain and all the logic in CVC
could
> > > live
> > > > in
> > > > > here (making this a 1:1 relationship with the native delegate).
> > > > 
> > > > Sounds fine. Will do.
> > > 
> > > Oh, now I remember why I didn't do this..
> > > 
> > > Current implementation keeps a per-ViewAndroid KeepScreenOn count. So 1:1
> with
> > > native PowerSaveBlocker is wrong, and a global count is not correct
either.
> > 
> > Could totally do one anchor view per PowerSaveBlocker (instead the current
> > behavior of one anchor view per ViewAndroid, and then keep a count). WDYT?
> 
> Do you know what the relationship with PowerSaveBlocker is to webcontents?

In theory, they have no relationship whatsoever. Eg on linux, PowerSaveBlocker
is implemented by talking directly to the x server I think.

In practice...

On android, PowerSaveBlocker influence cannot outlive WebContents since anchor
views are tied to WebContents.

Also just by chance, lifetime of all PowerSaveBlocker objects on android is tied
to WebContents.

> 
> Are they shared across multiple?  Or would one web contents have multiple
(i.e.
> one per frame or something)?

in theory both..

> 
> I think having one view per powersaver seems reasonable to me.

[Ted C, 2015-04-06 23:54:59]

On 2015/04/06 23:02:12, boliu wrote:
> On 2015/04/06 22:56:51, Ted C wrote:
> > On 2015/04/06 22:50:20, boliu wrote:
> > >
> >
>
https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
> > > File
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
> > > (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1041823004/diff/20001/content/public/android/...
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:12:
> > > private static void applyBlock(ViewAndroid view) {
> > > On 2015/04/06 22:47:00, boliu wrote:
> > > > On 2015/04/06 21:04:06, boliu wrote:
> > > > > On 2015/04/06 20:56:11, Ted C wrote:
> > > > > > I honestly would prefer this class remain and all the logic in CVC
> could
> > > > live
> > > > > in
> > > > > > here (making this a 1:1 relationship with the native delegate).
> > > > > 
> > > > > Sounds fine. Will do.
> > > > 
> > > > Oh, now I remember why I didn't do this..
> > > > 
> > > > Current implementation keeps a per-ViewAndroid KeepScreenOn count. So
1:1
> > with
> > > > native PowerSaveBlocker is wrong, and a global count is not correct
> either.
> > > 
> > > Could totally do one anchor view per PowerSaveBlocker (instead the current
> > > behavior of one anchor view per ViewAndroid, and then keep a count). WDYT?
> > 
> > Do you know what the relationship with PowerSaveBlocker is to webcontents?
> 
> In theory, they have no relationship whatsoever. Eg on linux, PowerSaveBlocker
> is implemented by talking directly to the x server I think.
> 
> In practice...
> 
> On android, PowerSaveBlocker influence cannot outlive WebContents since anchor
> views are tied to WebContents.
> 
> Also just by chance, lifetime of all PowerSaveBlocker objects on android is
tied
> to WebContents.
> 
> > 
> > Are they shared across multiple?  Or would one web contents have multiple
> (i.e.
> > one per frame or something)?
> 
> in theory both..

If the powersaveblocker is shared across webcontents, then you could add the
view
to a webcontents that gets deleted but have the blocker live on with no
associated
view.

That seems scary.  Unless we just add checks that it is in fact one per web
contents
for evers.



> 
> > 
> > I think having one view per powersaver seems reasonable to me.

[boliu, 2015-04-07 00:22:14]

On 2015/04/06 23:54:59, Ted C wrote:
> If the powersaveblocker is shared across webcontents, then you could add the
> view
> to a webcontents that gets deleted but have the blocker live on with no
> associated
> view.
> 
> That seems scary.  Unless we just add checks that it is in fact one per web
> contents
> for evers.

I think that's a losing proposition.

Some history... Originally android was implemeneted at a "global" level too by
setting KEEP_SCREEN_ON on the window. But that was a problem for webview,
because the embedding app could stomp that flag too.
(https://chromiumcodereview.appspot.com/20125004 I really don't like how that
patch left things half broken.)

I don't know how to solve this. Could iterate through all the WebContents, and
attach a view to each of them...

Anyway, I'll keep the existing behavior in this patch.

[boliu, 2015-04-07 19:11:58]

new patch set up, ptal

[Ted C, 2015-04-07 19:53:29]

I definitely like this approach more.

https://codereview.chromium.org/1041823004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
(right):

https://codereview.chromium.org/1041823004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:29:
assert mKeepScreenOnView == null;
It used to use a counter for incrementing and decrementing (only adding the view
when necessary).  Do you know if that is no longer needed/never needed in the
first place?

[boliu, 2015-04-07 20:03:19]

https://codereview.chromium.org/1041823004/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java
(right):

https://codereview.chromium.org/1041823004/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/PowerSaveBlocker.java:29:
assert mKeepScreenOnView == null;
On 2015/04/07 19:53:29, Ted C wrote:
> It used to use a counter for incrementing and decrementing (only adding the
view
> when necessary).  Do you know if that is no longer needed/never needed in the
> first place?

Because there used to be one anchor view per CVC, so it was counting the number
of PowerSaveBlockers using that anchor view.

Now there's one anchor view per PowerSaveBlocker, so no need for the count
anymore.

[Ted C, 2015-04-08 19:49:40]

lgtm

[boliu, 2015-04-08 20:16:54]

Daniel, still need your approval for stuff under content/browser :)

[no sievers, 2015-04-08 23:12:22]

lgtm

[boliu, 2015-04-08 23:16:47]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-04-08 23:18:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1041823004/40001

[commit-bot: I haz the power, 2015-04-09 03:59:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-09 04:00:00]

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