dart2js: add --preserve-uris flag.

dart2js: add --preserve-uris flag.

R=blois@google.com, johnniwinther@google.com, nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=41205
Reverted: https://code.google.com/p/dart/source/detail?r=41206

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

[floitsch, 2014-09-18 14:48:56]

floitsch@google.com changed reviewers:
+ johnniwinther@google.com

[Johnni Winther, 2014-09-19 07:54:07]

lgtm

https://codereview.chromium.org/582753002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/582753002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:279: new
OptionHandler('--hide-uris', passThrough),
Should we document this is verbose help?

[blois, 2014-09-22 21:26:08]

blois@google.com changed reviewers:
+ blois@google.com

[blois, 2014-09-22 21:26:09]

https://codereview.chromium.org/582753002/diff/20001/tests/compiler/dart2js/h...
File tests/compiler/dart2js/hide_uri_test.dart (right):

https://codereview.chromium.org/582753002/diff/20001/tests/compiler/dart2js/h...
tests/compiler/dart2js/hide_uri_test.dart:52: Expect.equals(!hideUris,
jsOutput.contains("lib.dart"));
I'm curious what the output is from an import such as 'foo/bar.dart'- ideally it
would be 'foo/bar.dart'. This allows resolving URIs relative to the entry
script.

[floitsch, 2014-09-22 21:28:10]

https://codereview.chromium.org/582753002/diff/20001/tests/compiler/dart2js/h...
File tests/compiler/dart2js/hide_uri_test.dart (right):

https://codereview.chromium.org/582753002/diff/20001/tests/compiler/dart2js/h...
tests/compiler/dart2js/hide_uri_test.dart:52: Expect.equals(!hideUris,
jsOutput.contains("lib.dart"));
On 2014/09/22 21:26:09, blois wrote:
> I'm curious what the output is from an import such as 'foo/bar.dart'- ideally
it
> would be 'foo/bar.dart'. This allows resolving URIs relative to the entry
> script.

I'm pretty sure it's absolute.
Fwiw the VM has similar issues, and sometimes has relative and sometimes
absolute URIs.

[blois, 2014-09-22 22:41:06]

lgtm- the path appears to be relative 'foo/bar.dart' in the nested case.

FWIW, the generated JS did appear to contain a filesystem path to
runtime_data.dart.

[floitsch, 2014-09-26 12:51:42]

floitsch@google.com changed reviewers:
+ nweiz@google.com

[floitsch, 2014-09-26 12:51:43]

PTAL.
I made hiding the default, and added support for pub.

https://codereview.chromium.org/582753002/diff/20001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/582753002/diff/20001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:279: new
OptionHandler('--hide-uris', passThrough),
On 2014/09/19 07:54:07, Johnni Winther wrote:
> Should we document this is verbose help?

Done.

[nweiz, 2014-09-26 19:19:38]

What does this flag do?

https://codereview.chromium.org/582753002/diff/60001/sdk/lib/_internal/pub_ge...
File sdk/lib/_internal/pub_generated/lib/src/dart.dart (right):

https://codereview.chromium.org/582753002/diff/60001/sdk/lib/_internal/pub_ge...
sdk/lib/_internal/pub_generated/lib/src/dart.dart:33: if (preserveUris)
options.add('--preserve-uris');
This is generated code, not the canonical pub source (note "pub_generated" in
the filename). Any changes here will be overwritten next time the async_await
compiler is re-run.

The original sources are still in sdk/lib/_internal/pub. If you run sdk/bin/pub
after changing them, it will regenderate the generated sources.

[floitsch, 2014-09-26 21:25:04]

On 2014/09/26 19:19:38, nweiz wrote:
> What does this flag do?
See dart2js.dart

--preserve-uris
  Preserve the source URIs in the reflection data. Without this flag the
  `uri` getter for `LibraryMirror`s is mangled in minified mode.

> 
>
https://codereview.chromium.org/582753002/diff/60001/sdk/lib/_internal/pub_ge...
> File sdk/lib/_internal/pub_generated/lib/src/dart.dart (right):
> 
>
https://codereview.chromium.org/582753002/diff/60001/sdk/lib/_internal/pub_ge...
> sdk/lib/_internal/pub_generated/lib/src/dart.dart:33: if (preserveUris)
> options.add('--preserve-uris');
> This is generated code, not the canonical pub source (note "pub_generated" in
> the filename). Any changes here will be overwritten next time the async_await
> compiler is re-run.
> 
> The original sources are still in sdk/lib/_internal/pub. If you run
sdk/bin/pub
> after changing them, it will regenderate the generated sources.

