Bugfix in DOM bindings: canonicalize URLs even if no ApplicationLoader is available

DartApplicationLoader.cpp

 // Copyright (c) 2009, Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 // notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
@@ skipping 68 matching lines @@
 
 void DartApplicationLoader::reportDartError(Dart_Handle error)
 {
     m_scriptHasError = true;
     DartUtilities::reportProblem(m_originDocument, error, m_libraryUrl);
 }
 
 Dart_Handle DartApplicationLoader::libraryTagHandlerCallback(Dart_LibraryTag tag, Dart_Handle library, Dart_Handle urlHandle)
 {
     ASSERT(Dart_CurrentIsolate());
-    ASSERT(DartDOMData::current()->applicationLoader());
-    return DartDOMData::current()->applicationLoader()->libraryTagHandler(tag, library, urlHandle);

[kustermann, 2013/07/10 13:50:39]

(Sidenote: AFAIK the styleguide allows long lines, so I didn't shorten them.)

-}
-
-Dart_Handle DartApplicationLoader::libraryTagHandler(Dart_LibraryTag tag, Dart_Handle library, Dart_Handle urlHandle)
-{
     ASSERT(Dart_IsLibrary(library));
 
     const String url = DartUtilities::toString(urlHandle);
 
     Dart_Handle libraryURLHandle = Dart_LibraryUrl(library);
     ASSERT(!Dart_IsError(libraryURLHandle));
     String libraryURL = DartUtilities::toString(libraryURLHandle);

[siva, 2013/07/10 18:10:26]

Why does this library to libraryURL translation have to happen eagerly. For the
case
url.startsWith("dart:") || .....
libraryURL is not used at all.

You could instead change the signature as:
static Dart_Handle CanonicalizeUrl(Dart_Handle urlHandle, String url,
Dart_Handle library);

and do the libraryURL translation inside CanonicalizeUrl if
url.startsWith("dart:") || url.startsWith("package:") is false.

[kustermann, 2013/07/11 12:34:11]

Done.

 
     if (tag == Dart_kCanonicalizeUrl) {
-        if (url.startsWith("dart:") || url.startsWith("package:"))
-            return urlHandle;
+          // If a dart application calls spawnUri, the DartVM will call this
+          // libraryTagHandler to canonicalize the url.
+          // DartDOMData::current()->applicationLoader() may be NULL at this point.
+          return DartApplicationLoader::CanonicalizeUrl(urlHandle, url, libraryURL);
+    } else {
+          ASSERT(DartDOMData::current()->applicationLoader());
+          return DartDOMData::current()->applicationLoader()->libraryTagHandler(tag, urlHandle, url, libraryURL);

[siva, 2013/07/10 18:10:26]

Similarly change the signature of libraryTagHandler to
Dart_Handle libraryTagHandler(Dart_LibraryTag tag, Dart_Handle urlHandle, String
url, Dart_Handle library);

and inside libraryTagHandler do the translation only if
tag == Dart_kSourceTag is true.

[kustermann, 2013/07/11 12:34:11]

Done.

+    }
+}
 
-        bool packageScheme = false;
-        if (libraryURL.startsWith("package:")) {
-            // KURL have problems concating package:foo/bar (without slashes right after colon)
-            // and relative urls. Therefore pretend to be a standard absolute URL.
-            packageScheme = true;
-            libraryURL = "http://" + libraryURL.substring(8);
-        }
+Dart_Handle DartApplicationLoader::CanonicalizeUrl(Dart_Handle urlHandle, String url, String libraryURL)
+{
+    if (url.startsWith("dart:") || url.startsWith("package:"))
+        return urlHandle;
 
-        const KURL canonical = KURL(KURL(KURL(), libraryURL), url);
-        String result = canonical.string();
-        if (packageScheme)
-            result = "package:" + result.substring(7);
-        return DartUtilities::stringToDartString(result);
+    bool packageScheme = false;
+    if (libraryURL.startsWith("package:")) {
+        // KURL have problems concating package:foo/bar (without slashes right after colon)

[siva, 2013/07/10 18:10:26]

KURL has problems......

[kustermann, 2013/07/11 12:34:11]

Done.

+        // and relative urls. Therefore pretend to be a standard absolute URL.
+        packageScheme = true;
+        libraryURL = "http://" + libraryURL.substring(8);

[siva, 2013/07/10 18:10:26]

These magic numbers 8, 7 etc. make me uncomfortable,
can we use named constants instead which are more descriptive
e.g:
const char* kPackagePrefix = "package:";
const int kPackagePrefixLength = strlen(kPackagePrefix);

const char* kHttpPrefix = "http://";
const int kHttpPrefixLength = strlen(kHttpPrefix);

[kustermann, 2013/07/11 12:34:11]

Done.

     }
 
+    const KURL canonical = KURL(KURL(KURL(), libraryURL), url);
+    String result = canonical.string();
+    if (packageScheme)
+        result = "package:" + result.substring(7);
+    return DartUtilities::stringToDartString(result);
+}
+
+Dart_Handle DartApplicationLoader::libraryTagHandler(Dart_LibraryTag tag, Dart_Handle urlHandle, String url, String libraryURL)
+{
     ASSERT(url != "dart:html");
 
     // Record the importer.
     if (tag == Dart_kImportTag)
         m_importedLibraries.add(url);
     else if (tag == Dart_kSourceTag)
         add(m_importersForSource, url, libraryURL);
     else
         ASSERT_NOT_REACHED();
 
@@ skipping 251 matching lines @@
     // We should never have a self dependence.
     ASSERT(key != value);
     UrlMultiMap::iterator iter = map.find(key);
     if (iter == map.end())
         iter = map.add(key, new UrlSet()).iterator;
     UrlSet* set = iter->value;
     set->add(value);
 }
 
 }