More burndown list work

More burndown list work


Move data


Merge remote-tracking branch 'origin/polish_trunk_sdk' into polish


fixes for latest trunk sdk


More ui polish


Move kind icons. More polish. In a known slightly buggy state.


More polish


Static data checkpoint


Checkpoint


Checkpoint


BUG=

[jacobr, 2013-02-14 01:01:49]

Sending this out more broadly.

[John Messerly, 2013-02-14 01:15:38]

You might want to upload to https://codereview.chromium.org/ -- that's the
main dashboard for Dart code reviews (and V8, Chrome, etc).


On Wed, Feb 13, 2013 at 5:01 PM, <jacobr@google.com> wrote:

> Sending this out more broadly.
>
>
https://codereview.appspot.**com/7301094/<https://codereview.appspot.com/7301...
>

[sigmund, 2013-02-14 04:27:57]

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart
File client/lib/ast.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode63
client/lib/ast.dart:63: final name;
style nit: add empty line before each comment

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode287
client/lib/ast.dart:287: Element._clone(Element e, Element newParent) :
style nit - I think the guide is to put the ':' in the next line, indent +4 that
line, and +2 subsequent lines (so you still get the alignment). For example:

  Element.clone(...)
      : something,
        something else,
        something else;

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode305
client/lib/ast.dart:305: this.name = ref.name,
same here ...

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode622
client/lib/ast.dart:622: LibraryElement._clone(LibraryElement e, Element
newParent) :
same comment about the ":" throughout the file...

https://codereview.appspot.com/7301094/diff/1/client/lib/library_loader.dart
File client/lib/library_loader.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/lib/library_loader.dart#...
client/lib/library_loader.dart:1: library library_loader;
+ copyright
+ top-level comment

