Advanced Touch API - Down gesture

[berrada, 2011-08-08 20:57:36]

Looks good. Couple minor comments.

On 2011/08/07 20:58:03, juantrujillo wrote:

[berrada, 2011-08-12 23:21:28]

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java
(right):

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:33: * Class
that implements touch capabilities of a device
Sentences should end with a dot.

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:58: private
void scrollElementInView(Coordinates where) {
This isn't used anywhere. You can remove it for now

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
File android/src/java/org/openqa/selenium/android/events/TouchScreen.java
(right):

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/events/TouchScreen.java:54:
Log.d("TouchApi", "MotionEvent => " + me);
Remove.

http://codereview.appspot.com/4853044/diff/1/java/server/src/org/openqa/selen...
File
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java
(right):

http://codereview.appspot.com/4853044/diff/1/java/server/src/org/openqa/selen...
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java:29:
public class Down extends WebElementHandler implements JsonParametersAware {
Maybe call it FingerDown?

http://codereview.appspot.com/4853044/diff/1/java/server/src/org/openqa/selen...
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java:31:
private static final String X = "x";
Those constants aren't needed.

[juantrujillo, 2011-08-12 23:47:35]

Hi,

I have submitted a new patch addressing the comments and replied with some
comments.

Thanks!

Juan.

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java
(right):

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:33: * Class
that implements touch capabilities of a device
On 2011/08/12 23:21:28, berrada wrote:
> Sentences should end with a dot.

Done.

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:58: private
void scrollElementInView(Coordinates where) {
On 2011/08/12 23:21:28, berrada wrote:
> This isn't used anywhere. You can remove it for now

Done.

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
File android/src/java/org/openqa/selenium/android/events/TouchScreen.java
(right):

http://codereview.appspot.com/4853044/diff/1/android/src/java/org/openqa/sele...
android/src/java/org/openqa/selenium/android/events/TouchScreen.java:54:
Log.d("TouchApi", "MotionEvent => " + me);
On 2011/08/12 23:21:28, berrada wrote:
> Remove.

Done.

http://codereview.appspot.com/4853044/diff/1/java/server/src/org/openqa/selen...
File
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java
(right):

http://codereview.appspot.com/4853044/diff/1/java/server/src/org/openqa/selen...
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java:29:
public class Down extends WebElementHandler implements JsonParametersAware {
This action could be used not only for finger, it could be used for touch
devices that use pen or similar external peripherals.  Maybe name it ActionDown
instead?
On 2011/08/12 23:21:28, berrada wrote:
> Maybe call it FingerDown?

http://codereview.appspot.com/4853044/diff/1/java/server/src/org/openqa/selen...
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java:31:
private static final String X = "x";
I'm using the constants in the setJsonParameters method below.  I did not want
to hard code the name of the parameters down there and that is actually the way
it's used throughout the API.  Please let me know if I should remove them.
On 2011/08/12 23:21:28, berrada wrote:
> Those constants aren't needed.

[berrada, 2011-08-18 21:38:25]

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/TouchScreen.java (left):

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/TouchScreen.java:27: void
singleTap(Coordinates where);
Java doc is missing.

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/interactions/touch/DownAction.java
(right):

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/interactions/touch/DownAction.java:25: *
Creates a down gesture
. missing.

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java (left):

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:234: //
todo: Refactor with mouse click
s/ todo/TODO(user): blah.

http://codereview.appspot.com/4853044/diff/9001/java/server/src/org/openqa/se...
File
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java
(right):

http://codereview.appspot.com/4853044/diff/9001/java/server/src/org/openqa/se...
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java:54:
if (allParameters.containsKey(X) && allParameters.containsKey(Y)) {
Not needed, the command should have X and Y.

[juantrujillo, 2011-08-18 21:55:01]

Hi,

I have submitted a patch addressing the comments.

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/TouchScreen.java (left):

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/TouchScreen.java:27: void
singleTap(Coordinates where);
On 2011/08/18 21:38:26, berrada wrote:
> Java doc is missing.

Done.

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/interactions/touch/DownAction.java
(right):

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/interactions/touch/DownAction.java:25: *
Creates a down gesture
On 2011/08/18 21:38:26, berrada wrote:
> . missing.

Done.

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java (left):

http://codereview.appspot.com/4853044/diff/9001/java/client/src/org/openqa/se...
java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:234: //
todo: Refactor with mouse click
On 2011/08/18 21:38:26, berrada wrote:
> s/ todo/TODO(user): blah.

Done.

http://codereview.appspot.com/4853044/diff/9001/java/server/src/org/openqa/se...
File
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java
(right):

http://codereview.appspot.com/4853044/diff/9001/java/server/src/org/openqa/se...
java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/Down.java:54:
if (allParameters.containsKey(X) && allParameters.containsKey(Y)) {
On 2011/08/18 21:38:26, berrada wrote:
> Not needed, the command should have X and Y.

Done.

[berrada, 2011-08-18 22:32:38]

LGTM submitted in r13450

On Thu, Aug 18, 2011 at 3:06 PM, <juantrujillo@google.com> wrote:

>
http://codereview.appspot.com/**4853044/<http://codereview.appspot.com/4853044/>
>



-- 
Dounia