<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]

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
>>>>>
>>>>
>>