Re-enable and add new HTML support

Re-enable and add new HTML support

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/cb8b774fef4a7d9530e2ff274849af288b0cb343

[Brian Wilkerson, 2015-06-15 20:31:56]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2015-06-15 21:34:39]

lgtm

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:782: return new
AnalysisErrorInfoImpl(null, null);
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:796: List<Source>
getHtmlFilesReferencing(Source source) {
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:812: return Source.EMPTY_LIST;
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").

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:858: List<Source>
getLibrariesReferencedFromHtml(Source htmlSource) {
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1033: Document parseDocument(Source
source) {
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1043: return null;
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1461: return Source.EMPTY_LIST;
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/engine.dart:783: @deprecated // use
parseDocument(source)
Rather than use an end-of-line comment, I'd recommend putting a sentence in the
doc comment pointing the client to [parseDocument].  That way it will be seen by
client authors in their autocomplete popups and on the documentation web site.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/html.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/html.dart:23: final
ListResultDescriptor<AnalysisError> DOCUMENT_ERRORS =
I'm concerned about the use of the term "document" here too, for similar reasons
to what I stated before.  How about "HTML_DOCUMENT_ERRORS"?

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/html.dart:180: Source source =
context.sourceFactory.resolveUri(target.source, src);
This presumes that all the rules for resolving URI's in HTML files are the same
as for resolving URI's in .dart files (e.g. "package:" and "dart:" URI's are
permitted, and relative URI's are resolved relative to the referencing file.) 
Is that in fact the case, or are URI's in HTML files more restricted?

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/html.dart:191: libraries.isEmpty ? Source.EMPTY_LIST :
libraries;
Again, it seems of dubious benefit to me to canonicalize the empty list.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/engine_test.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/engine_test.dart:5756: fail("Unexpected invocation
of analysisCache");
s/analysisCache/parseDocument/

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

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/context.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:782: return new
AnalysisErrorInfoImpl(null, null);
Done

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:796: List<Source>
getHtmlFilesReferencing(Source source) {
> Is it supposed to compute an approximate result based on the analysis that has
been done so far...

Yes.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:812: return Source.EMPTY_LIST;
> Nit: it seems like the benefit this confers is tiny, and I'm not convinced
it's
> worth the effort.

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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:858: List<Source>
getLibrariesReferencedFromHtml(Source htmlSource) {
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.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1033: Document parseDocument(Source
source) {
Done

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1043: return null;
> Instead of returning null, I'd suggest doing "throw new UnimplementedError();"
...

Done

> I'd also recommend adding a TODO comment ...

Done

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/context.dart:1461: return Source.EMPTY_LIST;
Done

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/engine.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/engine.dart:783: @deprecated // use
parseDocument(source)
> Rather than use an end-of-line comment ...

That's probably a good idea. I did it this way to be consistent with every other
place we've marked something as deprecated. I think I'll leave it as is for now
and change them all at once.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/html.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/html.dart:23: final
ListResultDescriptor<AnalysisError> DOCUMENT_ERRORS =
Done

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/html.dart:180: Source source =
context.sourceFactory.resolveUri(target.source, src);
> This presumes that all the rules for resolving URI's in HTML files are the
same
> as for resolving URI's in .dart files (e.g. "package:" and "dart:" URI's are
> permitted, and relative URI's are resolved relative to the referencing file.) 
> Is that in fact the case, or are URI's in HTML files more restricted?

I don't know whether package: or dart: URI's are allowed, but relative URI's
should be resolved relative to the referencing file. If we're handling more than
we should we can always come back and add an error message (while still
understanding the user's intent).

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/html.dart:191: libraries.isEmpty ? Source.EMPTY_LIST :
libraries;
> Again, it seems of dubious benefit to me to canonicalize the empty list.

I know that we're storing these lists, so the value depends on how many HTML
files we end up analyzing.

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/engine_test.dart (right):

https://codereview.chromium.org/1182303006/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/engine_test.dart:5756: fail("Unexpected invocation
of analysisCache");
Done

[Brian Wilkerson, 2015-06-16 16:52:23]

Committed patchset #2 (id:20001) manually as
cb8b774fef4a7d9530e2ff274849af288b0cb343 (presubmit successful).