http://gwt-code-reviews.appspot.com/13802/diff/1/13 File samples/styleloader/src/com/google/gwt/sample/styleloader/client/StyleLoader.java (right):
http://gwt-code-reviews.appspot.com/13802/diff/1/13#newcode34 Line 34: public class StyleLoader implements EntryPoint { Presuming we go straight to trunk, add this to ShowCase instead? http://gwt-code-reviews.appspot.com/13802/diff/1/9 File src/com/google/gwt/gen2/styleloader/client/StyleSheetLoadEvent.java (right): http://gwt-code-reviews.appspot.com/13802/diff/1/9#newcode31 Line 31: * Fires a style sheet load event on all registered handlers in the handler s/handler manager/source/ http://gwt-code-reviews.appspot.com/13802/diff/1/3 File src/com/google/gwt/gen2/styleloader/client/StyleSheetLoader.java (right): http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode50 Line 50: * </pre> You haven't shown how to use the class, though. No mention here of setting handlers, calling loadStyleSheet(), etc. Will need pointer to an example if this is to go into trunk. Should we mention that no provision is made to unload style sheets? http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode57 Line 57: private static final int DEFAULT_TIMEOUT = 180000; these are easier to read like so: DEFAULT_TIMEOUT_MILLIS = 3 * 60 * 1000; // 3 minutes http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode62 Line 62: * of 0px, but when the style sheet is loaded, the width changes to 5px. The 5 px isn't a requirement, right? Just > 0? http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode64 Line 64: * constructor that defines a height and width greater than 0px. should mention that this type must be unique per style sheet--people might be using this to shift between different styles at runtime, right? This is a nice trick. But it seems like you're assuming that stylesheets are applied atomically after they load. Are you sure about that? http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode67 Line 67: private Label refWidget; Why a widget? Why not just an element? Doesn't seem like you're doing anything widget-specific here. http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode110 Line 110: private static native HeadElement getHeadElement() If we put this in trunk, how about adding a getHead() method to Document instead of doing this? http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode128 Line 128: this.url = url; why no null check on url? http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode165 Line 165: * after it loads. What happens if this is called twice? Should you put in an assert to prevent that? http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode180 Line 180: * Set the timeout to wait before throwing an error. Kind of odd that this is mutable. I'd be more inclined to see it as a constructor arg--that is, keep the two args constructor with default timeout value, and provide a three args one to allow it to be set. http://gwt-code-reviews.appspot.com/13802/diff/1/3#newcode192 Line 192: public void waitForStyleSheet() { Why is this public? http://gwt-code-reviews.appspot.com/13802/diff/1/8 File src/com/google/gwt/gen2/styleloader/client/TimeoutEvent.java (right): http://gwt-code-reviews.appspot.com/13802/diff/1/8#newcode21 Line 21: * Represents a timeout event. Are there other places in the kit we should be using this event? http://gwt-code-reviews.appspot.com/13802/diff/1/8#newcode31 Line 31: * Fires a timeout event on all registered handlers in the handler manager.If "in the source", there is no HandlerManager here. Damn, this phrase permeates our javadoc and it's stale. http://gwt-code-reviews.appspot.com/13802/diff/1/10 File test/com/google/gwt/gen2/styleloader/client/StyleSheetLoaderTest.java (right): http://gwt-code-reviews.appspot.com/13802/diff/1/10#newcode31 Line 31: fail("Expected AssertionError"); This test will fail in web mode. http://gwt-code-reviews.appspot.com/13802/diff/1/10#newcode33 Line 33: assertTrue(true); No point to this assertion. The idiom as I've seen it is /* pass */ http://gwt-code-reviews.appspot.com/13802/diff/1/10#newcode40 Line 40: } catch (AssertionError e) { ditto and ditto http://gwt-code-reviews.appspot.com/13802 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
