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

Dmitriy Ermashov dmitriy.ermashov at oracle.com
Wed Oct 1 10:35:55 UTC 2014


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