Hi Andrew, Man,
Sorry for the delay, I was on vacation.
Andrew John Hughes wrote:
2009/9/23 Man Wong <[email protected]>:
Hi,
Webrev: http://icedtea.classpath.org/~mwong/webrevs/index.html
Summary: This is about jtreg bug in openjdk7, TestDialogTypeAhead
({openjdkdir}/jdk/test/java/awt/KeyboardFocusManager/TypeAhead
/TestDialogTypeAhead.java). It relates to bug ID 6446952, which in the
test itself includes a workaround and that causes it to fail in fedora
10/11. After removing the work around, as in my webrev, the test
passes. Does the fix look ok to push to the awt forest? Any comments?
Thanks.
Man Lung Wong
Ping!
Can someone from the AWT team please respond on this?
The patch removes a hack used to workaround bug 6446952, which
(apparently) occurs sporadically on some Windows platforms. According
to the bug report
(http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6446952), the bug
itself has been fixed in 'mustang b93', whatever that is, which
implies the hack is probably no longer needed even where 6446952 was
evidenced. What we do know from our testing is that this hack causes
the test to fail consistently on GNU/Linux platforms, and removing it
causes it to succeed.
Does this patch look ok to push to the AWT forest?
The patch is not Ok. It was meant to roll back the fix for 6446952, however the patch just
eliminates the test condition, that's why the test passes.
The idea of the test is to check that a character (SPACE, in the test) typed
ahead is properly
delivered to the dialog's "Ok" button after it gets focused. "Typed ahead"
means that the key event
is posted to the event queue in the process of showing a dialog, before focus is set on
the "Ok"
button.
The fix for 6446952 aimed at recreating such a scenario in order to make the key event
"typed head".
Without the fix there was no guarantee the test followed the scenario because
of timings.
If you look carefully at the changes for 6446952, you would notice that the main point of the fix
was to move the robotSema.raise() call from the frame's "b" button action listener (line 98) to the
dialog peer proxy (line 296).
With your changes robotSeam.raise() is not called at all. So, robotSema just
waits for
1000 ms and only then the type-ahead event is generated:
166 robot.keyPress(KeyEvent.VK_SPACE); <-- pressing the
"b" button
167 robot.keyRelease(KeyEvent.VK_SPACE);
168 try {
169 robotSema.doWait(1000);
170 } catch (InterruptedException ie) {
171 throw new RuntimeException("Interrupted!");
172 }
173 robot.keyPress(KeyEvent.VK_SPACE); <-- the
type-ahead event
174 robot.keyRelease(KeyEvent.VK_SPACE);
During that period of time the dialog is rather already shown and focused. In
that case the event
is not a type-ahead event and the test doesn't do its job.
Well, I reproduced the failure on Linux. This is ClassCastException caused by
changes in AWT made
after the fix for 6446952. Below is a fix that recreates the test scenario
without using a proxy.
The idea is as follows. A custom KeyboardFocusManager is set right before the
dialog is shown.
It overrides enqueueKeyEvents(long, Component) method in order to raise the
robotSema semaphore
that triggers typing ahead. This method must be called in the process of showing a dialog (see
Dialog.conditionalShow(..) for details), it initiates queuing key events until the target Component
gets focused (the "Ok" button in our case). We tie to this method in order to
catch the moment.
Well, this is a tricky test condition that can not be recreated straight
forward...
You may file an OpenJDK bugzilla bug (if it's not yet filed) and then test &
submit the changes
below. (Please feel free to ask questions.)
Thanks,
Anton.
diff --git a/test/java/awt/KeyboardFocusmanager/TypeAhead/TestDialogTypeAhead.java
b/test/java/awt/KeyboardFocusmanager/TypeAhead/TestDialogTypeAhead.java
--- a/test/java/awt/KeyboardFocusmanager/TypeAhead/TestDialogTypeAhead.java
+++ b/test/java/awt/KeyboardFocusmanager/TypeAhead/TestDialogTypeAhead.java
@@ -52,8 +52,6 @@ import java.awt.*;
import java.awt.*;
import java.lang.reflect.InvocationTargetException;
import java.awt.event.*;
-import java.awt.peer.DialogPeer;
-import java.awt.peer.ComponentPeer;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.lang.reflect.InvocationHandler;
@@ -98,7 +96,7 @@ public class TestDialogTypeAhead extends
f = new Frame("frame");
b = new Button("press");
- d = new TestDialog(f, "dialog", true, robotSema);
+ d = new Dialog(f, "dialog", true);
ok = new Button("ok");
d.add(ok);
d.pack();
@@ -126,6 +124,9 @@ public class TestDialogTypeAhead extends
b.addActionListener(new ActionListener() {
public void actionPerformed(ActionEvent e) {
System.err.println("B pressed");
+
+ KeyboardFocusManager.setCurrentKeyboardFocusManager(
+ new TestKFM(robotSema));
EventQueue.invokeLater(new Runnable() {
public void run() {
@@ -170,6 +171,11 @@ public class TestDialogTypeAhead extends
} catch (InterruptedException ie) {
throw new RuntimeException("Interrupted!");
}
+ if (!robotSema.getState()) {
+ throw new RuntimeException("robotSema hasn't been triggered");
+ }
+
+ System.err.println("typing ahead");
robot.keyPress(KeyEvent.VK_SPACE);
robot.keyRelease(KeyEvent.VK_SPACE);
waitForIdle();
@@ -278,66 +284,18 @@ public class TestDialogTypeAhead extends
}
}
- // Fix for 6446952.
- // In the process of showing the dialog we have to catch peer.show() call
- // so that to trigger key events just before it gets invoked.
- // We base on the fact that a modal dialog sets type-ahead markers
- // before it calls 'show' on the peer.
- // Posting the key events before dialog.setVisible(true) would be actually
not
- // good because it would be Ok to dispatch them to the current focus owner,
- // not to the dialog.
- class TestDialog extends Dialog {
- ComponentPeer origDialogPeer;
- ComponentPeer proxyInstPeer;
+ static class TestKFM extends DefaultKeyboardFocusManager {
Semaphore trigger;
- TestDialog(Frame owner, String title, boolean modal, Semaphore
trigger) {
- super(owner, title, modal);
+ public TestKFM(Semaphore trigger) {
this.trigger = trigger;
}
- public ComponentPeer getPeer() {
- ComponentPeer ret = super.getPeer();
- if (ret == proxyInstPeer) {
- return origDialogPeer;
- } else {
- return ret;
- }
- }
- public void addNotify() {
- super.addNotify();
- replacePeer();
- }
-
- void replacePeer() {
- origDialogPeer = getPeer();
-
- InvocationHandler handler = new InvocationHandler() {
- public Object invoke(Object proxy, Method method, Object[]
args) {
- if (method.getName() == "show") {
- trigger.raise();
- }
-
- Object ret = null;
- try {
- ret = method.invoke(origDialogPeer, args);
- } catch (IllegalAccessException iae) {
- throw new Error("Test error.", iae);
- } catch (InvocationTargetException ita) {
- throw new Error("Test error.", ita);
- }
- return ret;
- }
- };
-
- proxyInstPeer = (DialogPeer)Proxy.newProxyInstance(
- DialogPeer.class.getClassLoader(), new Class[]
{DialogPeer.class}, handler);
-
- try {
- Util.getField(Component.class, "peer").set(d, proxyInstPeer);
- } catch (IllegalAccessException iae) {
- throw new Error("Test error.", iae);
- }
+ protected synchronized void enqueueKeyEvents(long after,
+ Component untilFocused)
+ {
+ super.enqueueKeyEvents(after, untilFocused);
+ trigger.raise();
}
}
}// class TestDialogTypeAhead
