Making sure that UAC promts fired by the Chromoting plugin get focus (instead of being shown in the…

Making sure that UAC promts fired by the Chromoting plugin get focus (instead of being shown in the background). This CL also limits the maximum time the Chromoting Host Controller UAC promt can be suspended to 15 minutes.

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

[alexeypa (please no reviews), 2012-04-25 20:35:49]

PTAL.

[Jamie, 2012-04-25 21:20:37]

lgtm, but I would prefer to see this as two separate CLs unless there's a good
reason to combine them.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller.h:120: // elevetion promts properly.
Nit: s/elevetion/elevation/, s/promts/prompts/

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:51: const int kUacTimeoutSec = 15
* 60;
Is this strictly needed as part of this CL? It feels sufficiently complex to
warrant a follow-up.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:61: HRESULT
ActivateElevatedController(void* window_handle,
HWND?

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:124: void* window_handle_;
Store as an HWND?

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:187: 
It's not clear to me which of the previous functions are needed for the UAC
parenting and which are for the timeout, nor why Co[Un]initialize is needed now
and wasn't earlier.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
File remoting/host/plugin/daemon_installer_win.h (right):

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:29: static
scoped_ptr<DaemonInstallerWin> Create(void* window_handle,
HWND?

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:30: CompletionCallback done);
Unrelated to this CL, but I don't understand why we're using a factory pattern
for this class. For the controller its because the same interface is used
cross-platform, but it seems like we could just use "new" to instantiate this
class.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:46: // Returns the first top-level
(i.e. WS_OVERLAPPED or WS_POPUP) window is
Nit: s/is/in/

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:47: // the chain of parents of
|window|. Returns |window| if it is itself represent
Nit: "if it is itself a top-level" or "if it represents a top-level"

[alexeypa (please no reviews), 2012-04-25 21:59:24]

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
File remoting/host/plugin/daemon_controller.h (right):

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller.h:120: // elevetion promts properly.
On 2012/04/25 21:20:37, Jamie wrote:
> Nit: s/elevetion/elevation/, s/promts/prompts/

Done.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:51: const int kUacTimeoutSec = 15
* 60;
On 2012/04/25 21:20:37, Jamie wrote:
> Is this strictly needed as part of this CL? It feels sufficiently complex to
> warrant a follow-up.

I'll move it to a separate CL.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:61: HRESULT
ActivateElevatedController(void* window_handle,
On 2012/04/25 21:20:37, Jamie wrote:
> HWND?

Done.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:124: void* window_handle_;
On 2012/04/25 21:20:37, Jamie wrote:
> Store as an HWND?

Done.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_controller_win.cc:187: 
On 2012/04/25 21:20:37, Jamie wrote:
> It's not clear to me which of the previous functions are needed for the UAC
> parenting and which are for the timeout, 

I'll move the timeout change to a separate CL.

> nor why Co[Un]initialize is needed now and wasn't earlier.

ComThread::Init() & ComThread::CleanUp() were moved to comply with the style
guide.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
File remoting/host/plugin/daemon_installer_win.h (right):

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:29: static
scoped_ptr<DaemonInstallerWin> Create(void* window_handle,
On 2012/04/25 21:20:37, Jamie wrote:
> HWND?

Done.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:30: CompletionCallback done);
On 2012/04/25 21:20:37, Jamie wrote:
> Unrelated to this CL, but I don't understand why we're using a factory pattern
> for this class. For the controller its because the same interface is used
> cross-platform, but it seems like we could just use "new" to instantiate this
> class.

There are two different installer classes: DaemonCommandLineInstallerWin and
DaemonComInstallerWin

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:46: // Returns the first top-level
(i.e. WS_OVERLAPPED or WS_POPUP) window is
On 2012/04/25 21:20:37, Jamie wrote:
> Nit: s/is/in/

Done.

https://chromiumcodereview.appspot.com/10161034/diff/2001/remoting/host/plugi...
remoting/host/plugin/daemon_installer_win.h:47: // the chain of parents of
|window|. Returns |window| if it is itself represent
On 2012/04/25 21:20:37, Jamie wrote:
> Nit: "if it is itself a top-level" or "if it represents a top-level"

Done.

[commit-bot: I haz the power, 2012-04-25 22:01:47]

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

[commit-bot: I haz the power, 2012-04-25 23:50:08]

Change committed as 134014