code review: issues_3166_3167_gwt16_r4353.patch

Ray,

Would you mind reviewing this (or asking someone else to)? It implements Event.trigger*Event(), and the associated browser-specific implementations. It also includes some cruft to fix a GWTTestCase bug, which you need not review (I asked Scott to on another thread), but which is necessary to test this code.

Branch: /releases/1.6
Revision: 4253
Affected Files:
M      user/test/com/google/gwt/junit/client/TestManualAsync.java
A      user/test/com/google/gwt/user/client/ui/TriggerEventTest.java
M      user/test/com/google/gwt/user/client/ui/CustomButtonTest.java
M      user/src/com/google/gwt/user/client/DOM.java
M      user/src/com/google/gwt/user/client/impl/DOMImpl.java
M      user/src/com/google/gwt/user/client/impl/DOMImplIE6.java
M      user/src/com/google/gwt/user/client/impl/DOMImplStandard.java
M      user/src/com/google/gwt/user/client/Event.java
M      user/src/com/google/gwt/user/client/ui/CustomButton.java
M      user/src/com/google/gwt/user/client/ui/Image.java
M      user/super/com/google/gwt/junit/translatable/com/google/gwt/junit/client/GWTTestCase.java

Thanks,
joel.

[rjrjr, 2008-12-05 00:13:15]

Nice! Some feedback below, largely on the exquisite test.

http://codereview.appspot.com/8894/diff/1/9
File user/src/com/google/gwt/user/client/Event.java (right):

http://codereview.appspot.com/8894/diff/1/9#newcode287
Line 287: * @param detail the event's detail property
Any example of valid values of detail?

http://codereview.appspot.com/8894/diff/1/9#newcode354
Line 354: * @param button the event's button property
Can you include a pointer to the BUTTON_* constants, like "values from {@link
#BUTTON_LEFT} et al"?

http://codereview.appspot.com/8894/diff/1/7
File user/src/com/google/gwt/user/client/impl/DOMImplIE6.java (right):

