Web components based app to view dart docs. Still has rough edges.

Web components based app to view dart docs. Still has rough edges.

Please review the entire file contents rather than the diff. The previous version is just a checkpoint I accidentally pushed to master to avoid losing work if my local machine died.

Committed: https://github.com/dart-lang/dart-api-app/commit/e411036

[Andrei Mouravski, 2012-12-19 17:04:44]

Quick note.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/static/styl...
File client/web/static/styles.css (right):

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/static/styl...
client/web/static/styles.css:66: pre, code {
Please add:
padding: 0;

to fix: http://code.google.com/p/dart/issues/detail?id=4751

[Siggi Cherem (dart-lang), 2012-12-19 19:47:33]

The app is looking really cool.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart
File client/web/ast.dart (right):

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:1: library ast;
+ copyright
+ library level doc

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:14: List<String> LIBRARY_KINDS = ['variable', 'property',
'method', 'class', 'exception', 'typedef'];
style nit: 80 col (here and the line below)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:14: List<String> LIBRARY_KINDS = ['variable', 'property',
'method', 'class', 'exception', 'typedef'];
nit: rename to LIBRARY_ITEMS? for a moment I thought this was going to be kinds
of libraries, rather than kinds of items in a library that you'll display.

or LIBRARY_ELEMENTS - to match the Element terminology you use below

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:15: List<String> CLASS_KINDS = ['constructor', 'variable',
'property', 'method', 'operator'];
maybe rename as CLASS_ITEMS? or CLASS_ELEMENTS?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:22: final KIND_TITLES = {'property': 'Properties',
style nit: feel free to ignore, but I might prefer moving the hole list down one
line:

final Kind_TITLES = {
    'property': 'Properties',
    ...,
};

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:33: * Block of elements to render summary documentation for
that all share the
for that all => for all that

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:47: Reference jsonDeserializeReference(Map json) {
nit: consider reordering the code here to put the classes up front, and the json
deserialization at the bottom of the file?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:87: if (json != null) {
nit: consider switchting this to

if (json == null) return ret;

John and I are becoming big fans of this style :)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:97: if (json != null) {
same here

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:148: var result = <Element>[current];
'result' seems unused?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:154: next = child;
you could directly do:
 current = child;

and remove lines 151 and 158.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:176: //
https://github.com/dart-lang/dart-web-components/issues/212
dart-web-components ==> web-ui

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:184: int elementUiOrder(Element a, Element b) {
should Element implement Comparable and this function become 'compareTo' in
Element?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:196: /** Human readable type name for the node. */
nit: add empty line above comment, here and in every field below

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:207: final String _uri;
nit: move these private members down with the other private ones? One idea is to
put them next to the public properties that wrap them. For instance, put '_uri'
next to 'uri', and '_line' next to 'line'.
(same with _commentHtml, _typeParameters, etc)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:244: * Longer possibly multiple word description of the
[kind.
[kind. => [kind].

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:266: }
maybe use a recursive pattern here instead:

String get uri =>
  _uri != null ? _uri : (parent != null ? parent.uri : null);

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:278: }
similary - use recursive pattern for line too.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:289: }
style nit: feel free to ignore, maybe compact the code a bit the ternary form:
_refId = (parent == null) ? id : '${parent.refId}/$id';

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:298: for (var child in children) {
use List.some?

bool hasReference(String referenceId) =>
  children.some((c) => c.hasReference(referenceId));

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:319: // TODO(jacobr): write without recursion.
why :)?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:321: * Path from this [Element] to the root of the tree
starting at the root.
maybe rephrase as follows? 
Path from the root of the tree to this [Element]

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:364: /** Possibly normalized representation of the node
kind. */
strange to see 'possibly' here, maybe explain we can expect it to be normalized?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:397: if (children != null) {
nit: if(children == null) return ret;

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:407: Map<String, Element> _mapForKinds(Map<String, Element>
kinds) {
change argument type to Set<String> ?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:477: /** Interfaces the class implements. */
nit: + empty line above comment here and below

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:496: addSuperclasses(clazz) {
nit: consider adding here the type parameter and removing it from line 498?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:499: lookupReferenceId(clazz.superclass.refId);
one thing I was wondering is whether Reference could hold a pointer to the
resolved Element once it's resolved.

The idea is that when deserializing you could use a map to associate
referenceids to Reference objects, so there is a single Reference object with a
particular refid.

Once you have a Reference object, you only would need to do a lookupReferenceId
once on it, and you would be done.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:575: abstract class MethodElementBase extends Element {
consider renaming this class? some ideas:
* ClassMemberElement
* CallableElement
* MethodLikeElement

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:600: if (isSetter == true) {
this should not be null by this point, you could simply do if (isSetter)

Also - why not move this condition under PropertyElement (since it's not
possible to have this in operators and normal methods?)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:601: var sb = new StringBuffer('${name.substring(0,
name.length-1)}');
nit: spaces around '-' (name.length - 1)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:604: sb..add(' ')..add(parameters.first.type.name);
what you have seems fine, but it makes me ask some questions about cascades and
style:
- since sb already return themselves, the cascades are not needed. Do we prefer
them in this kind of scenario?
- since sb is a variable, do we prefer cascades or doing:
  sb.add(' ');
  sb.add(...);
  ?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:645: return type != null && type.refId == referenceId;
consider swapping the order of the conditions?
seems like the second kind of check is probably more efficient than
Element.hasReference, so maybe this will be faster:

bool hasReference(...) {
  if (type != null && ...) return true;
  return super.hasReference(...);
}

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/ast.dart#ne...
client/web/ast.dart:664: return '$name extends ${upperBound.shortDescription}';
mmm. I wonder if we want to simply make shortDecription also toString, so here
you'd do:

String toString() =>  upperBound == null ? name : '$name extends $upperBound';

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/doc_link.html
File client/web/doc_link.html (right):

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/doc_link.ht...
client/web/doc_link.html:7: <template instantiate="if short == true">
"if short == true" => "if short"

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/doc_link.ht...
client/web/doc_link.html:11: {{ref.shortDescription}}
if you end up switching shortDescription to be toString, this would become just
{{ref}}

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/doc_link.ht...
client/web/doc_link.html:25: /// Must be a Reference or Element.
use comment kind consistently (/// or /**). So far I've seen mostly /**, but up
to you.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/doc_link.ht...
client/web/doc_link.html:25: /// Must be a Reference or Element.
alternatively, we could possibly make Element extend Reference (be an already
resolved reference).

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/doc_link.ht...
client/web/doc_link.html:27: /** Whether a short version of the Element name
should be used. */
nit: + empty line above comment

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
File client/web/element_summary.html (right):

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:8: <element name="x-member-blocks"
constructor="MemberBlocks" extends="div">
you can now remove the 'constructor="MemberBlocks"'. We  infer it automatically
from the element name (remove the 'x' and make CamelCase from x-member-blocks)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:20: // TODO(jacobr): this is kinda a rediculous
way to do this
what specifically are you referring to? :)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:31: <element name="x-element-signature"
constructor="ElementSignature" extends="span">
if you want, remove 'constructor...'

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:34: <template instantiate="if (element is
MethodElementBase || element is TypedefElement) && element.returnType != null">
style suggestion: consider making the condition into a property in the
ElementSignature class. For instance:

  <template instantiate="if hasReturnType">
...

where
bool get hasReturnType =>
  (element is MethodElementBase || element isTypedefElement) &&
    element.returnType != null;

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:39: <span class="element-name"><x-doc-link
ref="{{element}}" short="{{true}}"></x-doc-link></span>
nit: long line, maybe move x-doc-link to it's own line?

if spaces are an issue, consider breaking between attributes?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:40: <template instantiate="if element is
MethodElement || element is ConstructorElement || element is TypedefElement">
similarly: consider a getter to describe this condition (hasArguments?)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:45: <span class = "arg-type">
nit: remove spaces around '='?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:70: <details class="element-details
{{kindCssClass(element)}} {{(element is MethodElementBase) ? 'member-details' :
'type-details'}}" data-id="{{element.id}}" open="{{currentMember != null &&
element.id == currentMember.id}}">
long line, consider wrapping on each attribute:
<details class="..." 
         data-id="{{...}}"
         open="{{}}">

also consider moving class definitions and complex conditions to the class
below. Here is a suggestion:

<details class="element-details {{kindClass}} {{otherClass}}"
    data-id="{{element.id}}"
    open="{{isOpen}}">

...
class ElementSummary ... {
   Element element;
   String get kindClass => kindCssClass(element);
   String get otherClass =>
     (element is MethodElementBase) ? 'member-details' : 'type-details';

   bool get isOpen => currentMember != null && element.id == currentMember.id;

}


We need a better name for otherClass, but something among these lines?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:78: <x-doc-link class="element-name
element-definition" ref="{{element}}"></x-doc-link>
long line

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/element_sum...
client/web/element_summary.html:105: x-class-hierarchy-subtree node? -->
maybe a bug in how we propagate properties you set in xtags?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html
File client/web/index.html (right):

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html#...
client/web/index.html:16: var _gaq = _gaq || [];
is this standard google analytics code? maybe put a comment about it?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html#...
client/web/index.html:32: <template instantiate="if currentElement == null ||
currentElement.loading != true">
similar suggestion: consider adding a property for this long condition

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html#...
client/web/index.html:40: <x-doc-link ref="{{currentLibrary}}"></x-doc-link>
long line, unindent?

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html#...
client/web/index.html:84: <ul class="libraries tree-list">
remove tabs (here and below)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html#...
client/web/index.html:99: <template instantiate="if currentLibrary.comment !=
null">
fix indentation around here...

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/index.html#...
client/web/index.html:198: });
nit: fix indentation (possibly tabs)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/model.dart
File client/web/model.dart (right):

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/model.dart#...
client/web/model.dart:10: // TODO(jacobr): specify the version # in the JSON
file.
+1 :)

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/model.dart#...
client/web/model.dart:15: * *
remove extra *

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/model.dart#...
client/web/model.dart:89: // after the UI updates.
let's add an issue for this in web-ui.

https://chromiumcodereview.appspot.com/11636011/diff/1/client/web/model.dart#...
client/web/model.dart:176: html.window.on.popState.add((e) {
in our todomvc sample we had to listen for both popState and hashChange to
ensure thigns work in IE9 and Opera 12. 

See http://code.google.com/p/dart/issues/detail?id=5483

[Jacob, 2013-01-02 19:54:58]

Replied to all comments

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart
File client/web/ast.dart (right):

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode1
client/web/ast.dart:1: library ast;
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> + copyright
> + library level doc

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode14
client/web/ast.dart:14: List<String> LIBRARY_KINDS = ['variable', 'property',
'method', 'class', 'exception', 'typedef'];
Changed to LIBRARY_CHILD_KINDS
these are the KIND names of library children that we should display in the UI.
Let me know if you want a different name.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: rename to LIBRARY_ITEMS? for a moment I thought this was going to be
kinds
> of libraries, rather than kinds of items in a library that you'll display.
> 
> or LIBRARY_ELEMENTS - to match the Element terminology you use below

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode14
client/web/ast.dart:14: List<String> LIBRARY_KINDS = ['variable', 'property',
'method', 'class', 'exception', 'typedef'];
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: rename to LIBRARY_ITEMS? for a moment I thought this was going to be
kinds
> of libraries, rather than kinds of items in a library that you'll display.
> 
> or LIBRARY_ELEMENTS - to match the Element terminology you use below

renamed to LIBRARY_ITEMS

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode15
client/web/ast.dart:15: List<String> CLASS_KINDS = ['constructor', 'variable',
'property', 'method', 'operator'];
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> maybe rename as CLASS_ITEMS? or CLASS_ELEMENTS?

renamed to CLASS_ITEMS.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode22
client/web/ast.dart:22: final KIND_TITLES = {'property': 'Properties',
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> style nit: feel free to ignore, but I might prefer moving the hole list down
one
> line:
> 
> final Kind_TITLES = {
>     'property': 'Properties',
>     ...,
> };
> 

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode33
client/web/ast.dart:33: * Block of elements to render summary documentation for
that all share the
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> for that all => for all that

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode47
client/web/ast.dart:47: Reference jsonDeserializeReference(Map json) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: consider reordering the code here to put the classes up front, and the
json
> deserialization at the bottom of the file?
Agreed. Moved to the bottom of the file.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode87
client/web/ast.dart:87: if (json != null) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: consider switchting this to
> 
> if (json == null) return ret;
> 
> John and I are becoming big fans of this style :)

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode97
client/web/ast.dart:97: if (json != null) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> same here

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode148
client/web/ast.dart:148: var result = <Element>[current];
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> 'result' seems unused?

yep that was obsolete. fixed.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode154
client/web/ast.dart:154: next = child;
The trouble is that isn't quite right.  You need to know when there is no
matching element.

On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> you could directly do:
>  current = child;
> 
> and remove lines 151 and 158.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode176
client/web/ast.dart:176: //
https://github.com/dart-lang/dart-web-components/issues/212
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> dart-web-components ==> web-ui

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode184
client/web/ast.dart:184: int elementUiOrder(Element a, Element b) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> should Element implement Comparable and this function become 'compareTo' in
> Element?

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode207
client/web/ast.dart:207: final String _uri;
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: move these private members down with the other private ones? One idea is
to
> put them next to the public properties that wrap them. For instance, put
'_uri'
> next to 'uri', and '_line' next to 'line'.
> (same with _commentHtml, _typeParameters, etc)

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode244
client/web/ast.dart:244: * Longer possibly multiple word description of the
[kind.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> [kind. => [kind].

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode266
client/web/ast.dart:266: }
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> maybe use a recursive pattern here instead:
> 
> String get uri =>
>   _uri != null ? _uri : (parent != null ? parent.uri : null);

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode278
client/web/ast.dart:278: }
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> similary - use recursive pattern for line too.

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode289
client/web/ast.dart:289: }
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> style nit: feel free to ignore, maybe compact the code a bit the ternary form:
> _refId = (parent == null) ? id : '${parent.refId}/$id';

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode319
client/web/ast.dart:319: // TODO(jacobr): write without recursion.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> why :)? 
performance :)
for this case the paths are pretty deep.  For the other couple of cases, you
early terminate most of the time so recursion doesn't hit performance much.
Anyway removed the premature optimization TODO.
removed comment...

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode321
client/web/ast.dart:321: * Path from this [Element] to the root of the tree
starting at the root.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> maybe rephrase as follows? 
> Path from the root of the tree to this [Element]

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode364
client/web/ast.dart:364: /** Possibly normalized representation of the node
kind. */
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> strange to see 'possibly' here, maybe explain we can expect it to be
normalized?
> 

rewrote comment.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode397
client/web/ast.dart:397: if (children != null) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: if(children == null) return ret;

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode407
client/web/ast.dart:407: Map<String, Element> _mapForKinds(Map<String, Element>
kinds) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> change argument type to Set<String> ?

This method isn't really needed. Removed.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode477
client/web/ast.dart:477: /** Interfaces the class implements. */
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: + empty line above comment here and below

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode496
client/web/ast.dart:496: addSuperclasses(clazz) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: consider adding here the type parameter and removing it from line 498?

i don't follow.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode499
client/web/ast.dart:499: lookupReferenceId(clazz.superclass.refId);
I don't think the times when references are resolved are performance
bottlenecks.  
In addition, keep in mind that it is very important for this to not occur at
deserialization time as all the libraries where the references are from may not
be available until those libs are later resolved.  The next big data model CL
change for this code is to only load library definitions as required.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> one thing I was wondering is whether Reference could hold a pointer to the
> resolved Element once it's resolved.
> 
> The idea is that when deserializing you could use a map to associate
> referenceids to Reference objects, so there is a single Reference object with
a
> particular refid.
> 
> Once you have a Reference object, you only would need to do a
lookupReferenceId
> once on it, and you would be done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode600
client/web/ast.dart:600: if (isSetter == true) {
sadly in the AST, setters aren't property elements. only getters are property
elements. fixed the == true ugliness.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> this should not be null by this point, you could simply do if (isSetter)
> 
> Also - why not move this condition under PropertyElement (since it's not
> possible to have this in operators and normal methods?)

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode601
client/web/ast.dart:601: var sb = new StringBuffer('${name.substring(0,
name.length-1)}');
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: spaces around '-' (name.length - 1)

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode604
client/web/ast.dart:604: sb..add(' ')..add(parameters.first.type.name);
my take is StringBuffer should just return void from these methods in the future
if we believe cascades are the way to do chaining.

On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> what you have seems fine, but it makes me ask some questions about cascades
and
> style:
> - since sb already return themselves, the cascades are not needed. Do we
prefer
> them in this kind of scenario?
> - since sb is a variable, do we prefer cascades or doing:
>   sb.add(' ');
>   sb.add(...);
>   ?

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode645
client/web/ast.dart:645: return type != null && type.refId == referenceId;
super.hasReference is cheap. 
In any case, both conditions will typically have to be checked as 99.9% of the
time, hasReference will return false.

On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> consider swapping the order of the conditions?
> seems like the second kind of check is probably more efficient than
> Element.hasReference, so maybe this will be faster:
> 
> bool hasReference(...) {
>   if (type != null && ...) return true;
>   return super.hasReference(...);
> }

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode664
client/web/ast.dart:664: return '$name extends ${upperBound.shortDescription}';
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> mmm. I wonder if we want to simply make shortDecription also toString, so here
> you'd do:
> 
> String toString() =>  upperBound == null ? name : '$name extends $upperBound';

I like to be specific about this as there are many reasonable toString
definitions for this class.

https://codereview.chromium.org/11636011/diff/1/client/web/doc_link.html
File client/web/doc_link.html (right):

https://codereview.chromium.org/11636011/diff/1/client/web/doc_link.html#newc...
client/web/doc_link.html:7: <template instantiate="if short == true">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> "if short == true" => "if short"

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/doc_link.html#newc...
client/web/doc_link.html:11: {{ref.shortDescription}}
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> if you end up switching shortDescription to be toString, this would become
just
> {{ref}}

The trouble is this isn't really the canonical way  to display the element name.
It is at least equally valid to think that .toString should return ref.name

https://codereview.chromium.org/11636011/diff/1/client/web/doc_link.html#newc...
client/web/doc_link.html:25: /// Must be a Reference or Element.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> alternatively, we could possibly make Element extend Reference (be an already
> resolved reference).
I'm not sure making Element extend Reference is a good idea.  Seems a bit
confusing.  I think there is however a cleaner solution.

https://codereview.chromium.org/11636011/diff/1/client/web/doc_link.html#newc...
client/web/doc_link.html:25: /// Must be a Reference or Element.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> use comment kind consistently (/// or /**). So far I've seen mostly /**, but
up
> to you.

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/doc_link.html#newc...
client/web/doc_link.html:27: /** Whether a short version of the Element name
should be used. */
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: + empty line above comment

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.html
File client/web/element_summary.html (right):

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:8: <element name="x-member-blocks"
constructor="MemberBlocks" extends="div">
Nice :)

Done.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> you can now remove the 'constructor="MemberBlocks"'. We  infer it
automatically
> from the element name (remove the 'x' and make CamelCase from x-member-blocks)

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:20: // TODO(jacobr): this is kinda a rediculous
way to do this
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> what specifically are you referring to? :)

No idea.. removed comment.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:31: <element name="x-element-signature"
constructor="ElementSignature" extends="span">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> if you want, remove 'constructor...'

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:34: <template instantiate="if (element is
MethodElementBase || element is TypedefElement) && element.returnType != null">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> style suggestion: consider making the condition into a property in the
> ElementSignature class. For instance:
> 
>   <template instantiate="if hasReturnType">
> ...
> 
> where
> bool get hasReturnType =>
>   (element is MethodElementBase || element isTypedefElement) &&
>     element.returnType != null;

That is better. Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:39: <span class="element-name"><x-doc-link
ref="{{element}}" short="{{true}}"></x-doc-link></span>
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> nit: long line, maybe move x-doc-link to it's own line?
> 
> if spaces are an issue, consider breaking between attributes?

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:40: <template instantiate="if element is
MethodElement || element is ConstructorElement || element is TypedefElement">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> similarly: consider a getter to describe this condition (hasArguments?)

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:70: <details class="element-details
{{kindCssClass(element)}} {{(element is MethodElementBase) ? 'member-details' :
'type-details'}}" data-id="{{element.id}}" open="{{currentMember != null &&
element.id == currentMember.id}}">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> long line, consider wrapping on each attribute:
> <details class="..." 
>          data-id="{{...}}"
>          open="{{}}">
> 
> also consider moving class definitions and complex conditions to the class
> below. Here is a suggestion:
> 
> <details class="element-details {{kindClass}} {{otherClass}}"
>     data-id="{{element.id}}"
>     open="{{isOpen}}">
> 
> ...
> class ElementSummary ... {
>    Element element;
>    String get kindClass => kindCssClass(element);
>    String get otherClass =>
>      (element is MethodElementBase) ? 'member-details' : 'type-details';
> 
>    bool get isOpen => currentMember != null && element.id == currentMember.id;
> 
> }
> 
> 
> We need a better name for otherClass, but something among these lines?

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:78: <x-doc-link class="element-name
element-definition" ref="{{element}}"></x-doc-link>
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> long line

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/element_summary.ht...
client/web/element_summary.html:105: x-class-hierarchy-subtree node? -->
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> maybe a bug in how we propagate properties you set in xtags?

acknowledged

https://codereview.chromium.org/11636011/diff/1/client/web/index.html
File client/web/index.html (right):

https://codereview.chromium.org/11636011/diff/1/client/web/index.html#newcode16
client/web/index.html:16: var _gaq = _gaq || [];
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> is this standard google analytics code? maybe put a comment about it?
It is. Added comment.

https://codereview.chromium.org/11636011/diff/1/client/web/index.html#newcode84
client/web/index.html:84: <ul class="libraries tree-list">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> remove tabs (here and below)

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/index.html#newcode99
client/web/index.html:99: <template instantiate="if currentLibrary.comment !=
null">
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> fix indentation around here...

Done.

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart
File client/web/model.dart (right):

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart#newcode10
client/web/model.dart:10: // TODO(jacobr): specify the version # in the JSON
file.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> +1 :)
acknowledged :)

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart#newcode15
client/web/model.dart:15: * *
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> remove extra *

rm * 

:)

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart#newcode89
client/web/model.dart:89: // after the UI updates.
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> let's add an issue for this in web-ui.
Referenced https://github.com/dart-lang/web-ui/issues/188 in the comment.

https://codereview.chromium.org/11636011/diff/1/client/web/static/styles.css
File client/web/static/styles.css (right):

https://codereview.chromium.org/11636011/diff/1/client/web/static/styles.css#...
client/web/static/styles.css:66: pre, code {
On 2012/12/19 17:04:44, Andrei Mouravski wrote:
> Please add:
> padding: 0;
> 
> to fix: http://code.google.com/p/dart/issues/detail?id=4751

Done.

[Siggi Cherem (dart-lang), 2013-01-02 21:40:55]

lgtm

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart
File client/web/ast.dart (right):

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode496
client/web/ast.dart:496: addSuperclasses(clazz) {
On 2013/01/02 19:54:58, Jacob wrote:
> On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> > nit: consider adding here the type parameter and removing it from line 498?
> 
> i don't follow.

In line 498 (now line 402) you seem to have a downcast, but it doesn't seem
necessary. You can simply do:

var superclassElement = loopupReferenceId(...);

I was initially thinking that if you wanted additional type information, we
could put the type annotation in the argument of this function, like this:

addSuperclasses(ClassElement clazz) {
  ...
}

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode499
client/web/ast.dart:499: lookupReferenceId(clazz.superclass.refId);
On 2013/01/02 19:54:58, Jacob wrote:
> I don't think the times when references are resolved are performance
> bottlenecks.  
> In addition, keep in mind that it is very important for this to not occur at
> deserialization time as all the libraries where the references are from may
not
> be available until those libs are later resolved.  The next big data model CL
> change for this code is to only load library definitions as required.
> On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> > one thing I was wondering is whether Reference could hold a pointer to the
> > resolved Element once it's resolved.
> > 
> > The idea is that when deserializing you could use a map to associate
> > referenceids to Reference objects, so there is a single Reference object
with
> a
> > particular refid.
> > 
> > Once you have a Reference object, you only would need to do a
> lookupReferenceId
> > once on it, and you would be done.
> 

We can wait and see later how this would work after the next model changes. My
suggestion was not so much about performance though, but about abstraction. It
might be easier to chat in person about it, but I think it would be a nice model
to work with if we reference objects were unique. You would still allow
reference objects with unresolved elements (when a library is not loaded, for
example), but then the reference objects will be easier to work with than using
only the string reference ids.

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart
File client/web/model.dart (right):

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart#newcode89
client/web/model.dart:89: // after the UI updates.
On 2013/01/02 19:54:58, Jacob wrote:
> On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> > let's add an issue for this in web-ui.
> Referenced https://github.com/dart-lang/web-ui/issues/188 in the comment.

Thanks! Sorry that I asked you to filed it and then marked it as a duplicate.
Somehow I didn't realize that this was already filed until you filed it and I
searched for other similar bugs =(.

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart#newcode176
client/web/model.dart:176: html.window.on.popState.add((e) {
On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> in our todomvc sample we had to listen for both popState and hashChange to
> ensure thigns work in IE9 and Opera 12. 
> 
> See http://code.google.com/p/dart/issues/detail?id=5483

you might have missed this comment...

https://codereview.chromium.org/11636011/diff/13001/client/web/ast.dart
File client/web/ast.dart (right):

https://codereview.chromium.org/11636011/diff/13001/client/web/ast.dart#newcode1
client/web/ast.dart:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
2013 =)

https://codereview.chromium.org/11636011/diff/13001/client/web/element_summar...
File client/web/element_summary.html (right):

https://codereview.chromium.org/11636011/diff/13001/client/web/element_summar...
client/web/element_summary.html:46: <span class = "arg-type">
remove spaces around =

[Jacob, 2013-01-03 00:09:33]

Also filed bug
http://code.google.com/p/dart/issues/detail?id=7665
due to an odd dartvm error when a method cascade is used in code of the form
return foo ? bar : baz..foobar();

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart
File client/web/ast.dart (right):

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode496
client/web/ast.dart:496: addSuperclasses(clazz) {
On 2013/01/02 21:40:55, Siggi Cherem (dart-lang) wrote:
> On 2013/01/02 19:54:58, Jacob wrote:
> > On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> > > nit: consider adding here the type parameter and removing it from line
498?
> > 
> > i don't follow.
> 
> In line 498 (now line 402) you seem to have a downcast, but it doesn't seem
> necessary. You can simply do:
> 
> var superclassElement = loopupReferenceId(...);
> 
> I was initially thinking that if you wanted additional type information, we
> could put the type annotation in the argument of this function, like this:
> 
> addSuperclasses(ClassElement clazz) {
>   ...
> }
just removed the ClassElement and changed it to a var. I can't remember why I
wanted a type there.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode499
client/web/ast.dart:499: lookupReferenceId(clazz.superclass.refId);
Lets chat about this offline.
On 2013/01/02 21:40:55, Siggi Cherem (dart-lang) wrote:
> On 2013/01/02 19:54:58, Jacob wrote:
> > I don't think the times when references are resolved are performance
> > bottlenecks.  
> > In addition, keep in mind that it is very important for this to not occur at
> > deserialization time as all the libraries where the references are from may
> not
> > be available until those libs are later resolved.  The next big data model
CL
> > change for this code is to only load library definitions as required.
> > On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> > > one thing I was wondering is whether Reference could hold a pointer to the
> > > resolved Element once it's resolved.
> > > 
> > > The idea is that when deserializing you could use a map to associate
> > > referenceids to Reference objects, so there is a single Reference object
> with
> > a
> > > particular refid.
> > > 
> > > Once you have a Reference object, you only would need to do a
> > lookupReferenceId
> > > once on it, and you would be done.
> > 
> 
> We can wait and see later how this would work after the next model changes. My
> suggestion was not so much about performance though, but about abstraction. It
> might be easier to chat in person about it, but I think it would be a nice
model
> to work with if we reference objects were unique. You would still allow
> reference objects with unresolved elements (when a library is not loaded, for
> example), but then the reference objects will be easier to work with than
using
> only the string reference ids.

https://codereview.chromium.org/11636011/diff/1/client/web/ast.dart#newcode575
client/web/ast.dart:575: abstract class MethodElementBase extends Element {
Went with MethodLikeElement.
ClassMemberElement is inaccurate due to top level methods.

On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> consider renaming this class? some ideas:
> * ClassMemberElement
> * CallableElement
> * MethodLikeElement

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart
File client/web/model.dart (right):

https://codereview.chromium.org/11636011/diff/1/client/web/model.dart#newcode176
client/web/model.dart:176: html.window.on.popState.add((e) {
On 2013/01/02 21:40:55, Siggi Cherem (dart-lang) wrote:
> On 2012/12/19 19:47:33, Siggi Cherem (dart-lang) wrote:
> > in our todomvc sample we had to listen for both popState and hashChange to
> > ensure thigns work in IE9 and Opera 12. 
> > 
> > See http://code.google.com/p/dart/issues/detail?id=5483
> 
> you might have missed this comment...

I did... done :)

https://codereview.chromium.org/11636011/diff/13001/client/web/ast.dart
File client/web/ast.dart (right):

https://codereview.chromium.org/11636011/diff/13001/client/web/ast.dart#newcode1
client/web/ast.dart:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
On 2013/01/02 21:40:55, Siggi Cherem (dart-lang) wrote:
> 2013 =)

noooo! that's what i get for taking too long to commit this cl. :)