<AWT Dev> Review Request for 8056911: Remove internal API usage from ExtendedRobot class
Dmitriy Ermashov
dmitriy.ermashov at oracle.com
Thu Oct 16 12:27:07 UTC 2014
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