[floitsch, 2014-09-26 21:25:13]

https://codereview.chromium.org/582753002/diff/60001/sdk/lib/_internal/pub_ge...
File sdk/lib/_internal/pub_generated/lib/src/dart.dart (right):

https://codereview.chromium.org/582753002/diff/60001/sdk/lib/_internal/pub_ge...
sdk/lib/_internal/pub_generated/lib/src/dart.dart:33: if (preserveUris)
options.add('--preserve-uris');
On 2014/09/26 19:19:37, nweiz wrote:
> This is generated code, not the canonical pub source (note "pub_generated" in
> the filename). Any changes here will be overwritten next time the async_await
> compiler is re-run.
> 
> The original sources are still in sdk/lib/_internal/pub. If you run
sdk/bin/pub
> after changing them, it will regenderate the generated sources.

Missed that. It would be nice if generated files were read-only to avoid this.

Done.

[nweiz, 2014-09-26 21:47:09]

lgtm

[floitsch, 2014-10-02 15:02:23]

floitsch@google.com changed reviewers:
+ ahe@google.com

[floitsch, 2014-10-02 15:02:24]

Updated URI to point to a site on dartlang.org (which will for now redirect to
https://code.google.com/p/dart/issues/detail?id=18903 )

Imho this is a great solution: we avoid emitting the uris by default, but it's
extremely easy to figure out why it's not included.

[ahe, 2014-10-02 15:07:12]

On 2014/10/02 15:02:24, floitsch wrote:
> Updated URI to point to a site on http://dartlang.org (which will for now
redirect to
> https://code.google.com/p/dart/issues/detail?id=18903 )
> 
> Imho this is a great solution: we avoid emitting the uris by default, but it's
> extremely easy to figure out why it's not included.

I don't think redirecting to a bug makes it easy to figure out what is going on.

[floitsch, 2014-10-02 15:09:52]

On 2014/10/02 15:07:12, ahe wrote:
> On 2014/10/02 15:02:24, floitsch wrote:
> > Updated URI to point to a site on http://dartlang.org (which will for now
> redirect to
> > https://code.google.com/p/dart/issues/detail?id=18903 )
> > 
> > Imho this is a great solution: we avoid emitting the uris by default, but
it's
> > extremely easy to figure out why it's not included.
> 
> I don't think redirecting to a bug makes it easy to figure out what is going
on.

I will update the bug when I land this CL, and we intend to add a nicer page
eventually.
Potentially we can just forward to a StackOverflow question, too. The important
thing is, that everything is in place.

[rhysbrettbowen, 2014-10-20 16:19:59]

On 2014/10/02 15:09:52, floitsch wrote:
> On 2014/10/02 15:07:12, ahe wrote:
> > On 2014/10/02 15:02:24, floitsch wrote:
> > > Updated URI to point to a site on http://dartlang.org (which will for now
> > redirect to
> > > https://code.google.com/p/dart/issues/detail?id=18903 )
> > > 
> > > Imho this is a great solution: we avoid emitting the uris by default, but
> it's
> > > extremely easy to figure out why it's not included.
> > 
> > I don't think redirecting to a bug makes it easy to figure out what is going
> on.
> 
> I will update the bug when I land this CL, and we intend to add a nicer page
> eventually.
> Potentially we can just forward to a StackOverflow question, too. The
important
> thing is, that everything is in place.

just checking how this is going?

[floitsch, 2014-10-20 16:33:13]

Committed patchset #6 (id:100001) manually as 41205 (presubmit successful).

[floitsch, 2014-10-20 16:42:17]

On 2014/10/20 16:33:13, floitsch wrote:
> Committed patchset #6 (id:100001) manually as 41205 (presubmit successful).

Was on my TODO list for this week.

[floitsch, 2014-10-20 16:48:28]

I had to revert.
Will debug tomorrow.

[floitsch, 2014-10-22 09:05:01]

Updated tests.
PTAL, but will land it TBR.

[floitsch, 2014-10-22 09:05:42]

Committed patchset #9 (id:160001) manually as 41244 (presubmit successful).

[Johnni Winther, 2014-10-22 12:33:13]

lgtm