Re-enable source maps

Re-enable source maps

This appears to have bit-rotted.

Fixes #384

R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/7dc5564ce9104bf7c31bcae23092d017ac9d7eaa

[vsm, 2015-11-09 15:51:09]

Description was changed from

==========
Re-enable source maps

This appears to have bit-rotted.

Fixes #384
==========

to

==========
Re-enable source maps

This appears to have bit-rotted.

Fixes #384
==========

[vsm, 2015-11-09 15:51:09]

vsm@google.com changed reviewers:
+ devoncarew@google.com, jmesserly@google.com

[Jennifer Messerly, 2015-11-09 18:38:46]

https://codereview.chromium.org/1425313003/diff/1/lib/src/codegen/js_printer....
File lib/src/codegen/js_printer.dart (right):

https://codereview.chromium.org/1425313003/diff/1/lib/src/codegen/js_printer....
lib/src/codegen/js_printer.dart:76: if (node.parent is CompilationUnit) {
We'll keep making the URI over and over, right? Since many things have the
CompilationUnit as their parent.

The idea of the old code is we'd visit the compilation unit at some point
(which, oops, should've been an invariant we asserted somewhere).

I think we lost the _visit call on the CompilationUnit. Maybe that's an
alternate way to fix this?

Alternatively, it's probably worth adding a unit != null check here, so we only
go down this path once, and then below, assert that unit must not be null. Also
exitNode needs to be changed to match the new logic here in enterNode ...

[vsm, 2015-11-09 19:15:42]

https://codereview.chromium.org/1425313003/diff/1/lib/src/codegen/js_printer....
File lib/src/codegen/js_printer.dart (right):

https://codereview.chromium.org/1425313003/diff/1/lib/src/codegen/js_printer....
lib/src/codegen/js_printer.dart:76: if (node.parent is CompilationUnit) {
On 2015/11/09 18:38:46, John Messerly wrote:
> We'll keep making the URI over and over, right? Since many things have the
> CompilationUnit as their parent.
> 
> The idea of the old code is we'd visit the compilation unit at some point
> (which, oops, should've been an invariant we asserted somewhere).
> 
> I think we lost the _visit call on the CompilationUnit. Maybe that's an
> alternate way to fix this?
> 
> Alternatively, it's probably worth adding a unit != null check here, so we
only
> go down this path once, and then below, assert that unit must not be null.
Also
> exitNode needs to be changed to match the new logic here in enterNode ...

If there is a natural visit on the CompilationUnit, that seems best - but since
we're mixing parts up, I didn't think there would be.  What do you think?

I'll make the other fixes you suggested.

https://codereview.chromium.org/1425313003/diff/1/lib/src/codegen/js_printer....
lib/src/codegen/js_printer.dart:76: if (node.parent is CompilationUnit) {
On 2015/11/09 18:38:46, John Messerly wrote:
> We'll keep making the URI over and over, right? Since many things have the
> CompilationUnit as their parent.
> 
> The idea of the old code is we'd visit the compilation unit at some point
> (which, oops, should've been an invariant we asserted somewhere).
> 
> I think we lost the _visit call on the CompilationUnit. Maybe that's an
> alternate way to fix this?
> 
> Alternatively, it's probably worth adding a unit != null check here, so we
only
> go down this path once, and then below, assert that unit must not be null.
Also
> exitNode needs to be changed to match the new logic here in enterNode ...

BTW, I tested this with Angular - seems like we have some declarations without a
unit?  The old code had a 

if (unit ==null && ... ) return;

Will dig in further.

[vsm, 2015-11-09 19:16:41]

PTAL

[Jennifer Messerly, 2015-11-09 23:21:11]

a few suggestions otherwise LGTM! Yeah, wow, this must've broke when we started
visiting things out of order to handle cycles between parts. Doh.

https://codereview.chromium.org/1425313003/diff/20001/lib/src/codegen/js_prin...
File lib/src/codegen/js_printer.dart (right):

https://codereview.chromium.org/1425313003/diff/20001/lib/src/codegen/js_prin...
lib/src/codegen/js_printer.dart:84: while (containingUnit is! CompilationUnit) {
I think this could be node.getAncestor((n) => n is CompilationUnit);

https://codereview.chromium.org/1425313003/diff/20001/lib/src/codegen/js_prin...
lib/src/codegen/js_printer.dart:118: if (_topLevelDeclarations.contains(node)) {
Could we just have a boolean "_isTopLevel" so we know to clear it? Or do they
nest sometimes? In which case, maybe a Stack. I presume enter/exit calls will
always be matched.

[vsm, 2015-11-10 14:35:48]

https://codereview.chromium.org/1425313003/diff/20001/lib/src/codegen/js_prin...
File lib/src/codegen/js_printer.dart (right):

https://codereview.chromium.org/1425313003/diff/20001/lib/src/codegen/js_prin...
lib/src/codegen/js_printer.dart:84: while (containingUnit is! CompilationUnit) {
On 2015/11/09 23:21:11, John Messerly wrote:
> I think this could be node.getAncestor((n) => n is CompilationUnit);

Done - thanks!

https://codereview.chromium.org/1425313003/diff/20001/lib/src/codegen/js_prin...
lib/src/codegen/js_printer.dart:118: if (_topLevelDeclarations.contains(node)) {
On 2015/11/09 23:21:11, John Messerly wrote:
> Could we just have a boolean "_isTopLevel" so we know to clear it? Or do they
> nest sometimes? In which case, maybe a Stack. I presume enter/exit calls will
> always be matched.

It does nest - I think it's proper nesting.  Modified to just keep track of the
current top-level decl.

[vsm, 2015-11-10 14:36:17]

Committed patchset #3 (id:40001) manually as
7dc5564ce9104bf7c31bcae23092d017ac9d7eaa (presubmit successful).