[Chromoting] Add InputInjector and InputInjectorWrapper for easy unittesting

[Chromoting] Add InputStub and InputEventSender for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputStub, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputStub to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputEventSender, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
Committed: https://crrev.com/1a5aae1dd974c23bb6e990cccaa84eb648a02955
Cr-Commit-Position: refs/heads/master@{#401681}

[Hzj_jie, 2016-06-15 23:04:23]

Patchset #4 (id:60001) has been deleted

[Hzj_jie, 2016-06-16 00:22:27]

Description was changed from

==========
[Chromoting] Add InputInjector and InputInjectorWrapper for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputInjector, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputInjector to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputInjectorWrapper, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[Chromoting] Add InputInjector and InputInjectorWrapper for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputInjector, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputInjector to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputInjectorWrapper, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

[Hzj_jie, 2016-06-16 00:22:27]

zijiehe@chromium.org changed reviewers:
+ joedow@chromium.org, lambroslambrou@chromium.org

[Lambros, 2016-06-17 23:00:11]

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:77: private
Set<Integer> mPressedTextKeys = new TreeSet<Integer>();
Remove this, it's not used.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/InputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjector.java:19: /**
Sends a mouse wheel event. */
Blank lines before comments please (here and elsewhere).
Don't need blank line before "Sends a mouse event." because
that's the first comment at the top of a block. But please
use blank lines elsewhere.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:21:
* class is not thread-safe.
I don't think it's worth documenting that a class is "not thread-safe"?
Keep it if you think it's useful, but generally in Chromium we try to
write classes that are only used on a single thread. The default
assumption is non-thread-safe.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:31:
/** Set of pressed keys for which we've sent TextEvent. */
Blank line before comment.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:41:
Preconditions.isTrue(button == TouchInputHandler.BUTTON_UNDEFINED
You could consider adding a button enum for this?
Doesn't have to be in this CL.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:42:
|| button == TouchInputHandler.BUTTON_LEFT
Line-continuation in Java is +8 spaces
from previous line.
'git cl format' will probably fix this.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:130:
public boolean sendKeyEvent(KeyEvent event) {
Add some JavaDoc for this. Maybe:
Converts the KeyEvent into lower-level events and sends them
to the host as either key-events or text-events.
This contains some logic for handling some special keys, and
avoids sending a key-up event for a key that was previously
injected as a text-event.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:204:
public void sendKeysPress(int[] keyCodes) {
Maybe add JavaDoc for this?
This is intended for sending key-combinations
such as Ctrl-Alt-Del. This injects a key-down
event for every key in the list, and then
injects the corresponding key-up events.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File
remoting/android/java/src/org/chromium/chromoting/SimulatedTouchInputStrategy.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/SimulatedTouchInputStrategy.java:51:
InputInjectorWrapper injector,
Wrong indentation (should be +8 spaces from previous line). git cl format

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:273:
InputInjectorWrapper injector) {
Wrong indentation.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
File
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:27:
assertTrue(false);
fail() instead of assertTrue(false) ?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:110:
assertTrue(false);
fail() ?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:112:
assertTrue(mText.equals(other.mText));
assertEquals() ?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:123:
public static final int INVALID_ID;
I'm not sure why you would need this? Why would you ever want an
invalid ID in a TouchEvent?
Are you saying that the ID is an optional field of TouchEvent?
And how is the comparison ignored, exactly?
Are you saying if one TouchEvent has an invalid ID, it will
compare unequal to a TouchEvent with a valid ID?
Why would you need a special value for that? Wouldn't a
plain == test do the same thing?
This scheme is very unclear to me.
Looking at the implementation below, it seems that two
TouchEvents with the same INVALID_ID number would
compare not-equal with each other. Why is this useful?
This also means a TouchEvent with INVALID_ID will
compare equal with itself, but not compare equal with
a copy of itself having the exact same fields!!!

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:124:
/**
Blank lines above comments.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:140:
INVALID_ID = -883;
Do we want a static block for this? I think we usually
initialize constants at the declaration.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:153:
|| left == INVALID_ID
Wrong indentation. git cl format

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:164:
// 3PI = 540
Can you clarify this comment? Or just remove it?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:165:
return left == right
This exact compare of floats will probably trigger a FindBugs warning.

If you are doing any arithmetic of floats that might be inexact, you
should fix this test to allow for a small error.

Otherwise, just suppress the warning if you see one.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:166:
|| left > 360
This is very weird.
Firstly: Why not define INVALID_DEGREES like you have done with INVALID_RADIANS?
Secondly: For example, if |left| is 123 and |right| is 999, this test will
report that they match! Is that what you intended?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:197:
if (dataSize() != 0 && other.dataSize() != 0) {
This is incorrect: && should probably be ||
If exactly one of these is 0:
The && will be false and the body will not execute, so this
function will no-op, behaving as if the contents matched, which is
not correct.

You could maybe remove the 'if' completely. For the (0,0)
case, the for-loop will be a no-op.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:258:
public void assertContainsMouseEvents(LinkedList<MouseEvent> other) {
Does this need to consume |other|?
Does this need to be a LinkedList?
Maybe you could simplify the call-sites by allowing
|other| to be any iterable list of MouseEvent (ideally, an
array could be passed in directly)?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:270:
/** Asserts current instance is emtpy. */
empty

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:332:
events.add((new TouchEventBuilder())
I don't think you need parentheses around new TouchEventBuilder() ?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:339:
// These tests send a single event for each finger that is moved.  E.G. If we
are injecting
Blank line.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:390:
// Note: This implementation is not consistent with jni.Client, which may return
false when
Blank line.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
File
remoting/android/javatests/src/org/chromium/chromoting/TouchEventBuilder.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/TouchEventBuilder.java:11:
/** A helper class to build a {@link TouchEvent}. */
{@link MockInputInjector.TouchEvent}

This builder is a top-level class, which is perhaps
another reason for making TouchEvent a top-level
class as well?

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/TouchEventBuilder.java:15:
// Following fields are the of the pending TouchEventData. They will be added to
{@link #data}
Blank line.

[Hzj_jie, 2016-06-19 23:41:40]

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:77: private
Set<Integer> mPressedTextKeys = new TreeSet<Integer>();
On 2016/06/17 23:00:09, Lambros wrote:
> Remove this, it's not used.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/InputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjector.java:19: /**
Sends a mouse wheel event. */
On 2016/06/17 23:00:09, Lambros wrote:
> Blank lines before comments please (here and elsewhere).
> Don't need blank line before "Sends a mouse event." because
> that's the first comment at the top of a block. But please
> use blank lines elsewhere.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:21:
* class is not thread-safe.
On 2016/06/17 23:00:10, Lambros wrote:
> I don't think it's worth documenting that a class is "not thread-safe"?
> Keep it if you think it's useful, but generally in Chromium we try to
> write classes that are only used on a single thread. The default
> assumption is non-thread-safe.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:31:
/** Set of pressed keys for which we've sent TextEvent. */
On 2016/06/17 23:00:09, Lambros wrote:
> Blank line before comment.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:41:
Preconditions.isTrue(button == TouchInputHandler.BUTTON_UNDEFINED
On 2016/06/17 23:00:09, Lambros wrote:
> You could consider adding a button enum for this?
> Doesn't have to be in this CL.

I thought we still discourage from using enum in Android code.
But yes, changing from integer to enum looks pretty huge, we need to change
InputStrategyInterface with all its implementation. I will do it in a different
change.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:42:
|| button == TouchInputHandler.BUTTON_LEFT
On 2016/06/17 23:00:09, Lambros wrote:
> Line-continuation in Java is +8 spaces
> from previous line.
> 'git cl format' will probably fix this.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:130:
public boolean sendKeyEvent(KeyEvent event) {
On 2016/06/17 23:00:10, Lambros wrote:
> Add some JavaDoc for this. Maybe:
> Converts the KeyEvent into lower-level events and sends them
> to the host as either key-events or text-events.
> This contains some logic for handling some special keys, and
> avoids sending a key-up event for a key that was previously
> injected as a text-event.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:204:
public void sendKeysPress(int[] keyCodes) {
On 2016/06/17 23:00:09, Lambros wrote:
> Maybe add JavaDoc for this?
> This is intended for sending key-combinations
> such as Ctrl-Alt-Del. This injects a key-down
> event for every key in the list, and then
> injects the corresponding key-up events.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File
remoting/android/java/src/org/chromium/chromoting/SimulatedTouchInputStrategy.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/SimulatedTouchInputStrategy.java:51:
InputInjectorWrapper injector,
On 2016/06/17 23:00:10, Lambros wrote:
> Wrong indentation (should be +8 spaces from previous line). git cl format

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:273:
InputInjectorWrapper injector) {
On 2016/06/17 23:00:10, Lambros wrote:
> Wrong indentation.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
File
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:27:
assertTrue(false);
On 2016/06/17 23:00:10, Lambros wrote:
> fail() instead of assertTrue(false) ?

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:110:
assertTrue(false);
On 2016/06/17 23:00:10, Lambros wrote:
> fail() ?

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:112:
assertTrue(mText.equals(other.mText));
On 2016/06/17 23:00:10, Lambros wrote:
> assertEquals() ?

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:123:
public static final int INVALID_ID;
On 2016/06/17 23:00:10, Lambros wrote:
> I'm not sure why you would need this? Why would you ever want an
> invalid ID in a TouchEvent?
> Are you saying that the ID is an optional field of TouchEvent?
> And how is the comparison ignored, exactly?
> Are you saying if one TouchEvent has an invalid ID, it will
> compare unequal to a TouchEvent with a valid ID?
> Why would you need a special value for that? Wouldn't a
> plain == test do the same thing?
> This scheme is very unclear to me.
> Looking at the implementation below, it seems that two
> TouchEvents with the same INVALID_ID number would
> compare not-equal with each other. Why is this useful?
> This also means a TouchEvent with INVALID_ID will
> compare equal with itself, but not compare equal with
> a copy of itself having the exact same fields!!!

In current TouchInputStrategyTest, there is no common comparison method for each
test case. So when I was implementing this common comparison method, I needed to
take care of different comparing requirements for a TouchEvent / TouchEventData.
i.e. For some test cases, I may only care about whether a touch event has been
created at a certain place. But for some test cases -- such as tracking a move
gesture of one finger -- I may care about the TouchEventData.getTouchPointId().
So I added a TouchEventBuilder to help generate a TouchEvent. If a certain field
of one TouchEventData is interested, we can write builder.with{field name} to
set it. Otherwise a default invalid data will be added into the final
TouchEventData. And when comparing two TouchEventDatas, if one of them has the
exact invalid data, it means we are not interesting to its value, and should not
impact the final comparison result. This logic is in idMatch / positionMatch /
degreeMatch functions.

And I believe two TouchEventDatas with the exact same fields will always be the
same in current logic.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:124:
/**
On 2016/06/17 23:00:10, Lambros wrote:
> Blank lines above comments.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:140:
INVALID_ID = -883;
On 2016/06/17 23:00:10, Lambros wrote:
> Do we want a static block for this? I think we usually
> initialize constants at the declaration.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:153:
|| left == INVALID_ID
On 2016/06/17 23:00:10, Lambros wrote:
> Wrong indentation. git cl format

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:164:
// 3PI = 540
On 2016/06/17 23:00:10, Lambros wrote:
> Can you clarify this comment? Or just remove it?

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:165:
return left == right
On 2016/06/17 23:00:10, Lambros wrote:
> This exact compare of floats will probably trigger a FindBugs warning.
> 
> If you are doing any arithmetic of floats that might be inexact, you
> should fix this test to allow for a small error.
> 
> Otherwise, just suppress the warning if you see one.

Indeed the floats should be exact equals, there should be no small errors. Since
all the numbers are coming from test cases. We do not have any random generated
floats.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:166:
|| left > 360
On 2016/06/17 23:00:10, Lambros wrote:
> This is very weird.
> Firstly: Why not define INVALID_DEGREES like you have done with
INVALID_RADIANS?
> Secondly: For example, if |left| is 123 and |right| is 999, this test will
> report that they match! Is that what you intended?
> 

Good point.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:197:
if (dataSize() != 0 && other.dataSize() != 0) {
On 2016/06/17 23:00:10, Lambros wrote:
> This is incorrect: && should probably be ||
> If exactly one of these is 0:
> The && will be false and the body will not execute, so this
> function will no-op, behaving as if the contents matched, which is
> not correct.
> 
> You could maybe remove the 'if' completely. For the (0,0)
> case, the for-loop will be a no-op.

I may explain more about this in the comments. This is by-design to cover
TouchInputStrategyTest.assertTouchEventInjected function. i.e. A test case may
only care about whether a TouchEvent with a certain TouchEventData.EventType has
been injected, but does not really care about the detail TouchEventData.
Considering sendTouchEvent with an empty TouchEventData array is totally wrong,
an empty TouchEventData array in TouchEvent is also an indicator to ignore this
field. I have added an assertion in sendTouchEvent to make sure no production
logic could eventually do this kind of thing. I have also added similar logic
for each INVALID_{field} above.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:258:
public void assertContainsMouseEvents(LinkedList<MouseEvent> other) {
On 2016/06/17 23:00:10, Lambros wrote:
> Does this need to consume |other|?
> Does this need to be a LinkedList?
> Maybe you could simplify the call-sites by allowing
> |other| to be any iterable list of MouseEvent (ideally, an
> array could be passed in directly)?

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:270:
/** Asserts current instance is emtpy. */
On 2016/06/17 23:00:10, Lambros wrote:
> empty

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:332:
events.add((new TouchEventBuilder())
On 2016/06/17 23:00:10, Lambros wrote:
> I don't think you need parentheses around new TouchEventBuilder() ?

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:339:
// These tests send a single event for each finger that is moved.  E.G. If we
are injecting
On 2016/06/17 23:00:10, Lambros wrote:
> Blank line.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:390:
// Note: This implementation is not consistent with jni.Client, which may return
false when
On 2016/06/17 23:00:10, Lambros wrote:
> Blank line.

Done.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
File
remoting/android/javatests/src/org/chromium/chromoting/TouchEventBuilder.java
(right):

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/TouchEventBuilder.java:11:
/** A helper class to build a {@link TouchEvent}. */
On 2016/06/17 23:00:10, Lambros wrote:
> {@link MockInputInjector.TouchEvent}
> 
> This builder is a top-level class, which is perhaps
> another reason for making TouchEvent a top-level
> class as well?

This is also a headache to me. TouchEvent equals to MouseEvent / WheelEvent /
KeyEvent and TextEvent, so it looks weird if we move only TouchEvent into a
top-level class. But other events are also too trivial to deserve a top-level
class.

https://codereview.chromium.org/2066683003/diff/80001/remoting/android/javate...
remoting/android/javatests/src/org/chromium/chromoting/TouchEventBuilder.java:15:
// Following fields are the of the pending TouchEventData. They will be added to
{@link #data}
On 2016/06/17 23:00:11, Lambros wrote:
> Blank line.

Done.

[Lambros, 2016-06-22 01:43:15]

lgtm, just some tiny nits.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:63:
// TODO (zijiehe): This function will be eventually removed after {@link
InputStrategyInterface}
No space between TODO and (.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:212:
if (keyCodes != null && keyCodes.length > 0) {
I don't think you need a null check, and the length check is not needed either.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
File
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:188:
// An empty TouchEventData array indicates mData field is not interested.
Spell it out more clearly. Perhaps:
An event with an empty mData array will
match an event with a non-empty mData array.
This is useful for tests which expect a TouchEvent, but
don't care about the content of mData.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:214:
* consumes these events from both |left| and |right|.
This no longer consumes from |right|.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:224:
assertTrue(left.size() >= right.length);
JUnit has no assertGreaterThan() method :(
Instead, use assertTrue(String, boolean) to log a message that
[left] was smaller than [right]
(reporting the sizes in place of [left] and [right]).

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:253:
* asserts they are equal, and consumes these events in both this instance and
|other|.
Doesn't consume |other|.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:326:
// These tests send a single event for each finger that is moved.  E.G. If we
are injecting
s/E.G. If/E.g. if/

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:388:
@org.chromium.base.annotations.SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY")
import org.chromium.base.annotations.SuppressFBWarnings
and just use SuppressFBWarnings here.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:391:
assertFalse(data == null || data.length == 0);
This would be much more readable as:

assertNotNull(data);
assertTrue(data.length != 0);

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:393:
assertFalse(data[i].getTouchPointId() == TouchEvent.INVALID_ID);
I think I prefer assertTrue(foo != INVALID) over assertFalse(foo == INVALID),
because positive checks are more readable than negative checks.
But feel free to disagree.

[Sergey Ulanov, 2016-06-22 06:39:01]

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

[Sergey Ulanov, 2016-06-22 06:39:02]

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjector.java:16: public
interface InputInjector {
This name is confusing because this interface doesn't really inject's anything.
We call InputInjector an object on the host side that injects input events to
the OS. Here InputInjector doesn't really inject anything. Maybe call it
InputStub, which is what we call similar interface in C++? It's not the best
name, but at least it would be consistent. Or if you have another idea for a
better name please use it here.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:22:
public final class InputInjectorWrapper {
I don't think this is the best name for this class. I would normally expect a
wrapper to implement the same interface as the one it wraps, which is not the
case here. Maybe call it InputEventSender or something like that?

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:23: //
These constants must match those in the generated struct
protocol::MouseEvent_MouseButton.
Maybe make these be part of the InputInjector interface?

[Hzj_jie, 2016-06-22 23:18:55]

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjector.java:16: public
interface InputInjector {
On 2016/06/22 06:39:01, Sergey Ulanov wrote:
> This name is confusing because this interface doesn't really inject's
anything.
> We call InputInjector an object on the host side that injects input events to
> the OS. Here InputInjector doesn't really inject anything. Maybe call it
> InputStub, which is what we call similar interface in C++? It's not the best
> name, but at least it would be consistent. Or if you have another idea for a
> better name please use it here.

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:22:
public final class InputInjectorWrapper {
On 2016/06/22 06:39:01, Sergey Ulanov wrote:
> I don't think this is the best name for this class. I would normally expect a
> wrapper to implement the same interface as the one it wraps, which is not the
> case here. Maybe call it InputEventSender or something like that?

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:63:
// TODO (zijiehe): This function will be eventually removed after {@link
InputStrategyInterface}
On 2016/06/22 01:43:14, Lambros wrote:
> No space between TODO and (.

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:212:
if (keyCodes != null && keyCodes.length > 0) {
On 2016/06/22 01:43:14, Lambros wrote:
> I don't think you need a null check, and the length check is not needed
either.

I believe Java allows an array to be null. And for each through a null array
will result a NullPointerException. So removing the null check equals to an
assert keyCodes != null. I agree we should not have this case in our code path,
but this check can make it safer. A no-op should be better than a crash error,
right?

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:23: //
These constants must match those in the generated struct
protocol::MouseEvent_MouseButton.
On 2016/06/22 06:39:02, Sergey Ulanov wrote:
> Maybe make these be part of the InputInjector interface?

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
File
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:188:
// An empty TouchEventData array indicates mData field is not interested.
On 2016/06/22 01:43:14, Lambros wrote:
> Spell it out more clearly. Perhaps:
> An event with an empty mData array will
> match an event with a non-empty mData array.
> This is useful for tests which expect a TouchEvent, but
> don't care about the content of mData.

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:214:
* consumes these events from both |left| and |right|.
On 2016/06/22 01:43:15, Lambros wrote:
> This no longer consumes from |right|.

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:224:
assertTrue(left.size() >= right.length);
On 2016/06/22 01:43:15, Lambros wrote:
> JUnit has no assertGreaterThan() method :(
> Instead, use assertTrue(String, boolean) to log a message that
> [left] was smaller than [right]
> (reporting the sizes in place of [left] and [right]).

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:253:
* asserts they are equal, and consumes these events in both this instance and
|other|.
On 2016/06/22 01:43:14, Lambros wrote:
> Doesn't consume |other|.

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:326:
// These tests send a single event for each finger that is moved.  E.G. If we
are injecting
On 2016/06/22 01:43:14, Lambros wrote:
> s/E.G. If/E.g. if/

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:388:
@org.chromium.base.annotations.SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY")
On 2016/06/22 01:43:14, Lambros wrote:
> import org.chromium.base.annotations.SuppressFBWarnings
> and just use SuppressFBWarnings here.

I have not seen any comments about using import or using full package qualified
name in the coding style. Do we always prefer to use import?

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:391:
assertFalse(data == null || data.length == 0);
On 2016/06/22 01:43:14, Lambros wrote:
> This would be much more readable as:
> 
> assertNotNull(data);
> assertTrue(data.length != 0);

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:393:
assertFalse(data[i].getTouchPointId() == TouchEvent.INVALID_ID);
On 2016/06/22 01:43:15, Lambros wrote:
> I think I prefer assertTrue(foo != INVALID) over assertFalse(foo == INVALID),
> because positive checks are more readable than negative checks.
> But feel free to disagree.

Done. :)

[Hzj_jie, 2016-06-23 00:57:30]

Description was changed from

==========
[Chromoting] Add InputInjector and InputInjectorWrapper for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputInjector, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputInjector to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputInjectorWrapper, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[Chromoting] Add InputStub and InputEventSender for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputStub, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputStub to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputEventSender, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

[Lambros, 2016-06-23 01:40:09]

Still lgtm. Please update the CL description with the new names of the classes.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:212:
if (keyCodes != null && keyCodes.length > 0) {
On 2016/06/22 23:18:54, Hzj_jie wrote:
> On 2016/06/22 01:43:14, Lambros wrote:
> > I don't think you need a null check, and the length check is not needed
> either.
> 
> I believe Java allows an array to be null. And for each through a null array
> will result a NullPointerException. So removing the null check equals to an
> assert keyCodes != null. I agree we should not have this case in our code
path,
> but this check can make it safer. A no-op should be better than a crash error,
> right?

Arguably, it can be better to crash than to silently ignore mistakes.
If the application crashes, we can become aware of the programmer
error and we can fix the problem.

I don't feel strongly - it's fine to keep or remove the check.
If you keep the null-check, maybe update JavaDoc to say that the
parameter can be null and nothing will be injected?

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
File
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:388:
@org.chromium.base.annotations.SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY")
On 2016/06/22 23:18:55, Hzj_jie wrote:
> On 2016/06/22 01:43:14, Lambros wrote:
> > import org.chromium.base.annotations.SuppressFBWarnings
> > and just use SuppressFBWarnings here.
> 
> I have not seen any comments about using import or using full package
qualified
> name in the coding style. Do we always prefer to use import?

Do you see any other examples in Chromium? A code-search for
"@org.chromium" finds nothing. I think we prefer import.

[Hzj_jie, 2016-06-23 18:03:24]

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/InputInjectorWrapper.java:212:
if (keyCodes != null && keyCodes.length > 0) {
On 2016/06/23 01:40:09, Lambros wrote:
> On 2016/06/22 23:18:54, Hzj_jie wrote:
> > On 2016/06/22 01:43:14, Lambros wrote:
> > > I don't think you need a null check, and the length check is not needed
> > either.
> > 
> > I believe Java allows an array to be null. And for each through a null array
> > will result a NullPointerException. So removing the null check equals to an
> > assert keyCodes != null. I agree we should not have this case in our code
> path,
> > but this check can make it safer. A no-op should be better than a crash
error,
> > right?
> 
> Arguably, it can be better to crash than to silently ignore mistakes.
> If the application crashes, we can become aware of the programmer
> error and we can fix the problem.
> 
> I don't feel strongly - it's fine to keep or remove the check.
> If you keep the null-check, maybe update JavaDoc to say that the
> parameter can be null and nothing will be injected?

Done.

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
File
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java
(right):

https://codereview.chromium.org/2066683003/diff/120001/remoting/android/javat...
remoting/android/javatests/src/org/chromium/chromoting/MockInputInjector.java:388:
@org.chromium.base.annotations.SuppressFBWarnings("FE_FLOATING_POINT_EQUALITY")
On 2016/06/23 01:40:09, Lambros wrote:
> On 2016/06/22 23:18:55, Hzj_jie wrote:
> > On 2016/06/22 01:43:14, Lambros wrote:
> > > import org.chromium.base.annotations.SuppressFBWarnings
> > > and just use SuppressFBWarnings here.
> > 
> > I have not seen any comments about using import or using full package
> qualified
> > name in the coding style. Do we always prefer to use import?
> 
> Do you see any other examples in Chromium? A code-search for
> mailto:"@org.chromium" finds nothing. I think we prefer import.
> 

Good point. No, I did not find one.

[Hzj_jie, 2016-06-23 19:23:44]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-06-23 19:23:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2066683003/#ps160001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2016-06-23 19:24:37]

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

[commit-bot: I haz the power, 2016-06-23 19:30:11]

Description was changed from

==========
[Chromoting] Add InputStub and InputEventSender for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputStub, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputStub to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputEventSender, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[Chromoting] Add InputStub and InputEventSender for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputStub, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputStub to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputEventSender, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

[commit-bot: I haz the power, 2016-06-23 19:30:12]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-06-23 19:31:40]

Description was changed from

==========
[Chromoting] Add InputStub and InputEventSender for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputStub, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputStub to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputEventSender, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277
==========

to

==========
[Chromoting] Add InputStub and InputEventSender for easy unittesting

Currently only TouchInputStrategy is testable with RemoteInputInjector. So this
change adds InputStub, which is an interface to represent a set of low level
functions to send users' activities to remote host machine. So one can use a
MockInputStub to log down all the events which should be sent to remote
host, and test the behavior of InputStrategyInterface(s) or other
implementations.
Meanwhile this change also adds an InputEventSender, which provides a set of
shortcut functions to directly send Android events instead of raw JNI types.

This change is part of Android Remote Desktop client refactor, a design doc is
@ https://goo.gl/MA6zjx.

BUG=615277

Committed: https://crrev.com/1a5aae1dd974c23bb6e990cccaa84eb648a02955
Cr-Commit-Position: refs/heads/master@{#401681}
==========

[commit-bot: I haz the power, 2016-06-23 19:31:41]

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