Thanks. Still waiting on scottb to review my JUnit patch, then I will commit.
On Mon, Dec 8, 2008 at 4:58 PM, Ray Ryan <[EMAIL PROTECTED]> wrote: > 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 -~----------~----~----~----~------~----~------~--~---
