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

Anthony Petrov anthony.petrov at oracle.com
Wed Sep 10 12:29:38 UTC 2014 

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