Content: Add convenience method for animations over SurfaceViews.

Content: Add convenience method for animations over SurfaceViews.

By default, Android optimizes what it shows on top of
SurfaceViews (saves power). Effectively, layouts determine
what gets drawn and post-layout animations outside of this
area may be 'clipped'.

This adds an convenient way to avoid this clipping. When an
animation wishes to animate over a SurfaceView, it just
needs to register itself in its ContentViewCore and the
rest will be taken care of automatically.

BUG=441805
Committed: https://crrev.com/428058320be722fbfba3a61c81108a27f216b36c
Cr-Commit-Position: refs/heads/master@{#315164}

[epenner, 2015-02-03 01:23:10]

epenner@chromium.org changed reviewers:
+ dtrainor@chromium.org, tedchoc@chromium.org

[epenner, 2015-02-03 01:29:13]

Ptal. This makes it a lot easier than tweaking animation positions by one pixel
etc.

I put the code here as it seemed to be easily accessible from code that would
create animations, but let me know if you think we should move the code, or add
the placeholder view to a different view (rather than the container view).

[epenner, 2015-02-04 23:30:31]

On 2015/02/03 01:29:13, epenner wrote:
> Ptal. This makes it a lot easier than tweaking animation positions by one
pixel
> etc.
> 
> I put the code here as it seemed to be easily accessible from code that would
> create animations, but let me know if you think we should move the code, or
add
> the placeholder view to a different view (rather than the container view).

Polite ping as I think Ted was out on vacation before.

[Ted C, 2015-02-05 00:49:55]

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3011:
private HashSet<Animator> mAnimationsOverContent = new HashSet<Animator>();
put these up with the other class variables.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3012:
private FrameLayout mWillDrawView = null;
= null is the default, so no need for that here.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3016:
if (!animation.isStarted()) throw new IllegalArgumentException("Must be
started.");
any reason why they must be started?

Couldn't the animation listener also check for started?  And tweak the logic a
bit to not rely on the animations size but instead of the nullity of the
mWillDrawView.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3024:
getContainerView().addView(mWillDrawView, new FrameLayout.LayoutParams(
What happens in the case of container not being a FrameLayout (i.e. in the case
of webview).

If we feel we need to support this, we might want to just call addView without
any params.

Then do mWillDrawView.getLayoutParams().width = MATCH_PARENT, height =
MATCH_PARENT (comparing if the view params changed and calling requestLayout()
if they do, although that might not be necessary as I don't know if adding the
view will actually look at the layout params until the next layout pass).

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3035:
if (mAnimationsOverContent.size() == 0) {
isEmtpy() here instead

[David Trainor- moved to gerrit, 2015-02-05 01:16:57]

I'm okay with this (pending Ted's nits).  My only request would be to make sure
we actively try to move things over to use this and send out a PSA.  Should we
do this for things like infobars (already upstream)?

[epennerAtGoogle, 2015-02-05 02:13:34]

epenner@google.com changed reviewers:
+ epenner@google.com

[epennerAtGoogle, 2015-02-05 02:13:35]

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3011:
private HashSet<Animator> mAnimationsOverContent = new HashSet<Animator>();
On 2015/02/05 00:49:54, Ted C wrote:
> put these up with the other class variables.

Oops. Done, thanks.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3012:
private FrameLayout mWillDrawView = null;
On 2015/02/05 00:49:54, Ted C wrote:
> = null is the default, so no need for that here.

Done.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3016:
if (!animation.isStarted()) throw new IllegalArgumentException("Must be
started.");
There's a few reasons I went this route. Firstly, we want to guarantee that we
never get stuck in this state, and that we never leak objects into the set.
Secondly, it's the easy and the best behavior to just always call this
immediately after calling start().

Regarding the first guarantee, if the animation is already started, we can be
the most certain that we will get an onAnimationEnd call eventually, and we also
don't need additional logic or latent error checks (for example if they add an
animation that is already started, or they add one and then never start it at
all).

We could avoid using animations.size(), but I think we will always need at least
an 'active animation count', as that being non-zero is case we care about.

My last thought is that I think the start() call is the best place to add the
placeholder view. I think we want to add it as soon as possible rather than in a
callback on some future frame, for example.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3024:
getContainerView().addView(mWillDrawView, new FrameLayout.LayoutParams(
We should chat on this one as I'm not actually very familiar with Android layout
logic. One thing I can say is that this doesn't need to work on Webview, so as
long as it doesn't break anything by adding this empty view, we are okay.

https://codereview.chromium.org/895843002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3035:
if (mAnimationsOverContent.size() == 0) {
On 2015/02/05 00:49:54, Ted C wrote:
> isEmtpy() here instead

Done.

[epennerAtGoogle, 2015-02-05 02:44:07]

On 2015/02/05 01:16:57, David Trainor wrote:
> I'm okay with this (pending Ted's nits).  My only request would be to make
sure
> we actively try to move things over to use this and send out a PSA.  Should we
> do this for things like infobars (already upstream)?

Sounds good. I'll send out a PSA today so I can incorporate any feedback if
there is any. I've got an upstream patch for the find-bars but I didn't add it
for info bars yet. Planned steps are:
1.) Land this and send PSA.
2.) Call it everywhere it's required upstream.
3.) Remove the setWillNotDraw(false), which actually enables the power savings
again.

[Ted C, 2015-02-05 17:59:05]

lgtm

https://codereview.chromium.org/895843002/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/895843002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3019:
if (!animation.isStarted()) throw new IllegalArgumentException("Must be
started.");
I wonder if we should actually have this method do the starting (i.e. change
this to startAnimationOverContent), change this assert to ensure it actually
hasn't started and start it at the bottom.  Just a thought.

https://codereview.chromium.org/895843002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3026:
getContainerView().addView(mContentWillDrawView, new FrameLayout.LayoutParams(
this will crash on WebView sadly.  In AbsoluteLayout#checkLayoutParams it makes
sure you're passing an AbsoluteLayout.LayoutParams object. 
MATCH_PARENT/MATCH_PARENT is the default for FrameLayout though if you don't
specify layout params, so I think you can just delete this layout definition
entirely.

[David Trainor- moved to gerrit, 2015-02-05 18:52:07]

I'm starting to wonder if this is the right place for this.  ContentViewCore
doesn't have to be actually bound to the view hierarchy.  Should this View be
managed by the actual rendering component
(ContentViewRenderView/CompositorViewHolder?).  I don't know if things have as
easy access to those, but I'm worried that some Animators might just add their
View to a ContentViewCore that isn't actually active.

https://chromiumcodereview.appspot.com/895843002/diff/140001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/895843002/diff/140001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3019:
if (!animation.isStarted()) throw new IllegalArgumentException("Must be
started.");
On 2015/02/05 17:59:05, Ted C wrote:
> I wonder if we should actually have this method do the starting (i.e. change
> this to startAnimationOverContent), change this assert to ensure it actually
> hasn't started and start it at the bottom.  Just a thought.

That might be a good idea.  I'm trying to think of any cases where this would
break (building AnimatorSets, etc.) but I think it would work.

[epenner, 2015-02-05 20:41:21]

New patchsets have been uploaded after l-g-t-m from tedchoc@chromium.org

[epennerAtGoogle, 2015-02-05 21:08:30]

> I'm starting to wonder if this is the right place for
> this.  ContentViewCore doesn't have to be actually bound
> to the view hierarchy.  Should this View be managed by
> the actual rendering component.

Yeah we should sort this out. I think there's two parts to this. One is where we
add the placeholder view, and one is where we track the active animations. If
the container view is always in the view hierarchy, then I think this is at
least 'valid' as is. If there's rare cases where we track the animations in the
wrong place, it won't really matter since we'll still create the placeholder in
the right place.

To be most optimal, it would be nice to track the animations 1-1 with the view
where we add the placeholder. At the moment, this would be some object that is
1-1 with the contentViewCore->containerView. I'll come by to discuss this part.

https://codereview.chromium.org/895843002/diff/140001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/895843002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3019:
if (!animation.isStarted()) throw new IllegalArgumentException("Must be
started.");
On 2015/02/05 18:52:07, David Trainor wrote:
> On 2015/02/05 17:59:05, Ted C wrote:
> > I wonder if we should actually have this method do the starting (i.e. change
> > this to startAnimationOverContent), change this assert to ensure it actually
> > hasn't started and start it at the bottom.  Just a thought.
> 
> That might be a good idea.  I'm trying to think of any cases where this would
> break (building AnimatorSets, etc.) but I think it would work.

I think this is great too, since the plan was for those to always be together.
I'll do it that way and then we can see if anything comes up.

https://codereview.chromium.org/895843002/diff/140001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:3026:
getContainerView().addView(mContentWillDrawView, new FrameLayout.LayoutParams(
On 2015/02/05 17:59:05, Ted C wrote:
> this will crash on WebView sadly.  In AbsoluteLayout#checkLayoutParams it
makes
> sure you're passing an AbsoluteLayout.LayoutParams object. 
> MATCH_PARENT/MATCH_PARENT is the default for FrameLayout though if you don't
> specify layout params, so I think you can just delete this layout definition
> entirely.

Thanks for that. I've changed it to simply add the view without LayoutParams,
with a comment.

[epenner, 2015-02-06 03:12:25]

Okay, after talking to David we decided the best place for this was in
WindowAndroid. Since we create the WindowAndroid and the
ContentViewRenderView/CompositorViewHolder at the same time and the latter has
the SurfaceView, we can also just toggle the SurfaceView's willNotDraw flag
directly and avoid the extra view.

Given all the code has moved around, please take another look.

[David Trainor- moved to gerrit, 2015-02-06 22:16:06]

lgtm

[epenner, 2015-02-06 22:24:51]

New patchsets have been uploaded after l-g-t-m from dtrainor@chromium.org

[epennerAtGoogle, 2015-02-06 22:26:37]

Just heads up for one more review. Mostly a code move to WindowAndroid, but also
some hookup code for each Activity.

[epennerAtGoogle, 2015-02-06 22:28:21]

Ah, got David's now. Going once, going twice...

[epenner, 2015-02-07 01:28:21]

The CQ bit was checked by epenner@chromium.org

[commit-bot: I haz the power, 2015-02-07 01:29:23]

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

[commit-bot: I haz the power, 2015-02-07 02:38:11]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-07 02:38:49]

Patchset 13 (id:??) landed as
https://crrev.com/428058320be722fbfba3a61c81108a27f216b36c
Cr-Commit-Position: refs/heads/master@{#315164}