Make print interceptable.

Make print interceptable.

BUG= http://dartbug.com/3486
R=lrn@google.com

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

[floitsch, 2013-10-12 19:32:37]

This is not yet ready to commit, but very close.

Missing pieces:
- how do we avoid dart2js to blow up when doing a simple hello world?
- dartium bindings would need to get updated. (looks like a trivial change).
- tests.

[Lasse Reichstein Nielsen, 2013-10-14 07:45:34]

https://codereview.chromium.org/27112002/diff/1/sdk/lib/core/core.dart
File sdk/lib/core/core.dart (right):

https://codereview.chromium.org/27112002/diff/1/sdk/lib/core/core.dart#newcod...
sdk/lib/core/core.dart:160: import "dart:async" show Zone;
Core imports async. Is that acceptable (initial size-wise)? 
Could we put Zone in a library of its own, reexported by async? (But I guess
Zone would use async a lot anyway).

[floitsch, 2013-10-14 13:28:47]

PTAL.
Size for "Hello World" is now much better, but unfortunately still very
noticeable (from 8931 bytes to 16134).

[vsm, 2013-10-14 14:49:30]

https://codereview.chromium.org/27112002/diff/8001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/27112002/diff/8001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:669: Dart_Handle result = Dart_SetField(internal_lib,
Florian: I assume this is the only thing we'd need to change in Dartium?

E.g., in line 177 here:
http://src.chromium.org/viewvc/multivm/trunk/webkit/Source/bindings/dart/Dart...

s/dart:core/dart:_collection-dev/

?

[floitsch, 2013-10-14 15:17:31]

https://codereview.chromium.org/27112002/diff/8001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/27112002/diff/8001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:669: Dart_Handle result = Dart_SetField(internal_lib,
On 2013/10/14 14:49:30, vsm wrote:
> Florian: I assume this is the only thing we'd need to change in Dartium?
> 
> E.g., in line 177 here:
>
http://src.chromium.org/viewvc/multivm/trunk/webkit/Source/bindings/dart/Dart...
> 
> s/dart:core/dart:_collection-dev/
> 
> ?
correct.

[floitsch, 2013-10-15 12:50:15]

PTAL.
@vsm: what should I do for dartium?

Size-increase is now down to a few bytes (for dart2js).

