Cleanup the isolate library to pave the way for web worker support.

Cleanup the isolate library to pave the way for web worker support.
Committed: https://code.google.com/p/dart/source/detail?r=16911

[ngeoffray, 2013-01-09 13:42:02]

This is another try for https://codereview.chromium.org/11574032/, so most of
the code has been reviewed. The change with that other CL is handling the v2
core library and the new implementation of Future.

[floitsch, 2013-01-09 14:53:14]

LGTM.

https://codereview.chromium.org/11778064/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11778064/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1150: // the
current isolate may have changed so if we read it from the JS
How are you sure that this prevents it from inlining?
It took me relatively long to figure out what this comment means.
Maybe explain more:
JS_CURRENT_ISOLATE() is a macro that is recognized by the compiler and which
expands into a String representing JS code.
By storing the result in a local variable we prevent the JS below to inline the
String to be "inlined" into the JavaScript function. If it was inlined it would
not get the current isolate, when converting the closure, but the one that was
active when the callback executes.

https://codereview.chromium.org/11778064/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1157:
DART_CLOSURE_TO_JS(invokeClosure),
indentation.

[ngeoffray, 2013-01-09 16:32:24]

Thanks Florian, PTAL, there is actually a test for non resolved ports, so I'm
dealing with _waitForPendingPorts in a different way.

https://codereview.chromium.org/11778064/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11778064/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1150: // the
current isolate may have changed so if we read it from the JS
On 2013/01/09 14:53:14, floitsch wrote:
> How are you sure that this prevents it from inlining?

Because the call will be emitted before the creation of the JavaScript function.

> It took me relatively long to figure out what this comment means.
> Maybe explain more:
> JS_CURRENT_ISOLATE() is a macro that is recognized by the compiler and which
> expands into a String representing JS code.
> By storing the result in a local variable we prevent the JS below to inline
the
> String to be "inlined" into the JavaScript function. If it was inlined it
would
> not get the current isolate, when converting the closure, but the one that was
> active when the callback executes.

Done.

https://codereview.chromium.org/11778064/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1157:
DART_CLOSURE_TO_JS(invokeClosure),
On 2013/01/09 14:53:14, floitsch wrote:
> indentation.

Done.

[floitsch, 2013-01-09 17:42:58]

LGTM.

https://codereview.chromium.org/11778064/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11778064/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:441: //
TODO(7795): Clean this up when we handler Timers in web
we handle Timers

also seems to fit on one line.

[ngeoffray, 2013-01-10 09:16:56]

Thanks Florian!

https://codereview.chromium.org/11778064/diff/12/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11778064/diff/12/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:441: //
TODO(7795): Clean this up when we handler Timers in web
On 2013/01/09 17:42:58, floitsch wrote:
> we handle Timers
> 
> also seems to fit on one line.

Done.

[ngeoffray, 2013-01-10 10:44:35]

On 2013/01/10 09:16:56, ngeoffray wrote:
> Thanks Florian!
> 
>
https://codereview.chromium.org/11778064/diff/12/sdk/lib/_internal/compiler/i...
> File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):
> 
>
https://codereview.chromium.org/11778064/diff/12/sdk/lib/_internal/compiler/i...
> sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:441: //
> TODO(7795): Clean this up when we handler Timers in web
> On 2013/01/09 17:42:58, floitsch wrote:
> > we handle Timers
> > 
> > also seems to fit on one line.
> 
> Done.

I decided to drop web workers support in this CL. We really need to support
Timers before supporting web workers. So this CL is only about cleaning up code
to pave the way for web worker support.

[erikcorry, 2013-01-16 09:21:36]

Unfortunately, this change broke minified mode in dart2js, as tested with:

./tools/test.py -mrelease -cdart2js -rd8 --use-sdk --time -pcolor --report
dart2js_native --minified

There are 3 failing tests after this change.

In addition I have two nits.

https://codereview.chromium.org/11778064/diff/5013/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11778064/diff/5013/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:384: var
globalWindow = JS('', "#['window']", globalThis);
I think it would be more idiomatic and more compact to write #.window instead of
#['window'].

https://codereview.chromium.org/11778064/diff/5013/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:407: && !new
RegExp('client.dart\.js').hasMatch(src)) {
This RegExp is strange.  It allows any character between 'client' and 'dart',
and the backslash has no effect, so it also allows any character there.

[floitsch, 2013-01-16 09:42:16]

https://codereview.chromium.org/11778064/diff/5013/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart (right):

https://codereview.chromium.org/11778064/diff/5013/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:384: var
globalWindow = JS('', "#['window']", globalThis);
On 2013/01/16 09:21:36, erikcorry wrote:
> I think it would be more idiomatic and more compact to write #.window instead
of
> #['window'].

Only if we can guarantee that "globalThis" is not an arbitrary expression.