Tightening up dart2js load timing.

Tightening up dart2js load timing.

The timing of main() execution in dart2js currently occurs after 'loaded' which is quite late in the page load phase (after all images) and is inconsistent with Dartium.

In order to get the timing more consistent with Dartium and more consistent in general this CL makes a couple of changes:
 - dart2js generated code is now executed synchronously- this gives end developers more control over timing of main().
 - A warning is generated if main() is executed during document parse time- this is primarily to ease the transition and let users know what needs to be done to more closely match Dartium behavior.
 - dart.js bootstrapper no longer waits for DOMContentLoaded- the modified script tags will be executed after parse phase, the additional delay should not be necessary.

Breaking changes:
 - dart2js main() execution in standard project templates now occurs after document parse but before all assets have been loaded
 - Script tags directly referencing dart.js files will now execute synchronously. It's recommended that defer tags be added to these, or that they be moved to the end of the body tag.
 - dart.js bootstrapper will now only auto-load script tags which occur before it on the page.

BUG=4718
R=sra@google.com, vsm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26149

[vsm, 2013-08-07 16:10:12]

lgtm, but do we need to change the editor templates as well?  E.g., to add a
defer?

This could use a breaking change notice as well.

https://chromiumcodereview.appspot.com/22488002/diff/21002/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/22488002/diff/21002/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2954:
scripts[i].removeEventListener('load', onLoad, false);
Can you add a comment here to explain what's going on?  If you hadn't explained
it ahead of time, I think I'd be pretty befuddled here.  Even still, it's tricky
code.

[ahe, 2013-08-07 16:24:47]

I don't think you can submit it as is.  It looks pretty good, but there are a
few nits.

I'll send more comments when I have more time...

