Remove LibraryEntity.libraryName

Remove LibraryEntity.libraryName

- use libraryName for LibraryElement.name if available.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/1498e533e0293a48bf74c84e4dd23b2e373c07dd

[Johnni Winther, 2017-01-13 09:27:29]

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

[Siggi Cherem (dart-lang), 2017-01-13 16:51:18]

https://codereview.chromium.org/2632643002/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/2632643002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/modelx.dart:1220: String get name =>
hasLibraryName ? libraryName : super.name;
I'm not sure I follow why we also need to change `name`. There are a few places
where we use `library.name` and it's not clear to me if we were assuming that it
defaults to script.name.

If we do this change, do we need libraryOrScriptName anymore?

[Johnni Winther, 2017-01-16 10:07:38]

https://codereview.chromium.org/2632643002/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/modelx.dart (right):

https://codereview.chromium.org/2632643002/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/modelx.dart:1220: String get name =>
hasLibraryName ? libraryName : super.name;
On 2017/01/13 16:51:18, Siggi Cherem (dart-lang) wrote:
> I'm not sure I follow why we also need to change `name`. There are a few
places
> where we use `library.name` and it's not clear to me if we were assuming that
it
> defaults to script.name.
> 
> If we do this change, do we need libraryOrScriptName anymore?

[name] is available on Entity but for a library it has no inherent semantics
since libraries are not referenced by name. Therefore we can use it as a (more
informal) discriminator.

It does seem to make [libraryOrScriptName] superfluous.

[Johnni Winther, 2017-02-07 09:00:06]

PTAL

[Siggi Cherem (dart-lang), 2017-02-07 23:31:53]

Do we need to fix js_backend/namer.dart:

  keyBase = library.name;

to something else? The old name was always the filename, right?

Nothing else obvious stands out from a quick search, but I wonder if we are
using `.name` in other places that this could affect it.

Related question: how far can we get without using the given library-name? If we
can, I'd love to avoid using the given library and mostly rely on the uri
internally, since it's guaranteed to be unique.

lgtm - modulo that we fix namer and any other use of the old .name

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:1081: /// Returns the library name
(as defined by the library tag) or for script
nit: script => scripts?

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/mi...
File pkg/compiler/lib/src/mirrors_used.dart (right):

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/mi...
pkg/compiler/lib/src/mirrors_used.dart:459: String libraryName = l.libraryName;
should this one be .name too?

[Johnni Winther, 2017-02-08 10:23:29]

On 2017/02/07 23:31:53, Siggi Cherem (dart-lang) wrote:
> Do we need to fix js_backend/namer.dart:
> 
>   keyBase = library.name;
> 
> to something else? The old name was always the filename, right?
> 
> Nothing else obvious stands out from a quick search, but I wonder if we are
> using `.name` in other places that this could affect it.
> 
> Related question: how far can we get without using the given library-name? If
we
> can, I'd love to avoid using the given library and mostly rely on the uri
> internally, since it's guaranteed to be unique.
> 
> lgtm - modulo that we fix namer and any other use of the old .name
> 

We don't need to fix the namer's uses of .name - these are suggestive anyway. If
we need a unique id we should add .canonicalUri to LibraryEntity.

I'd like LibraryEntity.name to be the canonical uri if the scheme is 'dart' or
'package' and the relative path otherwise. In any case .name must stay informal,
i.e. what we want to use in toString().

[Johnni Winther, 2017-02-08 10:27:10]

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/el...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/el...
pkg/compiler/lib/src/elements/elements.dart:1081: /// Returns the library name
(as defined by the library tag) or for script
On 2017/02/07 23:31:53, Siggi Cherem (dart-lang) wrote:
> nit: script => scripts?

Done.

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/mi...
File pkg/compiler/lib/src/mirrors_used.dart (right):

https://codereview.chromium.org/2632643002/diff/20001/pkg/compiler/lib/src/mi...
pkg/compiler/lib/src/mirrors_used.dart:459: String libraryName = l.libraryName;
On 2017/02/07 23:31:53, Siggi Cherem (dart-lang) wrote:
> should this one be .name too?

No. It is a deliberate dependency on the actual library name (it checks
.hasLibraryName before the use).

[Johnni Winther, 2017-02-08 10:37:09]

Description was changed from

==========
Remove LibraryEntity.libraryName

- use libraryName for LibraryElement.name if available.
==========

to

==========
Remove LibraryEntity.libraryName

- use libraryName for LibraryElement.name if available.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1498e533e0293a48bf74c84e4dd23b2e373c07dd
==========

[Johnni Winther, 2017-02-08 10:37:10]

Committed patchset #3 (id:40001) manually as
1498e533e0293a48bf74c84e4dd23b2e373c07dd (presubmit successful).