add support for package: to the editor

add support for package: to the editor 
Committed: https://code.google.com/p/dart/source/detail?r=7744

[keertip, 2012-05-16 23:31:37]

-  added compiler option for --package-root
-  added awareness to SystemLibraryManager for package:
-  augmented system library manager tests for package
-  preference to editor for package root
-  moved fonts preferences to separate page 
-  cleanup in plugin.xml's

[Brian Wilkerson, 2012-05-17 14:25:56]

LGTM. I didn't look very closely at the UI pieces, but everything else looks
fine, whether you choose to address my comments or not.

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/SystemLibraryManager.java (right):

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:120: public URI
expandRelativeDartUri(URI uri) throws AssertionError {
It seems strange to me to have a method with the "DartUri" in the name also
handle package: URI's. My first choice would be to have a separate "PackageUri"
method. My second choice would be to rename them to something more generic.

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:167: //
TODO(keertip): list the libraries in packages
I don't think there should ever be any bundled libraries in packages.

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/UrlLibrarySource.java (right):

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/UrlLibrarySource.java:1: // Copyright (c)
2011, the Dart project authors.  Please see the AUTHORS file
nit: copyright year

http://codereview.chromium.org/10377181/diff/2001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java
(right):

http://codereview.chromium.org/10377181/diff/2001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java:1: //
Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
nit: copyright year

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCore.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCore.java:114:
public static final String PACKAGE_ROOT = "package root";
Please comment this. I can't tell, without seeing where it's used, what this is.
My first guess would be that it's a directory path, but from the value I'm
guessing that's not right. (Also, please add a blank line after it.)

Looking at the next file I see that it's the name of a preference. Perhaps
renaming it to PACKAGE_ROOT_PREFERENCE_NAME would be more clear. Also, does it
really need to be in DartCore? I would prefer to see this localized to where
it's being used if that's possible.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.deploy/src/com/google/dart/tools/ui/build/CleanLibrariesAction.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.deploy/src/com/google/dart/tools/ui/build/CleanLibrariesAction.java:2:
* Copyright (c) 2011, the Dart project authors.
nit: copyright year

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.java:2:
* Copyright (c) 2011, the Dart project authors.
nit: copyright year

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.properties
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.properties:2:
# Copyright (c) 2000, 2010 IBM Corporation and others.
Shouldn't this copyright be changed?

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/problemsview/ProblemsView.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/problemsview/ProblemsView.java:2:
* Copyright (c) 2011, the Dart project authors.
nit: copyright year

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartOutlinePage.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartOutlinePage.java:572:
@Override
nit: What formatting setting is causing this annotation to get indented? My
settings don't do that.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/functions/DartOutlineInformationControl.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/functions/DartOutlineInformationControl.java:2:
* Copyright (c) 2011, the Dart project authors.
nit: copyright year

[keertip, 2012-05-17 15:49:58]

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/SystemLibraryManager.java (right):

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:120: public URI
expandRelativeDartUri(URI uri) throws AssertionError {
Rename is slated. It will be done in the next cl.

On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> It seems strange to me to have a method with the "DartUri" in the name also
> handle package: URI's. My first choice would be to have a separate
"PackageUri"
> method. My second choice would be to rename them to something more generic.

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:167: //
TODO(keertip): list the libraries in packages
Makes sense - removed

On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> I don't think there should ever be any bundled libraries in packages.

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/UrlLibrarySource.java (right):

http://codereview.chromium.org/10377181/diff/2001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/UrlLibrarySource.java:1: // Copyright (c)
2011, the Dart project authors.  Please see the AUTHORS file
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: copyright year

Done.

http://codereview.chromium.org/10377181/diff/2001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java
(right):

http://codereview.chromium.org/10377181/diff/2001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java:1: //
Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: copyright year

Done.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCore.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/DartCore.java:114:
public static final String PACKAGE_ROOT = "package root";
Added comments and renamed to make more sense. It is in the core plugin since
this value has to be passed on to the compiler.

 
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> Please comment this. I can't tell, without seeing where it's used, what this
is.
> My first guess would be that it's a directory path, but from the value I'm
> guessing that's not right. (Also, please add a blank line after it.)
> 
> Looking at the next file I see that it's the name of a preference. Perhaps
> renaming it to PACKAGE_ROOT_PREFERENCE_NAME would be more clear. Also, does it
> really need to be in DartCore? I would prefer to see this localized to where
> it's being used if that's possible.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.deploy/src/com/google/dart/tools/ui/build/CleanLibrariesAction.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.deploy/src/com/google/dart/tools/ui/build/CleanLibrariesAction.java:2:
* Copyright (c) 2011, the Dart project authors.
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: copyright year

Done.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.java:2:
* Copyright (c) 2011, the Dart project authors.
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: copyright year

Done.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.properties
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/PreferencesMessages.properties:2:
# Copyright (c) 2000, 2010 IBM Corporation and others.
Yes, indeed! done.

On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> Shouldn't this copyright be changed?

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/problemsview/ProblemsView.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/problemsview/ProblemsView.java:2:
* Copyright (c) 2011, the Dart project authors.
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: copyright year

Done.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartOutlinePage.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/DartOutlinePage.java:572:
@Override
Not sure, will figure it out.

On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: What formatting setting is causing this annotation to get indented? My
> settings don't do that.

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/functions/DartOutlineInformationControl.java
(right):

http://codereview.chromium.org/10377181/diff/2001/editor/tools/plugins/com.go...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/functions/DartOutlineInformationControl.java:2:
* Copyright (c) 2011, the Dart project authors.
On 2012/05/17 14:25:57, Brian Wilkerson wrote:
> nit: copyright year

Done.

[pquitslund, 2012-05-17 16:16:09]

LGTM

Fantastic.  Can't wait to give this a whirl!

http://codereview.chromium.org/10377181/diff/12001/editor/tools/plugins/com.g...
File editor/tools/plugins/com.google.dart.tools.deploy/plugin.xml (left):

http://codereview.chromium.org/10377181/diff/12001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.deploy/plugin.xml:54: </key>
Oh cool.  Good catch!

http://codereview.chromium.org/10377181/diff/12001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/DartBasePreferencePage.java
(right):

http://codereview.chromium.org/10377181/diff/12001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/DartBasePreferencePage.java:108:
job.schedule();
I'm guessing this causes a re-index/analysis and that it's noticeably slow?  If
so, we might consider (later) adding some kind of notification to the user
warning them that the world is getting rebuilt...

[keertip, 2012-05-17 17:57:27]

http://codereview.chromium.org/10377181/diff/12001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/DartBasePreferencePage.java
(right):

http://codereview.chromium.org/10377181/diff/12001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/preferences/DartBasePreferencePage.java:108:
job.schedule();
The job does show status while being run, but yes, you bring up a good point and
we can refine it going forward.

On 2012/05/17 16:16:09, pquitslund wrote:
> I'm guessing this causes a re-index/analysis and that it's noticeably slow? 
If
> so, we might consider (later) adding some kind of notification to the user
> warning them that the world is getting rebuilt...

[danrubel, 2012-05-17 20:19:44]

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
File compiler/java/com/google/dart/compiler/SystemLibraryManager.java (right):

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:49: public
static final File DEFAULT_PACKAGE_PATH = new File("packages");
DEFAULT_PACKAGE_ROOT

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:91: private
File packagePath;
Lets use "root" rather than "path" throughout to be consistent with the command
line option used in dart2js

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:210: }
Use same approach as above to convert file: back to package:

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:220: return
longToShortUriMap.get(uri);
if map returns null, then fall through and convert both file: and package:// to
package:

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java
(right):

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:135:
throws AssertionError, URISyntaxException {
Don't bother passing in packageRoot, but rather create a local 
new File("dirDoesNotExist") for the setPackagePath

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:137:
SystemLibraryManagerProvider.getAnyLibraryManager().setPackagePath(new
File(packageRoot));
get the existing package path and restore it on test complete... try / finally

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:148:

add tests for getRelativeUri and getShortUri

[keertip, 2012-05-17 22:09:12]

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
File compiler/java/com/google/dart/compiler/SystemLibraryManager.java (right):

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:49: public
static final File DEFAULT_PACKAGE_PATH = new File("packages");
On 2012/05/17 20:19:44, danrubel wrote:
> DEFAULT_PACKAGE_ROOT

Done.

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:91: private
File packagePath;
On 2012/05/17 20:19:44, danrubel wrote:
> Lets use "root" rather than "path" throughout to be consistent with the
command
> line option used in dart2js

Done.

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:210: }
On 2012/05/17 20:19:44, danrubel wrote:
> Use same approach as above to convert file: back to package:

Done.

http://codereview.chromium.org/10377181/diff/11003/compiler/java/com/google/d...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:220: return
longToShortUriMap.get(uri);
On 2012/05/17 20:19:44, danrubel wrote:
> if map returns null, then fall through and convert both file: and package://
to
> package:

Done.

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java
(right):

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:135:
throws AssertionError, URISyntaxException {
On 2012/05/17 20:19:44, danrubel wrote:
> Don't bother passing in packageRoot, but rather create a local 
> new File("dirDoesNotExist") for the setPackagePath

Done.

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:135:
throws AssertionError, URISyntaxException {
pointing to home dir instead, because using a non existent directory did not
resolve the uri correctly. 

On 2012/05/17 20:19:44, danrubel wrote:
> Don't bother passing in packageRoot, but rather create a local 
> new File("dirDoesNotExist") for the setPackagePath

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:137:
SystemLibraryManagerProvider.getAnyLibraryManager().setPackagePath(new
File(packageRoot));
On 2012/05/17 20:19:44, danrubel wrote:
> get the existing package path and restore it on test complete... try / finally

Done.

http://codereview.chromium.org/10377181/diff/11003/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.core_test/src/com/google/dart/tools/core/internal/model/SystemLibraryManagerProviderTest.java:148:

On 2012/05/17 20:19:44, danrubel wrote:
> add tests for getRelativeUri and getShortUri

Done.

[danrubel, 2012-05-17 23:07:39]

lgtm