https://chromiumcodereview.appspot.com/22488002/diff/21002/sdk/lib/_internal/...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://chromiumcodereview.appspot.com/22488002/diff/21002/sdk/lib/_internal/...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2942: if
(!self.document) {
Just a quick note, sorry.

This will break dart2js tests running on d8.  This is what we test on the main
build bots.

Try running:

./tools/test.py -mrelease -cdart2js -rd8

[blois, 2013-08-07 17:39:50]

Thanks for the feedback Peter, I realize you're in the middle of a bunch of
other stuff, so no rush.

I don't think this requires editor template changes as Dartium execution remains
unchanged and the way dart.js works it makes it roughly approximate defer
semantics.

https://codereview.chromium.org/22488002/diff/21002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/22488002/diff/21002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2942: if
(!self.document) {
On 2013/08/07 16:24:47, ahe wrote:
> Just a quick note, sorry.
> 
> This will break dart2js tests running on d8.  This is what we test on the main
> build bots.
> 
> Try running:
> 
> ./tools/test.py -mrelease -cdart2js -rd8

Ah, thanks!

https://codereview.chromium.org/22488002/diff/21002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2954:
scripts[i].removeEventListener('load', onLoad, false);
On 2013/08/07 16:10:12, vsm wrote:
> Can you add a comment here to explain what's going on?  If you hadn't
explained
> it ahead of time, I think I'd be pretty befuddled here.  Even still, it's
tricky
> code.
> 

Done.

[ahe, 2013-08-12 15:58:39]

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2984:
Isolate.\$isolateProperties.\$currentScript = currentScript;
Did you test this in minified mode? I think it needs to be:

'${namer.isolateName}.${namer.isolatePropertiesName}.\$currentScript =
currentScript;

Test it using:

test.py -mrelease -cdart2js -rdrt --minified

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2988: // Bug
dartbug.com/12281 this warning is to let users know how to match
I think you can avoid including this comment in the generated JS code by using
this trick:

      document.readyState == "loading") {'''
    // Bug dartbug.com/12281 this warning is to let users know how to match
    // Dartium execution timing.
'''

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2990:
console.warn("Dart script executed synchronously, use <script src='" +
Does this work on IE? I have heard that the console object doesn't exist when
the error console window isn't open in IE.

[ahe, 2013-08-12 15:59:27]

Hi Pete,

If you can get Stephen to accept this, don't wait for my approval :-)

Cheers,
Peter

[blois, 2013-08-12 17:00:07]

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2984:
Isolate.\$isolateProperties.\$currentScript = currentScript;
On 2013/08/12 15:58:39, ahe wrote:
> Did you test this in minified mode? I think it needs to be:
> 
> '${namer.isolateName}.${namer.isolatePropertiesName}.\$currentScript =
> currentScript;
> 
> Test it using:
> 
> test.py -mrelease -cdart2js -rdrt --minified

Fixed, thanks!

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2988: // Bug
dartbug.com/12281 this warning is to let users know how to match


On 2013/08/12 15:58:39, ahe wrote:
> I think you can avoid including this comment in the generated JS code by using
> this trick:
> 
>       document.readyState == "loading") {'''
>     // Bug dartbug.com/12281 this warning is to let users know how to match
>     // Dartium execution timing.
> '''

I wanted this comment in the file for the near term so devs would have a
reference for more information. The bug is for removing the warning and this
comment.

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2990:
console.warn("Dart script executed synchronously, use <script src='" +
On 2013/08/12 15:58:39, ahe wrote:
> Does this work on IE? I have heard that the console object doesn't exist when
> the error console window isn't open in IE.

Console isn't defined on FF in this case either- there's a check for console
above this block.

Tested on IE9, IE10, IE11 & FF.

[ahe, 2013-08-12 17:08:01]

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2988: // Bug
dartbug.com/12281 this warning is to let users know how to match
On 2013/08/12 17:00:07, blois wrote:
> 
> 
> On 2013/08/12 15:58:39, ahe wrote:
> > I think you can avoid including this comment in the generated JS code by
using
> > this trick:
> > 
> >       document.readyState == "loading") {'''
> >     // Bug dartbug.com/12281 this warning is to let users know how to match
> >     // Dartium execution timing.
> > '''
> 
> I wanted this comment in the file for the near term so devs would have a
> reference for more information. The bug is for removing the warning and this
> comment.

If you want devs to read this message, shouldn't it be part of the warning?

How about:

console.warn("Dart script executed synchronously, use <script src='" 
currentScript.src + "' defer></scr" + "ipt> to execute after parsing " +
"has completed. See also http://dartbug.com/12281.");

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2990:
console.warn("Dart script executed synchronously, use <script src='" +
On 2013/08/12 17:00:07, blois wrote:
> On 2013/08/12 15:58:39, ahe wrote:
> > Does this work on IE? I have heard that the console object doesn't exist
when
> > the error console window isn't open in IE.
> 
> Console isn't defined on FF in this case either- there's a check for console
> above this block.

Doh!

> 
> Tested on IE9, IE10, IE11 & FF.

[blois, 2013-08-12 17:43:45]

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/22488002/diff/34001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2988: // Bug
dartbug.com/12281 this warning is to let users know how to match
On 2013/08/12 17:08:01, ahe wrote:
> On 2013/08/12 17:00:07, blois wrote:
> > 
> > 
> > On 2013/08/12 15:58:39, ahe wrote:
> > > I think you can avoid including this comment in the generated JS code by
> using
> > > this trick:
> > > 
> > >       document.readyState == "loading") {'''
> > >     // Bug dartbug.com/12281 this warning is to let users know how to
match
> > >     // Dartium execution timing.
> > > '''
> > 
> > I wanted this comment in the file for the near term so devs would have a
> > reference for more information. The bug is for removing the warning and this
> > comment.
> 
> If you want devs to read this message, shouldn't it be part of the warning?
> 
> How about:
> 
> console.warn("Dart script executed synchronously, use <script src='" 
> currentScript.src + "' defer></scr" + "ipt> to execute after parsing " +
> "has completed. See also http://dartbug.com/12281.%22%29;

Fixed. I was figuring that the code comment was sufficient since the dev needs
to have the console open in the first place, but not really necessary since the
comment is small enough.

[sra1, 2013-08-12 21:22:26]

lgtm

https://chromiumcodereview.appspot.com/22488002/diff/49001/samples/third_part...
File samples/third_party/dromaeo/generate_dart2js_tests.py (right):

https://chromiumcodereview.appspot.com/22488002/diff/49001/samples/third_part...
samples/third_party/dromaeo/generate_dart2js_tests.py:65: script = '<script
type="text/javascript" src="%s">' % jsname
Does this want to be deferred too?

[blois, 2013-08-14 20:44:39]

https://codereview.chromium.org/22488002/diff/49001/samples/third_party/droma...
File samples/third_party/dromaeo/generate_dart2js_tests.py (right):

https://codereview.chromium.org/22488002/diff/49001/samples/third_party/droma...
samples/third_party/dromaeo/generate_dart2js_tests.py:65: script = '<script
type="text/javascript" src="%s">' % jsname
On 2013/08/12 21:22:26, sra1 wrote:
> Does this want to be deferred too?

D'oh! That's the correct location for this change.

[blois, 2013-08-14 20:51:50]

Committed patchset #4 manually as r26149 (presubmit successful).

[ricow1, 2013-08-15 08:21:30]

This broke IE bots, which had been moved to FYI

[ahe, 2013-08-15 08:37:34]

On 2013/08/15 08:21:30, ricow1 wrote:
> This broke IE bots, which had been moved to FYI

I think we have to revert this and investigate. 

Very frustrating, this CL was going to solve a lot of problems :-(

Cheers,
Peter