use Analyzer's computed constants instead of pattern matching the annotation AST

use Analyzer's computed constants instead of pattern matching the annotation AST

R=jacobr@google.com, paulberry@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/7589cca41af1e14a9e3b1e039a2ec04e32b726f7

[Jennifer Messerly, 2015-04-14 17:17:47]

jmesserly@google.com changed reviewers:
+ jacobr@google.com

[Jennifer Messerly, 2015-04-14 17:17:48]

Follow up to previous @JsName.name change, this time in a cleaner way. 

Really cool we can operate on the evaluated constant value, not just the AST. It
uncovered that we have this annotation defined in two places, but I didn't want
to muck with that yet :)

[Paul Berry, 2015-04-14 17:51:26]

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

[Paul Berry, 2015-04-14 17:51:27]

Interaction with analyzer lgtm, with one minor nit:

https://codereview.chromium.org/1084783005/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1084783005/diff/1/lib/src/utils.dart#newcode386
lib/src/utils.dart:386: return result is String ? result : null;
Not sure if it matters, but the analyzer stores both strings and symbols using
String internally.  So if you implement it this way, getStringConstantField will
interpret @JsName(@HTMLFontElement) as equivalent to @JsName('HTMLFontElement').

If you care about this and want to avoid it, consider checking fieldValue.type,
e.g.:

Element fieldClass = fieldValue.type.element;
if (fieldClass == null || !fieldClass.library.isDartCore || fieldClass.name !=
'String') return null;

Or, if you have easy access to the type provider, you can simply do:

if (fieldValue.type != typeProvider.stringType) return null;

[Jacob, 2015-04-14 20:54:33]

lgtm

https://codereview.chromium.org/1084783005/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1084783005/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2583: // TODO(jmesserly): move JsName to dart:js
until we move it to dart:js can't we just be permissive and allow it any
library?
e.g. only check for
type.name == 'JsName'
Otherwise it is a

[Jennifer Messerly, 2015-04-15 00:21:26]

Thanks! PTAL. I addressed those, as well as removed some *Impl suffixes and
TODOs that are no longer needed.

https://codereview.chromium.org/1084783005/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1084783005/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2583: // TODO(jmesserly): move JsName to dart:js
On 2015/04/14 20:54:33, Jacob wrote:
> until we move it to dart:js can't we just be permissive and allow it any
> library?
> e.g. only check for
> type.name == 'JsName'
> Otherwise it is a  

hmmm. The current situation does indeed seem odd. Filed
https://github.com/dart-lang/dev_compiler/issues/135, for now I can remove the
check, though I suspect we'll end up with two libraries in the end (public one,
and _internal for our SDK libs to use)

https://codereview.chromium.org/1084783005/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1084783005/diff/1/lib/src/utils.dart#newcode386
lib/src/utils.dart:386: return result is String ? result : null;
On 2015/04/14 17:51:27, Paul Berry wrote:
> Not sure if it matters, but the analyzer stores both strings and symbols using
> String internally.  So if you implement it this way, getStringConstantField
will
> interpret @JsName(@HTMLFontElement) as equivalent to
@JsName('HTMLFontElement').
> 
> If you care about this and want to avoid it, consider checking
fieldValue.type,
> e.g.:
> 
> Element fieldClass = fieldValue.type.element;
> if (fieldClass == null || !fieldClass.library.isDartCore || fieldClass.name !=
> 'String') return null;
> 
> Or, if you have easy access to the type provider, you can simply do:
> 
> if (fieldValue.type != typeProvider.stringType) return null;

ah good catch! I'll update it to fix that.

[Jacob, 2015-04-15 00:34:41]

lgtm

https://codereview.chromium.org/1084783005/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1084783005/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2525: // TODO(jmesserly): validate the library.
See issue #135.
Is this the style we're following for referencing issue #s?

[Jennifer Messerly, 2015-04-15 00:38:39]

https://codereview.chromium.org/1084783005/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1084783005/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2525: // TODO(jmesserly): validate the library.
See issue #135.
On 2015/04/15 00:34:41, Jacob wrote:
> Is this the style we're following for referencing issue #s?

No idea. I used to paste full links, but other github projects I've seen
sometimes use just numbers. In GitHub Flavored Markdown they auto-link, but the
code renderer doesn't do this (and in any case, most other tools wouldn't).

[Jennifer Messerly, 2015-04-15 00:38:50]

Committed patchset #3 (id:40001) manually as
7589cca41af1e14a9e3b1e039a2ec04e32b726f7 (presubmit successful).