Issue 4664055: Advanced Touch API - SingleTap gesture: initial tests, API and Android implementation
|
Created:
12 years, 10 months ago by juantrujillo Modified:
12 years, 8 months ago CC:
webdriver-eng_google.com, simonstewart Base URL:
http://selenium.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionThis is the initial part of the Advanced touch API, including the single tap gesture with some basic tests, use of the API and the associated implementation for Android.
Patch Set 1 #
Total comments: 43
Patch Set 2 : SingleTap Android impklementation changed and code refactor #
Total comments: 88
Patch Set 3 : Advanced Touch API for Android - Patch to include Dounia's and Eran's comments for SingleTap gesture #
Total comments: 67
Patch Set 4 : Advanced Touch API for Android - Patch for singleTapGesture #
Total comments: 23
Patch Set 5 : Advanced touch API for Android - Patch for SingleTap gesture fixing naming conventions #
Total comments: 29
Patch Set 6 : Advanced touch API for Android - Patch for SingleTap gesture reusing available code #Patch Set 7 : Minor fix in build file - java/client/src/org/openqa/selenium/interactions/internal/build.desc #Patch Set 8 : Advanced Touch API for Android - Removing unused variable in RemoteWebDriver #
Total comments: 28
Patch Set 9 : Advanced Touch API - SingleTap: Minor comments #Patch Set 10 : Advanced Touch API - SingleTap - Fixing last event detection in sequence of motion events #Patch Set 11 : Advanced touch API - Single Tap - Fixing detection of last motion event in sequence #Patch Set 12 : Advanced Touch API - Single Tap - Removed dependency on System Clock time for ME #Patch Set 13 : Advanced Touch API - SingleTap #Patch Set 14 : Advanced TOuch API - SingleTap - Indentation fix #MessagesTotal messages: 37
Hi Juan, Sorry for the delayed review. Here is a first set of comments. Cheers, http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:42: import org.openqa.selenium.interactions.internal.Coordinates; Please keep imports sorted by package. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... File android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java (right): http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java:58: addNewPostMapping("/session/:sessionId/element/:id/singleTap", SingleTapOnElement.class) Since single tap is equivalent to a click, you can use the same wire command. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... File android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java (right): http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java:2: Copyright 2010 WebDriver committers s/2010/2011 http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java:18: package org.openqa.selenium.android.server.handler; This handler isn't android specific, put it in the remote server. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... File java/client/src/org/openqa/selenium/htmlunit/HtmlUnitDriver.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/htmlunit/HtmlUnitDriver.java:509: public Touch getTouch() { This does not seem right. The drivers that will implement this interface are Android and iPhone only. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:2: Missing copyright. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:7: * Class comment is missing. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:25: import org.openqa.selenium.*; :( http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... File java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:17: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); Import java.util.Logger instead. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:17: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); What's "JDTC"? The logger should be a constant, and there is no reason to have this public: private static final Logger LOG = Logger.getLogger(BasicTouchInterfaceTest.class.getName()) http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:21: super.setUp(); This isn't needed, remove this method. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:25: protected void tearDown() throws Exception { Ditto. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:36: @Ignore(value = {IPHONE,SELENESE}, reason = "iPhone: Frame switching is unsupported") You probably want to ignore all (chrome, ff, etc. ) but Android. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:36: @Ignore(value = {IPHONE,SELENESE}, reason = "iPhone: Frame switching is unsupported") Bad copy paste? You need to update the reason here. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:37: public void testCanShowPageWithTitle() { This test does not do much.. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:48: log.log(java.util.logging.Level.WARNING, "JDTC testCanSingleTap in BasicTouchInterfaceTest driver ===> "+driver); Ditto, import java.util.logging. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:48: log.log(java.util.logging.Level.WARNING, "JDTC testCanSingleTap in BasicTouchInterfaceTest driver ===> "+driver); I'm not convinced those logs are needed. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:50: log.log(java.util.logging.Level.WARNING, "JDTC before singleTap in BasicTouchTest toSingleTap ===> "+toSingleTap); Ditto. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... File java/client/test/org/openqa/selenium/android/AndroidDriverTestSuite.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/android/AndroidDriverTestSuite.java:50: .onlyRun("BasicTouchInterfaceTest") Revert this.
Sign in to reply to this message.
Hi, I have included changes to consider the feedback and uploaded a new set of files. Juan. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:42: import org.openqa.selenium.interactions.internal.Coordinates; On 2011/07/12 00:10:20, berrada wrote: > Please keep imports sorted by package. Done. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... File android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java (right): http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java:58: addNewPostMapping("/session/:sessionId/element/:id/singleTap", SingleTapOnElement.class) I'd prefer to have it on a separate class, with its own routing url, for consistency and ease of implementation when providing the implementation of the touch interface. Please let me know if you disagree with this. On 2011/07/12 00:10:20, berrada wrote: > Since single tap is equivalent to a click, you can use the same wire command. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java:58: addNewPostMapping("/session/:sessionId/element/:id/singleTap", SingleTapOnElement.class) On 2011/07/12 00:10:20, berrada wrote: > Since single tap is equivalent to a click, you can use the same wire command. Done. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... File android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java (right): http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java:2: Copyright 2010 WebDriver committers On 2011/07/12 00:10:20, berrada wrote: > s/2010/2011 Done. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java:18: package org.openqa.selenium.android.server.handler; I put in the remote server under a package named touch, to maintain consistency with what we have been doing. On 2011/07/12 00:10:20, berrada wrote: > This handler isn't android specific, put it in the remote server. http://codereview.appspot.com/4664055/diff/1/android/src/java/org/openqa/sele... android/src/java/org/openqa/selenium/android/server/handler/SingleTapOnElement.java:18: package org.openqa.selenium.android.server.handler; On 2011/07/12 00:10:20, berrada wrote: > This handler isn't android specific, put it in the remote server. Done. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:2: On 2011/07/12 00:10:20, berrada wrote: > Missing copyright. Done. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:7: * On 2011/07/12 00:10:20, berrada wrote: > Class comment is missing. Done. http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/src/org/openqa/selen... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:25: import org.openqa.selenium.*; On 2011/07/12 00:10:20, berrada wrote: > :( Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... File java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java (right): http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:17: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); This is actually not needed, I forgot to remove it. Erased it from the project. On 2011/07/12 00:10:20, berrada wrote: > What's "JDTC"? > > The logger should be a constant, and there is no reason to have this public: > private static final Logger LOG = > Logger.getLogger(BasicTouchInterfaceTest.class.getName()) http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:17: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); Not needed. Removed from the project. On 2011/07/12 00:10:20, berrada wrote: > Import java.util.Logger instead. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:17: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); On 2011/07/12 00:10:20, berrada wrote: > Import java.util.Logger instead. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:17: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); On 2011/07/12 00:10:20, berrada wrote: > What's "JDTC"? > > The logger should be a constant, and there is no reason to have this public: > private static final Logger LOG = > Logger.getLogger(BasicTouchInterfaceTest.class.getName()) Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:21: super.setUp(); On 2011/07/12 00:10:20, berrada wrote: > This isn't needed, remove this method. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:25: protected void tearDown() throws Exception { On 2011/07/12 00:10:20, berrada wrote: > Ditto. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:36: @Ignore(value = {IPHONE,SELENESE}, reason = "iPhone: Frame switching is unsupported") On 2011/07/12 00:10:20, berrada wrote: > You probably want to ignore all (chrome, ff, etc. ) but Android. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:36: @Ignore(value = {IPHONE,SELENESE}, reason = "iPhone: Frame switching is unsupported") On 2011/07/12 00:10:20, berrada wrote: > Bad copy paste? You need to update the reason here. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:37: public void testCanShowPageWithTitle() { Removed. On 2011/07/12 00:10:20, berrada wrote: > This test does not do much.. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:37: public void testCanShowPageWithTitle() { On 2011/07/12 00:10:20, berrada wrote: > This test does not do much.. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:48: log.log(java.util.logging.Level.WARNING, "JDTC testCanSingleTap in BasicTouchInterfaceTest driver ===> "+driver); Not required, removed logs. On 2011/07/12 00:10:20, berrada wrote: > Ditto, import java.util.logging. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:48: log.log(java.util.logging.Level.WARNING, "JDTC testCanSingleTap in BasicTouchInterfaceTest driver ===> "+driver); On 2011/07/12 00:10:20, berrada wrote: > Ditto, import java.util.logging. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:48: log.log(java.util.logging.Level.WARNING, "JDTC testCanSingleTap in BasicTouchInterfaceTest driver ===> "+driver); Not required, removed logs. On 2011/07/12 00:10:20, berrada wrote: > I'm not convinced those logs are needed. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:48: log.log(java.util.logging.Level.WARNING, "JDTC testCanSingleTap in BasicTouchInterfaceTest driver ===> "+driver); On 2011/07/12 00:10:20, berrada wrote: > I'm not convinced those logs are needed. Done. http://codereview.appspot.com/4664055/diff/1/java/client/test/org/openqa/sele... java/client/test/org/openqa/selenium/BasicTouchInterfaceTest.java:50: log.log(java.util.logging.Level.WARNING, "JDTC before singleTap in BasicTouchTest toSingleTap ===> "+toSingleTap); On 2011/07/12 00:10:20, berrada wrote: > Ditto. Done.
Sign in to reply to this message.
Hi Juan, Here are the main points: * Re-use existing wire protocol commands where appropriate (click, moveto are suitable here). * Create an additional interface for touch-supporting drivers, avoid changes to other drivers. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:283: public WebDriver window(String nameOrHandle) { Why was the line duplicated? http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchSingleTap... File common/src/web/touchSingleTap.html (right): http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchSingleTap... common/src/web/touchSingleTap.html:1: <html> Why add another page to the set of test pages, rather than using an existing one? I can't find anything unique in this page to justify it. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/HasInputDevices.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/HasInputDevices.java:25: Mouse getMouse(); I recommend a different way to indicate support of touch gestures: Having another interface, HasTouchscreen, extending the HasInputDevices interface. This way, the AndroidDriver will implement the new interface while the rest of the drivers won't have to be changed. Conceptually, when a method can be invoked on a specific driver / element, it means the functionality is there and can be implemented. Returning null (like you did in the HtmlUnitDriver) or throwing an exception is a less clean solution to the problem of advertising a specific capability. With an additional interface, a user could ask (before invoking any action) if the instance of the driver she has implements HasTouchscreen and if so, create touch-based actions. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/Touch.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/Touch.java:25: This actually represents an input device - a touchscreen. Why not call it Touchscreen? http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/android/AndroidDriver.java:41: public class AndroidDriver extends RemoteWebDriver implements TakesScreenshot, Rotatable, The element should be locatable, not the driver. the Locatable interface means the object implementing this interface has a location on the screen/DOM. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/htmlunit/HtmlUnitDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/htmlunit/HtmlUnitDriver.java:508: If we have a separate interface (see my comment on the HasInputDevices change) than this is not needed. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/Actions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/Actions.java:53: This is a neat bit of refactoring - it makes sense to have a method for resetting the composite action (so it may better be called resetCompositeAction). It should be private, though - users should not be resetting the action themselves (build / perform does that). In addition, change the class so all sites invoking new CompositeAction directly will use this method. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/Actions.java:57: Why is this necessary, given the build() method? http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:25: */ This is very similar to the MouseAction. It'd make more sense to extract a base class from MouseAction (that will contain everything in MouseAction except moveToLocation) and use the newly-extracted class as a base class for touch-related actions (don't forget to modify MouseAction to extend this new base class as well) http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:31: */ This is a very good way of providing touch capability without adding too many methods to the existing Actions class. It definitely makes sense to have separate methods for touch gestures even if they map to existing methods (e.g. click) since intentions of code written using this class will be clearer. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:46: public TouchActions singleTap(WebElement onElement) { Should be mapped to a click action. The existing ClickAction invokes mouseMove as well - one way to take advantage of this flow is to add support for the mouseMove command in the server-side AndroidDriver and have it store the element (but not do anything else). http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/remote/DriverCommand.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/remote/DriverCommand.java:140: // These belong to the Advanced Touch API Single tap is equivalent of click - there's no need to have a separate wire command for it. Please re-use the existing command, add new ones when there's new functionality that is not mapped to an existing idiom. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/remote/RemoteWebElement.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/remote/RemoteWebElement.java:275: public Point getLocationOnScreenOnceScrolledIntoView() { nitpick: Do the code cleanup in a separate CL. When reviewing such a big CL, it's useful to be able to look the list of changed files and realize the reason why each of those files had to change. With this file, there's no reason to change it rather than code cleanup - so it's better not to have this file in the list of changed files for such a big change. http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... File java/client/test/org/openqa/selenium/ClickTest.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... java/client/test/org/openqa/selenium/ClickTest.java:37: driver.switchTo().defaultContent(); See comment about code cleanup in RemoteWebElement. http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... File java/client/test/org/openqa/selenium/TouchSingleTapTest.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:65: public void testCanSingleTapOnAnAnchorAndNotReloadThePage() { The common way to test page transitions is by having the action navigate to a separate page. Then you could simply test the page's title. It's clearer to test user-visible changes in such a test. http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... File java/client/test/org/openqa/selenium/android/AndroidDriverTestSuite.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... java/client/test/org/openqa/selenium/android/AndroidDriverTestSuite.java:48: .exclude(REMOTE) Cleanup - you probably want the rest of the tests running as well... http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java (right): http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:326: // Advanced Touch API Not needed if it maps to a click command. http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... File java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java (right): http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java:61: See comment about code cleanup on RemoteWebElement. http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... File java/server/src/org/openqa/selenium/remote/server/handler/WebElementHandler.java (right): http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... java/server/src/org/openqa/selenium/remote/server/handler/WebElementHandler.java:18: protected WebElement getElement() { Same, separate code cleanup.
Sign in to reply to this message.
Hi Juan, Here is another set of comment, I only went through the first set of the files in the CL. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:67: Rotatable, BrowserConnection, HasInputDevices { Move the implementation of HasInputDevices to its own class rather than an inner class and use an instance of the class here. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:87: SINGLE_TAP, SCROLL, FLICK_SLOW, FLICK_MEDIUM, FLICK_FAST; One enum per line. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:87: SINGLE_TAP, SCROLL, FLICK_SLOW, FLICK_MEDIUM, FLICK_FAST; I think it would be better to separate the gestures from the speed. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:612: List<MotionEvent> sequenceOfActionEvents = new ArrayList<MotionEvent>(); Use Lists.newArrayList()instead. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:619: Point destiny = origin; Destiny does not sound right :). s/destiny/destination or dest http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:629: private void getSequenceOfActionEvents(TypeOfMoveAction moveAction, Point origin, Point destiny) { ditto. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:629: private void getSequenceOfActionEvents(TypeOfMoveAction moveAction, Point origin, Point destiny) { moveAction is never used. This method does not really seem needed. Inline the code rather than calling this. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:631: MotionEvent downEvent = MotionEvent.obtain(downTime, Why not creating downTime here? It does not need to be a class variable as far as I can see. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:2: Copyright 2011 WebDriver committers No need to update the copyright. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:83: List<MotionEvent> events = new ArrayList<MotionEvent>(); s/new ArrayList<>()/Lists.newArrayList() http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:327: return null; Why returning null? http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:331: return null; Why this change? http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:334: public Coordinates getCoordinates() { Can you make this a class rather than an inner class? This class is getting really long. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/app/MainActivity.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:88: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); Is that needed? If so s/JDTC/MainActivity http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:193: // not in the UI thread fix indentation. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:196: // send to the Incomplete comment. http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchDestiny.html File common/src/web/touchDestiny.html (right): http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchDestiny.h... common/src/web/touchDestiny.html:4: </head> I don't think you need this. For testing singleTap you can reuse the same pages as the one used for testing click etc.
Sign in to reply to this message.
Hi all, I am currently working in the code refactor to address both Eran's and Dounia's comments for the code I previously submitted. I have the following questions, which are associated to the comments in the code, that I would like to clarify before submitting my patch: 2 questions - Actions.java: http://codereview.appspot.com/4664055/patch/8001/9022 1 question - TouchRelatedActions.java: http://codereview.appspot.com/4664055/patch/8001/9025 1 question - TouchActions.java: http://codereview.appspot.com/4664055/patch/8001/9023 Thanks a lot! Juan. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:67: Rotatable, BrowserConnection, HasInputDevices { Done. Implementation details moved to AndroidTouch.java On 2011/07/20 18:07:45, berrada wrote: > Move the implementation of HasInputDevices to its own class rather than an inner > class and use an instance of the class here. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:67: Rotatable, BrowserConnection, HasInputDevices { On 2011/07/20 18:07:45, berrada wrote: > Move the implementation of HasInputDevices to its own class rather than an inner > class and use an instance of the class here. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:87: SINGLE_TAP, SCROLL, FLICK_SLOW, FLICK_MEDIUM, FLICK_FAST; On 2011/07/20 18:07:45, berrada wrote: > One enum per line. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:87: SINGLE_TAP, SCROLL, FLICK_SLOW, FLICK_MEDIUM, FLICK_FAST; On 2011/07/20 18:07:45, berrada wrote: > I think it would be better to separate the gestures from the speed. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:283: public WebDriver window(String nameOrHandle) { Mistakenly copied it in my IDE. Fixed. On 2011/07/20 14:03:22, Eran wrote: > Why was the line duplicated? http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:283: public WebDriver window(String nameOrHandle) { On 2011/07/20 14:03:22, Eran wrote: > Why was the line duplicated? Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:612: List<MotionEvent> sequenceOfActionEvents = new ArrayList<MotionEvent>(); On 2011/07/20 18:07:45, berrada wrote: > Use Lists.newArrayList()instead. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:619: Point destiny = origin; You're right, destination sounds better. On 2011/07/20 18:07:45, berrada wrote: > Destiny does not sound right :). s/destiny/destination or dest http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:619: Point destiny = origin; On 2011/07/20 18:07:45, berrada wrote: > Destiny does not sound right :). s/destiny/destination or dest Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:629: private void getSequenceOfActionEvents(TypeOfMoveAction moveAction, Point origin, Point destiny) { On 2011/07/20 18:07:45, berrada wrote: > ditto. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:629: private void getSequenceOfActionEvents(TypeOfMoveAction moveAction, Point origin, Point destiny) { On 2011/07/20 18:07:45, berrada wrote: > moveAction is never used. > > This method does not really seem needed. Inline the code rather than calling > this. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:629: private void getSequenceOfActionEvents(TypeOfMoveAction moveAction, Point origin, Point destiny) { On 2011/07/20 18:07:45, berrada wrote: > ditto. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidDriver.java:631: MotionEvent downEvent = MotionEvent.obtain(downTime, On 2011/07/20 18:07:45, berrada wrote: > Why not creating downTime here? It does not need to be a class variable as far > as I can see. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:2: Copyright 2011 WebDriver committers On 2011/07/20 18:07:45, berrada wrote: > No need to update the copyright. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:83: List<MotionEvent> events = new ArrayList<MotionEvent>(); On 2011/07/20 18:07:45, berrada wrote: > s/new ArrayList<>()/Lists.newArrayList() Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:327: return null; Mistakenly deleted it using IDE. Fixed. On 2011/07/20 18:07:45, berrada wrote: > Why returning null? http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:327: return null; On 2011/07/20 18:07:45, berrada wrote: > Why returning null? Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:331: return null; Mistakenly deleted it using IDE. Fixed. On 2011/07/20 18:07:45, berrada wrote: > Why this change? http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:331: return null; On 2011/07/20 18:07:45, berrada wrote: > Why this change? Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:334: public Coordinates getCoordinates() { On 2011/07/20 18:07:45, berrada wrote: > Can you make this a class rather than an inner class? This class is getting > really long. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:334: public Coordinates getCoordinates() { Created the class AndroidCoordinates which implements implements Coordinates to handle implementation details. On 2011/07/20 18:07:45, berrada wrote: > Can you make this a class rather than an inner class? This class is getting > really long. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... File android/src/java/org/openqa/selenium/android/app/MainActivity.java (right): http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:88: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); Not required. Deleted it. On 2011/07/20 18:07:45, berrada wrote: > Is that needed? If so s/JDTC/MainActivity http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:88: java.util.logging.Logger log = java.util.logging.Logger.getLogger("JDTC"); On 2011/07/20 18:07:45, berrada wrote: > Is that needed? If so s/JDTC/MainActivity Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:193: // not in the UI thread On 2011/07/20 18:07:45, berrada wrote: > fix indentation. Done. http://codereview.appspot.com/4664055/diff/8001/android/src/java/org/openqa/s... android/src/java/org/openqa/selenium/android/app/MainActivity.java:196: // send to the On 2011/07/20 18:07:45, berrada wrote: > Incomplete comment. Done. http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchDestiny.html File common/src/web/touchDestiny.html (right): http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchDestiny.h... common/src/web/touchDestiny.html:4: </head> I'd prefer to have a separate set of tests, because I'm testing some additional things. Let me know what you think. On 2011/07/20 18:07:45, berrada wrote: > I don't think you need this. For testing singleTap you can reuse the same pages > as the one used for testing click etc. http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchSingleTap... File common/src/web/touchSingleTap.html (right): http://codereview.appspot.com/4664055/diff/8001/common/src/web/touchSingleTap... common/src/web/touchSingleTap.html:1: <html> I'd prefer to have a separate set of tests, because I'm testing some additional things, like clicking on images with or without links, which is quite important for the purpose of this API. Let me know what you think. On 2011/07/20 14:03:22, Eran wrote: > Why add another page to the set of test pages, rather than using an existing > one? I can't find anything unique in this page to justify it. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/HasInputDevices.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/HasInputDevices.java:25: Mouse getMouse(); On 2011/07/20 14:03:22, Eran wrote: > I recommend a different way to indicate support of touch gestures: Having > another interface, HasTouchscreen, extending the HasInputDevices interface. This > way, the AndroidDriver will implement the new interface while the rest of the > drivers won't have to be changed. > Conceptually, when a method can be invoked on a specific driver / element, it > means the functionality is there and can be implemented. Returning null (like > you did in the HtmlUnitDriver) or throwing an exception is a less clean solution > to the problem of advertising a specific capability. > With an additional interface, a user could ask (before invoking any action) if > the instance of the driver she has implements HasTouchscreen and if so, create > touch-based actions. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/HasInputDevices.java:25: Mouse getMouse(); Agree with this design decision. Created a new Interface called HasTouchScreen following this proposal. On 2011/07/20 14:03:22, Eran wrote: > I recommend a different way to indicate support of touch gestures: Having > another interface, HasTouchscreen, extending the HasInputDevices interface. This > way, the AndroidDriver will implement the new interface while the rest of the > drivers won't have to be changed. > Conceptually, when a method can be invoked on a specific driver / element, it > means the functionality is there and can be implemented. Returning null (like > you did in the HtmlUnitDriver) or throwing an exception is a less clean solution > to the problem of advertising a specific capability. > With an additional interface, a user could ask (before invoking any action) if > the instance of the driver she has implements HasTouchscreen and if so, create > touch-based actions. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/Touch.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/Touch.java:25: On 2011/07/20 14:03:22, Eran wrote: > This actually represents an input device - a touchscreen. Why not call it > Touchscreen? Done. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/android/AndroidDriver.java:41: public class AndroidDriver extends RemoteWebDriver implements TakesScreenshot, Rotatable, On 2011/07/20 14:03:22, Eran wrote: > The element should be locatable, not the driver. the Locatable interface means > the object implementing this interface has a location on the screen/DOM. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/htmlunit/HtmlUnitDriver.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/htmlunit/HtmlUnitDriver.java:508: On 2011/07/20 14:03:22, Eran wrote: > If we have a separate interface (see my comment on the HasInputDevices change) > than this is not needed. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/Actions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/Actions.java:53: Updated the code accordingly. Not sure what you mean with "change the class so all sites invoking new CompositeAction directly will use this method." On 2011/07/20 14:03:22, Eran wrote: > This is a neat bit of refactoring - it makes sense to have a method for > resetting the composite action (so it may better be called > resetCompositeAction). > It should be private, though - users should not be resetting the action > themselves (build / perform does that). > In addition, change the class so all sites invoking new CompositeAction directly > will use this method. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/Actions.java:57: This is used in TouchActions.java which extends Actions.java. The need for this method is to have access to the private attribute CompositeAction action, which is defined in Actions.java. Let me know if you see a cleaner way to use it. On 2011/07/20 14:03:22, Eran wrote: > Why is this necessary, given the build() method? http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:25: */ I think it's better to have them completely separate since there might be devices that incorporate a touch screen and a mouse, so it might be confusing for users to have them being defined as analog concepts. Dounia mentioned some possible scenarios involving capabilities like these, recently. We should try though to reuse implementation details when appropriate. Let me know what you think, so we take a final decision. On 2011/07/20 14:03:22, Eran wrote: > This is very similar to the MouseAction. It'd make more sense to extract a base > class from MouseAction (that will contain everything in MouseAction except > moveToLocation) and use the newly-extracted class as a base class for > touch-related actions (don't forget to modify MouseAction to extend this new > base class as well) http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:31: */ On 2011/07/20 14:03:22, Eran wrote: > This is a very good way of providing touch capability without adding too many > methods to the existing Actions class. It definitely makes sense to have > separate methods for touch gestures even if they map to existing methods (e.g. > click) since intentions of code written using this class will be clearer. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:46: public TouchActions singleTap(WebElement onElement) { Let's discuss this, because I am not totally clear of the parts that are already implemented in the Android driver and could be potentially reused (e.g. click, using move first). On 2011/07/20 14:03:22, Eran wrote: > Should be mapped to a click action. The existing ClickAction invokes mouseMove > as well - one way to take advantage of this flow is to add support for the > mouseMove command in the server-side AndroidDriver and have it store the element > (but not do anything else). http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/remote/DriverCommand.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/remote/DriverCommand.java:140: // These belong to the Advanced Touch API On 2011/07/20 14:03:22, Eran wrote: > Single tap is equivalent of click - there's no need to have a separate wire > command for it. Please re-use the existing command, add new ones when there's > new functionality that is not mapped to an existing idiom. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/remote/RemoteWebElement.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/remote/RemoteWebElement.java:275: public Point getLocationOnScreenOnceScrolledIntoView() { Great tip. Thanks! On 2011/07/20 14:03:22, Eran wrote: > nitpick: Do the code cleanup in a separate CL. When reviewing such a big CL, > it's useful to be able to look the list of changed files and realize the reason > why each of those files had to change. With this file, there's no reason to > change it rather than code cleanup - so it's better not to have this file in the > list of changed files for such a big change. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/remote/RemoteWebElement.java:275: public Point getLocationOnScreenOnceScrolledIntoView() { On 2011/07/20 14:03:22, Eran wrote: > nitpick: Do the code cleanup in a separate CL. When reviewing such a big CL, > it's useful to be able to look the list of changed files and realize the reason > why each of those files had to change. With this file, there's no reason to > change it rather than code cleanup - so it's better not to have this file in the > list of changed files for such a big change. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... File java/client/test/org/openqa/selenium/ClickTest.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... java/client/test/org/openqa/selenium/ClickTest.java:37: driver.switchTo().defaultContent(); On 2011/07/20 14:03:22, Eran wrote: > See comment about code cleanup in RemoteWebElement. Done. http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... File java/client/test/org/openqa/selenium/TouchSingleTapTest.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:65: public void testCanSingleTapOnAnAnchorAndNotReloadThePage() { I agree. The test you mention was actually implemented in the method testCanSingleTapOnALinkAndFollowIt; this one is a little different, because the idea is to validate that clicking on an anchor does not reload the current page. On 2011/07/20 14:03:22, Eran wrote: > The common way to test page transitions is by having the action navigate to a > separate page. Then you could simply test the page's title. It's clearer to test > user-visible changes in such a test. http://codereview.appspot.com/4664055/diff/8001/java/client/test/org/openqa/s... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:65: public void testCanSingleTapOnAnAnchorAndNotReloadThePage() { On 2011/07/20 14:03:22, Eran wrote: > The common way to test page transitions is by having the action navigate to a > separate page. Then you could simply test the page's title. It's clearer to test > user-visible changes in such a test. Done. http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java (right): http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:326: // Advanced Touch API On 2011/07/20 14:03:22, Eran wrote: > Not needed if it maps to a click command. Done. http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... File java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java (right): http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java:61: On 2011/07/20 14:03:22, Eran wrote: > See comment about code cleanup on RemoteWebElement. Done. http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... File java/server/src/org/openqa/selenium/remote/server/handler/WebElementHandler.java (right): http://codereview.appspot.com/4664055/diff/8001/java/server/src/org/openqa/se... java/server/src/org/openqa/selenium/remote/server/handler/WebElementHandler.java:18: protected WebElement getElement() { On 2011/07/20 14:03:22, Eran wrote: > Same, separate code cleanup. Done.
Sign in to reply to this message.
Hi all, I have uploaded the patch to include Dounia's and Eran's comments. Thanks! Juan. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:46: public TouchActions singleTap(WebElement onElement) { The Mouse and Keyboard interfaces are not currently implemented for Android, so reusing would not be possible. What is implemented is a click event not based on the interface Mouse. Additionally, with the appearance of new devices supporting simultaneously mouse and touch screen capabilities, it would be probably be a good idea to keep them as separate as possible. On 2011/07/22 22:21:24, juantrujillo wrote: > Let's discuss this, because I am not totally clear of the parts that are already > implemented in the Android driver and could be potentially reused (e.g. click, > using move first). > On 2011/07/20 14:03:22, Eran wrote: > > Should be mapped to a click action. The existing ClickAction invokes mouseMove > > as well - one way to take advantage of this flow is to add support for the > > mouseMove command in the server-side AndroidDriver and have it store the > element > > (but not do anything else). > http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/remote/DriverCommand.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/remote/DriverCommand.java:140: // These belong to the Advanced Touch API There is need for an implementation since the current click event is not based on the Mouse interface, so it would not be possible to reuse it this way. On 2011/07/20 14:03:22, Eran wrote: > Single tap is equivalent of click - there's no need to have a separate wire > command for it. Please re-use the existing command, add new ones when there's > new functionality that is not mapped to an existing idiom.
Sign in to reply to this message.
Hope this answers your questions. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/Actions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/Actions.java:53: On 2011/07/22 22:21:24, juantrujillo wrote: > Updated the code accordingly. Not sure what you mean with "change the class so > all sites invoking new CompositeAction directly will use this method." > On 2011/07/20 14:03:22, Eran wrote: > > This is a neat bit of refactoring - it makes sense to have a method for > > resetting the composite action (so it may better be called > > resetCompositeAction). > > It should be private, though - users should not be resetting the action > > themselves (build / perform does that). > > In addition, change the class so all sites invoking new CompositeAction > directly > > will use this method. > All of the code lines with 'action = new CompositeAction()' should be replaced with a call to resetCompositeAction(). For example, the call site in the build() method should do that. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/Actions.java:57: On 2011/07/22 22:21:24, juantrujillo wrote: > This is used in TouchActions.java which extends Actions.java. The need for this > method is to have access to the private attribute CompositeAction action, which > is defined in Actions.java. Let me know if you see a cleaner way to use it. > On 2011/07/20 14:03:22, Eran wrote: > > Why is this necessary, given the build() method? > In that case, it can be made protected. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:25: */ On 2011/07/22 22:21:24, juantrujillo wrote: > I think it's better to have them completely separate since there might be > devices that incorporate a touch screen and a mouse, so it might be confusing > for users to have them being defined as analog concepts. Dounia mentioned some > possible scenarios involving capabilities like these, recently. We should try > though to reuse implementation details when appropriate. Let me know what you > think, so we take a final decision. > On 2011/07/20 14:03:22, Eran wrote: > > This is very similar to the MouseAction. It'd make more sense to extract a > base > > class from MouseAction (that will contain everything in MouseAction except > > moveToLocation) and use the newly-extracted class as a base class for > > touch-related actions (don't forget to modify MouseAction to extend this new > > base class as well) > I agree that user-facing code should keep the distinction between a touch screen and a mouse. Users do need separate interfaces for these actions. These Action classes, however, are internal - their names refer the mouse because, at the time, there was no other device capable of actions on Locatable elements. The justification against keeping these classes separate is the code duplication - So I think that in this case it'd make sense to create a common superclass and name it in a way that does not refer specifically to mouse or touchscreen. Feel free to call and talk about it, if you still disagree. http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... File java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java (right): http://codereview.appspot.com/4664055/diff/8001/java/client/src/org/openqa/se... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:46: public TouchActions singleTap(WebElement onElement) { On 2011/07/23 21:15:18, juantrujillo wrote: > The Mouse and Keyboard interfaces are not currently implemented for Android, so > reusing would not be possible. What is implemented is a click event not based > on the interface Mouse. Additionally, with the appearance of new devices > supporting simultaneously mouse and touch screen capabilities, it would be > probably be a good idea to keep them as separate as possible. > On 2011/07/22 22:21:24, juantrujillo wrote: > > Let's discuss this, because I am not totally clear of the parts that are > already > > implemented in the Android driver and could be potentially reused (e.g. click, > > using move first). > > On 2011/07/20 14:03:22, Eran wrote: > > > Should be mapped to a click action. The existing ClickAction invokes > mouseMove > > > as well - one way to take advantage of this flow is to add support for the > > > mouseMove command in the server-side AndroidDriver and have it store the > > element > > > (but not do anything else). > > > The main thing I wanted re-used is the wire protocol command. I gave it some more thought and it seems like the mousemove command should also be reused. In the bigger context, this command is not only used to generate mouseMove events, but also to set the context for further mouse actions (like click).
Sign in to reply to this message.
http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/ActivityController.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:345: return lastEventInSequence; This should be synchronized. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidCoordinates.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:1: // Copyright 2011 Google Inc. All Rights Reserved. Fix copyright. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:10: private String elementId; elementId should be final. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:11: private Point point; point should be final. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:18: public org.openqa.selenium.Point getLocationOnScreen() { This is already imported, you can just use it. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:31: import org.openqa.selenium.Keyboard; Unused imports. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:79: private ActivityController controller = ActivityController.getInstance(); Remove trailing spaces. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:593: return null; Remove. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:596: public Mouse getMouse() { This isn't needed. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouch.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:1: // Copyright 2011 Google Inc. All Rights Reserved. Fix copyright here and apply to all files when needed. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:21: public class AndroidTouch implements TouchScreen { s/AndroidTouch/AndroidTouchScreen or TouchScreenImpl http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:23: ActivityController controller; should be final. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:24: AndroidDriver driver; should be final. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:25: List<MotionEvent> sequenceOfActionEventsToDispatch; This does not need to be a class variable. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:40: private void getSequenceOfActionEvents(Point origin, Point destination) { Inline this method as I doubt it will be resued. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:338: org.openqa.selenium.Point pointOfLocationOfElement = getLocation(); Use imports here and everywhere else. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/MainActivity.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/MainActivity.java:105: List<MotionEvent> events = (List<MotionEvent>) msg.obj; Remove trailing spaces. You can set your IDE to do that for you on every save action. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/events/TouchScreen.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/events/TouchScreen.java:31: * Class used to send touchScreen events to the screen directed to the webview. "touchScreen" aren't events :). http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java:2: Copyright 2011 WebDriver committers Revert this file. http://codereview.appspot.com/4664055/diff/18004/common/src/web/touchDestiny.... File common/src/web/touchDestiny.html (right): http://codereview.appspot.com/4664055/diff/18004/common/src/web/touchDestiny.... common/src/web/touchDestiny.html:3: <title>TouchDestiny</title> You can reuse the pages used for click tests. http://codereview.appspot.com/4664055/diff/18004/common/src/web/touchSingleTa... File common/src/web/touchSingleTap.html (right): http://codereview.appspot.com/4664055/diff/18004/common/src/web/touchSingleTa... common/src/web/touchSingleTap.html:2: <head> Reuse existing pages. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/HasInputDevices.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/HasInputDevices.java:3: Copyright 2007-2011 Google Inc. Revert this file. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/TouchScreen.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/TouchScreen.java:25: Remove extra line. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/android/AndroidDriver.java:2: Copyright 2011 WebDriver committers Revert this file if there are no changes. This seems to be missing the client side implementation. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:30: public TouchRelatedAction(TouchScreen touchScreen, Locatable locationProvider) { Pull up the { http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:30: * Implements the Advanced TouchScreen API reusing the available composite and builder design "Implements touch actions." or "Implements actions for touch enabled devices." "Advanced TouchScreen" isn't an API on its own :). http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:64: HasInputDevices, HasCapabilities, HasTouchScreen { The RemoteWebDriver should not implement this interface because it won't be supported by most browser. Instead pull this in its own class and use getExecuteMethod() to send the commands. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebElement.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebElement.java:275: public Point getLocationOnScreenOnceScrolledIntoView() { Revert this file. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/support/events/EventFiringWebDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/support/events/EventFiringWebDriver.java:293: public Object getAuxiliry() { This does not seem right. Let's pair later today to find a better solution. http://codereview.appspot.com/4664055/diff/18004/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/Pages.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/Pages.java:103: touchDestiny = appServer.whereIs("touchDestiny.html"); Remove this and reuse existing pages. http://codereview.appspot.com/4664055/diff/18004/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/android/AndroidDriverTestSuite.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/android/AndroidDriverTestSuite.java:49: .includeJavascriptTests() Revert this file. In general please revert all files where you made no changes. Also avoid adding trailing spaces. http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java (right): http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:329: .on(ResultType.SUCCESS, new RedirectResult("/session/:sessionId")); I think you should return an empty result. http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java (right): http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java:63: Session session = getSession(); Ditto. http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/handler/WebElementHandler.java (right): http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/handler/WebElementHandler.java:19: return getKnownElements().get(elementId); Ditto.
Sign in to reply to this message.
Hi, I have submitted a new patch addressing the comments for singleTapGesture. Thanks! Juan. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/ActivityController.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:345: return lastEventInSequence; On 2011/07/25 18:09:22, berrada wrote: > This should be synchronized. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidCoordinates.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:1: // Copyright 2011 Google Inc. All Rights Reserved. On 2011/07/25 18:09:22, berrada wrote: > Fix copyright. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:10: private String elementId; On 2011/07/25 18:09:22, berrada wrote: > elementId should be final. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:11: private Point point; On 2011/07/25 18:09:22, berrada wrote: > point should be final. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:18: public org.openqa.selenium.Point getLocationOnScreen() { On 2011/07/25 18:09:22, berrada wrote: > This is already imported, you can just use it. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:31: import org.openqa.selenium.Keyboard; On 2011/07/25 18:09:22, berrada wrote: > Unused imports. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:79: private ActivityController controller = ActivityController.getInstance(); On 2011/07/25 18:09:22, berrada wrote: > Remove trailing spaces. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:593: return null; On 2011/07/25 18:09:22, berrada wrote: > Remove. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:596: public Mouse getMouse() { On 2011/07/25 18:09:22, berrada wrote: > This isn't needed. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouch.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:1: // Copyright 2011 Google Inc. All Rights Reserved. On 2011/07/25 18:09:22, berrada wrote: > Fix copyright here and apply to all files when needed. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:21: public class AndroidTouch implements TouchScreen { On 2011/07/25 18:09:22, berrada wrote: > s/AndroidTouch/AndroidTouchScreen or TouchScreenImpl Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:23: ActivityController controller; On 2011/07/25 18:09:22, berrada wrote: > should be final. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:24: AndroidDriver driver; On 2011/07/25 18:09:22, berrada wrote: > should be final. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:25: List<MotionEvent> sequenceOfActionEventsToDispatch; I think it does need to be. It will be reused for motion events and is used in the sendMotionEvents method. On 2011/07/25 18:09:22, berrada wrote: > This does not need to be a class variable. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:25: List<MotionEvent> sequenceOfActionEventsToDispatch; On 2011/07/25 18:09:22, berrada wrote: > This does not need to be a class variable. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:40: private void getSequenceOfActionEvents(Point origin, Point destination) { I think it will be reused because each gesture starts with an ACTION_DOWN (origin) and end with an ACTION_UP (destination). On 2011/07/25 18:09:22, berrada wrote: > Inline this method as I doubt it will be resued. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouch.java:40: private void getSequenceOfActionEvents(Point origin, Point destination) { On 2011/07/25 18:09:22, berrada wrote: > Inline this method as I doubt it will be resued. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:338: org.openqa.selenium.Point pointOfLocationOfElement = getLocation(); Used, but require to specify full path to android.graphics.Point. On 2011/07/25 18:09:22, berrada wrote: > Use imports here and everywhere else. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:338: org.openqa.selenium.Point pointOfLocationOfElement = getLocation(); On 2011/07/25 18:09:22, berrada wrote: > Use imports here and everywhere else. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/MainActivity.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/MainActivity.java:105: List<MotionEvent> events = (List<MotionEvent>) msg.obj; On 2011/07/25 18:09:22, berrada wrote: > Remove trailing spaces. You can set your IDE to do that for you on every save > action. Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/events/TouchScreen.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/events/TouchScreen.java:31: * Class used to send touchScreen events to the screen directed to the webview. On 2011/07/25 18:09:22, berrada wrote: > "touchScreen" aren't events :). Done. http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java (right): http://codereview.appspot.com/4664055/diff/18004/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/server/AndroidDriverServlet.java:2: Copyright 2011 WebDriver committers On 2011/07/25 18:09:22, berrada wrote: > Revert this file. Done. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/TouchScreen.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/TouchScreen.java:25: On 2011/07/25 18:09:22, berrada wrote: > Remove extra line. Done. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/android/AndroidDriver.java:2: Copyright 2011 WebDriver committers Based on Eran's comments, my understanding is that it's the element that is locatable, not the Driver. On 2011/07/25 18:09:22, berrada wrote: > Revert this file if there are no changes. This seems to be missing the client > side implementation. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/android/AndroidDriver.java:2: Copyright 2011 WebDriver committers On 2011/07/25 18:09:22, berrada wrote: > Revert this file if there are no changes. This seems to be missing the client > side implementation. Done. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:30: public TouchRelatedAction(TouchScreen touchScreen, Locatable locationProvider) { Not sure what you mean here. On 2011/07/25 18:09:22, berrada wrote: > Pull up the { http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:30: public TouchRelatedAction(TouchScreen touchScreen, Locatable locationProvider) { On 2011/07/25 18:09:22, berrada wrote: > Pull up the { Done. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/touch/TouchActions.java:30: * Implements the Advanced TouchScreen API reusing the available composite and builder design On 2011/07/25 18:09:22, berrada wrote: > "Implements touch actions." or "Implements actions for touch enabled devices." > > "Advanced TouchScreen" isn't an API on its own :). Done. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:64: HasInputDevices, HasCapabilities, HasTouchScreen { Let's discuss this. On 2011/07/25 18:09:22, berrada wrote: > The RemoteWebDriver should not implement this interface because it won't be > supported by most browser. Instead pull this in its own class and use > getExecuteMethod() to send the commands. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:64: HasInputDevices, HasCapabilities, HasTouchScreen { On 2011/07/25 18:09:22, berrada wrote: > The RemoteWebDriver should not implement this interface because it won't be > supported by most browser. Instead pull this in its own class and use > getExecuteMethod() to send the commands. Done. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/support/events/EventFiringWebDriver.java (right): http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/support/events/EventFiringWebDriver.java:293: public Object getAuxiliry() { OK. On 2011/07/25 18:09:22, berrada wrote: > This does not seem right. Let's pair later today to find a better solution. http://codereview.appspot.com/4664055/diff/18004/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/support/events/EventFiringWebDriver.java:293: public Object getAuxiliry() { On 2011/07/25 18:09:22, berrada wrote: > This does not seem right. Let's pair later today to find a better solution. Done. http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java (right): http://codereview.appspot.com/4664055/diff/18004/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:329: .on(ResultType.SUCCESS, new RedirectResult("/session/:sessionId")); On 2011/07/25 18:09:22, berrada wrote: > I think you should return an empty result. Done.
Sign in to reply to this message.
Looks good overall, some minor comments. We're almost there! http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/ActivityController.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:16: */ Unnecessary :). http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:52: private MotionEvent lastEventInSequence; s/lastEventInSequence/lastMotionEventSent. Please add a comment explaining why we're keeping track of this, something like: "//We keep track of the last motion event send so the WebView.onTouchEvent() listener can detect when the last ME has been received" http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:161: private MotionEvent getLastEventInSequence(List<MotionEvent> eventsToSendToScreen) { I think you can inline this. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:586: return new AndroidTouchScreen(controller, AndroidDriver.this); Make this a class variable so we use only one instance throughout the session. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:49: Point destination = origin; This feels redundant and creates an object when it's not really needed. Remember that the memory allocated for android apps is limited and we should be conservative (there is already a bug opened because the app runs out of memory for many test runs). Remove destination and reuse origin, you can alse rename origin to "toTap" or something similar. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:57: private void moveScreenIfNeeded(Coordinates where) { s/moveScreenIfNeeded/scrollElementInView http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:89: Log.d("jdtc", "last event="+lastEventInSequence); This isn't needed. http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:97: private void initializeValues() { s/initializeValues/init http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:100: this.touch = new TouchScreenImpl(executeMethod); remove "this" to keep consistent with the others. http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/Utils.java (right): http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/Utils.java:29: class Utils { Utils is quite generic. Maybe call this CoordinatesUtils? Is this class package visible on purpose? If so can you add a /* package */ comment like this: /* package */ class CoordinatesUtils { .. } http://codereview.appspot.com/4664055/diff/19003/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/TouchSingleTapTest.java (right): http://codereview.appspot.com/4664055/diff/19003/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:41: @JavascriptEnabled Is this needed? http://codereview.appspot.com/4664055/diff/19003/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/SingleTapOnElement.java (right): http://codereview.appspot.com/4664055/diff/19003/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/handler/interactions/touch/SingleTapOnElement.java:56: public void setJsonParameters(Map<String, Object> allParameters) throws Exception { This does not seem needed, WebElementHandler already contains information about the elementId.
Sign in to reply to this message.
Hi, I have uploaded a new patch addressing the comments. Thanks, Juan. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/ActivityController.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:52: private MotionEvent lastEventInSequence; On 2011/07/28 15:30:38, berrada wrote: > s/lastEventInSequence/lastMotionEventSent. Please add a comment explaining why > we're keeping track of this, something like: "//We keep track of the last motion > event send so the WebView.onTouchEvent() listener can detect when the last ME > has been received" Done. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:161: private MotionEvent getLastEventInSequence(List<MotionEvent> eventsToSendToScreen) { I think invoking this method makes the code more readable and easy to follow. Let me know if you consider it makes sense. On 2011/07/28 15:30:38, berrada wrote: > I think you can inline this. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/ActivityController.java:348: public MotionEvent getLastEventInSequence() { Changed the name of this method to "getLastMotionEventSent", to match the new name of the variable it returns. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidDriver.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidDriver.java:586: return new AndroidTouchScreen(controller, AndroidDriver.this); On 2011/07/28 15:30:38, berrada wrote: > Make this a class variable so we use only one instance throughout the session. Done. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:49: Point destination = origin; On 2011/07/28 15:30:38, berrada wrote: > This feels redundant and creates an object when it's not really needed. Remember > that the memory allocated for android apps is limited and we should be > conservative (there is already a bug opened because the app runs out of memory > for many test runs). Remove destination and reuse origin, you can alse rename > origin to "toTap" or something similar. Done. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:57: private void moveScreenIfNeeded(Coordinates where) { On 2011/07/28 15:30:38, berrada wrote: > s/moveScreenIfNeeded/scrollElementInView Done. http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java (right): http://codereview.appspot.com/4664055/diff/19003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:89: Log.d("jdtc", "last event="+lastEventInSequence); On 2011/07/28 15:30:38, berrada wrote: > This isn't needed. Done. http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:97: private void initializeValues() { On 2011/07/28 15:30:38, berrada wrote: > s/initializeValues/init Done. http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:100: this.touch = new TouchScreenImpl(executeMethod); On 2011/07/28 15:30:38, berrada wrote: > remove "this" to keep consistent with the others. Done. http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/Utils.java (right): http://codereview.appspot.com/4664055/diff/19003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/Utils.java:29: class Utils { On 2011/07/28 15:30:38, berrada wrote: > Utils is quite generic. Maybe call this CoordinatesUtils? > > Is this class package visible on purpose? If so can you add a /* package */ > comment like this: > > /* package */ class CoordinatesUtils { > .. > } Done. http://codereview.appspot.com/4664055/diff/19003/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/TouchSingleTapTest.java (right): http://codereview.appspot.com/4664055/diff/19003/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:41: @JavascriptEnabled I think it's better to enable Javascript for each test, since we don't know what kinds of tests will be supported by this API, so it's a good way to validate behavior under Javascript scenarios. On 2011/07/28 15:30:38, berrada wrote: > Is this needed?
Sign in to reply to this message.
Hi, here are my remaining comments - looks much better now, almost done. The only remaining issue I haven't commented on is the name of the wire protocol command. I'll discuss that with Simon and send a separate mail. http://codereview.appspot.com/4664055/diff/33002/common/src/web/clicks.html File common/src/web/clicks.html (right): http://codereview.appspot.com/4664055/diff/33002/common/src/web/clicks.html#n... common/src/web/clicks.html:18: Consider using common/src/web/linked_image.html instead of modifying another file (it's a relatively recent addition, which is why you may have missed it). http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/Actions.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/Actions.java:36: protected Keyboard keyboard; Thanks for refactoring this class - looks much cleaner now. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:25: */ nitpick: This is still very similar to the base class for mouse actions, consider making it a common class. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java:216: // Interactions-related commands. Please separate this to another change - it will break every driver currently implementing mouse clicks. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:71: private JsonToWebElementConverter converter; Remove this field - it belongs in the AndroidDriver class http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/TouchScreenImpl.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/TouchScreenImpl.java:24: Rename to RemoteTouchScreen - like RemoteMouse (The "Impl" at the end of the class name doesn't really hint what it does and is not consistent with the rest of the classes). http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/TouchScreenImpl.java:33: public void singleTap(Coordinates where) { nitpick: Directly call parseFromCoordinates here - since the getElementIdMap method doesn't do anything else other than that. http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/TouchSingleTapTest.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:59: driver.get(pages.clicksPage); Since this test and the one above it share exactly the same code, except for the id of the element. You can extract a common, private method, to run the entire test (and get the element id as a parameter). http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:82: assertTrue. And I'm still not a big fan of this latch - I'd rather see a visible change in the page (but that's optional). http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:88: + "operations not supported") How does that differ from the 2nd test? http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/build.desc (right): http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/build.desc:67: "//third_party/java/junit", Move this line to be with the rest of the dependencies on internal targets. http://codereview.appspot.com/4664055/diff/33002/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java (right): http://codereview.appspot.com/4664055/diff/33002/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:301: .on(ResultType.SUCCESS, new EmptyResult()); this should be in a separate CL as it will break all other drivers.
Sign in to reply to this message.
Hi, I have submitted a new patch fixing the code with the comments Eran made. Thanks! Juan. http://codereview.appspot.com/4664055/diff/33002/common/src/web/clicks.html File common/src/web/clicks.html (right): http://codereview.appspot.com/4664055/diff/33002/common/src/web/clicks.html#n... common/src/web/clicks.html:18: Removed: The test that used it was not adding too much value. On 2011/07/29 13:33:43, Eran wrote: > Consider using common/src/web/linked_image.html instead of modifying another > file (it's a relatively recent addition, which is why you may have missed it). http://codereview.appspot.com/4664055/diff/33002/common/src/web/clicks.html#n... common/src/web/clicks.html:18: On 2011/07/29 13:33:43, Eran wrote: > Consider using common/src/web/linked_image.html instead of modifying another > file (it's a relatively recent addition, which is why you may have missed it). Done. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/Actions.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/Actions.java:36: protected Keyboard keyboard; On 2011/07/29 13:33:43, Eran wrote: > Thanks for refactoring this class - looks much cleaner now. Done. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:25: */ On 2011/07/29 13:33:43, Eran wrote: > nitpick: This is still very similar to the base class for mouse actions, > consider making it a common class. Done. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/TouchRelatedAction.java:25: */ Created a new abstract class called DisplayAction to extend from. On 2011/07/29 13:33:43, Eran wrote: > nitpick: This is still very similar to the base class for mouse actions, > consider making it a common class. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:71: private JsonToWebElementConverter converter; This field seems to be used in several parts of the code, not specifically related to Android (like in the execute method - line 383). I did not add the field: Are we sure this would not break other things if we move it? On 2011/07/29 13:33:43, Eran wrote: > Remove this field - it belongs in the AndroidDriver class http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/TouchScreenImpl.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/TouchScreenImpl.java:24: On 2011/07/29 13:33:43, Eran wrote: > Rename to RemoteTouchScreen - like RemoteMouse (The "Impl" at the end of the > class name doesn't really hint what it does and is not consistent with the rest > of the classes). Done. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/TouchScreenImpl.java:33: public void singleTap(Coordinates where) { On 2011/07/29 13:33:43, Eran wrote: > nitpick: Directly call parseFromCoordinates here - since the getElementIdMap > method doesn't do anything else other than that. Done. http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/TouchSingleTapTest.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:59: driver.get(pages.clicksPage); On 2011/07/29 13:33:43, Eran wrote: > Since this test and the one above it share exactly the same code, except for the > id of the element. You can extract a common, private method, to run the entire > test (and get the element id as a parameter). Done. http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:82: Will work on that for future versions. On 2011/07/29 13:33:43, Eran wrote: > assertTrue. And I'm still not a big fan of this latch - I'd rather see a visible > change in the page (but that's optional). http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:82: On 2011/07/29 13:33:43, Eran wrote: > assertTrue. And I'm still not a big fan of this latch - I'd rather see a visible > change in the page (but that's optional). Done. http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:88: + "operations not supported") On 2011/07/29 13:33:43, Eran wrote: > How does that differ from the 2nd test? Done. http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/TouchSingleTapTest.java:88: + "operations not supported") Both reuse the tap action, so does not seem to add much value: removed. On 2011/07/29 13:33:43, Eran wrote: > How does that differ from the 2nd test? http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... File java/client/test/org/openqa/selenium/build.desc (right): http://codereview.appspot.com/4664055/diff/33002/java/client/test/org/openqa/... java/client/test/org/openqa/selenium/build.desc:67: "//third_party/java/junit", On 2011/07/29 13:33:43, Eran wrote: > Move this line to be with the rest of the dependencies on internal targets. Done. http://codereview.appspot.com/4664055/diff/33002/java/server/src/org/openqa/s... File java/server/src/org/openqa/selenium/remote/server/DriverServlet.java (right): http://codereview.appspot.com/4664055/diff/33002/java/server/src/org/openqa/s... java/server/src/org/openqa/selenium/remote/server/DriverServlet.java:301: .on(ResultType.SUCCESS, new EmptyResult()); On 2011/07/29 13:33:43, Eran wrote: > this should be in a separate CL as it will break all other drivers. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:71: private JsonToWebElementConverter converter; On 2011/07/30 00:10:15, juantrujillo wrote: > This field seems to be used in several parts of the code, not specifically > related to Android (like in the execute method - line 383). I did not add the > field: Are we sure this would not break other things if we move it? > On 2011/07/29 13:33:43, Eran wrote: > > Remove this field - it belongs in the AndroidDriver class > Sorry, commented on the wrong line. I meant the 'touch' field which was just added.
Sign in to reply to this message.
Hi, I have submitted a new patch fixing the unused variable in RemoteWebDriver. Thanks! Juan. http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java (right): http://codereview.appspot.com/4664055/diff/33002/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/remote/RemoteWebDriver.java:71: private JsonToWebElementConverter converter; On 2011/08/01 12:32:31, Eran wrote: > On 2011/07/30 00:10:15, juantrujillo wrote: > > This field seems to be used in several parts of the code, not specifically > > related to Android (like in the execute method - line 383). I did not add the > > field: Are we sure this would not break other things if we move it? > > On 2011/07/29 13:33:43, Eran wrote: > > > Remove this field - it belongs in the AndroidDriver class > > > > Sorry, commented on the wrong line. I meant the 'touch' field which was just > added. Done.
Sign in to reply to this message.
This is probably for a separate CL, but (as Simon suggested) we could have the 'session/:sessionId/mouse/click' wire protocol and have 'session/:sessionId/click' redirect there by default, but redirect to 'session/:sessionId/touch/tap' on Android, until such time when we have a mouse implementation for Android. This way a naive implementation calls 'session/:sessionId/click' and the server will redirect to the correct URL (either 'session/:sessionId/mouse/click' or 'session/:sessionId/touch/tap'). This redirect may be temporary or permanent, depending on the driver. This is supported by the HTTP client we use.
Sign in to reply to this message.
Hi Juan, Couple more comments. We're almost there! http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidCoordinates.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:34: return point; Are those 3 method supposed to return the same thing? http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:61: String elementId = (String)where.getAuxiliry(); Inline this. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:86: if (ev.equals(lastEventInSequence)) { What if the last ME is lost? Maybe add a timeout on the waiting side? http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/build.desc (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/build.desc:1: java_library(name = "internal-api", Why do you need this build file? This seems to only contain touch related file, can you rename the rule to reflect that? http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/build.desc:1: java_library(name = "internal-api", Why do you need this build file? It seems that everything here is for touch related apis, can you rename the rule to reflect that? http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/touch/build.desc (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/touch/build.desc:5: "//java/client/src/org/openqa/selenium/internal:codecs", Some of those rules seem redundant/uneeded. I haven't checked, could you ensure all of those are actually needed?
Sign in to reply to this message.
Eran, I agree this change should probably included on a separate CL. On 2011/08/04 14:25:53, Eran wrote: > This is probably for a separate CL, but (as Simon suggested) we could have the > 'session/:sessionId/mouse/click' wire protocol and have > 'session/:sessionId/click' redirect there by default, but redirect to > 'session/:sessionId/touch/tap' on Android, until such time when we have a mouse > implementation for Android. > > This way a naive implementation calls 'session/:sessionId/click' and the server > will redirect to the correct URL (either 'session/:sessionId/mouse/click' or > 'session/:sessionId/touch/tap'). This redirect may be temporary or permanent, > depending on the driver. This is supported by the HTTP client we use.
Sign in to reply to this message.
Hi, I have included some comments that I would like to verify before submitting a new patch. Thanks a lot! Juan. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidCoordinates.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:34: return point; What I understand according to the names of the methods, is that they would all return something equivalent to the location of the element. I tried searching for classes that implement Coordinates in the project and they all return the point or don't provide any values. Let me know if you see any alternative value that can be returned, but it seems to me that AndroidWebElement does not have a way to get additional coordinate attributes. On 2011/08/04 20:34:29, berrada wrote: > Are those 3 method supposed to return the same thing? http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:34: return point; What I understand according to the names of the methods, is that they would all return something equivalent to the location of the element. I tried searching for classes that implement Coordinates in the project and they all return the point or don't provide any values. Let me know if you see any alternative value that can be returned, but it seems to me that AndroidWebElement does not have a way to get additional coordinate attributes. On 2011/08/04 20:34:29, berrada wrote: > Are those 3 method supposed to return the same thing? http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:61: String elementId = (String)where.getAuxiliry(); On 2011/08/04 20:34:29, berrada wrote: > Inline this. Done. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:86: if (ev.equals(lastEventInSequence)) { I'm not sure I understand what you mean the event could get lost: Do you consider we should add the timeout to validate cases where the event was not added to the list? On 2011/08/04 20:34:29, berrada wrote: > What if the last ME is lost? Maybe add a timeout on the waiting side? http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/build.desc (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/build.desc:1: java_library(name = "internal-api", On 2011/08/04 20:34:29, berrada wrote: > Why do you need this build file? This seems to only contain touch related file, > can you rename the rule to reflect that? Done. http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/build.desc:1: java_library(name = "internal-api", Renamed to touch-api On 2011/08/04 20:34:29, berrada wrote: > Why do you need this build file? It seems that everything here is for touch > related apis, can you rename the rule to reflect that? http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/build.desc:1: java_library(name = "internal-api", On 2011/08/04 20:34:29, berrada wrote: > Why do you need this build file? It seems that everything here is for touch > related apis, can you rename the rule to reflect that? Done. http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/touch/build.desc (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/touch/build.desc:5: "//java/client/src/org/openqa/selenium/internal:codecs", On 2011/08/04 20:34:29, berrada wrote: > Some of those rules seem redundant/uneeded. I haven't checked, could you ensure > all of those are actually needed? Done.
Sign in to reply to this message.
Hi Juan, Couple more comments. I don't see the changes you made, did you forget uploading the patch? http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:61: String elementId = (String)where.getAuxiliry(); I don't see the change, did you upload the patch? On 2011/08/05 18:54:30, juantrujillo wrote: > On 2011/08/04 20:34:29, berrada wrote: > > Inline this. > > Done. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:337: return new AndroidCoordinates(elementId, pointOfLocationOfElement); Can you reuse the object instead of creating one every time? http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:86: if (ev.equals(lastEventInSequence)) { I meant adding a timeout on the waiting thread just in case this ME does not get captured by onTouchEvent. Feel free to ping me if that's not clear. On 2011/08/05 18:54:30, juantrujillo wrote: > I'm not sure I understand what you mean the event could get lost: Do you > consider we should add the timeout to validate cases where the event was not > added to the list? > On 2011/08/04 20:34:29, berrada wrote: > > What if the last ME is lost? Maybe add a timeout on the waiting side? > http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:94: Rect previouslyFocusedRect) { Fix indentation. It should be 4 spaces. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:167: WebView view, boolean dialog, boolean userGesture, Message resultMsg) { Same here and everywhere else. http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/DisplayAction.java (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/DisplayAction.java:32: if (where == null) { return (where == null) ? null : where.getCoordinates(); http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/build.desc (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/build.desc:1: java_library(name = "internal-api", I don t see the change. On 2011/08/05 18:54:30, juantrujillo wrote: > On 2011/08/04 20:34:29, berrada wrote: > > Why do you need this build file? It seems that everything here is for touch > > related apis, can you rename the rule to reflect that? > > Done.
Sign in to reply to this message.
Hi, I uploaded a new patch for single tap addressing the comments. Thanks a lot! Juan. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidCoordinates.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidCoordinates.java:34: return point; On 2011/08/04 20:34:29, berrada wrote: > Are those 3 method supposed to return the same thing? Done. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidTouchScreen.java:61: String elementId = (String)where.getAuxiliry(); On 2011/08/08 20:40:37, berrada wrote: > I don't see the change, did you upload the patch? > > On 2011/08/05 18:54:30, juantrujillo wrote: > > On 2011/08/04 20:34:29, berrada wrote: > > > Inline this. > > > > Done. > Done. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/AndroidWebElement.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/AndroidWebElement.java:337: return new AndroidCoordinates(elementId, pointOfLocationOfElement); On 2011/08/08 20:40:37, berrada wrote: > Can you reuse the object instead of creating one every time? Done. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... File android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java (right): http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:86: if (ev.equals(lastEventInSequence)) { Let's sync on this, I still consider it's not necessary. On 2011/08/08 20:40:37, berrada wrote: > I meant adding a timeout on the waiting thread just in case this ME does not get > captured by onTouchEvent. Feel free to ping me if that's not clear. > > On 2011/08/05 18:54:30, juantrujillo wrote: > > I'm not sure I understand what you mean the event could get lost: Do you > > consider we should add the timeout to validate cases where the event was not > > added to the list? > > On 2011/08/04 20:34:29, berrada wrote: > > > What if the last ME is lost? Maybe add a timeout on the waiting side? > > > http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:94: Rect previouslyFocusedRect) { On 2011/08/08 20:40:37, berrada wrote: > Fix indentation. It should be 4 spaces. Done. http://codereview.appspot.com/4664055/diff/34003/android/src/java/org/openqa/... android/src/java/org/openqa/selenium/android/app/WebDriverWebView.java:167: WebView view, boolean dialog, boolean userGesture, Message resultMsg) { IDE problem. Fixed. On 2011/08/08 20:40:37, berrada wrote: > Same here and everywhere else. http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... File java/client/src/org/openqa/selenium/interactions/internal/DisplayAction.java (right): http://codereview.appspot.com/4664055/diff/34003/java/client/src/org/openqa/s... java/client/src/org/openqa/selenium/interactions/internal/DisplayAction.java:32: if (where == null) { On 2011/08/08 20:40:37, berrada wrote: > return (where == null) ? null : where.getCoordinates(); Done.
Sign in to reply to this message.
LGTM On 2011/08/12 23:03:23, juantrujillo wrote:
Sign in to reply to this message.
|
