Added cross-browser support for goog.module.ModuleLoader in debug mode

[nick santos, 2010-07-28 03:58:23]

there are actually good integration tests for this code in moduleloader_test.
did you run them?

http://codereview.appspot.com/1848053/diff/1/2
File closure/goog/module/moduleloader.js (right):

http://codereview.appspot.com/1848053/diff/1/2#newcode117
closure/goog/module/moduleloader.js:117: var scriptElt =
goog.dom.createElement('script');
this might be easier as an inner function, e.g., "createScript(i)"

http://codereview.appspot.com/1848053/diff/1/2#newcode132
closure/goog/module/moduleloader.js:132: this.readyState == 'complete') {
shouldn't we just use one?

http://codereview.appspot.com/1848053/diff/1/2#newcode133
closure/goog/module/moduleloader.js:133: if (!scriptEl.loadedByClosure) {
i think the idiomatic way to do this is to use a [] prop, like
scriptEl['closure_loaded'], to match goog.getUid

http://codereview.appspot.com/1848053/diff/1/2#newcode140
closure/goog/module/moduleloader.js:140: // TODO(bolinfest): Test this on Opera.
i have this written down somewhere. i can give you a definitive answer tomorrow.

[bolinfest, 2010-07-28 13:34:06]

Hi Nick, I ran moduleloader_test.html. On my Windows laptop, the following tests
fail on Chrome and IE7 without my change (at least as of Closure Library r155):
testLoadModuleA, testLoadModuleB, testLoadDebugModuleA, and
testLoadDebugModuleB.

With my change, the first two tests still fail, but at least
testLoadDebugModuleA and testLoadDebugModuleB pass. This is expected as I only
changed the behavior of ModuleLoader in debug mode.

http://codereview.appspot.com/1848053/diff/1/2
File closure/goog/module/moduleloader.js (right):

http://codereview.appspot.com/1848053/diff/1/2#newcode132
closure/goog/module/moduleloader.js:132: this.readyState == 'complete') {
On 2010/07/28 03:58:23, Nicholas.J.Santos wrote:
> shouldn't we just use one?

Well, to be honest, I was going off of this:
http://stackoverflow.com/questions/538745/how-to-tell-if-a-script-tag-failed-...

http://codereview.appspot.com/1848053/diff/1/2#newcode133
closure/goog/module/moduleloader.js:133: if (!scriptEl.loadedByClosure) {
On 2010/07/28 03:58:23, Nicholas.J.Santos wrote:
> i think the idiomatic way to do this is to use a [] prop, like
> scriptEl['closure_loaded'], to match goog.getUid

Done.

http://codereview.appspot.com/1848053/diff/1/2#newcode140
closure/goog/module/moduleloader.js:140: // TODO(bolinfest): Test this on Opera.
On 2010/07/28 03:58:23, Nicholas.J.Santos wrote:
> i have this written down somewhere. i can give you a definitive answer
tomorrow.

I actually updated this after you reviewed it, but I put the update in the bug
report. I created this test page for the function:

http://bolinfest.com/closure/scriptdeterminism4.html

And used it to test the following browsers:
Firefox 3.6.6
Chrome 5.0
IE 7
Safari 4.0.5
Opera 10.60

It works on all of those.

[nick santos, 2010-07-28 13:41:34]

i'm surprised, because the test is currently passing on our farm, which runs it
on IE7. are you sure you're not just running into xdomain issues when you run it
locally? (chrome and IE have additional restrictions on local uris).

i'd prefer to patch this into perforce directly so that we can run the tests.

http://codereview.appspot.com/1848053/diff/7001/8001
File closure/goog/module/moduleloader.js (right):

http://codereview.appspot.com/1848053/diff/7001/8001#newcode135
closure/goog/module/moduleloader.js:135: this.readyState == 'complete') {
here is the comment i wrote in notebook's module loader:

203:   // there are 4 behaviors when a script tag isn't loaded properly:
204:   // 1) IE fires onreadystatechange with "script.readystate=loaded", but
the
205:   //    the tag is empty. It doesn't fire onerror or onload.
206:   // 2) FF fires onerror. It doesn't fire onload or onreadystatechange.
207:   // 3) WebKit fires onload, but the tag is empty.
208:   //    It doesn't fire onerror or onreadystatechange.
209:   // 4) Opera fires both onreadystatechange and onload, but the tag is
empty.
210:   //    It doesn't fire onerror.

obviously, that's mostly about failure (rather than the success case, which is
what you care about).

but i think that 'loaded' is probably sufficient here.

[nick santos, 2010-07-28 13:41:52]

LGTM++

thanks for doing this!

[bolinfest, 2010-07-28 14:29:51]

np, thanks for the speedy review!

I take it you'll take care of integrating it locally and testing it on the
farm and patching it yourself? Specifically, do you need any thing else from
me?

I suspect you're right about the xdomain stuff being the issue when running
it locally.


On Wed, Jul 28, 2010 at 9:41 AM, <Nicholas.J.Santos@gmail.com> wrote:

> LGTM++
>
> thanks for doing this!
>
>
> http://codereview.appspot.com/1848053/show
>