Hi Sergey,
Thanks for review!
I've started a bug to track javadoc changes:
https://bugs.openjdk.java.net/browse/JDK-8062545
Thanks,
Dima
On 10/30/2014 03:33 PM, Sergey Bylokhov wrote:
Hi, Dmitriy.
The fix looks good.
----- [email protected]:
My fault.
Updated version:
http://cr.openjdk.java.net/~dermashov/8056911/webrev.05/
Thanks,
Dima
On 10/30/2014 02:59 PM, Sergey Bylokhov wrote:
Hi, Dmitriy.
I think that System.out.println() in ExtendedRobot is unnecessary.
----- [email protected]:
Hi Sergey,
Please review the updated version:
http://cr.openjdk.java.net/~dermashov/8056911/webrev.04
Additional bug for changing javadoc will be created before pushing
the
changes.
Thanks,
Dima
On 10/29/2014 06:52 PM, Sergey Bylokhov wrote:
Hi, Dmitriy.
Can you create a new CR for javadoc update and assign it to me.
In this review change the code only. Note that you should
preserve
an
old version of waitForIdle on osx if you filter out the new one.
On 16.10.2014 16:27, Dmitriy Ermashov wrote:
Hi,
Just a reminder. Please review the fix:
http://cr.openjdk.java.net/~dermashov/8056911/webrev.03/
Thanks,
Dima
On 10/01/2014 02:35 PM, Dmitriy Ermashov wrote:
Hi,
Just a kindly reminder.
Please review the changes to java.awt.Robot.
http://cr.openjdk.java.net/~dermashov/8056911/webrev.03/
Thanks,
Dima
On 09/26/2014 06:19 PM, Dmitriy Ermashov wrote:
Hi all,
Please review the updated version of changes in java.awt.Robot
class:
http://cr.openjdk.java.net/~dermashov/8056911/webrev.03/
We have an opened bug against realSync method usage on OS X
[1]
and
the fix due date is not clear now.
The possible solution could be "we should not use realSync()
on
OS
X until the bug fixed".
Thanks,
Dima
[1] https://bugs.openjdk.java.net/browse/JDK-8003533
On 09/23/2014 01:25 PM, Dmitriy Ermashov wrote:
Hi everyone!
Here is the updated version of changeset
Please review the webrev:
http://cr.openjdk.java.net/~dermashov/8056911/webrev.02/
Let me notice that the changes are not so harmful: no new
API,
small changes in javadoc (maybe no changes in javadoc are
required
at all).
The additional delay we have in ExtendedRobot class may (and
will)
conflict with existing autoDelay in Robot class, so It's
better
to
leave it where it is. Other useful methods are also stay in
ExtendedRobot.
Thanks,
Dima
On 09/10/2014 04:57 PM, Yuri Nesterenko wrote:
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