https://codereview.appspot.com/7301094/diff/1/client/lib/library_loader.dart#...
client/lib/library_loader.dart:20: if (package == null) {
nit: use the fast-exit style here too
if (package == null) return queue...;

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart
File client/lib/model.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode185
client/lib/model.dart:185: if (v == true) {
remove '== true':

if (v) args.add(k);

(it can't be null after 'v is bool')

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode201
client/lib/model.dart:201: for(var part in link.substring(2).split(new
RegExp('&'))) {
why use regexp here and not just '&'?

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode203
client/lib/model.dart:203: if (splitPoint >= 0 && splitPoint < part.length - 1)
{
the sencond check seems unnecessary because if splitPoint != -1 it is within
range.

if '=' is the last char, then part.substring(splitPoint + 1) will be the empty
string, is that something you want to convert into a boolean?

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode204
client/lib/model.dart:204: data[part.substring(0, splitPoint)] =
part.substring(splitPoint + 1);
do you need to do decodeUri?

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode254
client/lib/model.dart:254: callback(null);
if you anyways want to call callback, you can swap the order here.

getString(url)
  .catchError((e) { ... info; })
  .then(callback);

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode264
client/lib/model.dart:264: completer.complete(true);
seems that you don't need this completer, and you can simply do:

return http...getString('....json').then((text) {
    loadPackageJson(text);
    _onDataModelChanged();
    return true;
  });

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode387
client/lib/model.dart:387: var id = name;
remove variable/rename arg as 'id'?

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode389
client/lib/model.dart:389: if (existing == null ) {
extra space after null

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode406
client/lib/model.dart:406: if ((code >= 65 && code <= 90) || (code >= 48 && code
<= 57)) {
ditto - use constants or regex

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode441
client/lib/model.dart:441: // TODO(jacobr): this formula is primitive. We should
order by popularity
logn line

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode455
client/lib/model.dart:455: // TODO(jacobr): ugly way to do this.
you could do:
return results.take(maxResults).toList();

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart
File client/web/details.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode2
client/web/details.dart:2: import 'package:web_ui/web_ui.dart';
+ copyright

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode6
client/web/details.dart:6: class Details extends WebComponent {
+ dartdoc

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode11
client/web/details.dart:11: Element get summaryOnly =>
this.query(".summary-only");
mmm - I think this only works because we have shadow dom disabled. It should be
_root.query, I believe.

_root will give you an editor warning though.

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode22
client/web/details.dart:22: //  Swapper.swap(expandedDetails.parent, value ?
expandedDetails : summaryOnly,
formatting

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

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode4
client/web/doc_link.html:4: <template><template instantiate="if ref.refId !=
'void'"><a href="{{permalink(ref)}}"><template instantiate="if
short">{{ref.shortName}}</template><template instantiate="if
!short">{{ref.shortDescription}}</template></a></template><template
instantiate="if ref.refId == 'void'">{{ref.name}}</template></template>
One suggestion to get rid of the inner 2 conditionals, 

 <template>
  <template instantiate="if ref.refId != 'void'">
    <a href="{{permalink(ref)}}">{{refText}}</a>
  </template>
  <template instantiate="if ref.refId == 'void'">
    {{ref.name}}
  </template>
 </template>

And define in the class below:

String refText => short ? ref.shortName : ref.shortDescription;

Also - is there a way to disable a link using CSS? If so, we could turn that
condition into a css class, for example:

<a href="{{permalink(ref)}}" class="{{voidClass}}">{{refText}}</a>

then define
voidClass => ref.refId == 'void' ? 'disabled-special-css' : null;

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode19
client/web/doc_link.html:19: <template><a href="{{permalink(ref)}}"><template
instantiate="if ref.parent is
ClassElement">{{ref.parent.name}}.</template>{{ref.shortDescription}}</a></template>
here too, we could turn the inner if into a property with a normal conditional:

<a ...>{{linkName}}</a>

where
String get linkName {
 var prefix = ref.parent is ClassELement ? '${ref.parent.name}' : '';
 return '$prefix${ref.shortDescription}';
}

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode38
client/web/doc_link.html:38: <template><x-doc-type type="{{parameter.type}}"
param-name="{{parameter.name}}"></x-doc-type><template
if="parameter.defaultValue !=
null">&nbsp;=&nbsp;{{parameter.defaultValue}}</template></template>
same here - I'd avoid using the template conditionals in cases where all you are
making is conditions on text content, but nothing containing actual DOM
elements.

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode53
client/web/doc_link.html:53: <template><span class="args">(<template
iterate="param in element.requiredParameters"><span class="arg"><x-doc-parameter
parameter="{{param}}"></x-doc-parameter></span></template><template
if="!element.optionalParameters.isEmpty">{{element.optionalParametersStartSymbol}}<template
iterate="param in element.optionalParameters"><span class="arg"><x-doc-parameter
parameter="{{param}}"></x-doc-parameter></span></template>{{element.optionalParametersEndSymbol}}</template>)</span></template>
do you need the extra <span class="arg">?

can you reuse the x-doc-parameter span?

It might be that our syntax doesn't work correctly using
<x-doc-parameter class="arg" parameter=";...">

but if not, this syntax should hopefully work:
<span is="x-doc-parameter" class="arg" parameter="...">

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode68
client/web/doc_link.html:68: <template><template if="type is Element"><span
class="arg-type"><x-doc-type
type="{{type.returnType}}"></x-doc-type></span>&nbsp;<span
class="arg-name">{{paramName}}</span><x-doc-parameter-list
element="{{type}}"></x-doc-parameter-list></template><template if="type is
!Element"><template instantiate="if type != null"><span
class="arg-type"><x-doc-link
ref="{{type}}"></x-doc-link></span>&nbsp;</template><template if="paramName !=
null"><span
class="arg-name">{{paramName}}</span></template></template></template>
similarly here, can we combine the span and the x-tag?

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

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode163
client/web/index.html:163: <template if="currentType.isAbstract">
interestingly, you can also be implemented if you are not abstract... is it
worth showing those?

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode170
client/web/index.html:170: <!-- TODO(jacobr): only add the first few
subclasses-->
+1, with watchers it might be as easy as doing currentType.subclasses.take(10) 

with observers, we might need to do something else

John - would that be enough here, or something like:
toObservable(currentType.subclasses.take(10))

I'm afraid that we'll be observing the wrong thing.

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode270
client/web/index.html:270: <script src="packages/browser/dart.js"
type="text/javascript"></script>
FYI - we should be injecting this too (using the packages/browser dependency)

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html
File client/web/kind_icon.html (right):

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html#newcode2
client/web/kind_icon.html:2: <html><body>
+ copyright

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart
File client/web/search.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode1
client/web/search.dart:1: import 'dart:html';
+ copyright

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode2
client/web/search.dart:2: import 'dart:html' as html;
why both imports?

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode8
client/web/search.dart:8: class Search extends WebComponent {
+ comment

https://codereview.appspot.com/7301094/diff/1/client/web/search.html
File client/web/search.html (right):

https://codereview.appspot.com/7301094/diff/1/client/web/search.html#newcode1
client/web/search.html:1: <!DOCTYPE html>
+ copyright below
<!-- -->

https://codereview.appspot.com/7301094/diff/1/client/web/search.html#newcode22
client/web/search.html:22: <li class="{{kindCssClass(result.element)}}
search-result" tabindex="10"
wrap before tabindex instead?

https://codereview.appspot.com/7301094/diff/1/client/web/static/styles.css
File client/web/static/styles.css (right):

https://codereview.appspot.com/7301094/diff/1/client/web/static/styles.css#ne...
client/web/static/styles.css:867: .arg:not(:last-of-type)::after{
I wonder, how expensive is this selector?

[jacobr, 2013-02-14 05:37:14]

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart
File client/lib/ast.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode63
client/lib/ast.dart:63: final name;
On 2013/02/14 04:27:58, sigmund wrote:
> style nit: add empty line before each comment

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode287
client/lib/ast.dart:287: Element._clone(Element e, Element newParent) :
On 2013/02/14 04:27:58, sigmund wrote:
> style nit - I think the guide is to put the ':' in the next line, indent +4
that
> line, and +2 subsequent lines (so you still get the alignment). For example:
> 
>   Element.clone(...)
>       : something,
>         something else,
>         something else;

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode305
client/lib/ast.dart:305: this.name = ref.name,
On 2013/02/14 04:27:58, sigmund wrote:
> same here ... 

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/ast.dart#newcode622
client/lib/ast.dart:622: LibraryElement._clone(LibraryElement e, Element
newParent) :
On 2013/02/14 04:27:58, sigmund wrote:
> same comment about the ":" throughout the file...

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/library_loader.dart
File client/lib/library_loader.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/lib/library_loader.dart#...
client/lib/library_loader.dart:1: library library_loader;
On 2013/02/14 04:27:58, sigmund wrote:
> + copyright
> + top-level comment

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/library_loader.dart#...
client/lib/library_loader.dart:20: if (package == null) {
On 2013/02/14 04:27:58, sigmund wrote:
> nit: use the fast-exit style here too
> if (package == null) return queue...;

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart
File client/lib/model.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode185
client/lib/model.dart:185: if (v == true) {
On 2013/02/14 04:27:58, sigmund wrote:
> remove '== true':
> 
> if (v) args.add(k);
> 
> (it can't be null after 'v is bool')

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode201
client/lib/model.dart:201: for(var part in link.substring(2).split(new
RegExp('&'))) {
On 2013/02/14 04:27:58, sigmund wrote:
> why use regexp here and not just '&'?

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode203
client/lib/model.dart:203: if (splitPoint >= 0 && splitPoint < part.length - 1)
{
On 2013/02/14 04:27:58, sigmund wrote:
> the sencond check seems unnecessary because if splitPoint != -1 it is within
> range.
> 
> if '=' is the last char, then part.substring(splitPoint + 1) will be the empty
> string, is that something you want to convert into a boolean?

Agreed. Bools shouldn't have an =

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode204
client/lib/model.dart:204: data[part.substring(0, splitPoint)] =
part.substring(splitPoint + 1);
On 2013/02/14 04:27:58, sigmund wrote:
> do you need to do decodeUri?

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode254
client/lib/model.dart:254: callback(null);
On 2013/02/14 04:27:58, sigmund wrote:
> if you anyways want to call callback, you can swap the order here.
> 
> getString(url)
>   .catchError((e) { ... info; })
>   .then(callback);

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode264
client/lib/model.dart:264: completer.complete(true);
On 2013/02/14 04:27:58, sigmund wrote:
> seems that you don't need this completer, and you can simply do:
> 
> return http...getString('....json').then((text) {
>     loadPackageJson(text);
>     _onDataModelChanged();
>     return true;
>   });

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode389
client/lib/model.dart:389: if (existing == null ) {
On 2013/02/14 04:27:58, sigmund wrote:
> extra space after null

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode406
client/lib/model.dart:406: if ((code >= 65 && code <= 90) || (code >= 48 && code
<= 57)) {
On 2013/02/14 04:27:58, sigmund wrote:
> ditto - use constants or regex
added todo

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode441
client/lib/model.dart:441: // TODO(jacobr): this formula is primitive. We should
order by popularity
On 2013/02/14 04:27:58, sigmund wrote:
> logn line

Done.

https://codereview.appspot.com/7301094/diff/1/client/lib/model.dart#newcode455
client/lib/model.dart:455: // TODO(jacobr): ugly way to do this.
On 2013/02/14 04:27:58, sigmund wrote:
> you could do:
> return results.take(maxResults).toList();

Yep.  Wrote this code before the dart:core changes.

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart
File client/web/details.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode2
client/web/details.dart:2: import 'package:web_ui/web_ui.dart';
On 2013/02/14 04:27:58, sigmund wrote:
> + copyright

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode11
client/web/details.dart:11: Element get summaryOnly =>
this.query(".summary-only");
On 2013/02/14 04:27:58, sigmund wrote:
> mmm - I think this only works because we have shadow dom disabled. It should
be
> _root.query, I believe.
> 
> _root will give you an editor warning though. 

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/details.dart#newcode22
client/web/details.dart:22: //  Swapper.swap(expandedDetails.parent, value ?
expandedDetails : summaryOnly,
On 2013/02/14 04:27:58, sigmund wrote:
> formatting

Done.

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

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode4
client/web/doc_link.html:4: <template><template instantiate="if ref.refId !=
'void'"><a href="{{permalink(ref)}}"><template instantiate="if
short">{{ref.shortName}}</template><template instantiate="if
!short">{{ref.shortDescription}}</template></a></template><template
instantiate="if ref.refId == 'void'">{{ref.name}}</template></template>
I like the first suggestion but think the second suggestion is a flaw in the
current MDV design that we shouldn't contort the code to work around.
On 2013/02/14 04:27:58, sigmund wrote:
> One suggestion to get rid of the inner 2 conditionals, 
> 
>  <template>
>   <template instantiate="if ref.refId != 'void'">
>     <a href="{{permalink(ref)}}">{{refText}}</a>
>   </template>
>   <template instantiate="if ref.refId == 'void'">
>     {{ref.name}}
>   </template>
>  </template>
> 
> And define in the class below:
> 
> String refText => short ? ref.shortName : ref.shortDescription;
> 
> Also - is there a way to disable a link using CSS? If so, we could turn that
> condition into a css class, for example:
> 
> <a href="{{permalink(ref)}}" class="{{voidClass}}">{{refText}}</a>
> 
> then define
> voidClass => ref.refId == 'void' ? 'disabled-special-css' : null;

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode19
client/web/doc_link.html:19: <template><a href="{{permalink(ref)}}"><template
instantiate="if ref.parent is
ClassElement">{{ref.parent.name}}.</template>{{ref.shortDescription}}</a></template>
On 2013/02/14 04:27:58, sigmund wrote:
> here too, we could turn the inner if into a property with a normal
conditional:
> 
> <a ...>{{linkName}}</a>
> 
> where
> String get linkName {
>  var prefix = ref.parent is ClassELement ? '${ref.parent.name}' : '';
>  return '$prefix${ref.shortDescription}';
> }

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode38
client/web/doc_link.html:38: <template><x-doc-type type="{{parameter.type}}"
param-name="{{parameter.name}}"></x-doc-type><template
if="parameter.defaultValue !=
null">&nbsp;=&nbsp;{{parameter.defaultValue}}</template></template>
I really hate tweaking these until we can enter spaces in the templates so they
are readable.  Added these as TODOS
On 2013/02/14 04:27:58, sigmund wrote:
> same here - I'd avoid using the template conditionals in cases where all you
are
> making is conditions on text content, but nothing containing actual DOM
> elements.

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode53
client/web/doc_link.html:53: <template><span class="args">(<template
iterate="param in element.requiredParameters"><span class="arg"><x-doc-parameter
parameter="{{param}}"></x-doc-parameter></span></template><template
if="!element.optionalParameters.isEmpty">{{element.optionalParametersStartSymbol}}<template
iterate="param in element.optionalParameters"><span class="arg"><x-doc-parameter
parameter="{{param}}"></x-doc-parameter></span></template>{{element.optionalParametersEndSymbol}}</template>)</span></template>
I don't think 
On 2013/02/14 04:27:58, sigmund wrote:
> do you need the extra <span class="arg">?
> 
> can you reuse the x-doc-parameter span?
> 
> It might be that our syntax doesn't work correctly using
> <x-doc-parameter class="arg" parameter=";...">
> 
> but if not, this syntax should hopefully work:
> <span is="x-doc-parameter" class="arg" parameter="...">
> 
Added as TODO

https://codereview.appspot.com/7301094/diff/1/client/web/doc_link.html#newcode68
client/web/doc_link.html:68: <template><template if="type is Element"><span
class="arg-type"><x-doc-type
type="{{type.returnType}}"></x-doc-type></span>&nbsp;<span
class="arg-name">{{paramName}}</span><x-doc-parameter-list
element="{{type}}"></x-doc-parameter-list></template><template if="type is
!Element"><template instantiate="if type != null"><span
class="arg-type"><x-doc-link
ref="{{type}}"></x-doc-link></span>&nbsp;</template><template if="paramName !=
null"><span
class="arg-name">{{paramName}}</span></template></template></template>
On 2013/02/14 04:27:58, sigmund wrote:
> similarly here, can we combine the span and the x-tag?

added TODO

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

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode163
client/web/index.html:163: <template if="currentType.isAbstract">
On 2013/02/14 04:27:58, sigmund wrote:
> interestingly, you can also be implemented if you are not abstract... is it
> worth showing those?

Good point. Added todo.

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode163
client/web/index.html:163: <template if="currentType.isAbstract">
On 2013/02/14 04:27:58, sigmund wrote:
> interestingly, you can also be implemented if you are not abstract... is it
> worth showing those?

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode170
client/web/index.html:170: <!-- TODO(jacobr): only add the first few
subclasses-->
On 2013/02/14 04:27:58, sigmund wrote:
> +1, with watchers it might be as easy as doing currentType.subclasses.take(10)

> 
> with observers, we might need to do something else
> 
> John - would that be enough here, or something like:
> toObservable(currentType.subclasses.take(10))
> 
> I'm afraid that we'll be observing the wrong thing.

ack

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode270
client/web/index.html:270: <script src="packages/browser/dart.js"
type="text/javascript"></script>
On 2013/02/14 04:27:58, sigmund wrote:
> FYI - we should be injecting this too (using the packages/browser dependency)

so do I still need this line?

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html
File client/web/kind_icon.html (right):

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html#newcode2
client/web/kind_icon.html:2: <html><body>
On 2013/02/14 04:27:58, sigmund wrote:
> + copyright

What should the copyright in the html files look like?

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart
File client/web/search.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode2
client/web/search.dart:2: import 'dart:html' as html;
On 2013/02/14 04:27:58, sigmund wrote:
> why both imports?
Unfortunately, we are also importing a class called Element.
This lets us write
html.Element.focusEvent

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode8
client/web/search.dart:8: class Search extends WebComponent {
On 2013/02/14 04:27:58, sigmund wrote:
> + comment

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/search.html
File client/web/search.html (right):

https://codereview.appspot.com/7301094/diff/1/client/web/search.html#newcode1
client/web/search.html:1: <!DOCTYPE html>
On 2013/02/14 04:27:58, sigmund wrote:
> + copyright below
> <!-- -->

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/search.html#newcode22
client/web/search.html:22: <li class="{{kindCssClass(result.element)}}
search-result" tabindex="10"
On 2013/02/14 04:27:58, sigmund wrote:
> wrap before tabindex instead?

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/static/styles.css
File client/web/static/styles.css (right):

https://codereview.appspot.com/7301094/diff/1/client/web/static/styles.css#ne...
client/web/static/styles.css:867: .arg:not(:last-of-type)::after{
On 2013/02/14 04:27:58, sigmund wrote:
> I wonder, how expensive is this selector?

I've added a TODO to use the template instead of the selector.  This was a
convenient hack but it is kinda odd... the commas cannot be copied...

[sigmund, 2013-02-14 17:06:26]

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

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode270
client/web/index.html:270: <script src="packages/browser/dart.js"
type="text/javascript"></script>
On 2013/02/14 05:37:14, jacobr wrote:
> On 2013/02/14 04:27:58, sigmund wrote:
> > FYI - we should be injecting this too (using the packages/browser
dependency)
> 
> so do I still need this line?

nope. If for some reason this line is not in the output then we have a bug.

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html
File client/web/kind_icon.html (right):

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html#newcode2
client/web/kind_icon.html:2: <html><body>
On 2013/02/14 05:37:14, jacobr wrote:
> On 2013/02/14 04:27:58, sigmund wrote:
> > + copyright
> 
> What should the copyright in the html files look like?
Like a <!-- --> comment right under the doctype. Similar to what you have, but
pushed to the margin:
<!--
Copyright ...
...
...
-->

Here is an example:
https://github.com/dart-lang/web-ui/blob/master/test/data/input/attr_multiple...

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart
File client/web/search.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode2
client/web/search.dart:2: import 'dart:html' as html;
On 2013/02/14 05:37:14, jacobr wrote:
> On 2013/02/14 04:27:58, sigmund wrote:
> > why both imports?
> Unfortunately, we are also importing a class called Element.
> This lets us write
> html.Element.focusEvent

then maybe it should be:
import 'dart:html' hide Element;
import 'dart:html' as html show Element;

?

https://codereview.appspot.com/7301094/diff/30007/client/lib/model.dart
File client/lib/model.dart (right):

https://codereview.appspot.com/7301094/diff/30007/client/lib/model.dart#newco...
client/lib/model.dart:253: callback(null);
now that they are swapped, this call should be removed. 

[catchError] will trap the error and thus [callback] in [then] will be invoked
with the returned value of [catchError]. If [catchError] is void, then the
[callback] will be called with [null].

https://codereview.appspot.com/7301094/diff/30007/client/lib/model.dart#newco...
client/lib/model.dart:459: results.take(maxResults).toList();
note: missing a return here, or update the results variable

https://codereview.appspot.com/7301094/diff/30007/client/web/doc_link.html
File client/web/doc_link.html (right):

https://codereview.appspot.com/7301094/diff/30007/client/web/doc_link.html#ne...
client/web/doc_link.html:26: <template><a
href="{{permalink(ref)}}">{{linkName}}.{{ref.shortDescription}}</a></template>
note: remove the .{{ref.shortDescription}} (linkName already includes that)

https://codereview.appspot.com/7301094/diff/30007/client/web/element_summary....
File client/web/element_summary.html (right):

https://codereview.appspot.com/7301094/diff/30007/client/web/element_summary....
client/web/element_summary.html:1: <!-- Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
it's annoying, but doctype must be above the copyright.

[jacobr, 2013-02-14 18:03:30]

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

https://codereview.appspot.com/7301094/diff/1/client/web/index.html#newcode270
client/web/index.html:270: <script src="packages/browser/dart.js"
type="text/javascript"></script>
On 2013/02/14 17:06:26, sigmund wrote:
> On 2013/02/14 05:37:14, jacobr wrote:
> > On 2013/02/14 04:27:58, sigmund wrote:
> > > FYI - we should be injecting this too (using the packages/browser
> dependency)
> > 
> > so do I still need this line?
> 
> nope. If for some reason this line is not in the output then we have a bug.

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html
File client/web/kind_icon.html (right):

https://codereview.appspot.com/7301094/diff/1/client/web/kind_icon.html#newcode2
client/web/kind_icon.html:2: <html><body>
On 2013/02/14 17:06:26, sigmund wrote:
> On 2013/02/14 05:37:14, jacobr wrote:
> > On 2013/02/14 04:27:58, sigmund wrote:
> > > + copyright
> > 
> > What should the copyright in the html files look like?
> Like a <!-- --> comment right under the doctype. Similar to what you have, but
> pushed to the margin:
> <!--
> Copyright ...
> ...
> ...
> -->
> 
> Here is an example:
>
https://github.com/dart-lang/web-ui/blob/master/test/data/input/attr_multiple...

Done.

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart
File client/web/search.dart (right):

https://codereview.appspot.com/7301094/diff/1/client/web/search.dart#newcode2
client/web/search.dart:2: import 'dart:html' as html;
On 2013/02/14 17:06:26, sigmund wrote:
> On 2013/02/14 05:37:14, jacobr wrote:
> > On 2013/02/14 04:27:58, sigmund wrote:
> > > why both imports?
> > Unfortunately, we are also importing a class called Element.
> > This lets us write
> > html.Element.focusEvent
> 
> then maybe it should be:
> import 'dart:html' hide Element;
> import 'dart:html' as html show Element;
> 
> ?

Done.

https://codereview.appspot.com/7301094/diff/30007/client/lib/model.dart
File client/lib/model.dart (right):

https://codereview.appspot.com/7301094/diff/30007/client/lib/model.dart#newco...
client/lib/model.dart:253: callback(null);
On 2013/02/14 17:06:26, sigmund wrote:
> now that they are swapped, this call should be removed. 
> 
> [catchError] will trap the error and thus [callback] in [then] will be invoked
> with the returned value of [catchError]. If [catchError] is void, then the
> [callback] will be called with [null].

Done.

https://codereview.appspot.com/7301094/diff/30007/client/lib/model.dart#newco...
client/lib/model.dart:459: results.take(maxResults).toList();
On 2013/02/14 17:06:26, sigmund wrote:
> note: missing a return here, or update the results variable

Done.

https://codereview.appspot.com/7301094/diff/30007/client/web/doc_link.html
File client/web/doc_link.html (right):

https://codereview.appspot.com/7301094/diff/30007/client/web/doc_link.html#ne...
client/web/doc_link.html:26: <template><a
href="{{permalink(ref)}}">{{linkName}}.{{ref.shortDescription}}</a></template>
On 2013/02/14 17:06:26, sigmund wrote:
> note: remove the .{{ref.shortDescription}} (linkName already includes that)

Done.

https://codereview.appspot.com/7301094/diff/30007/client/web/element_summary....
File client/web/element_summary.html (right):

https://codereview.appspot.com/7301094/diff/30007/client/web/element_summary....
client/web/element_summary.html:1: <!-- Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/02/14 17:06:26, sigmund wrote:
> it's annoying, but doctype must be above the copyright.

Done.

[sigmund, 2013-02-14 23:50:45]

lgtm!