Do not register onLayoutListener unless it is needed

Do not register onLayoutListener unless it is needed

isKeyboardShowing calls getWindowVisibleDisplayFrame which may be very
expensive sine it involves ipc to other processes. Also layout happens a
lot more often in webview than chrome, so this disproportionately
affects webview.

BUG=487487
Committed: https://crrev.com/1b64b5edfacaa494b66db59631197d850cec60ae
Cr-Commit-Position: refs/heads/master@{#329973}

[jdduke (slow), 2015-05-14 21:42:03]

jdduke@chromium.org changed reviewers:
+ jdduke@chromium.org

[jdduke (slow), 2015-05-14 21:42:03]

https://codereview.chromium.org/1140163004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1140163004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:70:
activity.findViewById(android.R.id.content).addOnLayoutChangeListener(this);
Don't you want to remove the listener?

[jdduke (slow), 2015-05-14 21:43:23]

jdduke@chromium.org changed reviewers:
+ estade@chromium.org

[jdduke (slow), 2015-05-14 21:43:23]

estade@: Is there a reason we're sticking all of this keyboard visibility code
in WindowAndroid? If it's only used for Autofill, I'd much prefer we find a
separate place to host the code.

[boliu, 2015-05-14 21:45:12]

Internal b/21080709 has more context

https://codereview.chromium.org/1140163004/diff/1/ui/android/java/src/org/chr...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1140163004/diff/1/ui/android/java/src/org/chr...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:70:
activity.findViewById(android.R.id.content).addOnLayoutChangeListener(this);
On 2015/05/14 21:42:03, jdduke wrote:
> Don't you want to remove the listener?

Ooops. Fixed

[Evan Stade, 2015-05-14 21:48:02]

estade@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Evan Stade, 2015-05-14 21:48:03]

> estade@: Is there a reason we're sticking all of this keyboard visibility code
> in WindowAndroid? If it's only used for Autofill, I'd much prefer we find a
> separate place to host the code.

I was heavily guided by dtrainor. He can speak better than me as to the best
place for the code to live.

[jdduke (slow), 2015-05-14 21:51:55]

lgtm, but eventually I'd like to either move all the keyboard code into
ActivityWindowAndroid or find another place for it.

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:63:
activity.findViewById(android.R.id.content).addOnLayoutChangeListener(this);
Can this fail on WebView? I guess we shouldn't be calling it on WebView so it's
a natural failure point.

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:408: protected void
registerKeyboardVisibilityCallbacks() {
Should we be failing here, or just asserting false or something like that?
Adding keyboard listeners for anything but ActivityWindowAndroid doesn't make
any sense.

[David Trainor- moved to gerrit, 2015-05-14 22:03:10]

On 2015/05/14 21:48:03, Evan Stade wrote:
> > estade@: Is there a reason we're sticking all of this keyboard visibility
code
> > in WindowAndroid? If it's only used for Autofill, I'd much prefer we find a
> > separate place to host the code.
> 
> I was heavily guided by dtrainor. He can speak better than me as to the best
> place for the code to live.

lgtm

@jdduke: I think this is the right place for hooking into the Activity and top
level Views from ui/ and other lower layers (at least, when we first wrote
WindowAndroid that was the goal, but the class has been co-opted by a lot of
other pieces of code over the past few years...).  I did discuss this with other
Chrome for Android UI folks when I originally suggested this.  If you think it
should go somewhere else we can discuss it though.  If it ends up needing to
move, I can handle moving it because it isn't Evan's fault that we're talking
about changing the location.

@boliu: Nice catch on the failure to remove.  My bad, sorry I missed that in the
review.  I didn't know WebView used ActivityWindowAndroid (when WindowAndroid
was first written we explicitly split out ActivityWindowAndroid and WebView
always used the WindowAndroid base class).  The more you know :).

[David Trainor- moved to gerrit, 2015-05-14 22:05:04]

On 2015/05/14 22:03:10, David Trainor wrote:
> On 2015/05/14 21:48:03, Evan Stade wrote:
> > > estade@: Is there a reason we're sticking all of this keyboard visibility
> code
> > > in WindowAndroid? If it's only used for Autofill, I'd much prefer we find
a
> > > separate place to host the code.
> > 
> > I was heavily guided by dtrainor. He can speak better than me as to the best
> > place for the code to live.
> 
> lgtm
> 
> @jdduke: I think this is the right place for hooking into the Activity and top
> level Views from ui/ and other lower layers (at least, when we first wrote
> WindowAndroid that was the goal, but the class has been co-opted by a lot of
> other pieces of code over the past few years...).  I did discuss this with
other
> Chrome for Android UI folks when I originally suggested this.  If you think it
> should go somewhere else we can discuss it though.  If it ends up needing to
> move, I can handle moving it because it isn't Evan's fault that we're talking
> about changing the location.

