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

Dmitriy Ermashov dmitriy.ermashov at oracle.com
Thu Oct 30 10:33:20 UTC 2014


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