Alternate status page

Alternate status page

R=paulberry@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43020

[Brian Wilkerson, 2015-01-20 00:19:05]

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

[Brian Wilkerson, 2015-01-20 00:19:05]

This preserves the old behavior until people can give me feedback on the
changes.

[Paul Berry, 2015-01-20 17:28:11]

I hope we don't need to preserve the old behavior for long--it's a shame to have
to duplicate the code, and since the http server is pretty much only used for
in-house debugging I don't think we need to be too careful about staging changes
to it.  If you wanted to give me a quick demo at your desk today, I'd be happy
throwing away the old behavior immediately.

Just a few minor comments below, which you can take or leave as you see fit. 
The only thing I feel at all strongly about is the encodeHtml function. 
Otherwise LGTM.

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:68: static String VERSION =
'0.0.1';
We should talk about a policy for what will cause us to rev this version number.
 A version number isn't much use to people if we don't increment it, and
analysis server's been around for nearly a year now, so we probably should have
incremented it at least once by now :)

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/generated_protocol.dart:5300: static const
List<AnalysisErrorSeverity> VALUES = const <AnalysisErrorSeverity>[INFO,
WARNING, ERROR];
Not a blocker to this CL, but would it be better to code generate this list (and
the others below) as part of the generation of
pkg/analysis_server/lib/src/generated_protocol.dart, rather than hardcode them
here and risk them getting out of sync?

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/get_handler.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/get_handler.dart:1000: * Return a two digit decimal
representation of the given integer [value].
Nit: this function will produce nonsense if the integer is less than zero, and
it will produce more than two digits if the integer is greater than 99.  To
clarify this, how about changing the comment to "Return a two (or more) digit
decimal representation of the given unsigned integer [value]."

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/status/utilities.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/status/utilities.dart:11: String encodeHtml(String
string) {
I don't believe this function is necessary.  It should be possible to use the
utility class from the standard library:
https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:convert#....

(Note that the standard library implementation doesn't do as much escaping as
you do--that should also be ok provided that we output the necessary http
headers to tell the client that the data is UTF-8 encoded)

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_dart_protocol.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_dart_protocol.dart:259: writeln(' * A list
containing all of the services that are defined.');
This looks like it will run on all enums, not just enums describing services, so
the comment should say something like "A list containing all of the enum values
that are defined."

Also, was this part of an incomplete thought?  It doesn't look like you ran the
code generator, and I didn't notice you depending on the new constant anywhere.

[Brian Wilkerson, 2015-01-20 17:46:35]

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/analysis_server.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:68: static String VERSION =
'0.0.1';
> We should talk about a policy for what will cause us to rev this version
number.

Agreed.

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/generated_protocol.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/generated_protocol.dart:5300: static const
List<AnalysisErrorSeverity> VALUES = const <AnalysisErrorSeverity>[INFO,
WARNING, ERROR];
> ... would it be better to code generate this list ...?

These lists *are* being generated (and this file is generated_protocol.dart).

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/get_handler.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/get_handler.dart:1000: * Return a two digit decimal
representation of the given integer [value].
Done.

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/status/utilities.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/status/utilities.dart:11: String encodeHtml(String
string) {
> It should be possible to use the utility class from the standard library:

I tried that, but it failed to encode the right-arrow character. I only wrote
this function when I encountered a failure and didn't know what was causing it.
It could probably be cleaned up, but I'll save that for later.

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_dart_protocol.dart (right):

https://codereview.chromium.org/840953008/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_dart_protocol.dart:259: writeln(' * A list
containing all of the services that are defined.');
> This looks like it will run on all enums, not just enums describing services,
so
> the comment should say something like "A list containing all of the enum
values
> that are defined."

Done.

> Also, was this part of an incomplete thought?  It doesn't look like you ran
the
> code generator, and I didn't notice you depending on the new constant
anywhere.

Yes, I ran the generator, and yes, I'm using it.

[Brian Wilkerson, 2015-01-20 18:46:52]

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

[Paul Berry, 2015-01-20 18:47:21]

After seeing this in person, lgtm.  Thanks for all the time you spent on this,
Brian.  I'll take care of the htmlEscape stuff in a future CL.