Implementation of 'Server Domain' in dart analysis server.

Implementation of 'Server Domain' in dart analysis server.

R=brianwilkerson@google.com
BUG=

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

[Brian Wilkerson, 2014-05-24 15:19:48]

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

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:85: /// A set of the
[ServerService] to send notifications for.
I think we should consistently use the same (block) comment style everywhere.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/domain_server.dart (right):

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_server.dart:16: * The name of the
`server.version` request.
'version' --> 'getVersion'

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_server.dart:65: var subDatum =
request.getRequiredParameter(SUBSCRIPTIONS_PARAMETER);
Local variables should have full type information.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_server.dart:70: // TODO(scheglov) remove or
move to the 'analysis' domain
Is there a question about whether we want these API's?

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:27: for (var e in values) {
The body of the method should be indented.

We shouldn't use the for-in loop because it's too slow compared to a simple for
loop.

'e' --> 'element'

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:322: "be a list of names the
enumeration values $allValues"));
"names the enumeration values" --> "names from the list" ("enumeration" is an
implementation detail that doesn't mean anything to the user)

[scheglov, 2014-05-24 15:34:42]

PTAL

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

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/analysis_server.dart:85: /// A set of the
[ServerService] to send notifications for.
On 2014/05/24 15:19:49, Brian Wilkerson wrote:
> I think we should consistently use the same (block) comment style everywhere.

Done.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/domain_server.dart (right):

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_server.dart:16: * The name of the
`server.version` request.
On 2014/05/24 15:19:49, Brian Wilkerson wrote:
> 'version' --> 'getVersion'

Done.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_server.dart:65: var subDatum =
request.getRequiredParameter(SUBSCRIPTIONS_PARAMETER);
On 2014/05/24 15:19:49, Brian Wilkerson wrote:
> Local variables should have full type information.

Done.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/domain_server.dart:70: // TODO(scheglov) remove or
move to the 'analysis' domain
On 2014/05/24 15:19:49, Brian Wilkerson wrote:
> Is there a question about whether we want these API's?

We don't want these API's.
But I'd not like to remove the code that we are going to use soon in the
analysis domain setAnalysisRoot API.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:27: for (var e in values) {
On 2014/05/24 15:19:49, Brian Wilkerson wrote:
> The body of the method should be indented.
> 
> We shouldn't use the for-in loop because it's too slow compared to a simple
for
> loop.
> 
> 'e' --> 'element'

Done.

https://codereview.chromium.org/297153002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:322: "be a list of names the
enumeration values $allValues"));
On 2014/05/24 15:19:49, Brian Wilkerson wrote:
> "names the enumeration values" --> "names from the list" ("enumeration" is an
> implementation detail that doesn't mean anything to the user)

Done.

[Brian Wilkerson, 2014-05-24 15:49:11]

LGTM, once the remaining clean-up is done.

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

https://codereview.chromium.org/297153002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:251: /// An enumeration of the
services provided by the server domain.
I should have mentioned that there are lots of places that are using end-of-line
comments.

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

https://codereview.chromium.org/297153002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/protocol.dart:19: int get hashCode => ordinal;
I think we want to use @override annotations where appropriate.

https://codereview.chromium.org/297153002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/protocol.dart:28: var value = values[i];
And I should have mentioned that there are lots of places where type annotations
are missing. (But that I don't care about type annotations in test code.)

[scheglov, 2014-05-24 16:07:26]

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

https://codereview.chromium.org/297153002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/analysis_server.dart:251: /// An enumeration of the
services provided by the server domain.
On 2014/05/24 15:49:11, Brian Wilkerson wrote:
> I should have mentioned that there are lots of places that are using
end-of-line
> comments.

Fixed here and in protocol.dart file.
As far as I can see, /// is left only in tests.
I will fix it in the next CL.

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

https://codereview.chromium.org/297153002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/protocol.dart:19: int get hashCode => ordinal;
On 2014/05/24 15:49:11, Brian Wilkerson wrote:
> I think we want to use @override annotations where appropriate.

Done here and for toString().

https://codereview.chromium.org/297153002/diff/20001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/protocol.dart:28: var value = values[i];
On 2014/05/24 15:49:11, Brian Wilkerson wrote:
> And I should have mentioned that there are lots of places where type
annotations
> are missing. (But that I don't care about type annotations in test code.)

Got it.
Done.

[Brian Wilkerson, 2014-05-24 17:28:15]

LGTM

[scheglov, 2014-05-24 17:36:14]

Committed patchset #3 manually as r36607 (presubmit successful).

[Paul Berry, 2014-05-27 15:41:04]

lgtm