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

[Anthony Petrov]

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