<AWT Dev> Review Request for 8056911: Remove internal API usage from ExtendedRobot class
Dmitriy Ermashov
dmitriy.ermashov at oracle.com
Thu Oct 30 12:14:31 UTC 2014
My fault.
Updated version:
http://cr.openjdk.java.net/~dermashov/8056911/webrev.05/
Thanks,
Dima
On 10/30/2014 02:59 PM, Sergey Bylokhov wrote:
> Hi, Dmitriy.
> I think that System.out.println() in ExtendedRobot is unnecessary.
>
> ----- dmitriy.ermashov at oracle.com:
>
>> Hi Sergey,
>>
>> Please review the updated version:
>> http://cr.openjdk.java.net/~dermashov/8056911/webrev.04
>>
>> Additional bug for changing javadoc will be created before pushing the
>>
>> changes.
>>
>> Thanks,
>> Dima
>>
>> On 10/29/2014 06:52 PM, Sergey Bylokhov wrote:
>>> 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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>
>>>
More information about the awt-dev
mailing list