Ah just saw your message about moving it all into ActivityWindowAndroid.  I
would be fine with that as well :).

> 
> @boliu: Nice catch on the failure to remove.  My bad, sorry I missed that in
the
> review.  I didn't know WebView used ActivityWindowAndroid (when WindowAndroid
> was first written we explicitly split out ActivityWindowAndroid and WebView
> always used the WindowAndroid base class).  The more you know :).

[jdduke (slow), 2015-05-14 22:08:23]

On 2015/05/14 22:05:04, David Trainor wrote:
> On 2015/05/14 22:03:10, David Trainor wrote:
> > @jdduke: I think this is the right place for hooking into the Activity and
top
> > level Views from ui/ and other lower layers (at least, when we first wrote
> > WindowAndroid that was the goal, but the class has been co-opted by a lot of
> > other pieces of code over the past few years...).  I did discuss this with
> other
> > Chrome for Android UI folks when I originally suggested this.  If you think
it
> > should go somewhere else we can discuss it though.  If it ends up needing to
> > move, I can handle moving it because it isn't Evan's fault that we're
talking
> > about changing the location.
> 
> Ah just saw your message about moving it all into ActivityWindowAndroid.  I
> would be fine with that as well :).

Yeah, I probably should have dug through the review history to get the full
context. ActivityWindowAndroid it is!

[boliu, 2015-05-14 23:05:29]

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java
(right):

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:63:
activity.findViewById(android.R.id.content).addOnLayoutChangeListener(this);
On 2015/05/14 21:51:54, jdduke wrote:
> Can this fail on WebView? I guess we shouldn't be calling it on WebView so
it's
> a natural failure point.

Hmm? What do you mean? These were running on webview, and working. I think the
content UI is standard on android, it's like the "root view".

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:408: protected void
registerKeyboardVisibilityCallbacks() {
On 2015/05/14 21:51:54, jdduke wrote:
> Should we be failing here, or just asserting false or something like that?
> Adding keyboard listeners for anything but ActivityWindowAndroid doesn't make
> any sense.

I don't want to add random asserts that I don't actually is holds or not, this
close to the branch point..

[jdduke (slow), 2015-05-14 23:09:36]

still lgtm

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:408: protected void
registerKeyboardVisibilityCallbacks() {
On 2015/05/14 23:05:29, boliu wrote:
> On 2015/05/14 21:51:54, jdduke wrote:
> > Should we be failing here, or just asserting false or something like that?
> > Adding keyboard listeners for anything but ActivityWindowAndroid doesn't
make
> > any sense.
> 
> I don't want to add random asserts that I don't actually is holds or not, this
> close to the branch point..

How is it random? In what world should we be registering listeners when it turns
out that the listener won't ever be honored?

[boliu, 2015-05-14 23:12:47]

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1140163004/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:408: protected void
registerKeyboardVisibilityCallbacks() {
On 2015/05/14 23:09:36, jdduke wrote:
> On 2015/05/14 23:05:29, boliu wrote:
> > On 2015/05/14 21:51:54, jdduke wrote:
> > > Should we be failing here, or just asserting false or something like that?
> > > Adding keyboard listeners for anything but ActivityWindowAndroid doesn't
> make
> > > any sense.
> > 
> > I don't want to add random asserts that I don't actually is holds or not,
this
> > close to the branch point..
> 
> How is it random? In what world should we be registering listeners when it
turns
> out that the listener won't ever be honored?

Random as in it's not part of this fix (and I don't want to do the work to
verify the assert holds :p ).

Fix is safer if it doesn't add new assumptions.

[boliu, 2015-05-14 23:18:23]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-05-14 23:19:33]

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

[commit-bot: I haz the power, 2015-05-14 23:26:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-14 23:28:25]

Patchset 2 (id:??) landed as
https://crrev.com/1b64b5edfacaa494b66db59631197d850cec60ae
Cr-Commit-Position: refs/heads/master@{#329973}