<AWT Dev> Review Request for 8056911: Remove internal API usage from ExtendedRobot class

Thu Oct 30 12:33:57 UTC 2014

Hi, Dmitriy.
The fix looks good.

----- dmitriy.ermashov at oracle.com:

> 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.
> >
> > ----- dmitriy.ermashov at oracle.com:
> >
> >> 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
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>
> >>>