Updated patch.
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. On 2011/06/21 14:51:15, jlabanca wrote:
I suggest moving this to a comment at the top of the class instead of
in the
JavaDoc. Its just an implementation detail.
Done. 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. On 2011/06/21 14:51:15, jlabanca wrote:
directly repeated directly twice in the same sentence
It's quite direct. (fixed) 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; On 2011/06/21 14:51:15, jlabanca wrote:
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?
This is a trick lifted from the linker. Removing the script tag does not cause the definitions in the script to go away, it just removes it from the DOM saving some memory Here's one article on it: http://www.javascriptkit.com/javatutors/loadjavascriptcss2.shtml 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); On 2011/06/21 14:51:15, jlabanca wrote:
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.
Done. I assumed it would not fire loaded until its attached to the dom. 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); On 2011/06/21 14:51:15, jlabanca wrote:
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.
Good point, I need to test the return value, but in this case, I still wanted to see that there was no tag in the DOM because it is supposed to be removed now. 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()); On 2011/06/21 14:51:15, jlabanca wrote:
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.
If the injection fails, then nativeTest1Worked() returns false... so this test would pass. 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(); On 2011/06/21 14:51:15, jlabanca wrote:
Add a message like "Expected script injection to succeed."
Done. 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(); On 2011/06/21 14:51:15, jlabanca wrote:
reason?
Done. (here and elsewhere) http://gwt-code-reviews.appspot.com/1451818/diff/5008/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/5008/user/src/com/google/gwt/core/client/ScriptInjector.java#newcode281 user/src/com/google/gwt/core/client/ScriptInjector.java:281: private static void doSuccessCb(Callback<Void, Exception> callback) { I introduced a bug in my test and was pulling my hair out to debug it before I added this code. http://gwt-code-reviews.appspot.com/1451818/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
