Add support for capturing and changing the current isolate in the closure wrapper.

Add support for capturing and changing the current isolate in the closure wrapper.
Committed: https://code.google.com/p/dart/source/detail?r=5638

[ngeoffray, 2012-03-15 16:26:21]

https://chromiumcodereview.appspot.com/9702074/diff/4001/client/tests/client/...
File client/tests/client/client-leg.status (left):

https://chromiumcodereview.appspot.com/9702074/diff/4001/client/tests/client/...
client/tests/client/client-leg.status:32: dom/IsolateLightTest: Skip # Timeout
This test is now passing.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1349: invokeClosure(Function closure, var isolate,
var nbArgs, var arg1, var arg2) {
This is way better than before, as it is fully written in Dart :)

[kasperl, 2012-03-16 06:00:01]

First comments:

https://chromiumcodereview.appspot.com/9702074/diff/4001/client/tests/client/...
File client/tests/client/client-leg.status (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/client/tests/client/...
client/tests/client/client-leg.status:32: # Uses 'exploratory API' (see
client/dom/src/spawnDomIsolate
Please, please add a ).

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1372: return #(#, #, arguments.length, arguments[0],
arguments[1]);
Ugh. I would probably have preferred keeping these as #0, #1, and #2 but that
might just be me.

[kasperl, 2012-03-16 11:06:42]

LGTM.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1734: if (!node.arguments.isEmpty()) {
There's sooo much code in these cases. Maybe this would be a good opportunity to
factor them out into helper methods?

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1744: const LiteralDartString('bool'),
Not sure I understand the use of bool here.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1747: // Call a helper method from the isolate
library.
Maybe explain here why you cannot just grab hold of the current isolate?

[ahe, 2012-03-16 14:03:16]

DBC

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1350: if (nbArgs == 0) {
Please avoid abbreviations.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1376: JS_CURRENT_ISOLATE());
Why is this passed in? Wouldn't it be simpler to call JS_CURRENT_ISOLATE in
invokeClosure?

https://chromiumcodereview.appspot.com/9702074/diff/4001/lib/isolate/isolate_...
File lib/isolate/isolate_leg.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/lib/isolate/isolate_...
lib/isolate/isolate_leg.dart:48: * These methods are called by the compiler to
support switching
A documentation comment only applies to one element.

https://chromiumcodereview.appspot.com/9702074/diff/4001/lib/isolate/isolate_...
lib/isolate/isolate_leg.dart:51: void _callInIsolate(IsolateContext isolate,
Function function) {
I would rather have these methods in js_helper.

[ngeoffray, 2012-03-19 10:29:58]

Thanks Peter and Kasper for the comments.

https://chromiumcodereview.appspot.com/9702074/diff/4001/client/tests/client/...
File client/tests/client/client-leg.status (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/client/tests/client/...
client/tests/client/client-leg.status:32: # Uses 'exploratory API' (see
client/dom/src/spawnDomIsolate
On 2012/03/16 06:00:01, kasperl wrote:
> Please, please add a ).

Done :)

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
File frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1350: if (nbArgs == 0) {
On 2012/03/16 14:03:16, ahe wrote:
> Please avoid abbreviations.

Done.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1372: return #(#, #, arguments.length, arguments[0],
arguments[1]);
On 2012/03/16 06:00:01, kasperl wrote:
> Ugh. I would probably have preferred keeping these as #0, #1, and #2 but that
> might just be me.

As discussed, it felt simpler to use to just use # for now.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/lib/js_help...
frog/leg/lib/js_helper.dart:1376: JS_CURRENT_ISOLATE());
On 2012/03/16 14:03:16, ahe wrote:
> Why is this passed in? Wouldn't it be simpler to call JS_CURRENT_ISOLATE in
> invokeClosure?

No, because you need to fetch the isolate at this point. When you're in
invokeClosure, it's too late.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1734: if (!node.arguments.isEmpty()) {
On 2012/03/16 11:06:42, kasperl wrote:
> There's sooo much code in these cases. Maybe this would be a good opportunity
to
> factor them out into helper methods?

Done.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1744: const LiteralDartString('bool'),
On 2012/03/16 11:06:42, kasperl wrote:
> Not sure I understand the use of bool here.

Typo, replaced with 'var'.

https://chromiumcodereview.appspot.com/9702074/diff/4001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:1747: // Call a helper method from the isolate
library.
On 2012/03/16 11:06:42, kasperl wrote:
> Maybe explain here why you cannot just grab hold of the current isolate?

Done.

https://chromiumcodereview.appspot.com/9702074/diff/4001/lib/isolate/isolate_...
File lib/isolate/isolate_leg.dart (right):

https://chromiumcodereview.appspot.com/9702074/diff/4001/lib/isolate/isolate_...
lib/isolate/isolate_leg.dart:48: * These methods are called by the compiler to
support switching
On 2012/03/16 14:03:16, ahe wrote:
> A documentation comment only applies to one element.

Done.

https://chromiumcodereview.appspot.com/9702074/diff/4001/lib/isolate/isolate_...
lib/isolate/isolate_leg.dart:51: void _callInIsolate(IsolateContext isolate,
Function function) {
On 2012/03/16 14:03:16, ahe wrote:
> I would rather have these methods in js_helper.

I agree, and maybe long-term that's what will happen. However, these methods
currently use isolate library specific methods that I'm not sure we want to leak
in the leg compiler.

[Siggi Cherem (dart-lang), 2012-03-24 01:10:19]

lgtm

https://chromiumcodereview.appspot.com/9702074/diff/13002/samples/tests/sampl...
File samples/tests/samples/samples.status (right):

https://chromiumcodereview.appspot.com/9702074/diff/13002/samples/tests/sampl...
samples/tests/samples/samples.status:70: dartcombat/DartCombatTest: Fail #
Uncaught TypeError: Object #<FutureImpl> has no method 'receive$1'
good to know.. I added a bug to fix this.

[ngeoffray, 2012-03-25 19:00:52]

https://chromiumcodereview.appspot.com/9702074/diff/13002/samples/tests/sampl...
File samples/tests/samples/samples.status (right):

https://chromiumcodereview.appspot.com/9702074/diff/13002/samples/tests/sampl...
samples/tests/samples/samples.status:70: dartcombat/DartCombatTest: Fail #
Uncaught TypeError: Object #<FutureImpl> has no method 'receive$1'
On 2012/03/24 01:10:19, sigmund wrote:
> good to know.. I added a bug to fix this.

Note that I haven't investigated the failure, and it might as well be a bug in
leg.

[Siggi Cherem (dart-lang), 2012-03-26 18:01:42]

https://chromiumcodereview.appspot.com/9702074/diff/13002/samples/tests/sampl...
File samples/tests/samples/samples.status (right):

https://chromiumcodereview.appspot.com/9702074/diff/13002/samples/tests/sampl...
samples/tests/samples/samples.status:70: dartcombat/DartCombatTest: Fail #
Uncaught TypeError: Object #<FutureImpl> has no method 'receive$1'
On 2012/03/25 19:00:52, ngeoffray wrote:
> On 2012/03/24 01:10:19, sigmund wrote:
> > good to know.. I added a bug to fix this.
> 
> Note that I haven't investigated the failure, and it might as well be a bug in
> leg.

I recently changed sendport.call to return a future, rather than a receive port.
The error tells me that this is probably a place where I didn't fix the use of
sendport.call.