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

Dmitriy Ermashov dmitriy.ermashov at oracle.com
Thu Oct 30 12:49:04 UTC 2014 

Hi Sergey,

Thanks for review!
I've started a bug to track javadoc changes:
https://bugs.openjdk.java.net/browse/JDK-8062545

Thanks,
Dima

On 10/30/2014 03:33 PM, Sergey Bylokhov wrote:
> 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

More information about the awt-dev mailing list