Refactor, rename, and generally rationalize code in the Frog isolates library,

Refactor, rename, and generally rationalize code in the Frog isolates library,
in particular de-overloading the word "worker", in preparation for adding
iframe-based isolate managers.
Committed: https://code.google.com/p/dart/source/detail?r=6004

[eub, 2012-03-15 20:26:23]

Hi, Siggi, please have a look when you're back.  Thanks.

[Siggi Cherem (dart-lang), 2012-03-24 00:31:57]

Thanks for doing this Eli, in the end what was the bug you were tracking with
inCommandLine?

https://chromiumcodereview.appspot.com/9662024/diff/1/lib/isolate/frog/isolat...
File lib/isolate/frog/isolateimpl.dart (right):

https://chromiumcodereview.appspot.com/9662024/diff/1/lib/isolate/frog/isolat...
lib/isolate/frog/isolateimpl.dart:70: // XXX(eub): inside a web worker, is this
true (because we can request the
the former - true because we can delegate to the main manager.

https://chromiumcodereview.appspot.com/9662024/diff/1/lib/isolate/frog/isolat...
lib/isolate/frog/isolateimpl.dart:104: mainManager = new _MainManagerStub();
now that we made the distinction of main manager vs non-main manager, we should
only allocate this if isWorker is true (which means we probably have to split
nativeInit in 2 steps)

https://chromiumcodereview.appspot.com/9662024/diff/2001/lib/isolate/frog/iso...
File lib/isolate/frog/isolateimpl.dart (right):

https://chromiumcodereview.appspot.com/9662024/diff/2001/lib/isolate/frog/iso...
lib/isolate/frog/isolateimpl.dart:30: _Manager get _globalState() native "return
\$globalState;";
let's add a TODO to move 'manager' to be entirely done in JS (incorporating the
feedback we get from Florian)

https://chromiumcodereview.appspot.com/9662024/diff/2001/lib/isolate/frog/iso...
lib/isolate/frog/isolateimpl.dart:137: * Close the worker running this code if
all isolates are done.
nit: this might fit in a single line comment.

https://chromiumcodereview.appspot.com/9662024/diff/2001/lib/isolate/frog/iso...
lib/isolate/frog/isolateimpl.dart:245: // only on the command-line.  In the
browser where the isolate
I had a hard time understanding the second sentence, maybe split it in 2? maybe
remove the word 'where'?

https://chromiumcodereview.appspot.com/9662024/diff/2001/lib/isolate/frog/iso...
lib/isolate/frog/isolateimpl.dart:337: // terminate() is implemented by Worker.
don't we need it in the same way as we did with the previous functions? e.g:
void terminate() native "this.terminate();";

[eub, 2012-03-29 00:07:52]

Thanks, Siggi.  Just one question to wrap up...

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.dart
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.da...
lib/isolate/frog/isolateimpl.dart:70: // XXX(eub): inside a web worker, is this
true (because we can request the
On 2012/03/24 00:31:57, sigmund wrote:
> the former - true because we can delegate to the main manager.

(Sorry, this comment should have been gone; I always forget that gcl upload is
manual.)

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.da...
lib/isolate/frog/isolateimpl.dart:104: mainManager = new _MainManagerStub();
On 2012/03/24 00:31:57, sigmund wrote:
> now that we made the distinction of main manager vs non-main manager, we
should
> only allocate this if isWorker is true (which means we probably have to split
> nativeInit in 2 steps)

It looked like some of our code assumes that mainManager is never null, and that
it can postMessage() on it.  Shall we change that?

http://codereview.chromium.org/9662024/diff/2001/lib/isolate/frog/isolateimpl...
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9662024/diff/2001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:30: _Manager get _globalState() native "return
\$globalState;";
On 2012/03/24 00:31:58, sigmund wrote:
> let's add a TODO to move 'manager' to be entirely done in JS (incorporating
the
> feedback we get from Florian)
> 

Done.

http://codereview.chromium.org/9662024/diff/2001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:137: * Close the worker running this code if
all isolates are done.
On 2012/03/24 00:31:58, sigmund wrote:
> nit: this might fit in a single line comment.

Done.

http://codereview.chromium.org/9662024/diff/2001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:245: // only on the command-line.  In the
browser where the isolate
On 2012/03/24 00:31:58, sigmund wrote:
> I had a hard time understanding the second sentence, maybe split it in 2?
maybe
> remove the word 'where'?

Yeah, I don't know what the 'where' was doing in there.

http://codereview.chromium.org/9662024/diff/2001/lib/isolate/frog/isolateimpl...
lib/isolate/frog/isolateimpl.dart:337: // terminate() is implemented by Worker.
On 2012/03/24 00:31:58, sigmund wrote:
> don't we need it in the same way as we did with the previous functions? e.g:
> void terminate() native "this.terminate();";

Apparently not, as discussed.

[Siggi Cherem (dart-lang), 2012-03-29 00:24:48]

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.dart
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.da...
lib/isolate/frog/isolateimpl.dart:104: mainManager = new _MainManagerStub();
On 2012/03/29 00:07:52, eub wrote:
> On 2012/03/24 00:31:57, sigmund wrote:
> > now that we made the distinction of main manager vs non-main manager, we
> should
> > only allocate this if isWorker is true (which means we probably have to
split
> > nativeInit in 2 steps)
> 
> It looked like some of our code assumes that mainManager is never null, and
that
> it can postMessage() on it.  Shall we change that?

probably, I wonder if that code is reachable (e.g. is it guarded by inWorker in
most cases?). We can add a TODO to do so in a separate CL if you like.

[Siggi Cherem (dart-lang), 2012-03-29 17:21:57]

lgtm

http://codereview.chromium.org/9662024/diff/16001/lib/isolate/frog/isolateimp...
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9662024/diff/16001/lib/isolate/frog/isolateimp...
lib/isolate/frog/isolateimpl.dart:139: /// Close the worker running this code if
all isolates are done.
Oh, sorry - I meant to keep it also as
 /** single line. */
like the rest of the comments in this file.

[eub, 2012-03-29 21:20:44]

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.dart
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9662024/diff/1/lib/isolate/frog/isolateimpl.da...
lib/isolate/frog/isolateimpl.dart:104: mainManager = new _MainManagerStub();
On 2012/03/24 00:31:57, sigmund wrote:
> now that we made the distinction of main manager vs non-main manager, we
should
> only allocate this if isWorker is true (which means we probably have to split
> nativeInit in 2 steps)

Done.

http://codereview.chromium.org/9662024/diff/16001/lib/isolate/frog/isolateimp...
File lib/isolate/frog/isolateimpl.dart (right):

http://codereview.chromium.org/9662024/diff/16001/lib/isolate/frog/isolateimp...
lib/isolate/frog/isolateimpl.dart:139: /// Close the worker running this code if
all isolates are done.
On 2012/03/29 17:21:57, sigmund wrote:
> Oh, sorry - I meant to keep it also as
>  /** single line. */
> like the rest of the comments in this file.

oops, restored to match.

[Siggi Cherem (dart-lang), 2012-03-29 21:23:13]

lgtm, thanks!