Cleanups in Chromoting Host.

Cleanups in Chromoting Host.

1. Removed constants.[h|cc]
2. Removed platform if-defs in remoting_me2me_host.cc where they are not necessary.

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

[Wez, 2012-09-13 23:59:34]

LGTM

http://codereview.chromium.org/10913260/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10913260/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:204: want_user_interface = getuid() != 0;
nit: Indentation.

[Sergey Ulanov, 2012-09-14 00:01:11]

http://codereview.chromium.org/10913260/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10913260/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:204: want_user_interface = getuid() != 0;
On 2012/09/13 23:59:34, Wez wrote:
> nit: Indentation.

Done.

[Jamie, 2012-09-14 00:07:52]

http://codereview.chromium.org/10913260/diff/10001/remoting/host/host_exit_co...
File remoting/host/host_exit_codes.h (right):

http://codereview.chromium.org/10913260/diff/10001/remoting/host/host_exit_co...
remoting/host/host_exit_codes.h:11: //
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh
Please update the referenced file to point to this new file instead of
constants.h.

http://codereview.chromium.org/10913260/diff/10001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (left):

http://codereview.chromium.org/10913260/diff/10001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:72: #include <signal.h>
Isn't this required for the definition of SIGTERM? Even if we're pulling it in
through some other means, I think we should keep it for IWYU.

[Sergey Ulanov, 2012-09-14 00:11:45]

http://codereview.chromium.org/10913260/diff/10001/remoting/host/host_exit_co...
File remoting/host/host_exit_codes.h (right):

http://codereview.chromium.org/10913260/diff/10001/remoting/host/host_exit_co...
remoting/host/host_exit_codes.h:11: //
remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh
On 2012/09/14 00:07:53, Jamie wrote:
> Please update the referenced file to point to this new file instead of
> constants.h.

Done.

http://codereview.chromium.org/10913260/diff/10001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (left):

http://codereview.chromium.org/10913260/diff/10001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:72: #include <signal.h>
On 2012/09/14 00:07:53, Jamie wrote:
> Isn't this required for the definition of SIGTERM? Even if we're pulling it in
> through some other means, I think we should keep it for IWYU.

signal.h is already included in line 61.

[Jamie, 2012-09-14 01:05:32]

lgtm

[alexeypa (please no reviews), 2012-09-14 15:45:26]

The two REMOTING_MULTI_PROCESS-related issues a important. Everything else lgtm.

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (left):

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:313: #if !defined(REMOTING_MULTI_PROCESS)
This #if should stay. It is needed for multi-process refactoring. The network
process (AKA the me2me host) will receive the configuration over IPC.

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:674: #if !defined(REMOTING_MULTI_PROCESS)
This #if should stay. It is needed for multi-process refactoring. The network
process (AKA the me2me host) will receive the configuration over IPC.

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/usage...
File remoting/host/usage_stats_consent_win.cc (right):

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/usage...
remoting/host/usage_stats_consent_win.cc:28: const wchar_t kHostOmahaAppid[] =
L"{b210701e-ffc4-49e3-932b-370728c72662}";
nit: Strictly speaking it does not belong here. branding.cc is a better, though
far from perfect, place.

[Sergey Ulanov, 2012-09-14 17:35:02]

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (left):

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:313: #if !defined(REMOTING_MULTI_PROCESS)
On 2012/09/14 15:45:26, alexeypa wrote:
> This #if should stay. It is needed for multi-process refactoring. The network
> process (AKA the me2me host) will receive the configuration over IPC.

Right, it's just we never create config_watcher_ anyway, so there is no harm in
resetting it here.

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:674: #if !defined(REMOTING_MULTI_PROCESS)
On 2012/09/14 15:45:26, alexeypa wrote:
> This #if should stay. It is needed for multi-process refactoring. The network
> process (AKA the me2me host) will receive the configuration over IPC.

Same here. This change doesn't mean we actually watch the config file - these
values will never be actually set. I just want to reduce number of ifdefs
because they make this code less readable.

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/usage...
File remoting/host/usage_stats_consent_win.cc (right):

https://chromiumcodereview.appspot.com/10913260/diff/9014/remoting/host/usage...
remoting/host/usage_stats_consent_win.cc:28: const wchar_t kHostOmahaAppid[] =
L"{b210701e-ffc4-49e3-932b-370728c72662}";
On 2012/09/14 15:45:26, alexeypa wrote:
> nit: Strictly speaking it does not belong here. branding.cc is a better,
though
> far from perfect, place.

Yes, it was a mistake to put it here. I added new remoting/host/win/omaha.h
header and put it there. It's also used in the plugin, so it has to be in a
shared header.

[alexeypa (please no reviews), 2012-09-14 17:44:58]

lgtm

[Avi (use Gerrit), 2012-09-24 21:46:07]

Reverting.

Broke the official Win builder.

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/bu...
:

461>usage_stats_consent_win.obj :error LNK2001: unresolved external symbol
"wchar_t const * const remoting::kHostOmahaAppid"
(?kHostOmahaAppid@remoting@@3QB_WB)

[Lei Zhang, 2012-09-27 02:48:23]

On 2012/09/24 21:46:07, Avi wrote:
> Reverting.
> 
> Broke the official Win builder.
> 
>
http://build.chromium.org/p/chromium.chrome/builders/Google%2520Chrome%2520Wi...
> :
> 
> 461>usage_stats_consent_win.obj :error LNK2001: unresolved external symbol
> "wchar_t const * const remoting::kHostOmahaAppid"
> (?kHostOmahaAppid@remoting@@3QB_WB)

Erm, the official win builder broke again.