Move the |ElevatedDaemonController| implementation to |DaemonControllerDelegateWin|

Move the |ElevatedDaemonController| implementation to |DaemonControllerDelegateWin|

Changes in this CL include:
1. Moved the daemon controller implemetation from |ElevatedDaemonController| to |DaemonControllerDelegateWin|
2. Removed all code related to COM activation in |DaemonControllerDelegateWin|.
3. Removed code related to SetOwnerWindow in |DaemonControllerDelegateWin|.
4. Removed code related to the pin confirmation dialog in |DaemonControllerDelegateWin|
5. Cleaned up the COM-styled code to make it adhere to the chromium coding styles.

BUG=453172
Committed: https://crrev.com/69cccfced484ec798815c5804f7b9825c3e5da9e
Cr-Commit-Position: refs/heads/master@{#315129}

[weitao, 2015-01-31 00:13:52]

weitaosu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[weitao, 2015-01-31 00:17:28]

This is change #2. PTAL. More to come.

[Sergey Ulanov, 2015-01-31 01:06:58]

I'm looking forward to ElevatedController removal :-)

There are bunch of cleanups that can be done here and I'd prefer doing them
before landing this CL. I understand that you are trying to make a series of
small changes, but this CL would be smaller (i.e. easier to review) if you
simplify as I suggested in my comments.

Also please cleanup CL description. IMO it's not useful to include detailed plan
for the future changes there. Also comments like "If you prefer you can review
the ..." don't belong to the descriptions (it's not useful to whoever will be
reading this description after this CL lands).

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
File remoting/host/setup/daemon_controller_delegate_win.cc (right):

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:73: HRESULT
ReadConfig(const base::FilePath& filename,
return bool instead of HRESULT.

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:483: HRESULT
DaemonControllerDelegateWin::DoGetConfig(BSTR* config_out) {
Return bool instead of HRESULT here? The caller only needs to know if it
succeeded or not. Same for all other calls below.

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:483: HRESULT
DaemonControllerDelegateWin::DoGetConfig(BSTR* config_out) {
You don't really need this function. Just call ReadConfig() from
DaemonControllerDelegateWin::GetConfig()?

This and the previous comment also apply to most functions below.

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
File remoting/host/setup/daemon_controller_delegate_win.h (right):

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.h:8: #include
"base/win/scoped_comptr.h"
This can be removed.

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.h:12: class FilePath;
Don't need this.

https://codereview.chromium.org/877343004/diff/100001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.h:40: HRESULT
DoGetConfig(BSTR* config_out);
These don't need to be methods in this class. Make them static functions same as
in elevated_controller.cc?

[weitao, 2015-02-03 23:47:02]

Sergey, I cleaned up the code in the same CL as we discussed. Could you please
take another look?

[Sergey Ulanov, 2015-02-04 00:47:07]

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
File remoting/host/setup/daemon_controller_delegate_win.cc (right):

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:75: 
nit: remove this empty line

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:80: FILE_SHARE_READ |
FILE_SHARE_WRITE,
I don't think we need Write given that we are only reading from this file, but
see my next comment.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:93: if
(!::ReadFile(file.Get(), &buffer[0], size, &size, nullptr)) {
Maybe use base::ReadFileToString() to read the file? It would make this function
much simpler.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:104: if (value.get() ==
nullptr || !value->GetAsDictionary(&dictionary)) {
s/value.get() == nullptr/!value/

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:105: LOG(ERROR) << "Failed
to read '" << filename.value() << "'.";
Failed to parse.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:195: if
(!config_dict->GetString(kXmppLoginConfigPath, &email)) {
these 3 ifs can be merge into one.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:241: if
(!MoveConfigFileFromTemp(unprivileged_config_file_path))
nit: you can just replace these two ifs with

return Move(file1) && Move(file2);

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:273: void
OpenService(ScopedScHandle* service_out, DWORD access) {
I think ScopedScHandle supports move semantics, similar to scoped_ptr<>, so this
can just return ScopedScHandle.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:291:
service_out->Set(service.Take());
return service.Pass();

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:301: bool SetConfig(const
std::string& config) {
I don't think you really need this function. Move it inline in
SetConfigAndStart()?

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:436:
InvokeCompletionCallback(done, false);
LOG the error here?

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:450: // Write the updated
config.
Empty line above this one.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:474:
FileVersionInfo::CreateFileVersionInfoForModule(binary));
It would be easier to return STRINGIZE(VERSION). But GetVersion() can be removed
from DaemonController. It's not used anymore, so maybe just stub it for now,
return an empty string.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:525:
InvokeCompletionCallback(done, result);
InvokeCompletionCallback(done, StartDaemon())?

[weitao, 2015-02-05 00:14:35]

PTAL

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
File remoting/host/setup/daemon_controller_delegate_win.cc (right):

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:75: 
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> nit: remove this empty line

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:93: if
(!::ReadFile(file.Get(), &buffer[0], size, &size, nullptr)) {
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> Maybe use base::ReadFileToString() to read the file? It would make this
function
> much simpler.

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:104: if (value.get() ==
nullptr || !value->GetAsDictionary(&dictionary)) {
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> s/value.get() == nullptr/!value/

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:105: LOG(ERROR) << "Failed
to read '" << filename.value() << "'.";
On 2015/02/04 00:47:07, Sergey Ulanov wrote:
> Failed to parse.

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:195: if
(!config_dict->GetString(kXmppLoginConfigPath, &email)) {
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> these 3 ifs can be merge into one.

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:241: if
(!MoveConfigFileFromTemp(unprivileged_config_file_path))
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> nit: you can just replace these two ifs with
> 
> return Move(file1) && Move(file2);

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:273: void
OpenService(ScopedScHandle* service_out, DWORD access) {
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> I think ScopedScHandle supports move semantics, similar to scoped_ptr<>, so
this
> can just return ScopedScHandle. 

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:291:
service_out->Set(service.Take());
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> return service.Pass();

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:301: bool SetConfig(const
std::string& config) {
On 2015/02/04 00:47:07, Sergey Ulanov wrote:
> I don't think you really need this function. Move it inline in
> SetConfigAndStart()?

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:436:
InvokeCompletionCallback(done, false);
On 2015/02/04 00:47:07, Sergey Ulanov wrote:
> LOG the error here?

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:450: // Write the updated
config.
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> Empty line above this one.

Done.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:474:
FileVersionInfo::CreateFileVersionInfoForModule(binary));
On 2015/02/04 00:47:07, Sergey Ulanov wrote:
> It would be easier to return STRINGIZE(VERSION). But GetVersion() can be
removed
> from DaemonController. It's not used anymore, so maybe just stub it for now,
> return an empty string.

Done. Will remove it later.

https://codereview.chromium.org/877343004/diff/220001/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:525:
InvokeCompletionCallback(done, result);
On 2015/02/04 00:47:06, Sergey Ulanov wrote:
> InvokeCompletionCallback(done, StartDaemon())?

Done.

[Sergey Ulanov, 2015-02-05 20:53:42]

lgtm

https://codereview.chromium.org/877343004/diff/150003/remoting/host/setup/dae...
File remoting/host/setup/daemon_controller_delegate_win.cc (right):

https://codereview.chromium.org/877343004/diff/150003/remoting/host/setup/dae...
remoting/host/setup/daemon_controller_delegate_win.cc:371: if
(service.IsValid()) {
nit: It's better to move the non-error case to the end of the function, i.e.:

if (service.IsValid())
  return DaemonController::STATE_UNKNOWN;

SERVICE_STATUS status;
if (!::QueryServiceStatus(service.Get(), &status)) {
  PLOG(ERROR) << "Failed to query the state of the '"
              << kWindowsServiceName << "' service";
  return DaemonController::STATE_UNKNOWN;
}

return ConvertToDaemonState(status.dwCurrentState);

[weitao, 2015-02-06 22:42:07]

New patchsets have been uploaded after l-g-t-m from sergeyu@chromium.org

[weitao, 2015-02-06 22:42:34]

The CQ bit was checked by weitaosu@chromium.org

[commit-bot: I haz the power, 2015-02-06 22:42:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877343004/270001

[commit-bot: I haz the power, 2015-02-06 23:18:39]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-02-06 23:19:27]

Patchset 15 (id:??) landed as
https://crrev.com/69cccfced484ec798815c5804f7b9825c3e5da9e
Cr-Commit-Position: refs/heads/master@{#315129}