<AWT Dev> Review Request for 8039749: Migrate needed functionality from all subclasses of java.awt.Robot in jdk/test directory to the ancestor

[Dmitriy Ermashov]

Or it could be a Toolkit.nativeEventQueueSync. It would allow us to call 
the method in a static way.

Many functional tests have a call like
((SunToolkit) Toolkit.getDefaultToolkit()).realSync()
They call realSync without instantiating a robot.

It would be great just replace it with
Toolkit.getDefaultToolkit().nativeEventQueueSync()
in existing tests and nothing more.

Thanks,
Dima

On 08/29/2014 04:35 PM, Anthony Petrov wrote:
> Yes, that sounds good to me. We need to consider whether this should 
> be Toolkit or Robot class though, and settle on the name of the new 
> method. For example, if we go with Robot, then it could be 
> Robot.waitForNativeIdle() or alike.
>
> -- 
> best regards,
> Anthony
>
> On 8/29/2014 4:24 PM, Dmitriy Ermashov wrote:
>> Hi Anthony,
>>
>> Ok, let's summarize:
>> We leave all functionality in ExtendedRobot except of realSync() call,
>> which in turn we move to a new method in java.awt.Toolkit.
>>
>> I believe it could be a reasonable compromise.
>>
>> Is it ok?
>>
>> Thanks,
>> Dima
>>
>> On 08/28/2014 10:03 PM, Anthony Petrov wrote:
>>> Hi Dmitriy,
>>>
>>> On 8/28/2014 1:14 PM, Dmitriy Ermashov wrote:
>>>> On 08/28/2014 12:09 PM, Anthony Petrov wrote:
>>>>> On 8/27/2014 4:54 PM, Yuri Nesterenko wrote:
>>>>>> On 08/27/2014 04:25 PM, Dmitriy Ermashov wrote:
>>>>>>> At first, let me focus on fact that the actual motivation of moving
>>>>>>> functionality to java.awt.Robot is the Jigsaw project. We (SQE)
>>>>>>> will be
>>>>>>> unable to use internal java API after the project will be finished
>>>>>>> (ExtendedRobot just will not compile, for example) and it will be a
>>>>>>> reason of failing huge amount of regression and functional tests.
>>>>>
>>>>> Could you please clarify as to why it won't compile? IIUC, all the 
>>>>> GUI
>>>>> tests that require Robot will be in a single module (the java.desktop
>>>>> one I suppose?), so why wouldn't ExtendedRobot compile while the 
>>>>> tests
>>>>> themselves would if they all are located in the same module?
>>>> All jtreg tests are in default package, ExtendedRobot class as well.
>>>> When we trying to run any jtreg test that uses ExtendedRobot it fails
>>>> with the following error:
>>>> java.lang.IllegalAccessError: tried to access class sun.awt.SunToolkit
>>>> from class ExtendedRobot
>>>>
>>>> As you can see the problem is not in tests that uses ExtendedRobot, 
>>>> but
>>>> in code that uses sun.* and other internal packages. This is the
>>>> limitation we are trying to deal with.
>>>
>>> Thank you for the clarification. If I read this correctly, I see that
>>> the only reason to migrate all the code from ExtendedRobot to the
>>> public Robot class is the fact that you're unable to call one internal
>>> method from SunToolkit. I really don't think this is a good
>>> justification for introducing a bunch of new public APIs that external
>>> developers never requested and that we'll have to support forever.
>>>
>>> Please find another way to call realSync() from ExtendedRobot and
>>> leave the latter alone. One suggestion that I've already proposed is
>>> to introduce a new method in Toolkit class
>>> (Toolkit.nativeEventQueueSync() ?) that would call the internal
>>> realSync() method. Or perhaps the jigsaw projects could provide some
>>> other mechanisms to access module-private APIs - please discuss this
>>> with jigsaw folks.
>>>
>>>
>>>>>>> As for waitForIdle() method, we do not change it's use-case. The
>>>>>>> java.awt.Robot class is used only for GUI actions. And
>>>>>>> waitForIdle() is
>>>>>>> used for ensuring of finishing all events in EventQueue.
>>>>>>> The implementation with realSync() just flushes native queue as 
>>>>>>> well
>>>>>>> and
>>>>>>> it is just an improved version of existing one.
>>>>>>>
>>>>>>> Anyway, the changing of waitForIdle() implementation is 
>>>>>>> discussable.
>>>>>>> The
>>>>>>> other solution may be in adding new method with realSync call.
>>>>>>
>>>>>> Which would require touching some 340 tests in apprx 950 places, 
>>>>>> sorry
>>>>>> for statistics.
>>>>>
>>>>> Yes, I realize that some changes will be needed for this. My concern
>>>>> is that having realSync() being called unconditionally from an
>>>>> existing public method may have an impact on existing applications
>>>>> that use the Robot for tasks other than testing (e.g. for simple
>>>>> automation tasks, etc.) And I'm not sure if we're able to estimate 
>>>>> all
>>>>> the potential side effects that this change can bring.
>>>> We already have a kind of condition: a variable isAutoWaitForIdle.
>>>> waitForIdle method for now is called only from inside other Robot
>>>> methods and in case when isAutoWaitForIdle flag is set to true
>>>> (fortunately it's default value is false).
>>>
>>> Robot.waitForIdle() is a public API. You don't (and can't) know who
>>> calls this method, when, and why. Changing its implementation the way
>>> you're proposing looks dangerous to me.
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>>
>>>>
>>>> Thanks,
>>>> Dima
>>>>>
>>>>> Again, if the tests go into the java.desktop module, then I don't see
>>>>> a problem with calling realSync() from sun.awt.SunToolkit directly as
>>>>> you did before. If this is not the case, then I think I'd prefer to
>>>>> introduce a separate single public method in the Toolkit (or Robot?)
>>>>> class that would allow applications (and tests) to actually perform
>>>>> the realSync() operation.
>>>>>
>>>>> -- 
>>>>> best regards,
>>>>> Anthony
>>>>>
>>>>>>
>>>>>> -yan
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Dima
>>>>>>>
>>>>>>> On 08/27/2014 03:16 PM, Anthony Petrov wrote:
>>>>>>>> Hi Dmitriy,
>>>>>>>>
>>>>>>>> While I realize that all the new methods are useful when 
>>>>>>>> writing JDK
>>>>>>>> regression tests, do you have any evidence that would suggest that
>>>>>>>> these same methods could be useful to and/or have been 
>>>>>>>> requested by
>>>>>>>> external developers? All of them look like convenient APIs and
>>>>>>>> I'm not
>>>>>>>> entirely convinced if we should ultimately add them all to our
>>>>>>>> public
>>>>>>>> Robot API. Personally, I don't see a problem with using the custom
>>>>>>>> ExtendedRobot class specifically for tests. This also helps
>>>>>>>> reduce the
>>>>>>>> JDK and JRE static footprints, btw. But then again, I'm not an SQE
>>>>>>>> engineer.
>>>>>>>>
>>>>>>>> I don't have a strong opinion on this and I'm leaving the final
>>>>>>>> decision to Sergey and other AWT team members, but I just thought
>>>>>>>> I'd
>>>>>>>> bring this up here.
>>>>>>>>
>>>>>>>> As for the implementation, I see that you're adding realSync() 
>>>>>>>> calls
>>>>>>>> in some places where they were not previously there. For example,
>>>>>>>> calling Robot.waitForIdle() before the fix would not cause a
>>>>>>>> realSync() to occur, while after your fix it does. This is a
>>>>>>>> significant change from threading and native event queue
>>>>>>>> synchronization perspectives, and I'm not sure if this should be
>>>>>>>> done.
>>>>>>>> Again, I do know that in tests it is useful to call realSync() 
>>>>>>>> here
>>>>>>>> and there, but I'm not sure we should spread the calls all over 
>>>>>>>> the
>>>>>>>> Robot implementation simply because of this reason. The calls 
>>>>>>>> may in
>>>>>>>> fact produce some unwanted side-effects for existing applications
>>>>>>>> employing the Robot (e.g. introducing unwanted delays, or 
>>>>>>>> performing
>>>>>>>> excessive synchronization while the app didn't really need it,
>>>>>>>> etc.) I
>>>>>>>> consider this change very risky.
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> best regards,
>>>>>>>> Anthony
>>>>>>>>
>>>>>>>> On 8/26/2014 5:40 PM, Dmitriy Ermashov wrote:
>>>>>>>>> Hi awt team!
>>>>>>>>>
>>>>>>>>> A few months ago we have consolidated methods required by
>>>>>>>>> functional
>>>>>>>>> and
>>>>>>>>> regression tests in ExtendedRobot class. After a period of
>>>>>>>>> extensive
>>>>>>>>> testing, it's time to migrate them to java.awt.Robot.
>>>>>>>>>
>>>>>>>>> Please review the changeset:
>>>>>>>>> http://cr.openjdk.java.net/~dermashov/8039749/webrev.00/
>>>>>>>>>
>>>>>>>>> Corresponding bug:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8039749
>>>>>>>>>
>>>>>>>>> Note that this change is an important prerequisite to a massive
>>>>>>>>> regression and functional test suites change for Jigsaw.
>>>>>>>>> As one can see the webrev contains changes in class signature. 
>>>>>>>>> The
>>>>>>>>> CCC
>>>>>>>>> request will take place after the code review.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Dima
>>>>>>>
>>>>>>
>>>>
>>