<AWT Dev> Review Request for 8056911: Remove internal API usage from ExtendedRobot class
Sergey Bylokhov
sergey.bylokhov at oracle.com
Thu Oct 30 11:59:09 UTC 2014
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
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
> >
More information about the awt-dev
mailing list