When URL mapping was specified, library loads where not being called for the URLs that were mapped.

When URL mapping was specified, library loads where not being called for the URLs that were mapped.
Committed: https://code.google.com/p/dart/source/detail?r=1632

[Anton Muhin, 2011-11-17 18:15:36]

LGTM.  Feel free/postpone comments.

http://codereview.chromium.org/8588016/diff/5/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8588016/diff/5/bin/gen_snapshot.cc#newcode111
bin/gen_snapshot.cc:111: static bool MapLibraryUrl(const char* url_chars,
maybe consider passing only const char** url_chars to be overwritten by mapped
path if present.

http://codereview.chromium.org/8588016/diff/5/bin/gen_snapshot.cc#newcode172
bin/gen_snapshot.cc:172: if (tag == kCanonicalizeUrl) {
just a suggestion (feel free ignore).  Control flow might grow too complicated
what about something along those lines:

bool is_dart_scheme_url = strncmp(url_chars, kDartScheme,...)

if (tag == kCanonicalizeUrl) {
  if (is_dart_scheme_url) {
    return url;
  }
  // ln. 185--205 go here
} else {
  bool mapped = MapLibraryUrl(url_chars, &url_chars);
  if (dart_scheme_url && !mapped) {
     Dart_Error(...);
  }
  // Code of LoadSourceFile goes here.
}

Another benefit is we got rid of LoadSoureFile helper.
}

http://codereview.chromium.org/8588016/diff/5/bin/process_script.cc
File bin/process_script.cc (right):

http://codereview.chromium.org/8588016/diff/5/bin/process_script.cc#newcode52
bin/process_script.cc:52: static Dart_Handle LibraryTagHandler(Dart_LibraryTag
tag,
I believe this code is subject for refactoring, correct.

[siva, 2011-11-17 19:09:46]

Thanks.

http://codereview.chromium.org/8588016/diff/5/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8588016/diff/5/bin/gen_snapshot.cc#newcode111
bin/gen_snapshot.cc:111: static bool MapLibraryUrl(const char* url_chars,
I prefer keeping the input read only and not overwriting it in case there is a
need later to print it in some error or diagnostics.

On 2011/11/17 18:15:36, antonmuhin wrote:
> maybe consider passing only const char** url_chars to be overwritten by mapped
> path if present.

http://codereview.chromium.org/8588016/diff/5/bin/gen_snapshot.cc#newcode172
bin/gen_snapshot.cc:172: if (tag == kCanonicalizeUrl) {
Redid the loop per your suggestion.

On 2011/11/17 18:15:36, antonmuhin wrote:
> just a suggestion (feel free ignore).  Control flow might grow too complicated
> what about something along those lines:
> 
> bool is_dart_scheme_url = strncmp(url_chars, kDartScheme,...)
> 
> if (tag == kCanonicalizeUrl) {
>   if (is_dart_scheme_url) {
>     return url;
>   }
>   // ln. 185--205 go here
> } else {
>   bool mapped = MapLibraryUrl(url_chars, &url_chars);
>   if (dart_scheme_url && !mapped) {
>      Dart_Error(...);
>   }
>   // Code of LoadSourceFile goes here.
> }
> 
> Another benefit is we got rid of LoadSoureFile helper.
> }

http://codereview.chromium.org/8588016/diff/5/bin/process_script.cc
File bin/process_script.cc (right):

http://codereview.chromium.org/8588016/diff/5/bin/process_script.cc#newcode52
bin/process_script.cc:52: static Dart_Handle LibraryTagHandler(Dart_LibraryTag
tag,
Yes. I would like to remove including the file process_script.cc in
gen_snapshot.cc as this handler is
repeated.

On 2011/11/17 18:15:36, antonmuhin wrote:
> I believe this code is subject for refactoring, correct.

[Anton Muhin, 2011-11-17 19:15:07]

LGTM!