Pass a DesktopEnvironmentFactory when creating DesktopProcess.

Pass a DesktopEnvironmentFactory when starting a DesktopProcess, allow the caller to control the kind of desktop environment to be created.

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

[alexeypa (please no reviews), 2013-01-30 00:48:58]

PTAL.

[alexeypa (please no reviews), 2013-01-31 17:33:54]

Ping.

[Wez, 2013-02-01 01:02:10]

LGTM w/ some nits, but I'm not sure that the use of gmock is safe.

sergeyu: Can you take a look please?  The test code creates some mock objects
and adds expectations on them, from within expectation-triggered invocations,
which I'm not sure is safe.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process.h (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process.h:34: // |desktop_environment_factory| must
outlive |this|.
Out of date comment?

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process.h:67: scoped_ptr<DesktopEnvironmentFactory>
desktop_environment_factory_;
nit: Suggest comment "Factory used to create integration components for use by
each client." or similar.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process_main.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_main.cc:135: // Create proper kind of desktop
environment factory.
nit: "... proper kind of ..." -> "... platform-dependent ..."?

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process_unittest.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_unittest.cc:199: InvokeWithoutArgs(this,
&DesktopProcessTest::CreateVideoCapturer));
This will end up adding expectations after the test has started running code,
which isn't normally safe in gmock - you may have to switch to using a full fake
implementation here. :(

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_unittest.cc:200: 
nit: Don't really need a blank line here.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_session_agent.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_session_agent.cc:186: // Create an instance of desktop
environment.
nit: Suggest "Create a desktop environment for the new session."

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_session_agent.h (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_session_agent.h:59: virtual DesktopEnvironmentFactory&
GetDesktopEnvironmentFactory() = 0;
nit: This could be desktop_environment_factory(), since it's just a getter.

[Wez, 2013-02-01 01:03:03]

On 2013/01/31 17:33:54, alexeypa wrote:
> Ping.

Sorry for not getting to this sooner, BTW.

[alexeypa (please no reviews), 2013-02-01 17:28:43]

FYI

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process.h (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process.h:34: // |desktop_environment_factory| must
outlive |this|.
On 2013/02/01 01:02:11, Wez wrote:
> Out of date comment?

Done.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process.h:67: scoped_ptr<DesktopEnvironmentFactory>
desktop_environment_factory_;
On 2013/02/01 01:02:11, Wez wrote:
> nit: Suggest comment "Factory used to create integration components for use by
> each client." or similar.

Done.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process_main.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_main.cc:135: // Create proper kind of desktop
environment factory.
On 2013/02/01 01:02:11, Wez wrote:
> nit: "... proper kind of ..." -> "... platform-dependent ..."?

Done.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process_unittest.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_unittest.cc:199: InvokeWithoutArgs(this,
&DesktopProcessTest::CreateVideoCapturer));
On 2013/02/01 01:02:11, Wez wrote:
> This will end up adding expectations after the test has started running code,
> which isn't normally safe in gmock - you may have to switch to using a full
fake
> implementation here. :(

We use this pattern (create a new object and set a bunch of expectations - all
on the main thread) in many places. |g_gmock_mutex| is taken when expectations
are set/checked. It appears to be safe enough to create expectations on-the-fly
(on a newly created object).

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_unittest.cc:200: 
On 2013/02/01 01:02:11, Wez wrote:
> nit: Don't really need a blank line here.

Done.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_session_agent.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_session_agent.cc:186: // Create an instance of desktop
environment.
On 2013/02/01 01:02:11, Wez wrote:
> nit: Suggest "Create a desktop environment for the new session."

Done.

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_session_agent.h (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_session_agent.h:59: virtual DesktopEnvironmentFactory&
GetDesktopEnvironmentFactory() = 0;
On 2013/02/01 01:02:11, Wez wrote:
> nit: This could be desktop_environment_factory(), since it's just a getter.

Done.

[Wez, 2013-02-01 17:30:24]

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
File remoting/host/desktop_process_unittest.cc (right):

https://chromiumcodereview.appspot.com/12087073/diff/8001/remoting/host/deskt...
remoting/host/desktop_process_unittest.cc:199: InvokeWithoutArgs(this,
&DesktopProcessTest::CreateVideoCapturer));
On 2013/02/01 17:28:43, alexeypa wrote:
> On 2013/02/01 01:02:11, Wez wrote:
> > This will end up adding expectations after the test has started running
code,
> > which isn't normally safe in gmock - you may have to switch to using a full
> fake
> > implementation here. :(
> 
> We use this pattern (create a new object and set a bunch of expectations - all
> on the main thread) in many places. |g_gmock_mutex| is taken when expectations
> are set/checked. It appears to be safe enough to create expectations
on-the-fly
> (on a newly created object).

Yes, I think that the newly-created-object aspect is the key thing. Cool.

[commit-bot: I haz the power, 2013-02-01 17:32:24]

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

[commit-bot: I haz the power, 2013-02-01 17:46:52]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-02-01 19:54:07]

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

[commit-bot: I haz the power, 2013-02-01 23:19:16]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-02-01 23:46:09]

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

[commit-bot: I haz the power, 2013-02-04 15:06:20]

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

[commit-bot: I haz the power, 2013-02-04 17:28:18]

Change committed as 180443