Remove _WorkerStub hack to make benefit closing an isolate.

Remove _WorkerStub hack to make benefit closing an isolate.

R=ngeoffray@google.com

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

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

[ahe, 2013-05-13 16:59:37]

A tricky situation that occurred after switching native classes to use
interceptors.

To provoke this error, you need to create a Worker (using dart:html) and an
isolate in the same program.  Now there are two native types that match "Worker"
and that's not good with interceptors.  I'd say it wasn't good when monkey
patching, but it sorta worked.

https://codereview.chromium.org/15132006/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(left):

https://codereview.chromium.org/15132006/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:535:
_globalState.managers.remove(sender.id);
Before this call would fail.  The interceptor for Worker doesn't not have a
get$id method.

[sra1, 2013-05-14 03:25:28]

I never understood the hack so I'll let Nicolas review it.

https://chromiumcodereview.appspot.com/15132006/diff/1/dart/sdk/lib/_internal...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):

https://chromiumcodereview.appspot.com/15132006/diff/1/dart/sdk/lib/_internal...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:538:
static getId(/* Worker or _ManagerStub */ worker) {
Should these methods have private names?  Many other methods here do.

[ahe, 2013-05-14 06:11:10]

https://chromiumcodereview.appspot.com/15132006/diff/1/dart/sdk/lib/_internal...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):

https://chromiumcodereview.appspot.com/15132006/diff/1/dart/sdk/lib/_internal...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:538:
static getId(/* Worker or _ManagerStub */ worker) {
On 2013/05/14 03:25:28, sra1 wrote:
> Should these methods have private names?  Many other methods here do.

I don't understand why the other methods are private. IsolateNatives is not
accessible from user code so it's just an artificial barrier for other compiler
engineers. I'm a strong believer in not making methods package private unless
doing so would expose functionality to users that shouldn't be exposed.

Since package privacy in Dart isn't just a convention you can work around in a
tight spot when debugging by prints, I prefer to not use it at all unless I
must.

[ngeoffray, 2013-05-14 07:23:43]

LGTM! Thanks for fixing this Peter.

@Stephen: What would be the best place to assert that two native Dart classes
are not using the same JS prototype?

[ahe, 2013-05-14 11:16:52]

Committed patchset #3 manually as r22669 (presubmit successful).

[ngeoffray, 2013-05-14 11:20:47]

DBC

https://codereview.chromium.org/15132006/diff/10001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/15132006/diff/10001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2500: //
checks and needs to handle 'native' classes for 'holders'.
You can remove the TODO.

[ahe, 2013-05-14 11:38:53]

I reopened bug 9556.

https://codereview.chromium.org/15132006/diff/10001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/15132006/diff/10001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2500: //
checks and needs to handle 'native' classes for 'holders'.
On 2013/05/14 11:20:47, ngeoffray wrote:
> You can remove the TODO.

I already committed the CL.

[ahe, 2013-05-14 18:27:15]

PTAL

[ngeoffray, 2013-05-15 08:33:10]

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart
(right):

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2526: //
TODO(9556): Remove 'holders'.  The general interceptor is used on type
This is the correct fix, so you can remove the TODO. There is still one
reference to 9556 in native_emitter, so Stephen or I will fix it.

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:398:
JS("void", r"self.postMessage(#)", msg);
What's 'self'?

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:657: //
We also store the id on the worker itself so that we can unregister it.
Update comment?

[ahe, 2013-05-15 09:06:30]

Thank you for taking a look.

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:398:
JS("void", r"self.postMessage(#)", msg);
On 2013/05/15 08:33:10, ngeoffray wrote:
> What's 'self'?

self is a way to refer to the global context object that works in HTML pages and
in Web Workers.  It does not work in d8 and Firefox jsshell, because that would
have been too easy.

See: http://www.w3.org/TR/workers/#the-global-scope
and: http://www.w3.org/TR/Window/#dfn-self-attribute

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:657: //
We also store the id on the worker itself so that we can unregister it.
On 2013/05/15 08:33:10, ngeoffray wrote:
> Update comment?

I actually think the comment still applies, I'm just using an expando.  What you
you say?

[ngeoffray, 2013-05-15 09:09:56]

LGTM

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:398:
JS("void", r"self.postMessage(#)", msg);
On 2013/05/15 09:06:30, ahe wrote:
> On 2013/05/15 08:33:10, ngeoffray wrote:
> > What's 'self'?
> 
> self is a way to refer to the global context object that works in HTML pages
and
> in Web Workers.  It does not work in d8 and Firefox jsshell, because that
would
> have been too easy.
> 
> See: http://www.w3.org/TR/workers/#the-global-scope
> and: http://www.w3.org/TR/Window/#dfn-self-attribute

Thanks for the explanation, could you add it to the code. And why can't you use
globalThis then?

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:657: //
We also store the id on the worker itself so that we can unregister it.
On 2013/05/15 09:06:30, ahe wrote:
> On 2013/05/15 08:33:10, ngeoffray wrote:
> > Update comment?
> 
> I actually think the comment still applies, I'm just using an expando.  What
you
> you say?

Fine by me.

[ahe, 2013-05-15 09:19:08]

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart
(right):

https://codereview.chromium.org/15132006/diff/23001/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/lib/isolate_helper.dart:398:
JS("void", r"self.postMessage(#)", msg);
On 2013/05/15 09:09:56, ngeoffray wrote:
> On 2013/05/15 09:06:30, ahe wrote:
> > On 2013/05/15 08:33:10, ngeoffray wrote:
> > > What's 'self'?
> > 
> > self is a way to refer to the global context object that works in HTML pages
> and
> > in Web Workers.  It does not work in d8 and Firefox jsshell, because that
> would
> > have been too easy.
> > 
> > See: http://www.w3.org/TR/workers/#the-global-scope
> > and: http://www.w3.org/TR/Window/#dfn-self-attribute
> 
> Thanks for the explanation, could you add it to the code. And why can't you
use
> globalThis then?

I'd like to get rid of globalThis, and I'd like to educate people on the self
property that works well across DOM and web workers.

I have this idea about adding a JS command-line support shim, for example:

(function() {
  this.self = this;
  function Console() {
  }
  Console.prototype.log = function(message) { print(message); }
  this.console = new Console();
  this.importScripts = function(/* Array */ scripts) {
    for (var i = 0; i < scripts.length; i++) {
      load(scripts[i]);
    }
  }
})()

This way we can simplify how we check for presence of features making
hello-world much smaller.

[ahe, 2013-05-16 15:06:49]

Committed patchset #6 manually as r22796 (presubmit successful).