LGTM Rietveld can't show inter-diff diffs, so I can't see, say, your version 1 and comments on the left and the new patch on the right. That's a shame.
rjrjr On Tue, Dec 9, 2008 at 7:50 AM, Joel Webber <[EMAIL PROTECTED]> wrote: > Don't know why I can't attach another patch set to that codereview > instance, so here it is. Maybe you can attach it (or maybe I'm just dumb and > can't figure it out). > > > On Mon, Dec 8, 2008 at 3:48 PM, <[EMAIL PROTECTED]> wrote: > >> 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. >> >> >> http://codereview.appspot.com/8894 >> > > --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
