Refactor service code and service method parameters

Refactor service code and service method parameters

- Move service isolate code into service_isolate.cc
- Reorganize service isolate code
- Unify isolate and root message handlers
- Add MethodParameter classes
- Add parameter list to service method table
- Use parameter list for getCpuProfile RPC
- Update service_test.cc to work with required isolateId parameter.

R=turnidge@google.com

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

[Cutch, 2015-02-12 17:39:18]

johnmccutchan@google.com changed reviewers:
+ turnidge@google.com

[turnidge, 2015-02-12 18:08:57]

Some small comments.

Overall a nice change.  I'll be glad to have service.cc smaller and more
homogenous.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcode44
runtime/vm/service.cc:44: EmbedderServiceHandler*
Service::root_service_handler_head_ = NULL;
Add a TODO to combine isolate and root service handlers and their apis.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:245: class MethodParameter {
Consider adding standalone unit tests for BoolParameter, IdParameter,
EnumParameter.

I know it is hard to do right now because they are file local.  Consider moving
parameter types to service_parameter.h/service_parameter.cc.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:374: }
We don't complain about unexpected extra parameters.  Should we do this?

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:2230: { "getCpuProfile", HandleIsolateGetCpuProfile,
cpu_profile_params, },
Would be nice to add validators for everything as soon as we can.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.h
File runtime/vm/service.h (right):

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.h#newcode31
runtime/vm/service.h:31: static void HandleRootMessage(const Array& message);
We should either get rid of HandleRootMessage/HandleIsolateMessage or make
InvokeMethod private.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service_isolate.h
File runtime/vm/service_isolate.h (right):

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service_isolate.h...
runtime/vm/service_isolate.h:18: static bool IsName(const char* name);
Maybe "NameEquals?"

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service_isolate.h...
runtime/vm/service_isolate.h:20: static bool HasServiceIsolate();
ServiceIsolate::HasServiceIsolate is repetitive -- maybe
ServiceIsolate:IsInitialized or ServiceIsolate::Exists or something like that?

[Cutch, 2015-02-12 18:18:47]

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcode44
runtime/vm/service.cc:44: EmbedderServiceHandler*
Service::root_service_handler_head_ = NULL;
On 2015/02/12 18:08:57, turnidge wrote:
> Add a TODO to combine isolate and root service handlers and their apis.

Done.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:245: class MethodParameter {
On 2015/02/12 18:08:57, turnidge wrote:
> Consider adding standalone unit tests for BoolParameter, IdParameter,
> EnumParameter.
> 
> I know it is hard to do right now because they are file local.  Consider
moving
> parameter types to service_parameter.h/service_parameter.cc.

I've added a TODO.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.cc#newcod...
runtime/vm/service.cc:2230: { "getCpuProfile", HandleIsolateGetCpuProfile,
cpu_profile_params, },
On 2015/02/12 18:08:57, turnidge wrote:
> Would be nice to add validators for everything as soon as we can.

Partially done. Only the isolate parameter is listed now.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.h
File runtime/vm/service.h (right):

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service.h#newcode31
runtime/vm/service.h:31: static void HandleRootMessage(const Array& message);
On 2015/02/12 18:08:57, turnidge wrote:
> We should either get rid of HandleRootMessage/HandleIsolateMessage or make
> InvokeMethod private.

I've made InvokeMethod private for now.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service_isolate.h
File runtime/vm/service_isolate.h (right):

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service_isolate.h...
runtime/vm/service_isolate.h:18: static bool IsName(const char* name);
On 2015/02/12 18:08:57, turnidge wrote:
> Maybe "NameEquals?"

Done.

https://codereview.chromium.org/920813003/diff/1/runtime/vm/service_isolate.h...
runtime/vm/service_isolate.h:20: static bool HasServiceIsolate();
On 2015/02/12 18:08:57, turnidge wrote:
> ServiceIsolate::HasServiceIsolate is repetitive -- maybe
> ServiceIsolate:IsInitialized or ServiceIsolate::Exists or something like that?

ServiceIsolate::Exists

[turnidge, 2015-02-12 18:25:39]

lgtm

[Cutch, 2015-02-12 19:10:16]

New patchsets have been uploaded after l-g-t-m from turnidge@google.com

[Cutch, 2015-02-12 19:11:08]

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