Simplify joystick scrolling code and reenable test.

Simplify joystick scrolling code and reenable test.

I haven't been able to repro the joystick scrolling test flakiness
locally and the bot logs are also cleared up by now.  Strip out some
unnecessary logic (which might help address the cause of flakiness,
although I didn't find a smoking gun) and attempt to reenable.

BUG=538781
Committed: https://crrev.com/a3b6e7915f14028b6a5f62d2852bfe9fd6730783
Cr-Commit-Position: refs/heads/master@{#369259}

[aelias_OOO_until_Jul13, 2016-01-09 03:17:52]

aelias@chromium.org changed reviewers:
+ tdresser@chromium.org

[aelias_OOO_until_Jul13, 2016-01-09 03:18:41]

aelias@chromium.org changed reviewers:
+ sshelke@nvidia.com
- tdresser@chromium.org

[aelias_OOO_until_Jul13, 2016-01-09 03:19:02]

Hi sshelke@, PTAL.

[sshelke, 2016-01-11 12:46:50]

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java
(right):

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:72:
Log.d(TAG, "Joystick event: " + event.getAxisValue(MotionEvent.AXIS_X) + ","
This message can be misleading since we are processing LS Joystick events only.
It would be better if we mention source of joystick event which is LS not
RS/LT/RT.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:96:
if (mLastAnimateTimeMillis == 0) {
Page scrolling should be stopped (or should not be initiated)under following
scenario.
1) Page is scrolling through joystick.
2) User clicks on address bar.
3) ImeAdapter pops up.

if (!mEnabled || !mView.getContainerView().hasFocus()) check is removed then
page will keep scrolling even when ImeAdapter is popped up, which does not look
good.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:104:
mLastAnimateTimeMillis = timeMillis;
assert mLastAnimateTimeMillis > 0

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:108:
private void stop() {
Please refer comment given in animateScroll.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
(right):

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:274:
scrollWithJoystick(0.1f, 1f);
I ran ContentViewScrollingTest#testJoystickScroll individually and it failed on
tegra devices. Though it ran successfully if ran without
@RerunWithUpdatedContainerView annotation. Please check.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:274:
scrollWithJoystick(0.1f, 1f);
I guess, we should also have test to check behavior if Y-axis movement falls in
deadzone.

[aelias_OOO_until_Jul13, 2016-01-13 02:32:22]

aelias@chromium.org changed reviewers:
+ tedchoc@chromium.org

[aelias_OOO_until_Jul13, 2016-01-13 02:32:32]

+tedchoc@ for OWNERS review.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java
(right):

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:72:
Log.d(TAG, "Joystick event: " + event.getAxisValue(MotionEvent.AXIS_X) + ","
On 2016/01/11 at 12:46:50, sshelke wrote:
> This message can be misleading since we are processing LS Joystick events
only. It would be better if we mention source of joystick event which is LS not
RS/LT/RT.

Done.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:96:
if (mLastAnimateTimeMillis == 0) {
On 2016/01/11 at 12:46:50, sshelke wrote:
> Page scrolling should be stopped (or should not be initiated)under following
scenario.
> 1) Page is scrolling through joystick.
> 2) User clicks on address bar.
> 3) ImeAdapter pops up.
> 
> if (!mEnabled || !mView.getContainerView().hasFocus()) check is removed then
page will keep scrolling even when ImeAdapter is popped up, which does not look
good.

OK, thanks for letting me know the scenario that line was for.  I added an
additional call to setEnabled in ContentViewCore.onFocusChanged, which seems
like the cleanest way to address it.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:104:
mLastAnimateTimeMillis = timeMillis;
On 2016/01/11 at 12:46:50, sshelke wrote:
> assert mLastAnimateTimeMillis > 0

That seems redundant now since I added an early-return for that condition in
this same method.

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java
(right):

https://codereview.chromium.org/1571043002/diff/1/content/public/android/java...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:274:
scrollWithJoystick(0.1f, 1f);
On 2016/01/11 at 12:46:50, sshelke wrote:
> I ran ContentViewScrollingTest#testJoystickScroll individually and it failed
on tegra devices. Though it ran successfully if ran without
@RerunWithUpdatedContainerView annotation. Please check.

The same failure happened on the trybot.  I removed the annotation as it's
related to a change of focus to fullscreen video, where it's expected that the
WebView will no longer receive joystick events.  (Maybe this was the cause of
failure causing the test to get disabled.)

> I guess, we should also have test to check behavior if Y-axis movement falls
in deadzone.

Done.

[sshelke, 2016-01-13 08:49:41]

Test runs successfully on tegra devices. lgtm

[Ted C, 2016-01-13 17:29:33]

owners lgtm

https://codereview.chromium.org/1571043002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java
(left):

https://codereview.chromium.org/1571043002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:103:
if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) {
I guess the greater than check isn't needed because AnimationUtils does not use
the wall clock so it "shouldn't" go backwards right?

[aelias_OOO_until_Jul13, 2016-01-13 20:18:22]

https://codereview.chromium.org/1571043002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java
(left):

https://codereview.chromium.org/1571043002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:103:
if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) {
On 2016/01/13 at 17:29:33, Ted C wrote:
> I guess the greater than check isn't needed because AnimationUtils does not
use the wall clock so it "shouldn't" go backwards right?

Right.  Also, if it did go backwards through some arcane scenario I don't know
of, the effect would just be a scroll in the wrong direction, not a crash or
anything.  So I removed it as unnecessary complexity (misleading a reader who
might assume the check is there for a reason).

[aelias_OOO_until_Jul13, 2016-01-13 20:18:30]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2016-01-13 20:19:43]

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

[commit-bot: I haz the power, 2016-01-13 20:25:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-13 20:26:36]

Description was changed from

==========
Simplify joystick scrolling code and reenable test.

I haven't been able to repro the joystick scrolling test flakiness
locally and the bot logs are also cleared up by now.  Strip out some
unnecessary logic (which might help address the cause of flakiness,
although I didn't find a smoking gun) and attempt to reenable.

BUG=538781
==========

to

==========
Simplify joystick scrolling code and reenable test.

I haven't been able to repro the joystick scrolling test flakiness
locally and the bot logs are also cleared up by now.  Strip out some
unnecessary logic (which might help address the cause of flakiness,
although I didn't find a smoking gun) and attempt to reenable.

BUG=538781

Committed: https://crrev.com/a3b6e7915f14028b6a5f62d2852bfe9fd6730783
Cr-Commit-Position: refs/heads/master@{#369259}
==========

[commit-bot: I haz the power, 2016-01-13 20:26:37]

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