Hi, Dmitriy. The fix looks good. ----- [email protected]:
> My fault. > > Updated version: > http://cr.openjdk.java.net/~dermashov/8056911/webrev.05/ > > Thanks, > Dima > > On 10/30/2014 02:59 PM, Sergey Bylokhov wrote: > > 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 > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>> > >>>
