Full review this time.
http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/src/com/google/gwt/core/client/ScriptInjector.java File user/src/com/google/gwt/core/client/ScriptInjector.java (right): http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/src/com/google/gwt/core/client/ScriptInjector.java#newcode23 user/src/com/google/gwt/core/client/ScriptInjector.java:23: * can pull in as few dependencies as possible and live in the Core module. I suggest moving this to a comment at the top of the class instead of in the JavaDoc. Its just an implementation detail. http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/src/com/google/gwt/core/client/ScriptInjector.java#newcode52 user/src/com/google/gwt/core/client/ScriptInjector.java:52: * Builder for directly injecting a script body directly into the DOM. directly repeated directly twice in the same sentence http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/src/com/google/gwt/core/client/ScriptInjector.java#newcode55 user/src/com/google/gwt/core/client/ScriptInjector.java:55: private boolean removeTag = true; removeTag should default to false. I think the most common use case is to inject a library, in which case you do not want to remove it. What was the rationale for defaulting to true? http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/src/com/google/gwt/core/client/ScriptInjector.java#newcode140 user/src/com/google/gwt/core/client/ScriptInjector.java:140: attachListeners(scriptElement, callback); You should attach the listeners before setting the source in case the script loads synchronously. I know that with images in IE, onload fires synchronously (as soon as you set the url) if the image is cached. http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/test/com/google/gwt/core/client/ScriptInjectorTest.java File user/test/com/google/gwt/core/client/ScriptInjectorTest.java (right): http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/test/com/google/gwt/core/client/ScriptInjectorTest.java#newcode70 user/test/com/google/gwt/core/client/ScriptInjectorTest.java:70: JavaScriptObject scriptElement = findScriptTextInThisWindow(scriptBody); You can now get the scriptElement from inject() instead of searching for it. You could sanity check that the scriptElement lives in the head element of the correct window. http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/test/com/google/gwt/core/client/ScriptInjectorTest.java#newcode73 user/test/com/google/gwt/core/client/ScriptInjectorTest.java:73: assertFalse("cleanup failed", nativeTest1Worked()); If the script fails to inject (worked is false), you'll hit this error, which is misleading. If you move the assertTrue() above the assertFalse(), then the fail case returns a more meaningful error. Of course that means you won't cleanup, but if an error occurs anyway, that's okay. And, if an error occurs, you probably don't have to cleanup because the script was never injected. http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/test/com/google/gwt/core/client/ScriptInjectorTest.java#newcode150 user/test/com/google/gwt/core/client/ScriptInjectorTest.java:150: fail(); Add a message like "Expected script injection to succeed." http://gwt-code-reviews.appspot.com/1451818/diff/10011/user/test/com/google/gwt/core/client/ScriptInjectorTest.java#newcode186 user/test/com/google/gwt/core/client/ScriptInjectorTest.java:186: fail(); reason? http://gwt-code-reviews.appspot.com/1451818/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
