<AWT Dev> Review Request for 8056911: Remove internal API usage from ExtendedRobot class
Dmitriy Ermashov
dmitriy.ermashov at oracle.com
Fri Sep 26 14:19:33 UTC 2014
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