Hi, Dmitriy. I think that System.out.println() in ExtendedRobot is unnecessary.
----- [email protected]: > Hi Sergey, > > Please review the updated version: > http://cr.openjdk.java.net/~dermashov/8056911/webrev.04 > > Additional bug for changing javadoc will be created before pushing the > > changes. > > Thanks, > Dima > > On 10/29/2014 06:52 PM, Sergey Bylokhov wrote: > > Hi, Dmitriy. > > Can you create a new CR for javadoc update and assign it to me. > > In this review change the code only. Note that you should preserve > an > > old version of waitForIdle on osx if you filter out the new one. > > > > On 16.10.2014 16:27, Dmitriy Ermashov wrote: > >> Hi, > >> > >> Just a reminder. Please review the fix: > >> http://cr.openjdk.java.net/~dermashov/8056911/webrev.03/ > >> > >> Thanks, > >> Dima > >> > >> On 10/01/2014 02:35 PM, Dmitriy Ermashov wrote: > >>> Hi, > >>> > >>> Just a kindly reminder. > >>> Please review the changes to java.awt.Robot. > >>> http://cr.openjdk.java.net/~dermashov/8056911/webrev.03/ > >>> > >>> Thanks, > >>> Dima > >>> > >>> On 09/26/2014 06:19 PM, Dmitriy Ermashov wrote: > >>>> Hi all, > >>>> > >>>> Please review the updated version of changes in java.awt.Robot > class: > >>>> http://cr.openjdk.java.net/~dermashov/8056911/webrev.03/ > >>>> > >>>> We have an opened bug against realSync method usage on OS X [1] > and > >>>> the fix due date is not clear now. > >>>> The possible solution could be "we should not use realSync() on > OS > >>>> X until the bug fixed". > >>>> > >>>> Thanks, > >>>> Dima > >>>> > >>>> [1] https://bugs.openjdk.java.net/browse/JDK-8003533 > >>>> > >>>> > >>>> On 09/23/2014 01:25 PM, Dmitriy Ermashov wrote: > >>>>> Hi everyone! > >>>>> > >>>>> Here is the updated version of changeset > >>>>> Please review the webrev: > >>>>> http://cr.openjdk.java.net/~dermashov/8056911/webrev.02/ > >>>>> > >>>>> Let me notice that the changes are not so harmful: no new API, > >>>>> small changes in javadoc (maybe no changes in javadoc are > required > >>>>> at all). > >>>>> The additional delay we have in ExtendedRobot class may (and > will) > >>>>> conflict with existing autoDelay in Robot class, so It's better > to > >>>>> leave it where it is. Other useful methods are also stay in > >>>>> ExtendedRobot. > >>>>> > >>>>> Thanks, > >>>>> Dima > >>>>> > >>>>> On 09/10/2014 04:57 PM, Yuri Nesterenko wrote: > >>>>>> OK, it's not a big problem indeed. For existing 80 or something > > >>>>>> tests > >>>>>> it's just one extra line, and we have to do it manually > anyway. > >>>>>> And there will be no new name in this case, no confusion, > >>>>>> and no javadoc. > >>>>>> Let's drop this idea. > >>>>>> > >>>>>> -yan > >>>>>> > >>>>>> > >>>>>> On 09/10/2014 04:29 PM, Anthony Petrov wrote: > >>>>>>> Yes, an instance method would look even better. It's up to SQE > > >>>>>>> to agree > >>>>>>> with this proposal. I'm fine either way as long as the method > has a > >>>>>>> proper name. > >>>>>>> > >>>>>>> -- > >>>>>>> best regards, > >>>>>>> Anthony > >>>>>>> > >>>>>>> On 9/10/2014 4:24 PM, Sergey Bylokhov wrote: > >>>>>>>> On 10.09.2014 16:00, Anthony Petrov wrote: > >>>>>>>>> Taking into account Sergey's willingness to accept the risk > of > >>>>>>>>> modifying the existing Robot.waitForIdle(void) method, the > >>>>>>>>> proposal > >>>>>>>>> sounds good to me in general. > >>>>>>>>> > >>>>>>>>> The only concern that I have is the name of the new static > >>>>>>>>> method. > >>>>>>>>> Plain Robot.sync() could simply be confused with > >>>>>>>>> Toolkit.sync(), and > >>>>>>>>> they're different. IMO, it would be better to name it > >>>>>>>>> nativeSync(), or > >>>>>>>>> syncNativeEventQueue(), or alike, in order to avoid any > >>>>>>>>> confusion with > >>>>>>>>> the existing method that only operates on the Java event > queue. > >>>>>>>> It is questionable, why methods like waitForIdle() was not > >>>>>>>> static from > >>>>>>>> the beginning(note it is synchronized on the robot object), > and > >>>>>>>> should > >>>>>>>> we add a new static methods now? Probably creation of the > Robot > >>>>>>>> is not a > >>>>>>>> big problem? > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> best regards, > >>>>>>>>> Anthony > >>>>>>>>> > >>>>>>>>> On 9/10/2014 3:45 PM, Yuri Nesterenko wrote: > >>>>>>>>>> So it will be in Robot. > >>>>>>>>>> (1) Now, we have some 82 tests calling the static > realSync() > >>>>>>>>>> without instantiating Robot. And why not? > >>>>>>>>>> > >>>>>>>>>> (2) ExtendedRobot will stay. Now it has waitForIdle(void) > >>>>>>>>>> overridden > >>>>>>>>>> to call its own waitForIdle(defaultDelay). This delay is > >>>>>>>>>> distinct from autoDelay defined in Robot. It is explicit > >>>>>>>>>> and in use not after every event but with every native sync > > >>>>>>>>>> call. > >>>>>>>>>> So we need it, I'm sorry. > >>>>>>>>>> If we just update waitForIdle(), our change in > >>>>>>>>>> ExtendedRobot would be, apparently, > >>>>>>>>>> to left waitForIdle(void) overridden and > >>>>>>>>>> waitForIdle(int) to call super.waitForIdle(). > >>>>>>>>>> Looks less than perfect. > >>>>>>>>>> > >>>>>>>>>> I'd propose such a change in Robot, ideally, in principle: > >>>>>>>>>> (1) add a new public waitForIdle(int syncDelay) just > >>>>>>>>>> like in ExtendedRobot; > >>>>>>>>>> (2) add a couple of public helper methods to handle this > >>>>>>>>>> syncDelay > >>>>>>>>>> (3) update javadoc for waitForIdle(void) and make its > >>>>>>>>>> functionality > >>>>>>>>>> similar to that in ExtendedRobot -- which is almost the > >>>>>>>>>> same as Sergey suggests > >>>>>>>>>> (4) Now, add a static method like Robot.sync() (?) for > tests not > >>>>>>>>>> requiring interaction. > >>>>>>>>>> > >>>>>>>>>> Please, if you agree or disagree in principle, let us > know. > >>>>>>>>>> Dmitriy will prepare the change when he is back, and I'll > be > >>>>>>>>>> ready to update the tests. > >>>>>>>>>> > >>>>>>>>>> Thank you, > >>>>>>>>>> -yan > >>>>>>>>>> > >>>>>>>>>> On 09/09/2014 11:31 PM, Phil Race wrote: > >>>>>>>>>>> I lost track of this thread. If testing really needs new > API > >>>>>>>>>>> - and Yuri, I fully understand the jigsaw issue - > >>>>>>>>>>> then I think Robot is a more appropriate place for it than > > >>>>>>>>>>> Toolkit, > >>>>>>>>>>> given that testing is a primary use case for Robot. > >>>>>>>>>>> > >>>>>>>>>>> -phil. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> On 9/9/14 5:33 AM, Anthony Petrov wrote: > >>>>>>>>>>>> Well, if you want to take the risk, let's do it this way > then. > >>>>>>>>>>>> > >>>>>>>>>>>> -- > >>>>>>>>>>>> best regards, > >>>>>>>>>>>> Anthony > >>>>>>>>>>>> > >>>>>>>>>>>> On 9/9/2014 3:28 PM, Sergey Bylokhov wrote: > >>>>>>>>>>>>> On 09.09.2014 15:15, Anthony Petrov wrote: > >>>>>>>>>>>>>> and the current waitForIdle() would simply call > >>>>>>>>>>>>>> waitForIdle(false). > >>>>>>>>>>>>>> This would be safe enough and elegant enough as no new > > >>>>>>>>>>>>>> method names > >>>>>>>>>>>>>> would have to be introduced. > >>>>>>>>>>>>> I suggest to take this as a backup plan if something > will go > >>>>>>>>>>>>> wrong. We > >>>>>>>>>>>>> have a good coverage and we have enough time for > testing. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> -- > >>>>>>>>>>>>>> best regards, > >>>>>>>>>>>>>> Anthony > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On 9/9/2014 3:07 PM, Sergey Bylokhov wrote: > >>>>>>>>>>>>>>> On 09.09.2014 14:40, Anthony Petrov wrote: > >>>>>>>>>>>>>>>> Hi Dmitriy, > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Robot.sync() could also sound confusing if one > >>>>>>>>>>>>>>>> remembers there's > >>>>>>>>>>>>>>>> Toolkit.sync() already. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Why not name it waitForNativeIdle(), analogous to the > > >>>>>>>>>>>>>>>> already > >>>>>>>>>>>>>>>> existing > >>>>>>>>>>>>>>>> Robot.waitForIdle()? The methods perform similar > >>>>>>>>>>>>>>>> actions, the > >>>>>>>>>>>>>>>> only > >>>>>>>>>>>>>>>> difference is the event queue they perform these > >>>>>>>>>>>>>>>> actions on. > >>>>>>>>>>>>>>>> If we > >>>>>>>>>>>>>>>> choose this name, then the javadoc could be copied > almost > >>>>>>>>>>>>>>>> verbatim > >>>>>>>>>>>>>>>> from the waitForIdle() method with a few additional > >>>>>>>>>>>>>>>> "native" > >>>>>>>>>>>>>>>> words > >>>>>>>>>>>>>>>> inserted here and there. > >>>>>>>>>>>>>>> The simpler way to fix it is to reimplement > >>>>>>>>>>>>>>> Robot.waitForIdle() on > >>>>>>>>>>>>>>> top > >>>>>>>>>>>>>>> of SunToolkit.realsync() and extends of documentation > of > >>>>>>>>>>>>>>> this > >>>>>>>>>>>>>>> method. > >>>>>>>>>>>>>>> Because most of the time waitForIdle/autoWaitForIdle > are > >>>>>>>>>>>>>>> used > >>>>>>>>>>>>>>> after > >>>>>>>>>>>>>>> some > >>>>>>>>>>>>>>> key/mouse manipulation, I guess it is too verbose to > >>>>>>>>>>>>>>> write this: > >>>>>>>>>>>>>>> Robot.waitForNativeIdle() > >>>>>>>>>>>>>>> Robot.waitForIdle() > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> In general, I'm fine with this approach of resolving > > >>>>>>>>>>>>>>>> this issue. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> -- > >>>>>>>>>>>>>>>> best regards, > >>>>>>>>>>>>>>>> Anthony > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On 9/4/2014 3:25 PM, Dmitriy Ermashov wrote: > >>>>>>>>>>>>>>>>> Ok, I've prepared an updated version of patch > >>>>>>>>>>>>>>>>> > http://cr.openjdk.java.net/~dermashov/8056911/webrev.01/ > >>>>>>>>>>>>>>>>> Please, review new version. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> I've moved a method implementation to Robot class. > >>>>>>>>>>>>>>>>> We've decided > >>>>>>>>>>>>>>>>> that > >>>>>>>>>>>>>>>>> adding new method to Toolkit class with similar to > >>>>>>>>>>>>>>>>> sync() method > >>>>>>>>>>>>>>>>> functionality but with a different name could hardly > look > >>>>>>>>>>>>>>>>> elegant, and > >>>>>>>>>>>>>>>>> besides, new solution could be less conspicuous. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>>> Dima > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> On 09/01/2014 04:49 PM, Yuri Nesterenko wrote: > >>>>>>>>>>>>>>>>>> Phil, > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> perhaps javadoc should be changed, yes. > >>>>>>>>>>>>>>>>>> It is the first public spec for Dmitriy. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> As to the narrow purpose, are you serious? > >>>>>>>>>>>>>>>>>> We have nothing to replace this invention. Original > idea > >>>>>>>>>>>>>>>>>> was to replace Toolkit.sync() with realSync() but > >>>>>>>>>>>>>>>>>> after JDK-6252005 Denis left us, there was no > resources > >>>>>>>>>>>>>>>>>> and no hard demand to avoid the internal API. > >>>>>>>>>>>>>>>>>> Now it is here. Demand is here, not resources. Are > > >>>>>>>>>>>>>>>>>> you going > >>>>>>>>>>>>>>>>>> to design and implement something new to help SQE? > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> -yan > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> On 08/29/2014 07:33 PM, Phil Race wrote: > >>>>>>>>>>>>>>>>>>> So you are proposing adding a new *public* API for > > >>>>>>>>>>>>>>>>>>> this narrow > >>>>>>>>>>>>>>>>>>> purpose. > >>>>>>>>>>>>>>>>>>> And it calls out a whole bunch of internal classes > > >>>>>>>>>>>>>>>>>>> in its > >>>>>>>>>>>>>>>>>>> apI doc > >>>>>>>>>>>>>>>>>>> !?! > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> 2642 * <p> The method calls {@link > >>>>>>>>>>>>>>>>>>> sun.awt.SunToolkit#realSync} to > >>>>>>>>>>>>>>>>>>> 2643 * sync with native event queue > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> @throws sun.awt.SunToolkit.IllegalThreadException > if > >>>>>>>>>>>>>>>>>>> called on > >>>>>>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>>> AWT event > >>>>>>>>>>>>>>>>>>> 2646 * dispatching thread > >>>>>>>>>>>>>>>>>>> 2647 * @throws > >>>>>>>>>>>>>>>>>>> sun.awt.SunToolkit.OperationTimedOut if > >>>>>>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>>> 2648 * {@link > >>>>>>>>>>>>>>>>>>> sun.awt.SunToolkit.OperationTimedOut} > >>>>>>>>>>>>>>>>>>> exception occurs in > >>>>>>>>>>>>>>>>>>> 2649 * {@link > >>>>>>>>>>>>>>>>>>> sun.awt.SunToolkit#realSync} > >>>>>>>>>>>>>>>>>>> 2650 * @throws > sun.awt.SunToolkit.InfiniteLoop > >>>>>>>>>>>>>>>>>>> if the > >>>>>>>>>>>>>>>>>>> 2651 * {@link > >>>>>>>>>>>>>>>>>>> sun.awt.SunToolkit.InfiniteLoop} > >>>>>>>>>>>>>>>>>>> exception occurs in > >>>>>>>>>>>>>>>>>>> 2652 * {@link > >>>>>>>>>>>>>>>>>>> sun.awt.SunToolkit#realSync} > >>>>>>>>>>>>>>>>>>> 2653 * @throws ClassCastException if default > > >>>>>>>>>>>>>>>>>>> toolkit > >>>>>>>>>>>>>>>>>>> is not > >>>>>>>>>>>>>>>>>>> SunToolkit > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> I am also not sure how confident I am that the > >>>>>>>>>>>>>>>>>>> statement > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> his method guarantees that after > >>>>>>>>>>>>>>>>>>> 2637 * return no additional Java events will > be > >>>>>>>>>>>>>>>>>>> generated, > >>>>>>>>>>>>>>>>>>> unless > >>>>>>>>>>>>>>>>>>> 2638 * cause by user. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> is actually guaranteed. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> My initial reaction to such a proposal is instead > > >>>>>>>>>>>>>>>>>>> look hard > >>>>>>>>>>>>>>>>>>> for a > >>>>>>>>>>>>>>>>>>> better > >>>>>>>>>>>>>>>>>>> way even if it means re-writing all the tests. > >>>>>>>>>>>>>>>>>>> There's also the proposed 'jdk.*' name space that > > >>>>>>>>>>>>>>>>>>> can be > >>>>>>>>>>>>>>>>>>> considered > >>>>>>>>>>>>>>>>>>> but re-writing the tests would be better. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> -phil. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> On 8/29/14 8:11 AM, Dmitriy Ermashov wrote: > >>>>>>>>>>>>>>>>>>>> Hi awt team, > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Please review a fix for 8056911, remove internal > > >>>>>>>>>>>>>>>>>>>> API usage > >>>>>>>>>>>>>>>>>>>> from > >>>>>>>>>>>>>>>>>>>> ExtendedRobot class. > >>>>>>>>>>>>>>>>>>>> We have to throw out all calls of sun.* packages > > >>>>>>>>>>>>>>>>>>>> from tests > >>>>>>>>>>>>>>>>>>>> because of > >>>>>>>>>>>>>>>>>>>> incompatibility with Jigsaw project. > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > http://cr.openjdk.java.net/~dermashov/8056911/webrev.00/ > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> The CCC request will take place after the review > > >>>>>>>>>>>>>>>>>>>> process > >>>>>>>>>>>>>>>>>>>> will be > >>>>>>>>>>>>>>>>>>>> completed. > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>>>>>> Dima > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > > >
