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

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Wed Oct 29 15:52:41 UTC 2014 

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
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>


-- 
Best regards, Sergey.

More information about the awt-dev mailing list