Add enable_vp9 option to host config.

Add enable_vp9 option to host config.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277876

[Wez, 2014-06-16 18:21:27]

PTAL!

[Wez, 2014-06-17 03:35:26]

Sergey, can you take a look, please?

[Wez, 2014-06-17 17:27:14]

Jamie, can you take a look, please?

[dcaiafa, 2014-06-17 17:32:54]

lgtm

[Wez, 2014-06-17 17:35:42]

On 2014/06/17 17:32:54, dcaiafa wrote:
> lgtm

Thanks!

[Wez, 2014-06-17 17:35:45]

The CQ bit was checked by wez@chromium.org

[Jamie, 2014-06-17 17:35:49]

https://codereview.chromium.org/331853009/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/331853009/diff/1/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:148: }
Nit: Now that we have two setter/getter pairs, it would be more readable if they
were adjacent and had the same ordering (get before set or vice versa).

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:826: // If the enable VP9 option is
unspecified, or false, then VP9 is not offered.
It actually falls back on the command-line, I think. Is that the right
semantics? I would expect the command-line to override anything set in the
config file.

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:1203: // Enable VP9 if configured via the
host config.
It can also be configured on the command-line. I think I would just remove the
detail from the command. Maybe even just remove the comment.

[commit-bot: I haz the power, 2014-06-17 17:37:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/331853009/1

[Sergey Ulanov, 2014-06-17 17:37:59]

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:826: // If the enable VP9 option is
unspecified, or false, then VP9 is not offered.
I think this should also mention the optional CL flag.

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:827:
config->GetBoolean(kEnableVp9ConfigPath, &enable_vp9_);
Shouldn't command-line option override the flag in the config?
I would prefer initializing enable_vp9_ to false in the constructor and then
call CommandLine::ForCurrentProcess()->HasSwitch() here

[Wez, 2014-06-17 17:58:23]

https://codereview.chromium.org/331853009/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/331853009/diff/1/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:148: }
On 2014/06/17 17:35:48, Jamie wrote:
> Nit: Now that we have two setter/getter pairs, it would be more readable if
they
> were adjacent and had the same ordering (get before set or vice versa).

Done.

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:827:
config->GetBoolean(kEnableVp9ConfigPath, &enable_vp9_);
On 2014/06/17 17:37:59, Sergey Ulanov wrote:
> Shouldn't command-line option override the flag in the config?
> I would prefer initializing enable_vp9_ to false in the constructor and then
> call CommandLine::ForCurrentProcess()->HasSwitch() here

Done.

https://codereview.chromium.org/331853009/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:1203: // Enable VP9 if configured via the
host config.
On 2014/06/17 17:35:48, Jamie wrote:
> It can also be configured on the command-line. I think I would just remove the
> detail from the command. Maybe even just remove the comment.

Done.

[Wez, 2014-06-17 17:58:54]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-06-17 18:00:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/331853009/20001

[commit-bot: I haz the power, 2014-06-17 22:02:47]

Change committed as 277876