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

Yeah, that looks good to me too.

--
best regards,
Anthony

On 8/29/2014 4:52 PM, Dmitriy Ermashov wrote:
> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>
>