Start chromoting host in the service process though a method call

Start chromoting host in  the service process though a method call

This change exposes method calls to configure the chromoting host and allow
it to be started from a method.

This will allow us to use IPC message to start the chromoting host.

TEST=unit_tests --gtest_filter=ServiceProcess*
BUG=50243, 50244
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55507

[Alpha Left Google, 2010-08-09 19:09:26]

I won't check this in as is.

As ServiceProcessTest.RunChromotingUntilShutdown is actually making connections
and is taking too long to run. I'll need to disable it or move it to a offline
test suite.

[Sergey Ulanov, 2010-08-09 22:29:16]

http://codereview.chromium.org/3084024/diff/1/3
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/3084024/diff/1/3#newcode185
chrome/service/service_process.cc:185: void
ServiceProcess::SaveChromotingConfig(const std::string& login,
I think this method belongs to tests, should be moved to
service_process_unittest.cc.

[Alpha Left Google, 2010-08-09 22:34:19]

http://codereview.chromium.org/3084024/diff/1/3
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/3084024/diff/1/3#newcode185
chrome/service/service_process.cc:185: void
ServiceProcess::SaveChromotingConfig(const std::string& login,
On 2010/08/09 22:29:16, sergeyu wrote:
> I think this method belongs to tests, should be moved to
> service_process_unittest.cc.

This method is actually for setting the chromoting config from the browser
through IPC, it's just not hooked up in this patch.

[Sergey Ulanov, 2010-08-09 23:10:17]

http://codereview.chromium.org/3084024/diff/1/3
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/3084024/diff/1/3#newcode185
chrome/service/service_process.cc:185: void
ServiceProcess::SaveChromotingConfig(const std::string& login,
On 2010/08/09 22:34:19, Alpha wrote:
> On 2010/08/09 22:29:16, sergeyu wrote:
> > I think this method belongs to tests, should be moved to
> > service_process_unittest.cc.
> 
> This method is actually for setting the chromoting config from the browser
> through IPC, it's just not hooked up in this patch.
I see. Then it should be separated into multiple methods: we don't need to
update all config parameters every time. 
login and token may need to be updated without changing anything else,
host_id and private_key should be generated only once per machine (and probably
it is better to do this in the host process),
host_name is not really necessary in the host config, we should probably remove
it.
Beside that, in future we will need to add some other config settings, and here
you have one function parameter for each config settings, so this method will
become a monster with 100 parameters.
A possible solution is to use DictionaryValue (or something like map<string,
string>) to pass list all options you need to be updated in the config.

[Alpha Left Google, 2010-08-09 23:22:10]

On 2010/08/09 23:10:17, sergeyu wrote:
> http://codereview.chromium.org/3084024/diff/1/3
> File chrome/service/service_process.cc (right):
> 
> http://codereview.chromium.org/3084024/diff/1/3#newcode185
> chrome/service/service_process.cc:185: void
> ServiceProcess::SaveChromotingConfig(const std::string& login,
> On 2010/08/09 22:34:19, Alpha wrote:
> > On 2010/08/09 22:29:16, sergeyu wrote:
> > > I think this method belongs to tests, should be moved to
> > > service_process_unittest.cc.
> > 
> > This method is actually for setting the chromoting config from the browser
> > through IPC, it's just not hooked up in this patch.
> I see. Then it should be separated into multiple methods: we don't need to
> update all config parameters every time. 
> login and token may need to be updated without changing anything else,
> host_id and private_key should be generated only once per machine (and
probably
> it is better to do this in the host process),
> host_name is not really necessary in the host config, we should probably
remove
> it.
> Beside that, in future we will need to add some other config settings, and
here
> you have one function parameter for each config settings, so this method will
> become a monster with 100 parameters.
> A possible solution is to use DictionaryValue (or something like map<string,
> string>) to pass list all options you need to be updated in the config.

The use case of this API is that we'll only call it once and assume it is valid
all the time.

I'd like to revise this after we have finalized how we start a service process.

[Sergey Ulanov, 2010-08-09 23:50:48]

On Mon, Aug 9, 2010 at 4:22 PM, <hclam@chromium.org> wrote:

> On 2010/08/09 23:10:17, sergeyu wrote:
>
>> http://codereview.chromium.org/3084024/diff/1/3
>> File chrome/service/service_process.cc (right):
>>
>
>  http://codereview.chromium.org/3084024/diff/1/3#newcode185
>> chrome/service/service_process.cc:185: void
>> ServiceProcess::SaveChromotingConfig(const std::string& login,
>> On 2010/08/09 22:34:19, Alpha wrote:
>> > On 2010/08/09 22:29:16, sergeyu wrote:
>> > > I think this method belongs to tests, should be moved to
>> > > service_process_unittest.cc.
>> >
>> > This method is actually for setting the chromoting config from the
>> browser
>> > through IPC, it's just not hooked up in this patch.
>> I see. Then it should be separated into multiple methods: we don't need to
>> update all config parameters every time.
>> login and token may need to be updated without changing anything else,
>> host_id and private_key should be generated only once per machine (and
>>
> probably
>
>> it is better to do this in the host process),
>> host_name is not really necessary in the host config, we should probably
>>
> remove
>
>> it.
>> Beside that, in future we will need to add some other config settings, and
>>
> here
>
>> you have one function parameter for each config settings, so this method
>> will
>> become a monster with 100 parameters.
>> A possible solution is to use DictionaryValue (or something like
>> map<string,
>> string>) to pass list all options you need to be updated in the config.
>>
>
> The use case of this API is that we'll only call it once and assume it is
> valid
> all the time.
>
> I'd like to revise this after we have finalized how we start a service
> process.
>
Add TODO to refactor this method?
LGTM otherwise.


>
>

>
>
> http://codereview.chromium.org/3084024/show
>