Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1334353002:
- Add getters for the current packageRoot or packageMap. (Closed)
|
Created:
5 years, 3 months ago by Ivan Posva Modified:
5 years, 2 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org, Lasse Reichstein Nielsen Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Description- Add getters for the current packageRoot or packageMap to the Isolate class.
- Simplify loading in builtin.dart.
BUG=
R=asiva@google.com, lrn@google.com
Committed: https://github.com/dart-lang/sdk/commit/ba6c0ace3ef417526fa6bf97082ac30e9986cca0
Patch Set 1 #
Total comments: 7
Patch Set 2 : Update tests and move to ToT. #
Total comments: 6
Patch Set 3 : Updated documentation comment. #
Total comments: 4
Patch Set 4 : Moved accessors for packageRoot/packageMap to class Isolate. #
Total comments: 2
Messages
Total messages: 14 (3 generated)
iposva@google.com changed reviewers: + asiva@google.com, floitsch@google.com, sethladd@google.com
lrn@google.com changed reviewers: + lrn@google.com
Platform part LGTM. (but I'd still like to have a blocking sync version too). https://codereview.chromium.org/1334353002/diff/1/runtime/bin/builtin.dart File runtime/bin/builtin.dart (right): https://codereview.chromium.org/1334353002/diff/1/runtime/bin/builtin.dart#ne... runtime/bin/builtin.dart:92: bool _packagesReady() => (_packageRoot != null) || (_packageMap != null); Consider making this a getter? https://codereview.chromium.org/1334353002/diff/1/runtime/bin/builtin.dart#ne... runtime/bin/builtin.dart:435: _pendingPackageLoads = []; The above loop should have left the _pendingPackageLoads empty. (It might still be a good idea to reallocate it, since it releases the backing store, and the next time you use it you will allocate a new backing store in newspace, but the comment doesn't make sense). https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:194: * not recognized. Change to: The package map maps recognized package names to a directory that package URIs for that package are resolved against. The "returning null" part is default behavior for maps..
lgtm https://codereview.chromium.org/1334353002/diff/1/runtime/bin/platform_patch.... File runtime/bin/platform_patch.dart (right): https://codereview.chromium.org/1334353002/diff/1/runtime/bin/platform_patch.... runtime/bin/platform_patch.dart:21: var hook = VMLibraryHooks.getPackageRoot; indentation seems to be off. https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:194: * not recognized. On 2015/09/14 12:56:09, Lasse Reichstein Nielsen wrote: > Change to: > The package map maps recognized package names to a directory that > package URIs for that package are resolved against. > > The "returning null" part is default behavior for maps.. The comment seems a bit hard to read maybe : The package map, maps recognized package names to a directory against which package URIs for that package are resolved.
https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:194: * not recognized. On 2015/09/14 18:12:07, siva wrote: > The comment seems a bit hard to read maybe : > The package map, maps recognized package names to a directory against which > package URIs for that package are resolved. I'm not sure how to parse that.
https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart File sdk/lib/io/platform.dart (right): https://codereview.chromium.org/1334353002/diff/1/sdk/lib/io/platform.dart#ne... sdk/lib/io/platform.dart:194: * not recognized. On 2015/09/15 07:49:53, Lasse Reichstein Nielsen wrote: > On 2015/09/14 18:12:07, siva wrote: > > The comment seems a bit hard to read maybe : > > The package map, maps recognized package names to a directory against which > > package URIs for that package are resolved. > > I'm not sure how to parse that. Lasse and I agreed on the following wording: The package map maps the name of a package that is available to the program, to a URI that package URIs for that package are resolved against.
lgtm https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... File pkg/analysis_server/test/integration/integration_tests.dart (right): https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... pkg/analysis_server/test/integration/integration_tests.dart:621: } Add a TODO to support --packages as well. https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... pkg/analysis_server/test/integration/integration_tests.dart:635: _process.exitCode.then((int code) { Tricky. They should consider documenting that this is not waiting for the .then call here. https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... File pkg/docgen/lib/src/generator.dart (left): https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... pkg/docgen/lib/src/generator.dart:283: '--categories=Client,Server'])..catchError((error) { I'm not sure what the ".." does here. It's ... suspicious. There is no reason to not use just ".". https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... File pkg/docgen/lib/src/generator.dart (right): https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... pkg/docgen/lib/src/generator.dart:293: } This is slightly different from the original because the original returns a future without a catchError. That's not something that can be simulated using an async function (it splits the future). It's also completely unnecessary, so I think the change is fine. https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/test/only_li... File pkg/docgen/test/only_lib_content_in_pkg_test.dart (right): https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/test/only_li... pkg/docgen/test/only_lib_content_in_pkg_test.dart:18: var packageRoot = (await Platform.packageRoot).toFilePath(); I don't think the toFilePath() should be there. Also, this appears to assume that Platform.packageRoot returns '' if it's not there, not null. Maybe just: var packageRoot = (await Platfrom.packageRoot)?.toFilePath(); https://codereview.chromium.org/1334353002/diff/20001/tests/standalone/full_c... File tests/standalone/full_coverage_test.dart (right): https://codereview.chromium.org/1334353002/diff/20001/tests/standalone/full_c... tests/standalone/full_coverage_test.dart:216: packageRoot = (await Platform.packageRoot).toFilePath(); May be null, use "?.toFilePath()". The test may need to support .packages (add a TODO somewhere). https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/cod... File tests/standalone/io/code_collection_test.dart (right): https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/cod... tests/standalone/io/code_collection_test.dart:48: } else { Add TODO: support .packages. https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/htt... File tests/standalone/io/http_client_stays_alive_test.dart (right): https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/htt... tests/standalone/io/http_client_stays_alive_test.dart:32: var arguments = ['--package-root=${(await Platform.packageRoot).toFilePath()}', Long line. .toFilePath() -> ?.toFilePath() https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... File tests/standalone/io/platform_test.dart (right): https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... tests/standalone/io/platform_test.dart:52: String platformPackageRoot = (await Platform.packageRoot).toFilePath(); . -> ?. https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... tests/standalone/io/platform_test.dart:61: var commandPackageRoot = arg.replaceFirst("--package-root=", ""); overkill :) var argPackageRoot = "--package-root="; if (!arg.startsWith(argPackageRoot)) return false; var content = arg.substring(argPackageRoot.length); return content == platformPackageRoot; (or is the "endsWith" necessary?)
On 2015/09/22 09:48:25, Lasse Reichstein Nielsen wrote: > lgtm > > https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... > File pkg/analysis_server/test/integration/integration_tests.dart (right): > > https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... > pkg/analysis_server/test/integration/integration_tests.dart:621: } > Add a TODO to support --packages as well. > > https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... > pkg/analysis_server/test/integration/integration_tests.dart:635: > _process.exitCode.then((int code) { > Tricky. They should consider documenting that this is not waiting for the .then > call here. > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > File pkg/docgen/lib/src/generator.dart (left): > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > pkg/docgen/lib/src/generator.dart:283: > '--categories=Client,Server'])..catchError((error) { > I'm not sure what the ".." does here. It's ... suspicious. There is no reason to > not use just ".". > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > File pkg/docgen/lib/src/generator.dart (right): > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > pkg/docgen/lib/src/generator.dart:293: } > This is slightly different from the original because the original returns a > future without a catchError. That's not something that can be simulated using an > async function (it splits the future). > It's also completely unnecessary, so I think the change is fine. > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/test/only_li... > File pkg/docgen/test/only_lib_content_in_pkg_test.dart (right): > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/test/only_li... > pkg/docgen/test/only_lib_content_in_pkg_test.dart:18: var packageRoot = (await > Platform.packageRoot).toFilePath(); > I don't think the toFilePath() should be there. Also, this appears to assume > that Platform.packageRoot returns '' if it's not there, not null. > > Maybe just: > > var packageRoot = (await Platfrom.packageRoot)?.toFilePath(); > > https://codereview.chromium.org/1334353002/diff/20001/tests/standalone/full_c... > File tests/standalone/full_coverage_test.dart (right): > > https://codereview.chromium.org/1334353002/diff/20001/tests/standalone/full_c... > tests/standalone/full_coverage_test.dart:216: packageRoot = (await > Platform.packageRoot).toFilePath(); > May be null, use "?.toFilePath()". > > The test may need to support .packages (add a TODO somewhere). > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/cod... > File tests/standalone/io/code_collection_test.dart (right): > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/cod... > tests/standalone/io/code_collection_test.dart:48: } else { > Add TODO: support .packages. > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/htt... > File tests/standalone/io/http_client_stays_alive_test.dart (right): > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/htt... > tests/standalone/io/http_client_stays_alive_test.dart:32: var arguments = > ['--package-root=${(await Platform.packageRoot).toFilePath()}', > Long line. > > .toFilePath() -> ?.toFilePath() > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... > File tests/standalone/io/platform_test.dart (right): > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... > tests/standalone/io/platform_test.dart:52: String platformPackageRoot = (await > Platform.packageRoot).toFilePath(); > . -> ?. > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... > tests/standalone/io/platform_test.dart:61: var commandPackageRoot = > arg.replaceFirst("--package-root=", ""); > overkill :) > > var argPackageRoot = "--package-root="; > if (!arg.startsWith(argPackageRoot)) return false; > var content = arg.substring(argPackageRoot.length); > return content == platformPackageRoot; > > (or is the "endsWith" necessary?) All of these comments became obsolete with the move of these properties to the Isolate class. Please take another look. Thanks, -Ivan
On 2015/10/01 11:31:55, Ivan Posva wrote: > On 2015/09/22 09:48:25, Lasse Reichstein Nielsen wrote: > > lgtm > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... > > File pkg/analysis_server/test/integration/integration_tests.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... > > pkg/analysis_server/test/integration/integration_tests.dart:621: } > > Add a TODO to support --packages as well. > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/analysis_server/tes... > > pkg/analysis_server/test/integration/integration_tests.dart:635: > > _process.exitCode.then((int code) { > > Tricky. They should consider documenting that this is not waiting for the > .then > > call here. > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > > File pkg/docgen/lib/src/generator.dart (left): > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > > pkg/docgen/lib/src/generator.dart:283: > > '--categories=Client,Server'])..catchError((error) { > > I'm not sure what the ".." does here. It's ... suspicious. There is no reason > to > > not use just ".". > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > > File pkg/docgen/lib/src/generator.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/lib/src/gene... > > pkg/docgen/lib/src/generator.dart:293: } > > This is slightly different from the original because the original returns a > > future without a catchError. That's not something that can be simulated using > an > > async function (it splits the future). > > It's also completely unnecessary, so I think the change is fine. > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/test/only_li... > > File pkg/docgen/test/only_lib_content_in_pkg_test.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/20001/pkg/docgen/test/only_li... > > pkg/docgen/test/only_lib_content_in_pkg_test.dart:18: var packageRoot = (await > > Platform.packageRoot).toFilePath(); > > I don't think the toFilePath() should be there. Also, this appears to assume > > that Platform.packageRoot returns '' if it's not there, not null. > > > > Maybe just: > > > > var packageRoot = (await Platfrom.packageRoot)?.toFilePath(); > > > > > https://codereview.chromium.org/1334353002/diff/20001/tests/standalone/full_c... > > File tests/standalone/full_coverage_test.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/20001/tests/standalone/full_c... > > tests/standalone/full_coverage_test.dart:216: packageRoot = (await > > Platform.packageRoot).toFilePath(); > > May be null, use "?.toFilePath()". > > > > The test may need to support .packages (add a TODO somewhere). > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/cod... > > File tests/standalone/io/code_collection_test.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/cod... > > tests/standalone/io/code_collection_test.dart:48: } else { > > Add TODO: support .packages. > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/htt... > > File tests/standalone/io/http_client_stays_alive_test.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/htt... > > tests/standalone/io/http_client_stays_alive_test.dart:32: var arguments = > > ['--package-root=${(await Platform.packageRoot).toFilePath()}', > > Long line. > > > > .toFilePath() -> ?.toFilePath() > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... > > File tests/standalone/io/platform_test.dart (right): > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... > > tests/standalone/io/platform_test.dart:52: String platformPackageRoot = (await > > Platform.packageRoot).toFilePath(); > > . -> ?. > > > > > https://codereview.chromium.org/1334353002/diff/40001/tests/standalone/io/pla... > > tests/standalone/io/platform_test.dart:61: var commandPackageRoot = > > arg.replaceFirst("--package-root=", ""); > > overkill :) > > > > var argPackageRoot = "--package-root="; > > if (!arg.startsWith(argPackageRoot)) return false; > > var content = arg.substring(argPackageRoot.length); > > return content == platformPackageRoot; > > > > (or is the "endsWith" necessary?) > > All of these comments became obsolete with the move of these properties to the > Isolate class. Please take another look. > > Thanks, > -Ivan P.S. There is no test for packageMap as that is not yet wired through the Isolate.spawnUri API.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as ba6c0ace3ef417526fa6bf97082ac30e9986cca0 (presubmit successful).
Message was sent while issue was closed.
nweiz@google.com changed reviewers: + nweiz@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/1334353002/diff/60001/sdk/lib/isolate/isolate... File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/1334353002/diff/60001/sdk/lib/isolate/isolate... sdk/lib/isolate/isolate.dart:136: external static Future<Uri> get packageRoot; Are there any guarantees about whether this is absolute?
Message was sent while issue was closed.
https://codereview.chromium.org/1334353002/diff/60001/sdk/lib/isolate/isolate... File sdk/lib/isolate/isolate.dart (right): https://codereview.chromium.org/1334353002/diff/60001/sdk/lib/isolate/isolate... sdk/lib/isolate/isolate.dart:136: external static Future<Uri> get packageRoot; It should be absolute. We should add this to the documentation. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
