[PATCH xorg-gtest v2 4/8] xserver: move starting the process into the XServer object

[Chase Douglas]

On 07/10/2012 08:28 PM, Peter Hutterer wrote:
> Changes from hardcoded vararg call to requiring the caller to set up options
> for logfile, config, etc. beforehand. The display string is automatically
> added to the command line.
>
> If no binary is given by the caller, the previously set binary path is used.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> Changes to v1:
> - have one Start() call only handling with/without binary
> - changes to accommodate for Process::Start(vector<std::string>) instead of
>    previous const char *
>
>   include/xorg/gtest/xorg-gtest-xserver.h |    8 ++++++++
>   src/environment.cpp                     |    7 +------
>   src/xserver.cpp                         |   19 +++++++++++++++++++
>   3 files changed, 28 insertions(+), 6 deletions(-)
>
> diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
> index 1ba2a0f..0b7e7a0 100644
> --- a/include/xorg/gtest/xorg-gtest-xserver.h
> +++ b/include/xorg/gtest/xorg-gtest-xserver.h
> @@ -47,6 +47,14 @@ class XServer : public xorg::testing::Process {
>       XServer();
>
>       /**
> +     * Start a new server. If no binary is given, the server started is the
> +     * default compiled-in server binary.
> +     *
> +     * @param [in] program Path to the XServer binary
> +     */
> +    void Start(std::string program = std::string());

I'm still not sure we need the program argument. I would rather we stick 
to either setting the program path with SetServerPath(), or providing it 
in Start() as you have here. My preference would be to just drop 
SetServerPath(), but either approach is fine with me.

If you do leave this, pass in a const reference rather than a copy of 
the string. I see that you reuse the variable in the method, but using 
that as a reason to copy the string in would be leaking the 
implementation into the API.

> +
> +    /**
>        * Waits until this server is ready to take connections.
>        */
>       void WaitForConnections(void);
> diff --git a/src/environment.cpp b/src/environment.cpp
> index 7573b62..1521564 100644
> --- a/src/environment.cpp
> +++ b/src/environment.cpp
> @@ -144,12 +144,7 @@ void xorg::testing::Environment::SetUp() {
>     d_->server.SetDisplayNumber(d_->display);
>     d_->server.SetOption("-logfile", d_->path_to_log_file);
>     d_->server.SetOption("-config", d_->path_to_conf);
> -  d_->server.Start(d_->path_to_server, d_->path_to_server.c_str(),
> -                    display_string,
> -                    "-logverbose", "10",
> -                    "-logfile", d_->path_to_log_file.c_str(),
> -                    "-config", d_->path_to_conf.c_str(),
> -                    NULL);
> +  d_->server.Start(d_->path_to_server);

Since you use d_->path_to_server as the default in Start(), there's no 
need to pass it in here.

>     d_->server.WaitForConnections();
>
>     Process::SetEnv("DISPLAY", display_string, true);
> diff --git a/src/xserver.cpp b/src/xserver.cpp
> index 62bd48f..2f9c364 100644
> --- a/src/xserver.cpp
> +++ b/src/xserver.cpp
> @@ -253,6 +253,25 @@ void xorg::testing::XServer::WaitForConnections(void) {
>     throw std::runtime_error("Unable to open connection to dummy X server");
>   }
>
> +void xorg::testing::XServer::Start(std::string program) {
> +  if (program.empty())
> +    program = d_->path_to_server;
> +
> +  std::vector<std::string> args;
> +  std::map<std::string, std::string>::iterator it;
> +
> +  args.push_back(program);
> +  args.push_back(std::string(GetDisplayString()));
> +
> +  for (it = d_->options.begin(); it != d_->options.end(); it++) {
> +    args.push_back(it->first);
> +    if (!it->second.empty())
> +      args.push_back(it->second);
> +  }
> +
> +  Process::Start(program, args);
> +}
> +
>   void xorg::testing::XServer::SetOption(std::string key, std::string value) {
>     d_->options[key] = value;
>   }
>

With the fixes:

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>