Only add JS once, and not to very single patched library.

Only add JS once, and not to very single patched library.


BUG=

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

[ahe, 2012-08-21 11:28:31]

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:86:
compiler.addForeignFunctions(library);
I'm not sure about this. What do you need?

[Anders Johnsen, 2012-08-21 11:40:23]

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:86:
compiler.addForeignFunctions(library);
On 2012/08/21 11:28:31, ahe wrote:
> I'm not sure about this. What do you need?

The problem is not here, but in the other file. We add JS scope to every library
that is being patches. This was okay before the latest changes, but the library
and the patch library is one and same library. This means that if we patch
isolate, we'll end up adding JS twice. My argument is that we should only add JS
(and the other helper methods in ForeignFunctions) here, and _only_ here.

[ahe, 2012-08-21 12:01:16]

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:86:
compiler.addForeignFunctions(library);
On 2012/08/21 11:40:23, ajohnsen wrote:
> On 2012/08/21 11:28:31, ahe wrote:
> > I'm not sure about this. What do you need?
> 
> The problem is not here, but in the other file. We add JS scope to every
library
> that is being patches. This was okay before the latest changes, but the
library
> and the patch library is one and same library. This means that if we patch
> isolate, we'll end up adding JS twice. My argument is that we should only add
JS
> (and the other helper methods in ForeignFunctions) here, and _only_ here.

I don't think the patch libraries need all the foreign functions. Some of them
are pretty powerful and should only be used in our interceptors.

If you're unifying dart:isolate, then I don't think you should need to enable
native at all for the isolate library.

[Anders Johnsen, 2012-08-21 14:31:10]

PTAL

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
File lib/compiler/implementation/native_handler.dart (right):

https://chromiumcodereview.appspot.com/10828413/diff/1/lib/compiler/implement...
lib/compiler/implementation/native_handler.dart:86:
compiler.addForeignFunctions(library);
On 2012/08/21 12:01:16, ahe wrote:
> On 2012/08/21 11:40:23, ajohnsen wrote:
> > On 2012/08/21 11:28:31, ahe wrote:
> > > I'm not sure about this. What do you need?
> > 
> > The problem is not here, but in the other file. We add JS scope to every
> library
> > that is being patches. This was okay before the latest changes, but the
> library
> > and the patch library is one and same library. This means that if we patch
> > isolate, we'll end up adding JS twice. My argument is that we should only
add
> JS
> > (and the other helper methods in ForeignFunctions) here, and _only_ here.
> 
> I don't think the patch libraries need all the foreign functions. Some of them
> are pretty powerful and should only be used in our interceptors.
> 
> If you're unifying dart:isolate, then I don't think you should need to enable
> native at all for the isolate library.

Thank you for explaining, I've reverted this particular change. I agree, don't
expose more then needed!

[ahe, 2012-08-21 14:46:12]

LGTM!