On Thu, Sep 18, 2008 at 3:33 PM, John Tamplin <[EMAIL PROTECTED]> wrote:
> This is not a complete review, but some initial feedback: > > - Shouldn't the selenium changes be part of a separate patch? > Regardless, Freeland or Eric would be better to review that portion. > > Yep, that was already committed to gwt-incubator trunk, the patch is older then the commit. > > - > - Did you read the > GWTC<http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/866faa00c999069/703f4dfa4d5ab1b7> > > threads<http://groups.google.com/group/Google-Web-Toolkit-Contributors/msg/eb3e5cee315a4e25>about > trying to do this in GWT 1.4? The consensus was that it was not safe > to rely on being able to remove all properties, and that even if it were > possible it would be fragile to future browser changes (the second thread > has a test HTML file, though it is missing __proto__). I think you will > need to rely on a prefix, just as we do in FastStringMap. At there very > least, you should include "__proto__", "prototype", "constructor", > "toString", "watch", "unwatch", and "valueOf" in your test. An alternative > would be to not attempt to clean up things like that and simply document > that no JS-defined property can be used in the string, though that seems > problematic. > > What code are you looking at? I'm not removing any properties and already testing watch. > > - Are you sure the complexity regarding a separate hosted-mode > implementation is justified? Aside from more opportunities for errors, it > means two separate implementations are not tested to be exactly the same, > so > any holes in the tests are an opportunity for divergent behavior in hosted > mode. > > Did you have a chance to run the benchmark I sent you? > > - > - Missing newlines at the end of all the .gwt.xml files. > > Our gwt.xml are supposed to have an extra new line at the end of them? > > - > > -- > John A. Tamplin > Software Engineer (GWT), Google > > > > -- "There are only 10 types of people in the world: Those who understand binary, and those who don't" --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
