Ok, I *finally* got the freakin' JUnit infrastructure changes committed, which means it's time to get this stuff landed as well. The 1.6 release branch has drifted a bit since I last put this patch up for review, so I'll send out a new one momentarily. There should be no substantive changes over the last patch, other than conflict resolution.
On Thu, Dec 11, 2008 at 5:37 PM, Joel Webber <[email protected]> wrote: > I'm going through all the JUnit test sequences that Scott and I came up > with right now. We really have to land these two patches together -- if we > don't, then all the Event.trigger() methods are only going to cause everyone > to run headlong into the same problem I did. I'll be sending an updated > patch Real Soon Now. > > > On Wed, Dec 10, 2008 at 9:10 PM, Scott Blum <[email protected]> wrote: > >> Update: this is still in progress, but more complicated than it seems. I >> might look at ways to get the main patch in without the JUnit stuff. >> >> >> On Wed, Dec 10, 2008 at 1:24 PM, Joel Webber <[email protected]> wrote: >> >>> 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 -~----------~----~----~----~------~----~------~--~---
