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 -~----------~----~----~----~------~----~------~--~---
