fixes #157, renaming local library identifiers if needed.

fixes #157, renaming local library identifiers if needed.

readability regression, but fairly simple fix. looks okay in libraries with few imports, bad if they have a lot. ultimately I think this gets cleaned up as part of emitting modules + handling module lowering in JS codegen, see issue #34.

R=vsm@google.com

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

[Jennifer Messerly, 2015-05-05 19:33:28]

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

[vsm, 2015-05-05 22:01:19]

lgtm

https://codereview.chromium.org/1122133003/diff/20001/lib/runtime/dart/_inter...
File lib/runtime/dart/_interceptors.js (right):

https://codereview.chromium.org/1122133003/diff/20001/lib/runtime/dart/_inter...
lib/runtime/dart/_interceptors.js:1: var _interceptors, core, _js_helper,
_internal, _foreign_helper, _js_embedded_names;
It might be more readable to just list these out line by line.  E.g.,:

var _js_helper = _js_helper || {};

Alternatively,

var core = dart.required(core); // Asserts core is loaded already
var _js_helper = dart.lazy(_js_helper); // Defined if needed.

[Jennifer Messerly, 2015-05-05 22:04:45]

thanks! going to make that change and send it back

https://codereview.chromium.org/1122133003/diff/20001/lib/runtime/dart/_inter...
File lib/runtime/dart/_interceptors.js (right):

https://codereview.chromium.org/1122133003/diff/20001/lib/runtime/dart/_inter...
lib/runtime/dart/_interceptors.js:1: var _interceptors, core, _js_helper,
_internal, _foreign_helper, _js_embedded_names;
On 2015/05/05 22:01:18, vsm wrote:
> It might be more readable to just list these out line by line.  E.g.,:
> 
> var _js_helper = _js_helper || {};
> 
> Alternatively,
> 
> var core = dart.required(core); // Asserts core is loaded already
> var _js_helper = dart.lazy(_js_helper); // Defined if needed.

sounds good! yeah, almost did something like that but wasn't sure how much to
change, this is exactly the nudge I needed :). Really like the dart.required
assertion idea.

[Jennifer Messerly, 2015-05-05 22:23:46]

PTAL

[vsm, 2015-05-06 00:35:56]

https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:163: var helper =
_libraryMightNotBeLoaded(library) ? 'import' : 'lazyImport';
Shouldn't this be switched?  I.e., lazyImport if the library might not be
loaded?

[Jennifer Messerly, 2015-05-06 16:04:54]

https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:163: var helper =
_libraryMightNotBeLoaded(library) ? 'import' : 'lazyImport';
On 2015/05/06 00:35:55, vsm wrote:
> Shouldn't this be switched?  I.e., lazyImport if the library might not be
> loaded?

haha, yes. I'll fix, does CL look good otherwise?

[vsm, 2015-05-06 18:16:14]

On 2015/05/06 16:04:54, John Messerly wrote:
>
https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
> File lib/src/codegen/js_codegen.dart (right):
> 
>
https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
> lib/src/codegen/js_codegen.dart:163: var helper =
> _libraryMightNotBeLoaded(library) ? 'import' : 'lazyImport';
> On 2015/05/06 00:35:55, vsm wrote:
> > Shouldn't this be switched?  I.e., lazyImport if the library might not be
> > loaded?
> 
> haha, yes. I'll fix, does CL look good otherwise?

Yes, LGTM other than that!

[Jennifer Messerly, 2015-05-06 20:46:06]

nice, fixed. landing!

[Jennifer Messerly, 2015-05-06 20:47:43]

Committed patchset #5 (id:80001) manually as
d9fadc32b2fa2efc382d56a610abd2570e5497ff (presubmit successful).

[Jennifer Messerly, 2015-05-06 20:52:30]

https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1122133003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:163: var helper =
_libraryMightNotBeLoaded(library) ? 'import' : 'lazyImport';
On 2015/05/06 16:04:54, John Messerly wrote:
> On 2015/05/06 00:35:55, vsm wrote:
> > Shouldn't this be switched?  I.e., lazyImport if the library might not be
> > loaded?
> 
> haha, yes. I'll fix, does CL look good otherwise?

Oh funny, I figured out how this happened. Merging it into my branch fixing
use-before-define issues, I had inverted this function, it's now
`_libraryIsLoaded` instead of `_libraryMightNotBeLoaded` ... hah. Now resolving
that conflict ... :)