http://codereview.appspot.com/8894/diff/1/7#newcode230
Line 230: evt.relatedTarget = relatedTarget;
(Pretending I know what I'm talking about.) relatedTarget is a real field in
correct browsers, right? Is it a bit confusing to use it here in IE, where it
normally doesn't exist? Could we call this gwtTarget instead?

http://codereview.appspot.com/8894/diff/1/10
File user/src/com/google/gwt/user/client/ui/CustomButton.java (right):

http://codereview.appspot.com/8894/diff/1/10#newcode746
Line 746: // TODO(jgw): fill in these parameters somehow.
If this is actually possible, could you file an issue on it and point to the
issue here? Also, do we usually put our initials in our TODOs? (I hope so, but
thought I got scolded for doing so.)

http://codereview.appspot.com/8894/diff/1/11
File user/src/com/google/gwt/user/client/ui/Image.java (right):

http://codereview.appspot.com/8894/diff/1/11#newcode209
Line 209: Event.triggerLoadEvent(image.getElement());
Yes!

http://codereview.appspot.com/8894/diff/1/4
File user/test/com/google/gwt/user/client/ui/CustomButtonTest.java (right):

http://codereview.appspot.com/8894/diff/1/4#newcode162
Line 162: public void testSyntheticClick() {
Yes!!

http://codereview.appspot.com/8894/diff/1/4#newcode178
Line 178: assertEquals("Expecting one click event", "click", events.get(0));
I guess you're doing all three (over, down, up) b/c triggerClickEvent can't be
relied upon to do so? You might document that.

http://codereview.appspot.com/8894/diff/1/3
File user/test/com/google/gwt/user/client/ui/TriggerEventTest.java (right):

http://codereview.appspot.com/8894/diff/1/3#newcode30
Line 30: public class TriggerEventTest extends GWTTestCase {
A thing of beauty. I have to take your word on which events bubble and which
don't. But then again I suppose I have the word of the tests themselves. :-)

Should you be testing capture as well? Or does GWT stay clear of that?

http://codereview.appspot.com/8894/diff/1/3#newcode57
Line 57: assertTrue("Expecting child to receive the event before parent",
"Expecting parent to receive the event after the child", to avoid a dup message

http://codereview.appspot.com/8894/diff/1/3#newcode157
Line 157: init();
just call this from gwtSetup rather than in each method

http://codereview.appspot.com/8894/diff/1/3#newcode166
Line 166: Event.triggerClickEvent(child, 0, 0, 0, 0, 0, false, false, false,
false);
Seems like you should test all three core event types here.

http://codereview.appspot.com/8894/diff/1/3#newcode177
Line 177: public void onBrowserEvent(Event event) {
Seems like you could provide default implementations of onBrowserEvent that do
assertEquals(eventType, event.getType()), and get rid of a good chunk of these
anon classes. Perhaps even put it in a shared superclass.

http://codereview.appspot.com/8894/diff/1/3#newcode233
Line 233: assertTrue("Expected parent to receive event",
listener.parentReceived);
shouldn't you be checking the values of the various fields as well? You're
proving SCREEN_X et al, but I don't see that we know they're actually coming
through. Ditto below.

http://codereview.appspot.com/8894/diff/1/3#newcode275
Line 275: assertTrue("Expecting click or dblclick",
no, you're just expecting a dblclick, and this should be an assertEquals (or
just a call to super, eh?)

http://codereview.appspot.com/8894/diff/1/3#newcode307
Line 307: Event.triggerErrorEvent(child);
wouldn't this make more sense on the img? Can input elements every really
receive an error event?

[jgw, 2008-12-08 20:48:58]

For reasons not clear to me, I can't seem to add an updated patch set. I'll send
it to the gwt-contrib thread and hope you can do it :)

http://codereview.appspot.com/8894/diff/1/9
File user/src/com/google/gwt/user/client/Event.java (right):

http://codereview.appspot.com/8894/diff/1/9#newcode287
Line 287: * @param detail the event's detail property
On 2008/12/05 00:13:15, rjrjr wrote:
> Any example of valid values of detail?

Detail is one of those weird fields that means different things for different
events. It's defined as part of the DOM spec (in other words, if you don't know
what it is, you probably won't care).

http://codereview.appspot.com/8894/diff/1/7
File user/src/com/google/gwt/user/client/impl/DOMImplIE6.java (right):

http://codereview.appspot.com/8894/diff/1/7#newcode230
Line 230: evt.relatedTarget = relatedTarget;
On 2008/12/05 00:13:15, rjrjr wrote:
> (Pretending I know what I'm talking about.) relatedTarget is a real field in
> correct browsers, right? Is it a bit confusing to use it here in IE, where it
> normally doesn't exist? Could we call this gwtTarget instead?

Well, we're actually synthesizing a value that *is* identical to relatedTarget
on other browsers (and that IE doesn't implement), so it doesn't seem
unreasonable to simply synthesize it here.

In the long run, it would make more sense to shift the public APIs to use
'relatedTarget', but that's probably a task for another day.

http://codereview.appspot.com/8894/diff/1/10
File user/src/com/google/gwt/user/client/ui/CustomButton.java (right):

http://codereview.appspot.com/8894/diff/1/10#newcode746
Line 746: // TODO(jgw): fill in these parameters somehow.
On 2008/12/05 00:13:15, rjrjr wrote:
> If this is actually possible, could you file an issue on it and point to the
> issue here? Also, do we usually put our initials in our TODOs? (I hope so, but
> thought I got scolded for doing so.)

Whoops, I didn't mean to leave this TODO in there. I've changed this comment to
explain why those values are unavailable (this shouldn't matter in practice, as
click events often don't have useful mouse coordinates).

http://codereview.appspot.com/8894/diff/1/4
File user/test/com/google/gwt/user/client/ui/CustomButtonTest.java (right):

http://codereview.appspot.com/8894/diff/1/4#newcode178
Line 178: assertEquals("Expecting one click event", "click", events.get(0));
On 2008/12/05 00:13:15, rjrjr wrote:
> I guess you're doing all three (over, down, up) b/c triggerClickEvent can't be
> relied upon to do so? You might document that.

Actually, this is intended to test that this series of events should *cause*
CustomButton to fire a synthetic click event. I've added documentation to that
effect.

http://codereview.appspot.com/8894/diff/1/3
File user/test/com/google/gwt/user/client/ui/TriggerEventTest.java (right):

http://codereview.appspot.com/8894/diff/1/3#newcode30
Line 30: public class TriggerEventTest extends GWTTestCase {
On 2008/12/05 00:13:15, rjrjr wrote:
> A thing of beauty. I have to take your word on which events bubble and which
> don't. But then again I suppose I have the word of the tests themselves. :-)
> 
> Should you be testing capture as well? Or does GWT stay clear of that?

We don't do event capture, because there's really no reliable way to synthesize
it on IE (it's sort of possible, but there are all kinds of edge cases that
can't be made to work sensibly).

http://codereview.appspot.com/8894/diff/1/3#newcode57
Line 57: assertTrue("Expecting child to receive the event before parent",
On 2008/12/05 00:13:15, rjrjr wrote:
> "Expecting parent to receive the event after the child", to avoid a dup
message

Done.

http://codereview.appspot.com/8894/diff/1/3#newcode157
Line 157: init();
On 2008/12/05 00:13:15, rjrjr wrote:
> just call this from gwtSetup rather than in each method

Whoops, thanks. That's a lot simpler, isn't it?

http://codereview.appspot.com/8894/diff/1/3#newcode166
Line 166: Event.triggerClickEvent(child, 0, 0, 0, 0, 0, false, false, false,
false);
On 2008/12/05 00:13:15, rjrjr wrote:
> Seems like you should test all three core event types here.

Good point. Just refactored the test to do click, keypress, and focus.

http://codereview.appspot.com/8894/diff/1/3#newcode177
Line 177: public void onBrowserEvent(Event event) {
On 2008/12/05 00:13:15, rjrjr wrote:
> Seems like you could provide default implementations of onBrowserEvent that do
> assertEquals(eventType, event.getType()), and get rid of a good chunk of these
> anon classes. Perhaps even put it in a shared superclass.

Fair enough. I've gone through and factored out a few helper classes to simplify
these cases. There are still a few anonymous inners, but it's not as ugly as
before.

http://codereview.appspot.com/8894/diff/1/3#newcode233
Line 233: assertTrue("Expected parent to receive event",
listener.parentReceived);
On 2008/12/05 00:13:15, rjrjr wrote:
> shouldn't you be checking the values of the various fields as well? You're
> proving SCREEN_X et al, but I don't see that we know they're actually coming
> through. Ditto below.

These values are checked in assertMouseCoordinates() and assertAllShiftKeysOn().

http://codereview.appspot.com/8894/diff/1/3#newcode275
Line 275: assertTrue("Expecting click or dblclick",
On 2008/12/05 00:13:15, rjrjr wrote:
> no, you're just expecting a dblclick, and this should be an assertEquals (or
> just a call to super, eh?)

Cleaned up a bit with previously-mentioned refactoring. FWIW, that assertion was
meant to imply that either a click or a dblclick could have been safely
received, but it made more sense in a previous form.

http://codereview.appspot.com/8894/diff/1/3#newcode307
Line 307: Event.triggerErrorEvent(child);
On 2008/12/05 00:13:15, rjrjr wrote:
> wouldn't this make more sense on the img? Can input elements every really
> receive an error event?

Good point. This and testTriggerLoadEvent() both reference the img element now.