frog isolate fixes: minor changes to the isolate library + architecture.py

frog isolate fixes: minor changes to the isolate library + architecture.py

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

[Siggi Cherem (dart-lang), 2011-12-22 00:24:02]

http://codereview.chromium.org/8999030/diff/2001/tools/testing/architecture.py
File tools/testing/architecture.py (right):

http://codereview.chromium.org/8999030/diff/2001/tools/testing/architecture.p...
tools/testing/architecture.py:53: <script type="%(script_type)s"
src="%(source_script)s"></script>
this move was useful because isolates derive the worker URL from this tag -
placing it above was confusing the isolate code. 

(see TODO message in isolate.dart:_computeThisScript)

[mattsh, 2011-12-22 01:55:15]

LGTM

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart
File frog/world.dart (right):

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode219
frog/world.dart:219: && (!_isDomWorker(existing) || !_isIsolateWorker(named))) {
is there a way to simplify this?  (Read a few times and can't quite follow it.)

[Siggi Cherem (dart-lang), 2011-12-22 18:51:15]

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart
File frog/world.dart (right):

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode219
frog/world.dart:219: && (!_isDomWorker(existing) || !_isIsolateWorker(named))) {
On 2011/12/22 01:55:15, mattsh wrote:
> is there a way to simplify this?  (Read a few times and can't quite follow
it.)

How about this (not-normalized form is a bit easier to read):

if (!((_isDomWorker(named) && _isIsolateWorker(existing)
|| (_isDomWorker(existing) && _isIsolateWorker(named))) {
 ...
}

[Jennifer Messerly, 2011-12-22 19:49:44]

lgtm, with some suggestions

http://codereview.chromium.org/8999030/diff/3006/frog/lib/isolate.dart
File frog/lib/isolate.dart (right):

http://codereview.chromium.org/8999030/diff/3006/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:173: class _Worker native "*Worker" {
I'd add a comment here that this type is defined in dart:dom, but we don't want
to depend on that from corelib, and instead we'll do a runtime tests to see if
Worker is present.

http://codereview.chromium.org/8999030/diff/3006/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:302: } catch(var e, var trace) {
Is trace actually working? if so ... that's cool :)
I wonder if we'll want to send the two objects separately.

http://codereview.chromium.org/8999030/diff/3006/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:499: static _Worker _newWorker(url) native "return new
Worker(url);";
we aren't very consistent about it, but personally I've been trying to omit the
semicolons ... saves a few bytes :). (although any decent minifier should do
that).

http://codereview.chromium.org/8999030/diff/3006/frog/lib/isolate.dart#newcod...
frog/lib/isolate.dart:506: final worker = _newWorker(_thisScript);
Are you using "final" to avoid dynamic dispatch? If so -- I'd put in an explicit
_Worker type, and add a comment that something is wrong in the dispatch.

unless you were just trying to make the code more clear in which case, I like
using "final" :)

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart
File frog/world.dart (right):

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode219
frog/world.dart:219: && (!_isDomWorker(existing) || !_isIsolateWorker(named))) {
On 2011/12/22 18:51:15, sigmund wrote:
> On 2011/12/22 01:55:15, mattsh wrote:
> > is there a way to simplify this?  (Read a few times and can't quite follow
> it.)
> 
> How about this (not-normalized form is a bit easier to read):
> 
> if (!((_isDomWorker(named) && _isIsolateWorker(existing)
> || (_isDomWorker(existing) && _isIsolateWorker(named))) {
>  ...
> }

How about:

} else if (!named.isWebWorker && !existing.isWebWorker) {
...
}

we shouldn't ever get in the state where DOM Worker is conflicting with DOM
Worker ... so that collapses the two cases

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode235
frog/world.dart:235: _isDomWorker(Element e) {
personally I'd put this as a getter on Element. Even though we only use it from
world right now: "named.isWebWorker" reads better imho (or "isWorker" -- but
Worker is kind of generic, and most people are familiar with the "web worker"
term)

[Siggi Cherem (dart-lang), 2011-12-22 21:17:23]

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart
File frog/world.dart (right):

http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode235
frog/world.dart:235: _isDomWorker(Element e) {
On 2011/12/22 19:49:44, John Messerly wrote:
> personally I'd put this as a getter on Element. Even though we only use it
from
> world right now: "named.isWebWorker" reads better imho (or "isWorker" -- but
> Worker is kind of generic, and most people are familiar with the "web worker"
> term)

Seems really strange to add this on Element. I don't know if this will be the
only type conflict that we will ever have though. An alternative I was
discussing with Matt was to change the if test above to be:

if (!_isConflictHarmless(named, element)) {
  ...
}

and define here:
_isConflictHarmless(e1, e2) {
  if (e1.library != dom && e1.library != corelib) return false;
  if (e2.library != dom && e2.library != corelib) return false;
  return e1.nativeName == 'Worker' && e2.nativeName == 'Worker';
}

[Jennifer Messerly, 2011-12-22 21:26:50]

On 2011/12/22 21:17:23, sigmund wrote:
> http://codereview.chromium.org/8999030/diff/3006/frog/world.dart
> File frog/world.dart (right):
> 
> http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode235
> frog/world.dart:235: _isDomWorker(Element e) {
> On 2011/12/22 19:49:44, John Messerly wrote:
> > personally I'd put this as a getter on Element. Even though we only use it
> from
> > world right now: "named.isWebWorker" reads better imho (or "isWorker" -- but
> > Worker is kind of generic, and most people are familiar with the "web
worker"
> > term)
> 
> Seems really strange to add this on Element. I don't know if this will be the
> only type conflict that we will ever have though. An alternative I was
> discussing with Matt was to change the if test above to be:
> 
> if (!_isConflictHarmless(named, element)) {
>   ...
> }
> 
> and define here:
> _isConflictHarmless(e1, e2) {
>   if (e1.library != dom && e1.library != corelib) return false;
>   if (e2.library != dom && e2.library != corelib) return false;
>   return e1.nativeName == 'Worker' && e2.nativeName == 'Worker';
> }

Types are Elements. You're just asking a Type if it's a Worker. We ask types
what kind of type they are all the time (isObject, isVar, isList, ...)

I don't see why you're trying so hard to ensure that e1 is from "dom", and e2 is
from "corelib". If it were me, I'd write that function as:

// Trust our builtin libs (dom, corelib, coreimpl) not to conflict with
eachother
bool _isConflictHarmless(e1, e2) => e1.library.isBuiltin &&
e2.library.isBuiltin;

class Library {
 ... 
  bool get isBuiltin() => name.startsWith('dart:');
 ...
}

(Honestly that entire "internalError" block could go away -- it was just to help
debugging potential conflicts, but they're unlikely in practice. that's why I
don't think it needs to be super precise about whitelisting Worker. If past
experience is any guide I suspect Worker won't be the only type we'll want to
whitelist in the long run)

[Siggi Cherem (dart-lang), 2011-12-22 21:50:41]

On 2011/12/22 21:26:50, John Messerly wrote:
> On 2011/12/22 21:17:23, sigmund wrote:
> > http://codereview.chromium.org/8999030/diff/3006/frog/world.dart
> > File frog/world.dart (right):
> > 
> > http://codereview.chromium.org/8999030/diff/3006/frog/world.dart#newcode235
> > frog/world.dart:235: _isDomWorker(Element e) {
> > On 2011/12/22 19:49:44, John Messerly wrote:
> > > personally I'd put this as a getter on Element. Even though we only use it
> > from
> > > world right now: "named.isWebWorker" reads better imho (or "isWorker" --
but
> > > Worker is kind of generic, and most people are familiar with the "web
> worker"
> > > term)
> > 
> > Seems really strange to add this on Element. I don't know if this will be
the
> > only type conflict that we will ever have though. An alternative I was
> > discussing with Matt was to change the if test above to be:
> > 
> > if (!_isConflictHarmless(named, element)) {
> >   ...
> > }
> > 
> > and define here:
> > _isConflictHarmless(e1, e2) {
> >   if (e1.library != dom && e1.library != corelib) return false;
> >   if (e2.library != dom && e2.library != corelib) return false;
> >   return e1.nativeName == 'Worker' && e2.nativeName == 'Worker';
> > }
> 
> Types are Elements. You're just asking a Type if it's a Worker. We ask types
> what kind of type they are all the time (isObject, isVar, isList, ...)
> 
> I don't see why you're trying so hard to ensure that e1 is from "dom", and e2
is
> from "corelib". If it were me, I'd write that function as:
> 
> // Trust our builtin libs (dom, corelib, coreimpl) not to conflict with
> eachother
> bool _isConflictHarmless(e1, e2) => e1.library.isBuiltin &&
> e2.library.isBuiltin;
> 
> class Library {
>  ... 
>   bool get isBuiltin() => name.startsWith('dart:');
>  ...
> }
> 
> (Honestly that entire "internalError" block could go away -- it was just to
help
> debugging potential conflicts, but they're unlikely in practice. that's why I
> don't think it needs to be super precise about whitelisting Worker. If past
> experience is any guide I suspect Worker won't be the only type we'll want to
> whitelist in the long run)

After our live discussion, I went ahead and removed this specialized code and
made this internalError an info message. We basically are now assuming that
builtin libraries don't conflict with each other. If later we open 'native' more
broadly, we'll revisit this check and do something more clever.