Use VP8 over PseudoTCP by default.

Use VP8 over PseudoTCP by default. 

Added option in simple_host to choose video codec. Removed ugly ifdefs from chromoting_host.cc.

BUG=None
TEST=Unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67164

[Alpha Left Google, 2010-11-23 00:38:25]

http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor.h
File remoting/host/event_executor.h (right):

http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor...
remoting/host/event_executor.h:17: class EventExecutor : public
protocol::InputStub {
We'll make EventExecutor go away so create a static factory method defined in
event_executor.h but no need define a new class.

[Alpha Left Google, 2010-11-23 00:39:31]

Other than defining EventExecutor this patch looks good to me.

[dmac, 2010-11-23 01:14:56]

http://codereview.chromium.org/5298001/diff/12001/remoting/host/capturer.h
File remoting/host/capturer.h (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/capturer.h#ne...
remoting/host/capturer.h:86: static Capturer* Create(MessageLoop* message_loop);
Can you add a comment on this method? Also, should it be up at the top of the
class definition?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:35: context, config,
any reason not to have these up on line 34?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:63: if (MessageLoop::current() !=
context_->main_message_loop()) {
DCHECK that protocol_config_ is set?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:67: ChromotingHost(ChromotingHostContext*
context, MutableHostConfig* config,
do we want to leave this exposed if we have the factory methods?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:71: static ChromotingHost*
Create(ChromotingHostContext* context,
Can we comment these?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:113: // |config| is transfered to the object.
Must be called before Start().
s/transfered/transferred

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:114: void
set_protocol_config(protocol::CandidateSessionConfig* config);
Should we just pass it in in create?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/simple_host_p...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:51: static char* GetEnvironmentVar(const
char* x) { return getenv(x); }
use routines in base/environment.h for dealing with environment?

http://codereview.chromium.org/5298001/diff/12001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:58: const char kFakeSwitchName[] = "fake";
should these switches be in chrome/switches? or perhaps we should make a
remoting/switches file?

[Sergey Ulanov, 2010-11-23 01:59:17]

http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor.h
File remoting/host/event_executor.h (right):

http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor...
remoting/host/event_executor.h:17: class EventExecutor : public
protocol::InputStub {
On 2010/11/23 00:38:25, Alpha wrote:
> We'll make EventExecutor go away so create a static factory method defined in
> event_executor.h but no need define a new class.

I think it is still better to have EventExecutor class:
 1. It shows that EventExecutorWin, EventExecutorLinux and EventExecutorMac are
related to each other.
 2. We may need to add some other methods in here that are not part of
InputStub.
 3. It might be necessary to share some implementation details between different
event executors (e.g. event queuing).
 4. EventExecutor::Create() looks better than CreateEventExecutor().

http://codereview.chromium.org/5298001/diff/12001/remoting/host/capturer.h
File remoting/host/capturer.h (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/capturer.h#ne...
remoting/host/capturer.h:86: static Capturer* Create(MessageLoop* message_loop);
On 2010/11/23 01:14:57, dmac wrote:
> Can you add a comment on this method? Also, should it be up at the top of the
> class definition?

Done.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:35: context, config,
On 2010/11/23 01:14:57, dmac wrote:
> any reason not to have these up on line 34?

Done.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.cc:63: if (MessageLoop::current() !=
context_->main_message_loop()) {
On 2010/11/23 01:14:57, dmac wrote:
> DCHECK that protocol_config_ is set?
I've added DCHECK in set_protocol_config(). This guarantees that it is always
set.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:67: ChromotingHost(ChromotingHostContext*
context, MutableHostConfig* config,
On 2010/11/23 01:14:57, dmac wrote:
> do we want to leave this exposed if we have the factory methods?

Done.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:71: static ChromotingHost*
Create(ChromotingHostContext* context,
On 2010/11/23 01:14:57, dmac wrote:
> Can we comment these?

Done.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:113: // |config| is transfered to the object.
Must be called before Start().
On 2010/11/23 01:14:57, dmac wrote:
> s/transfered/transferred

Done.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/chromoting_ho...
remoting/host/chromoting_host.h:114: void
set_protocol_config(protocol::CandidateSessionConfig* config);
On 2010/11/23 01:14:57, dmac wrote:
> Should we just pass it in in create?
In most cases it is desired to use default config, so it is better if we have
this separated from constructor

http://codereview.chromium.org/5298001/diff/12001/remoting/host/simple_host_p...
File remoting/host/simple_host_process.cc (right):

http://codereview.chromium.org/5298001/diff/12001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:51: static char* GetEnvironmentVar(const
char* x) { return getenv(x); }
On 2010/11/23 01:14:57, dmac wrote:
> use routines in base/environment.h for dealing with environment?
This can be done in a separate CL. I've added TODO.

http://codereview.chromium.org/5298001/diff/12001/remoting/host/simple_host_p...
remoting/host/simple_host_process.cc:58: const char kFakeSwitchName[] = "fake";
On 2010/11/23 01:14:57, dmac wrote:
> should these switches be in chrome/switches?
These options are for simple_host, and they don't work for chrome.

> or perhaps we should make a remoting/switches file?
I don't think we need it right now. We will not need simple_host in the future
anyway.

[Alpha Left Google, 2010-11-23 05:15:24]

http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor.h
File remoting/host/event_executor.h (right):

http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor...
remoting/host/event_executor.h:17: class EventExecutor : public
protocol::InputStub {
On 2010/11/23 01:59:18, sergeyu wrote:
> On 2010/11/23 00:38:25, Alpha wrote:
> > We'll make EventExecutor go away so create a static factory method defined
in
> > event_executor.h but no need define a new class.
> 
> I think it is still better to have EventExecutor class:
>  1. It shows that EventExecutorWin, EventExecutorLinux and EventExecutorMac
are
> related to each other.
>  2. We may need to add some other methods in here that are not part of
> InputStub.
>  3. It might be necessary to share some implementation details between
different
> event executors (e.g. event queuing).
>  4. EventExecutor::Create() looks better than CreateEventExecutor().

I suggest we add such class when we need it. Right now I still don't see what is
needed more than this just the InputStub interface. And it is supposed to just
implement the InputStub interface..

[Sergey Ulanov, 2010-11-23 20:32:48]

On 2010/11/23 05:15:24, Alpha wrote:
>
http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor.h
> File remoting/host/event_executor.h (right):
> 
>
http://codereview.chromium.org/5298001/diff/6001/remoting/host/event_executor...
> remoting/host/event_executor.h:17: class EventExecutor : public
> protocol::InputStub {
> On 2010/11/23 01:59:18, sergeyu wrote:
> > On 2010/11/23 00:38:25, Alpha wrote:
> > > We'll make EventExecutor go away so create a static factory method defined
> in
> > > event_executor.h but no need define a new class.
> > 
> > I think it is still better to have EventExecutor class:
> >  1. It shows that EventExecutorWin, EventExecutorLinux and EventExecutorMac
> are
> > related to each other.
> >  2. We may need to add some other methods in here that are not part of
> > InputStub.
> >  3. It might be necessary to share some implementation details between
> different
> > event executors (e.g. event queuing).
> >  4. EventExecutor::Create() looks better than CreateEventExecutor().
> 
> I suggest we add such class when we need it. Right now I still don't see what
is
> needed more than this just the InputStub interface. And it is supposed to just
> implement the InputStub interface..
Ok. I've removed EventExecutor

[Alpha Left Google, 2010-11-23 20:34:14]

Thanks. LGTM.

[dmaclach1, 2010-11-23 22:51:11]

LGTM

On Tue, Nov 23, 2010 at 12:34 PM,  <hclam@chromium.org> wrote:
> Thanks. LGTM.
>
>
>
> http://codereview.chromium.org/5298001/
>