Making the me2me host compiling and running on Windows. This includes making it a window applicatio…

Making the me2me host compiling and running on Windows. This includes making it a window application and converting syslog to EventLog.


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

[alexeypa (please no reviews), 2012-03-01 17:24:35]

Could you take a look, please?

[alexeypa (please no reviews), 2012-03-01 17:29:45]

I guess Jamie needs to looks at this too, I guess. Let's split it this way:

Lambros: host_event*.* and me2me_virtual_host.py
Jamie: everything else.

[Wez, 2012-03-01 18:03:05]

Some drive-by comments.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger_win.cc:114: raw_strings[i] = strings[i].c_str();
nit: Add comment to clarify that each string16 retains ownership of the c_str()
version here?

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger_win.cc:118: EVENTLOG_INFORMATION_TYPE,
nit: This should arguably be EVENTLOG_ERROR_TYPE for OnAccessDenied() events.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:94: default_config_dir =
default_config_dir.Append(kDefaultConfigDir);
nit: Fold the |default_config_dir| logic into a static helper function, to
simplify this method.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:335: // the command line from
GetCommandLineW().
nit: At a first glance, it's not clear whether you're saying we don't need to
call Init(), or explaining why we're passing (0, NULL).

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:194: PathService::Get(base::DIR_PROFILE,
&config_path);
This change moves the config from the home directory to the profile, so the CL
description should mention that.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
File remoting/tools/me2me_virtual_host.py (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
remoting/tools/me2me_virtual_host.py:586: signal.signal(s, signal_handler)
We won't be using this script on Windows & Mac, though?

[Jamie, 2012-03-01 18:22:32]

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger.h (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:20: virtual ~HostEventLogger() {}
Be sure to run this by the clang bots. It has some rules about in-line ctor and
dtor, though I can't remember exactly what they are.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:22: #include "base/path_service.h"
If this is only needed on Windows, it should probably be #ifdef'd

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:194: PathService::Get(base::DIR_PROFILE,
&config_path);
On 2012/03/01 18:03:06, Wez wrote:
> This change moves the config from the home directory to the profile, so the CL
> description should mention that.

Better still, unless that change is required for this CL to work, split it into
a separate one.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/remoting.gyp#n...
remoting/remoting.gyp:254: '<(INTERMEDIATE_DIR)/remoting_host.h',
Assuming these outputs just contain messages, it would be clearer to name them
remoting_host_messages.

[alexeypa (please no reviews), 2012-03-01 20:33:58]

PTAL.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger.h (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:20: virtual ~HostEventLogger() {}
On 2012/03/01 18:22:32, Jamie wrote:
> Be sure to run this by the clang bots. It has some rules about in-line ctor
and
> dtor, though I can't remember exactly what they are.

Will do.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger_win.cc:114: raw_strings[i] = strings[i].c_str();
On 2012/03/01 18:03:06, Wez wrote:
> nit: Add comment to clarify that each string16 retains ownership of the
c_str()
> version here?

Done.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger_win.cc:118: EVENTLOG_INFORMATION_TYPE,
On 2012/03/01 18:03:06, Wez wrote:
> nit: This should arguably be EVENTLOG_ERROR_TYPE for OnAccessDenied() events.

Done.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:22: #include "base/path_service.h"
On 2012/03/01 18:22:32, Jamie wrote:
> If this is only needed on Windows, it should probably be #ifdef'd

base/path_service.h is a cross-platfporm header. #ifdef'ing it would be
confusing.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:94: default_config_dir =
default_config_dir.Append(kDefaultConfigDir);
On 2012/03/01 18:03:06, Wez wrote:
> nit: Fold the |default_config_dir| logic into a static helper function, to
> simplify this method.

Done.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:335: // the command line from
GetCommandLineW().
On 2012/03/01 18:03:06, Wez wrote:
> nit: At a first glance, it's not clear whether you're saying we don't need to
> call Init(), or explaining why we're passing (0, NULL).

Done.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:194: PathService::Get(base::DIR_PROFILE,
&config_path);
On 2012/03/01 18:03:06, Wez wrote:
> This change moves the config from the home directory to the profile, so the CL
> description should mention that.

This change does not change the location. There is no GetHomeDir()
implementation for Windows (for a reason). The homegrown implementation we had
was using the user profile folder as the home dir.

Actually this is something that we need to address later. Windows guidelines do
not recommend putting files to the profile folder. So we will need to move the
files to %LOCALAPPDATA% to make it more Windowsy.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/remoting.gyp#n...
remoting/remoting.gyp:254: '<(INTERMEDIATE_DIR)/remoting_host.h',
On 2012/03/01 18:22:32, Jamie wrote:
> Assuming these outputs just contain messages, it would be clearer to name them
> remoting_host_messages.

Done.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
File remoting/tools/me2me_virtual_host.py (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
remoting/tools/me2me_virtual_host.py:586: signal.signal(s, signal_handler)
On 2012/03/01 18:03:06, Wez wrote:
> We won't be using this script on Windows & Mac, though?

I guess so. It was an easy way to get me2me configuration created. I'll move
this file from the CL.

[Jamie, 2012-03-01 20:59:09]

lgtm

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:22: #include "base/path_service.h"
On 2012/03/01 20:33:58, alexeypa wrote:
> On 2012/03/01 18:22:32, Jamie wrote:
> > If this is only needed on Windows, it should probably be #ifdef'd
> 
> base/path_service.h is a cross-platfporm header. #ifdef'ing it would be
> confusing.

I guess the question is why are we using it in a platform-specific way if it's a
cross-platform class. If it's just because we write the config to a different
place on different platforms (profile vs. home dir) then it's not a big deal,
but it feels like there's an opportunity for making things more consistent here.
Probably not in this CL, however.

[Wez, 2012-03-01 21:11:36]

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:194: PathService::Get(base::DIR_PROFILE,
&config_path);
On 2012/03/01 20:33:58, alexeypa wrote:
> On 2012/03/01 18:03:06, Wez wrote:
> > This change moves the config from the home directory to the profile, so the
CL
> > description should mention that.
> 
> This change does not change the location. There is no GetHomeDir()
> implementation for Windows (for a reason). The homegrown implementation we had
> was using the user profile folder as the home dir.

The home-grown code was using HOMEDRIVE and HOMEPATH, rather than USERPROFILE.

> Actually this is something that we need to address later. Windows guidelines
do
> not recommend putting files to the profile folder. So we will need to move the
> files to %LOCALAPPDATA% to make it more Windowsy.

PathService::Get(DIR_LOCAL_APP_DATA, ...) should achieve that.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
File remoting/tools/me2me_virtual_host.py (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
remoting/tools/me2me_virtual_host.py:586: signal.signal(s, signal_handler)
On 2012/03/01 20:33:58, alexeypa wrote:
> On 2012/03/01 18:03:06, Wez wrote:
> > We won't be using this script on Windows & Mac, though?
> 
> I guess so. It was an easy way to get me2me configuration created. I'll move
> this file from the CL.

There is a separate register_host.py script for registering the
remoting_simple_host that should still work, although the output path will need
updating if we move the registration to DIR_LOCAL_APP_DATA, of course.

[Lambros, 2012-03-01 21:15:41]

LGTM with comments.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger.h (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:18: class HostEventLogger {
It's unfortunate that it has to be refactored this way to accommodate the
Windows ReportEvent() API.  I originally had this as a cross-platform class
which passed the strings to a platform-specific SystemEventLogger class.  I
think it would be nicer if the SystemEventLogger API could be widened to accept
a list of strings (log-type, log-severity, list of messages, whatever), and then
hide the ugly Windows-specific details behind that API.

I guess this will have to do for now.  The downside is that it becomes painful
to add new event types to the HostStatusObserver for logging - you now have to
do this separately for all platforms (Win, Linux, Mac), including hand-editing
of the .mc file to match whatever is logged on the other platforms.  I would
prefer if that could be avoided.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:20: virtual ~HostEventLogger() {}
On 2012/03/01 18:22:32, Jamie wrote:
> Be sure to run this by the clang bots. It has some rules about in-line ctor
and
> dtor, though I can't remember exactly what they are.
Should be OK (and preferred!) for a pure interface class.  +1 to running it
through clang :)

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:28: HostEventLogger() {}
nit: Maybe get rid of this ctor, and just make the dtor protected?

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger_win.cc:46: void Log(DWORD event_id, const
std::vector<string16>& strings);
Style guide generally prohibits overloading.  Better to rename one of these :(

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
File remoting/tools/me2me_virtual_host.py (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/tools/me2me_vi...
remoting/tools/me2me_virtual_host.py:586: signal.signal(s, signal_handler)
This script is almost entirely Linux-specific.  I don't think it's a good idea
to try to extend this script to other platforms.  If there is some code in here
that's useful on Windows, feel free to factor it out to a separate module. 
Otherwise, best to leave this script as-is.

[alexeypa (please no reviews), 2012-03-01 22:32:40]

FYI.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger.h (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:18: class HostEventLogger {
On 2012/03/01 21:15:41, Lambros wrote:
> It's unfortunate that it has to be refactored this way to accommodate the
> Windows ReportEvent() API.  I originally had this as a cross-platform class
> which passed the strings to a platform-specific SystemEventLogger class.  I
> think it would be nicer if the SystemEventLogger API could be widened to
accept
> a list of strings (log-type, log-severity, list of messages, whatever), and
then
> hide the ugly Windows-specific details behind that API.
> 
> I guess this will have to do for now.  The downside is that it becomes painful
> to add new event types to the HostStatusObserver for logging - you now have to
> do this separately for all platforms (Win, Linux, Mac), including hand-editing
> of the .mc file to match whatever is logged on the other platforms.  I would
> prefer if that could be avoided.

Yes, I saw SystemEventLogger. Unfortunately it does not work nicely with the the
recommended way events should be reported to Windows Event Log. Basically each
message has to come from the message file and the format string should be
defined in the messages file. It is of course possible to define a single event
"%1" - a placeholder for a string and report each and every message using this
event. The cons of this approach are:

1. It is against Windows guidelines for apps.
2. It doesn't allow Windows style localization of events reported to the event
log. Strange but Windows whats that.
3. It inhibits filtering capabilities of Event Log Viewer which are not that
advanced. 

Let's discuss it face to face because I'm not sure what is the best way myself.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger.h:28: HostEventLogger() {}
On 2012/03/01 21:15:41, Lambros wrote:
> nit: Maybe get rid of this ctor, and just make the dtor protected?

It is required by the compiler and it is protected (as you have noticed already.
:-)).

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
File remoting/host/host_event_logger_win.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/host_even...
remoting/host/host_event_logger_win.cc:46: void Log(DWORD event_id, const
std::vector<string16>& strings);
On 2012/03/01 21:15:41, Lambros wrote:
> Style guide generally prohibits overloading.  Better to rename one of these :(

Done.

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:194: PathService::Get(base::DIR_PROFILE,
&config_path);
On 2012/03/01 21:11:36, Wez wrote:
> The home-grown code was using HOMEDRIVE and HOMEPATH, rather than USERPROFILE.

Yes, I saw that. They can only be different if you change them after login, if
I'm not mistaken. Which is the same thing as changing HOME on Linux. It will
break things.

> PathService::Get(DIR_LOCAL_APP_DATA, ...) should achieve that.

Yes. We'll need to come up with a better name than ".config" anyway. So I'll
better remove this file from this CL for now.

PS. You guys are so picky. :-)

[Wez, 2012-03-01 22:46:30]

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
File remoting/host/simple_host_process.cc (right):

https://chromiumcodereview.appspot.com/9567010/diff/1/remoting/host/simple_ho...
remoting/host/simple_host_process.cc:194: PathService::Get(base::DIR_PROFILE,
&config_path);
On 2012/03/01 22:32:41, alexeypa wrote:
> On 2012/03/01 21:11:36, Wez wrote:
> > The home-grown code was using HOMEDRIVE and HOMEPATH, rather than
USERPROFILE.
> 
> Yes, I saw that. They can only be different if you change them after login, if
> I'm not mistaken. Which is the same thing as changing HOME on Linux. It will
> break things.

Nope.  The home directory and profile directory can differ.  :)

> > PathService::Get(DIR_LOCAL_APP_DATA, ...) should achieve that.
> 
> Yes. We'll need to come up with a better name than ".config" anyway. So I'll
> better remove this file from this CL for now.
> 
> PS. You guys are so picky. :-)

:D

[commit-bot: I haz the power, 2012-03-02 04:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9567010/6003

[commit-bot: I haz the power, 2012-03-02 15:10:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9567010/6003

[commit-bot: I haz the power, 2012-03-02 16:51:46]

Change committed as 124667