Eliminate unused url mapping parameter in dartutils

Eliminate unused url mapping parameter in dartutils

Change the LibraryTagHandler used in unit tests to match
the real tag handler better, eliminating special treatment
of url canonicalization. This required changing the library
url of tests so tests don’t get loaded using dart scheme urls,
since builtin.dart doesn’t handle dart: urls.

R=iposva@google.com

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

[Ivan Posva, 2014-05-07 22:04:06]

LGTM especially if we can completely remove CanonicalizeURL.

-Ivan

https://codereview.chromium.org/270503007/diff/1/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/270503007/diff/1/runtime/vm/unit_test.cc#newc...
runtime/vm/unit_test.cc:84: return DartUtils::CanonicalizeURL(library,
url_chars);
This is the only surviving call to CanonicalizeURL and it can probably be
morphed into ResolveUri.

[hausner, 2014-05-08 23:10:02]

PTAL.

https://codereview.chromium.org/270503007/diff/1/runtime/vm/unit_test.cc
File runtime/vm/unit_test.cc (right):

https://codereview.chromium.org/270503007/diff/1/runtime/vm/unit_test.cc#newc...
runtime/vm/unit_test.cc:84: return DartUtils::CanonicalizeURL(library,
url_chars);
On 2014/05/07 22:04:06, Ivan Posva wrote:
> This is the only surviving call to CanonicalizeURL and it can probably be
> morphed into ResolveUri.

Eliminated this use of CanonicalizeURL, which required a series of changes to
the library tag handler and unit tests.

[Ivan Posva, 2014-05-09 06:31:35]

Thanks for the additional cleanup.

LGTM

-Ivan

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/benchmark_tes...
File runtime/vm/benchmark_test.cc (right):

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/benchmark_tes...
runtime/vm/benchmark_test.cc:441: TestCase::LoadCoreTestScript(kScriptChars,
NULL);
Please add a comment explaining why LoadCoreTestScript was needed here. As far
as I understand it is due to the dart:_internal import, but it would be good to
document this fact.

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/benchmark_tes...
runtime/vm/benchmark_test.cc:470: TestCase::LoadCoreTestScript(kScriptChars,
NULL);
ditto

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:5032: Dart_Handle lib =
TestCase::LoadCoreTestScript(kScriptChars, NULL);
ditto

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1088: Dart_Handle h_lib =
TestCase::LoadCoreTestScript(kScript, NULL);
It is not clear why you need LoadCoreTestScript here.

[hausner, 2014-05-09 17:07:18]

Thank you.

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/benchmark_tes...
File runtime/vm/benchmark_test.cc (right):

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/benchmark_tes...
runtime/vm/benchmark_test.cc:441: TestCase::LoadCoreTestScript(kScriptChars,
NULL);
On 2014/05/09 06:31:35, Ivan Posva wrote:
> Please add a comment explaining why LoadCoreTestScript was needed here. As far
> as I understand it is due to the dart:_internal import, but it would be good
to
> document this fact.

Done. Yes, it's because importing dart:_internal can only be done from a dart:
library.

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/benchmark_tes...
runtime/vm/benchmark_test.cc:470: TestCase::LoadCoreTestScript(kScriptChars,
NULL);
On 2014/05/09 06:31:35, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/dart_api_impl...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/dart_api_impl...
runtime/vm/dart_api_impl_test.cc:5032: Dart_Handle lib =
TestCase::LoadCoreTestScript(kScriptChars, NULL);
On 2014/05/09 06:31:35, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/270503007/diff/20001/runtime/vm/service_test....
runtime/vm/service_test.cc:1088: Dart_Handle h_lib =
TestCase::LoadCoreTestScript(kScript, NULL);
On 2014/05/09 06:31:35, Ivan Posva wrote:
> It is not clear why you need LoadCoreTestScript here.
Looking at this again, the test fooled me. It does not need to be loaded into a
core library. The test outputs a number of error messages about not being able
to find native methods, but it passes anyway.

[hausner, 2014-05-09 18:25:50]

Committed patchset #3 manually as r35987 (presubmit successful).