[kasperl, 2013-10-15 13:14:45]

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_collection_dev/p...
File sdk/lib/_collection_dev/print.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:7: /**
Isn't this a seriously weird place for this functionality to live? What does it
have to do with collections?

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:16: Function zonePrint = null;
I'd consider using print as a prefix (so something like printToZone,
printToConsole).

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
sdk/lib/_internal/lib/collection_dev_patch.dart:13:
Primitives.printString(line);
Maybe inline the Primitives.printString method in here?

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/async/async.dart
File sdk/lib/async/async.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/async/async.dart#...
sdk/lib/async/async.dart:21: import "dart:_collection-dev" show deprecated,
zonePrint, internalPrint;
internalPrint -> consolePrint?

[floitsch, 2013-10-15 13:46:48]

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_collection_dev/p...
File sdk/lib/_collection_dev/print.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:7: /**
On 2013/10/15 13:14:46, kasperl wrote:
> Isn't this a seriously weird place for this functionality to live? What does
it
> have to do with collections?

_collection_dev is currently our internal library.
We need to rename it to _internal_lib or similar, but since it's not user-facing
it never went high enough on our TODO lists.

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:16: Function zonePrint = null;
On 2013/10/15 13:14:46, kasperl wrote:
> I'd consider using print as a prefix (so something like printToZone,
> printToConsole).

Done.

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
sdk/lib/_internal/lib/collection_dev_patch.dart:13:
Primitives.printString(line);
On 2013/10/15 13:14:46, kasperl wrote:
> Maybe inline the Primitives.printString method in here?

Could do, but I thought that we wanted to avoid "JS" outside the js_helper
library.
Should I inline?

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/async/async.dart
File sdk/lib/async/async.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/async/async.dart#...
sdk/lib/async/async.dart:21: import "dart:_collection-dev" show deprecated,
zonePrint, internalPrint;
On 2013/10/15 13:14:46, kasperl wrote:
> internalPrint -> consolePrint?

Done.

[kasperl, 2013-10-15 14:01:27]

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
sdk/lib/_internal/lib/collection_dev_patch.dart:13:
Primitives.printString(line);
On 2013/10/15 13:46:49, floitsch wrote:
> On 2013/10/15 13:14:46, kasperl wrote:
> > Maybe inline the Primitives.printString method in here?
> 
> Could do, but I thought that we wanted to avoid "JS" outside the js_helper
> library.
> Should I inline?

I think it would be nice to get rid of Primitives.printString. We do use the JS
helper from outside libraries -- for one example you can take a look at the
patch that contains hash maps, etc.

[Lasse Reichstein Nielsen, 2013-10-15 14:12:02]

lgtm

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/_collection_dev/p...
File sdk/lib/_collection_dev/print.dart (right):

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:8: * This function is set by the first
allocation of a Zone.
And it is updated to always match [Zone.current] ?

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:14: * programs.
Too technical.
Just say that it decouples Zones from the core library.

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/core/print.dart
File sdk/lib/core/print.dart (right):

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/core/print.dart#n...
sdk/lib/core/print.dart:10: printToZone(line);
Switch the two cases? 
I find it slightly easier to read a positive test.

Do we have any idea whether the majority of programs will use one or the other?

[floitsch, 2013-10-15 15:56:19]

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
sdk/lib/_internal/lib/collection_dev_patch.dart:13:
Primitives.printString(line);
On 2013/10/15 14:01:27, kasperl wrote:
> On 2013/10/15 13:46:49, floitsch wrote:
> > On 2013/10/15 13:14:46, kasperl wrote:
> > > Maybe inline the Primitives.printString method in here?
> > 
> > Could do, but I thought that we wanted to avoid "JS" outside the js_helper
> > library.
> > Should I inline?
> 
> I think it would be nice to get rid of Primitives.printString. We do use the
JS
> helper from outside libraries -- for one example you can take a look at the
> patch that contains hash maps, etc.

Done.

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/_collection_dev/p...
File sdk/lib/_collection_dev/print.dart (right):

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:8: * This function is set by the first
allocation of a Zone.
On 2013/10/15 14:12:02, Lasse Reichstein Nielsen wrote:
> And it is updated to always match [Zone.current] ?

No need. The function calls Zone.current.print(line).

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/_collection_dev/p...
sdk/lib/_collection_dev/print.dart:14: * programs.
On 2013/10/15 14:12:02, Lasse Reichstein Nielsen wrote:
> Too technical.
> Just say that it decouples Zones from the core library.

Done.

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/core/print.dart
File sdk/lib/core/print.dart (right):

https://codereview.chromium.org/27112002/diff/23001/sdk/lib/core/print.dart#n...
sdk/lib/core/print.dart:10: printToZone(line);
On 2013/10/15 14:12:02, Lasse Reichstein Nielsen wrote:
> Switch the two cases? 
done.

> I find it slightly easier to read a positive test.
> 
> Do we have any idea whether the majority of programs will use one or the
other?

Once a zone is instantiated it will be printToZone. So it depends on the usage
of zones.

[floitsch, 2013-10-15 16:02:24]

Committed patchset #8 manually as r28650 (presubmit successful).

[Ivan Posva, 2013-10-16 07:19:51]

-Ivan

https://codereview.chromium.org/27112002/diff/34001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/27112002/diff/34001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:665:
Dart_LookupLibrary(NewString("dart:_collection-dev"));
In my opinion this has nothing to do with any collections and we keep throwing
more stuff in there that doesn't belong.

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:525: _CustomizedZone parent = _degelationTarget;
Why is this a _CustomizedZone when all the other code expects a _BaseZone for
the _delegationTarget?

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:530: parent, new _ZoneDelegate(parent.parent), zone,
line);
See question below about repeatedly allocating _ZoneDelegates.

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:698: new _ZoneDelegate(this).print(this, line);
I am wondering whether it is really necessary to allocate a new _ZoneDelegate
for each operation in a _CustomizedZone.

[floitsch, 2013-10-16 07:38:55]

https://codereview.chromium.org/27112002/diff/34001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/27112002/diff/34001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:665:
Dart_LookupLibrary(NewString("dart:_collection-dev"));
On 2013/10/16 07:19:51, Ivan Posva wrote:
> In my opinion this has nothing to do with any collections and we keep throwing
> more stuff in there that doesn't belong.

Yes. we want to rename it, but since it's not user-visible it never reached the
top of our TODO lists.
Filed dartbug.com/14140.

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart
File sdk/lib/async/zone.dart (right):

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:525: _CustomizedZone parent = _degelationTarget;
On 2013/10/16 07:19:51, Ivan Posva wrote:
> Why is this a _CustomizedZone when all the other code expects a _BaseZone for
> the _delegationTarget?

That was a bad merge and mistake. Had to do a follow-up CL to unbreak the build.

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
sdk/lib/async/zone.dart:698: new _ZoneDelegate(this).print(this, line);
On 2013/10/16 07:19:51, Ivan Posva wrote:
> I am wondering whether it is really necessary to allocate a new _ZoneDelegate
> for each operation in a _CustomizedZone.

It's not and on my TODO list.

[Ivan Posva, 2013-10-16 07:44:42]

On 2013/10/16 07:38:55, floitsch wrote:
> https://codereview.chromium.org/27112002/diff/34001/runtime/bin/dartutils.cc
> File runtime/bin/dartutils.cc (right):
> 
>
https://codereview.chromium.org/27112002/diff/34001/runtime/bin/dartutils.cc#...
> runtime/bin/dartutils.cc:665:
> Dart_LookupLibrary(NewString("dart:_collection-dev"));
> On 2013/10/16 07:19:51, Ivan Posva wrote:
> > In my opinion this has nothing to do with any collections and we keep
throwing
> > more stuff in there that doesn't belong.
> 
> Yes. we want to rename it, but since it's not user-visible it never reached
the
> top of our TODO lists.
> Filed dartbug.com/14140.
> 
> https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart
> File sdk/lib/async/zone.dart (right):
> 
>
https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
> sdk/lib/async/zone.dart:525: _CustomizedZone parent = _degelationTarget;
> On 2013/10/16 07:19:51, Ivan Posva wrote:
> > Why is this a _CustomizedZone when all the other code expects a _BaseZone
for
> > the _delegationTarget?
> 
> That was a bad merge and mistake. Had to do a follow-up CL to unbreak the
build.
> 
>
https://codereview.chromium.org/27112002/diff/34001/sdk/lib/async/zone.dart#n...
> sdk/lib/async/zone.dart:698: new _ZoneDelegate(this).print(this, line);
> On 2013/10/16 07:19:51, Ivan Posva wrote:
> > I am wondering whether it is really necessary to allocate a new
_ZoneDelegate
> > for each operation in a _CustomizedZone.
> 
> It's not and on my TODO list.

Thanks for the clarifications.

-Ivan

[ahe, 2013-11-04 09:01:19]

DBC

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/_internal/lib/col...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/_internal/lib/col...
sdk/lib/_internal/lib/collection_dev_patch.dart:5: import 'dart:_foreign_helper'
show JS;
In the future, please do not add dart2js secret implementation details to
libraries that do not need them. In particular, do not import
dart:_foreign_helper.

In this case, you should have written:

import 'dart:_js_helper' show Primitives;

patch void printToConsole(String line) {
  Primitives.printString(line);
}

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_helper.dart (left):

https://codereview.chromium.org/27112002/diff/34001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:289: static void printString(String string)
{
In the future, do not move dart2js primitives out of the Primitives class.

The proper thing would have been to call Primitives.printString from
printToConsole.

[ahe, 2013-11-04 09:02:21]

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
File sdk/lib/_internal/lib/collection_dev_patch.dart (right):

https://codereview.chromium.org/27112002/diff/16001/sdk/lib/_internal/lib/col...
sdk/lib/_internal/lib/collection_dev_patch.dart:13:
Primitives.printString(line);
On 2013/10/15 14:01:27, kasperl wrote:
> On 2013/10/15 13:46:49, floitsch wrote:
> > On 2013/10/15 13:14:46, kasperl wrote:
> > > Maybe inline the Primitives.printString method in here?
> > 
> > Could do, but I thought that we wanted to avoid "JS" outside the js_helper
> > library.
> > Should I inline?
> 
> I think it would be nice to get rid of Primitives.printString. We do use the
JS
> helper from outside libraries -- for one example you can take a look at the
> patch that contains hash maps, etc.

OK conflicting messages.

I'll discuss this with Kasper.