Re-enable and add new HTML support

pkg/analyzer/lib/src/context/context.dart

Index: pkg/analyzer/lib/src/context/context.dart
diff --git a/pkg/analyzer/lib/src/context/context.dart b/pkg/analyzer/lib/src/context/context.dart
index 52ab424cb7f83e9e331cdc5b2a166229551f11ce..6b1a8a4d9a0e5f5e011e0efd894309973ff1aca0 100644
--- a/pkg/analyzer/lib/src/context/context.dart
+++ b/pkg/analyzer/lib/src/context/context.dart
@@ -36,7 +36,9 @@ import 'package:analyzer/src/task/driver.dart';
 import 'package:analyzer/src/task/manager.dart';
 import 'package:analyzer/task/dart.dart';
 import 'package:analyzer/task/general.dart';
+import 'package:analyzer/task/html.dart';
 import 'package:analyzer/task/model.dart';
+import 'package:html/dom.dart' show Document;
 
 /**
  * Type of callback functions used by PendingFuture. Functions of this type
@@ -560,16 +562,27 @@ class AnalysisContextImpl implements InternalAnalysisContext {
   }
 
   @override
-  List<AnalysisError> computeErrors(Source source) =>
-      _computeResult(source, DART_ERRORS);
+  List<AnalysisError> computeErrors(Source source) {
+    String name = source.shortName;
+    if (AnalysisEngine.isDartFileName(name)) {
+      return _computeResult(source, DART_ERRORS);
+    } else if (AnalysisEngine.isHtmlFileName(name)) {
+      return _computeResult(source, HTML_ERRORS);
+    }
+    return AnalysisError.NO_ERRORS;
+  }
 
   @override
   List<Source> computeExportedLibraries(Source source) =>
       _computeResult(source, EXPORTED_LIBRARIES);
 
   @override
-  // TODO(brianwilkerson) Implement this.
-  HtmlElement computeHtmlElement(Source source) => null;
+  HtmlElement computeHtmlElement(Source source) {
+    // TODO(brianwilkerson) Implement this. For the time being, we compute the
+    // data that will implicitly be computed by creating an element.
+    _computeResult(source, REFERENCED_LIBRARIES);
+    return null;
+  }
 
   @override
   List<Source> computeImportedLibraries(Source source) =>
@@ -758,7 +771,15 @@ class AnalysisContextImpl implements InternalAnalysisContext {
 
   @override
   AnalysisErrorInfo getErrors(Source source) {
-    return dartWorkManager.getErrors(source);
+    String name = source.shortName;
+    if (AnalysisEngine.isDartFileName(name)) {
+      return dartWorkManager.getErrors(source);
+    } else if (AnalysisEngine.isHtmlFileName(name)) {
+      List<AnalysisError> errors = analysisCache.getValue(source, HTML_ERRORS);
+      // TODO(brianwilkerson) We don't currently have line info for HTML files.
+      return new AnalysisErrorInfoImpl(errors, null);
+    }
+    return new AnalysisErrorInfoImpl(null, null);

[Paul Berry, 2015/06/15 21:34:38]

The old task model does this instead:

return new AnalysisErrorInfoImpl(AnalysisError.NO_ERRORS, null);

Would that be better?  It might help avoid NPE's in clients.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done

   }
 
   @override
@@ -773,39 +794,20 @@ class AnalysisContextImpl implements InternalAnalysisContext {
 
   @override
   List<Source> getHtmlFilesReferencing(Source source) {

[Paul Berry, 2015/06/15 21:34:38]

What's the contract on this function?  Is it supposed to compute an approximate
result based on the analysis that has been done so far, or is it supposed to
parse and analyze the HTML files as needed before returning so that it can
produce a precise result?  I can't tell from the documentation above this method
in the AnalysisContext class.

It looks the the current implementation computes an approximate result based on
the analysis that has been done so far.

[Brian Wilkerson, 2015/06/16 16:28:08]

Yes.

-    // TODO(brianwilkerson) Implement this.
-    SourceKind sourceKind = getKindOf(source);
-    if (sourceKind == null) {
+    if (!AnalysisEngine.isDartFileName(source.shortName)) {
       return Source.EMPTY_LIST;
     }
     List<Source> htmlSources = <Source>[];
-//    while (true) {
-//      if (sourceKind == SourceKind.PART) {
-//        List<Source> librarySources = getLibrariesContaining(source);
-//        for (Source source in _cache.sources) {
-//          CacheEntry entry = _cache.get(source);
-//          if (entry.getValue(SOURCE_KIND) == SourceKind.HTML) {
-//            List<Source> referencedLibraries =
-//                (entry as HtmlEntry).getValue(HtmlEntry.REFERENCED_LIBRARIES);
-//            if (_containsAny(referencedLibraries, librarySources)) {
-//              htmlSources.add(source);
-//            }
-//          }
-//        }
-//      } else {
-//        for (Source source in _cache.sources) {
-//          CacheEntry entry = _cache.get(source);
-//          if (entry.getValue(SOURCE_KIND) == SourceKind.HTML) {
-//            List<Source> referencedLibraries =
-//                (entry as HtmlEntry).getValue(HtmlEntry.REFERENCED_LIBRARIES);
-//            if (_contains(referencedLibraries, source)) {
-//              htmlSources.add(source);
-//            }
-//          }
-//        }
-//      }
-//      break;
-//    }
+    List<Source> librarySources = getLibrariesContaining(source);
+    for (Source source in _cache.sources) {
+      if (AnalysisEngine.isHtmlFileName(source.shortName)) {
+        List<Source> referencedLibraries =
+            analysisCache.getValue(source, REFERENCED_LIBRARIES);
+        if (_containsAny(referencedLibraries, librarySources)) {
+          htmlSources.add(source);
+        }
+      }
+    }
     if (htmlSources.isEmpty) {
       return Source.EMPTY_LIST;

[Paul Berry, 2015/06/15 21:34:38]

Nit: it seems like the benefit this confers is tiny, and I'm not convinced it's
worth the effort.  If we drop these three lines and just return htmlSources, the
client won't see any difference in behavior (unless they are doing something
bizarre, like trying to detect an empty list by comparing the result to
Source.EMPTY_LIST instead of simply calling ".isEmpty").

[Brian Wilkerson, 2015/06/16 16:28:08]

The places where we've seen benefit are where the result is being stored. I
don't know if that's happening with the result of this method, so it might not
be necessary here, but it made a noticeable difference in some other places to
store lots of pointers to a single empty list rather than lots of pointers to
lots of empty lists. I think I'll leave it for now.

     }
@@ -854,12 +856,10 @@ class AnalysisContextImpl implements InternalAnalysisContext {
 
   @override
   List<Source> getLibrariesReferencedFromHtml(Source htmlSource) {

[Paul Berry, 2015/06/15 21:34:38]

Similar concern about the contract on this function--it seems to return an
approximate result based on analysis so far, and I see no indication in the
comments to lead the user to expect this behavior.

[Brian Wilkerson, 2015/06/16 16:28:08]

In general, the "get" methods are lazy, returning results if we have them and
not doing any work if we don't. There's usually a corresponding "compute" method
that does whatever work is necessary, but there isn't one for this method. But
given that the old implementation is also lazy, I think this one should be too.

Personally, I think we need to redesign the AnalysisContext API at some point.

-    // TODO(brianwilkerson) Implement this.
-//    cache.CacheEntry entry = getReadableSourceEntryOrNull(htmlSource);
-//    if (entry is HtmlEntry) {
-//      HtmlEntry htmlEntry = entry;
-//      return htmlEntry.getValue(HtmlEntry.REFERENCED_LIBRARIES);
-//    }
+    CacheEntry entry = _cache.get(htmlSource);
+    if (entry != null) {
+      return entry.getValue(REFERENCED_LIBRARIES);
+    }
     return Source.EMPTY_LIST;
   }
 
@@ -1030,12 +1030,17 @@ class AnalysisContextImpl implements InternalAnalysisContext {
   }
 
   @override
-  ht.HtmlUnit parseHtmlUnit(Source source) {
+  Document parseDocument(Source source) {

[Paul Berry, 2015/06/15 21:34:38]

How about calling this "parseHtmlDocument" instead?  I'm concerned that to the
author of a client of AnalysisContext (particularly a client that doesn't need
the analyzer's HTML abilities), it won't be obvious what kind of document we're
talking about.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done

     if (!AnalysisEngine.isHtmlFileName(source.shortName)) {
       return null;
     }
-    // TODO(brianwilkerson) Implement HTML analysis.
-    return null; //_computeResult(source, null);
+    return _computeResult(source, DOCUMENT);
+  }
+
+  @override
+  @deprecated // use parseDocument(source)
+  ht.HtmlUnit parseHtmlUnit(Source source) {
+    return null;

[Paul Berry, 2015/06/15 21:34:38]

Instead of returning null, I'd suggest doing "throw new UnimplementedError();"
since that will make it a little easier for a client author to track down what
is going wrong.

I'd also recommend adding a TODO comment to remind us that once we've switched
over to the new task model, we should remove the parseHtmlUnit() method from the
AnalysisContext class, so that client authors will be alerted to the fact that
their code is going to break before runtime.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done
Done

   }
 
   @override
@@ -1058,20 +1063,6 @@ class AnalysisContextImpl implements InternalAnalysisContext {
     });
   }
 
-  void _evaluatePendingFutures() {
-    for (AnalysisTarget target in _pendingFutureTargets.keys) {
-      CacheEntry cacheEntry = _cache.get(target);
-      List<PendingFuture> pendingFutures = _pendingFutureTargets[target];
-      for (int i = 0; i < pendingFutures.length;) {
-        if (pendingFutures[i].evaluate(cacheEntry)) {
-          pendingFutures.removeAt(i);
-        } else {
-          i++;
-        }
-      }
-    }
-  }
-
   @override
   void recordLibraryElements(Map<Source, LibraryElement> elementMap) {
     elementMap.forEach((Source librarySource, LibraryElement library) {
@@ -1327,6 +1318,19 @@ class AnalysisContextImpl implements InternalAnalysisContext {
   }
 
   /**
+   * Return `true` if the given list of [sources] contains any of the given
+   * [targetSources].
+   */
+  bool _containsAny(List<Source> sources, List<Source> targetSources) {
+    for (Source targetSource in targetSources) {
+      if (_contains(sources, targetSource)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /**
    * Set the contents of the given [source] to the given [contents] and mark the
    * source as having changed. The additional [offset], [oldLength] and
    * [newLength] information is used by the context to determine what reanalysis
@@ -1402,6 +1406,20 @@ class AnalysisContextImpl implements InternalAnalysisContext {
     return entries;
   }
 
+  void _evaluatePendingFutures() {
+    for (AnalysisTarget target in _pendingFutureTargets.keys) {
+      CacheEntry cacheEntry = _cache.get(target);
+      List<PendingFuture> pendingFutures = _pendingFutureTargets[target];
+      for (int i = 0; i < pendingFutures.length;) {
+        if (pendingFutures[i].evaluate(cacheEntry)) {
+          pendingFutures.removeAt(i);
+        } else {
+          i++;
+        }
+      }
+    }
+  }
+
   /**
    * Return a list containing all of the change notices that are waiting to be
    * returned. If there are no notices, then return either `null` or an empty
@@ -1425,11 +1443,22 @@ class AnalysisContextImpl implements InternalAnalysisContext {
    */
   List<Source> _getSources(SourceKind kind) {
     List<Source> sources = <Source>[];
-    for (Source source in _cache.sources) {
-      CacheEntry entry = _cache.get(source);
-      if (entry.getValue(SOURCE_KIND) == kind) {
-        sources.add(source);
+    if (kind == SourceKind.LIBRARY || kind == SourceKind.PART) {
+      for (Source source in _cache.sources) {
+        CacheEntry entry = _cache.get(source);
+        if (entry.getValue(SOURCE_KIND) == kind) {
+          sources.add(source);
+        }
       }
+    } else if (kind == SourceKind.HTML) {
+      for (Source source in _cache.sources) {
+        if (AnalysisEngine.isHtmlFileName(source.shortName)) {
+          sources.add(source);
+        }
+      }
+    }
+    if (sources == null) {
+      return Source.EMPTY_LIST;

[Paul Berry, 2015/06/15 21:34:38]

Unreachable code.  Sources will never be null.

See also my comment on line 812 above--even if we change the condition to "if
(sources.isEmpty)" I'm not convinced that it confers enough benefit to be worth
it.

[Brian Wilkerson, 2015/06/16 16:28:08]

Done

     }
     return sources;
   }