Hello, Dmirty. The new version of the fix looks good to me.
With best regards. Petr. On 31.03.2014, at 15:51, Dmitriy Ermashov <[email protected]> wrote: > Hi, > > Please, review the latest version of changeset for: > https://bugs.openjdk.java.net/browse/JDK-8038631 > > Webrev is here: > http://cr.openjdk.java.net/~yan/8038631/webrev.01/ > > > On 31.03.2014 14:15, Petr Pchelko wrote: >>>> 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. >>> We have to use small delay between mousePress and mouseRelease events to >>> emulate real mouse click event. And larger delay can be put after >>> mouseRelease call, for ex. 500 ms, as you wrote. >>> Don't you mind of this solution? >> And couldn't you reuse DEFAULT_SPEED for sleeping in mouseClick? The comment >> states it's used there but it's not.. >> >>>> 5. The click method could be more consistent with others if called >>>> "mouseClick". No strong opinion on this. >>> For now we have several hundreds of functional tests, most of them use >>> method named click(). It seems more reasonable to create method click() and >>> use it than search the usage of it in all tests and fix to mouseClick() >>> call. >> Ok, no problem. I'm fine either way. >> >> With best regards. Petr. >> >> On 31.03.2014, at 14:11, Dmitriy Ermashov <[email protected]> >> wrote: >> >>> On 31.03.2014 13:12, Petr Pchelko wrote: >>>> 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. >>> No objections. Will rename it to ExtendedRobot. >>>> 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? >>> Will fix in next version of webrev. >>>> 3. Could you please use @code instead of <code> >>> Will fix in next version of webrev. >>>> 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. >>> We have to use small delay between mousePress and mouseRelease events to >>> emulate real mouse click event. And larger delay can be put after >>> mouseRelease call, for ex. 500 ms, as you wrote. >>> Don't you mind of this solution? >>>> 5. The click method could be more consistent with others if called >>>> "mouseClick". No strong opinion on this. >>> For now we have several hundreds of functional tests, most of them use >>> method named click(). It seems more reasonable to create method click() and >>> use it than search the usage of it in all tests and fix to mouseClick() >>> call. >>>> 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 >>> I believe, javadoc should be fixed. >>> Will fix in next version of webrev. >>>> 7. Could you please add @Override annotations where appropriate >>>> (waitForIdle for example) >>> Will fix in next version of webrev. >>>> 8. "public void type" may be keyType would be better? For consistency with >>>> other methods. >>> Same as 5. >>>> 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. >>> Will fix in next version of webrev. >>>> Thank you. >>>> With best regards. Petr. >>>> >>>> On 31.03.2014, at 12:40, Dmitriy Ermashov <[email protected]> >>>> 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 >>>>> >>> -- >>> Thanks, >>> Dima >>> > > -- > Thanks, > Dima >
