<AWT Dev> Review Request for 8056911: Remove internal API usage from ExtendedRobot class
Yuri Nesterenko
yuri.nesterenko at oracle.com
Wed Sep 10 12:57:51 UTC 2014
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