<AWT Dev> Review request for JDK-8038631: Create wrapper for awt.Robot with additional functionality

[Petr Pchelko]

Hello, Dmitry.

Some comments:

1. The name RobotWrapper does not fit in my opinion. The "Wrapper" is more useful for delegation not extension and it feels like you should pass the normal Robot to the wrapper constructor.. I think something like "ExtendedRobot" would feel better. This is my personal preference so it's arguable.

2. I'm not sure I understand why is this location of the util works? in JavaDoc you state that it should be included from regtesthelpers but you put it to the test lib. Is it some feature of jtreg I'm not aware of?

3. Could you please use @code instead of <code>

4. The default value for realSync additional delay doesn't seem enough. We normally use 500 ms as the frame creation and first draw could take more time. 

5. The click method could be more consistent with others if called "mouseClick". No strong opinion on this.

6. "Clicks mouse button 1 with the default pause between Press and Release." this statement does not seem clear. How to override the default value? Why does this method click with the default pause and the previous one does not? or does it? It's not clear from the JavaDoc.

7. Could you please add @Override annotations where appropriate (waitForIdle for example)

8. "public void type" may be keyType would be better? For consistency with other methods.

9. Could the method you are using for converting KeyChar to KeyCode be replaces with KeyEvent.getExtendedKeyCodeForChar? Or KeyStroke API? This switch is ugly and it would be awesome to replace it.

Thank you.
With best regards. Petr.

On 31.03.2014, at 12:40, Dmitriy Ermashov <dmitriy.ermashov at oracle.com> wrote:

> Hi,
> Please, review the changeset for:
> https://bugs.openjdk.java.net/browse/JDK-8038631
> 
> Webrev is here:
> http://cr.openjdk.java.net/~yan/8038631/webrev.00
> 
> Presently for testing of JDK client we use test suites of two kinds, historically called regression and functional (internal). In JDK9 we plan an attempt to unify them and ultimately get rid of the functional suites.
> 
> One of the first technical problems in this refactoring attempt is a multitude of the java.awt.Robot wrappers. There are some really elaborate libraries enhansing Robot which all we cannot and should not port to jtreg.
> 
> Fortunately, test writers almost never use complex features of these wrappers. So here's our plan:
> (1) describe the real practice of the robot use in the functional tests (please don't worry, that's out of scope of this request);
> (2) write a minimal useful RobotWrapper extending java.awt.Robot;
> (3) start functional tests refactoring;
> (4) cautiously move enhancements to the java.awt.Robot trying not to break backward compatibility of thousands existing tests. For instance, waitForIdle should use realSync() but there definitely
> are plenty of tests using it on EDT: or maybe not -- we should spent some time for background research in parallel with (3).
> 
> -- 
> Thanks,
> Dima
>