Merge lp:~alan-griffiths/mir/simplify-connect-code into lp:mir

[Alan Griffiths]

client: remove a path that could no longer execute

client: remove a path that could no longer execute.

"connect_result->has_error()" is always true as connect_result->error() is set in the constructor.

Further, connect errors always result in the server disconnecting without replying, so we never get an error in the result object.

All of this means the error message is misleading and the logic overly complex.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2869
http://jenkins.qa.ubuntu.com/job/mir-ci/4644/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3627
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/48
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2538
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3577
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/793
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/793/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3577
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3577/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6346
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22751

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4644/rebuild

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

ok

[Cemil Azizoglu (cemil-azizoglu)]

oops

review: Approve

[Chris Halse Rogers (raof)]

Yup, correct.

review: Approve

[Daniel van Vugt (vanvugt)]

It seems like connect_done=true and set_error_message("Connect failed") should be mutually exclusive things. Because if the connect failed it's arguably never done. Unless done means "attempted". Needs checking...

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1793/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3637
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/58
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2548
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3587
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/346/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3587
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3587/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6356
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22765

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Unrelated failures. I sense these are going to pop up elsewhere too...

[ FAILED ] ExchangeBufferTest.server_can_send_buffer
[ FAILED ] FD leak was detected

[Daniel van Vugt (vanvugt)]

--> bug 1487967

[Chris Halse Rogers (raof)]

The connect_done variable is tracking whether or not the connection data is going to be modified by the connected() callback; the connection has been done regardless of whether it succeeded or not.

[Daniel van Vugt (vanvugt)]

It's a matter of clear communication and readability such that a person reading some small portion of the file does not get the wrong idea. We can obviously do better but it's not a new issue in this branch so not a blocker.

[Daniel van Vugt (vanvugt)]

review: Abstain

[Kevin DuBois (kdub)]

> It seems like connect_done=true and set_error_message("Connect failed") should
> be mutually exclusive things. Because if the connect failed it's arguably
> never done. Unless done means "attempted". Needs checking...

+1, I think that it would be a bit easier if we didn't have the constraint that the actual Mir{Surface,Connection,etc}* was given out to the client before we have a response from the server. We'd need some sort of misdirection to accommodate the no-wait guarantees of the client api, but imo, would be less confusing. At any rate, beyond scope of this cleanup, lgtm.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2015-08-12 06:37:54 +0000
+++ src/client/mir_connection.cpp	2015-08-21 11:58:59 +0000
@@ -254,21 +254,12 @@
 
 void MirConnection::connected(mir_connected_callback callback, void * context)
 {
-    bool safe_to_callback = true;
     try
     {
         std::lock_guard<decltype(mutex)> lock(mutex);
 
         if (!connect_result->has_platform() || !connect_result->has_display_configuration())
-        {
-            if (!connect_result->has_error())
-            {
-                // We're handling an error scenario that means we're not sync'd
-                // with the client code - a callback isn't safe (or needed)
-                safe_to_callback = false;
-                set_error_message("Connect failed");
-            }
-        }
+            set_error_message("Connect failed");
 
         connect_done = true;
 
@@ -307,7 +298,7 @@
                                  boost::diagnostic_information(e));
     }
 
-    if (safe_to_callback) callback(this, context);
+    callback(this, context);
     connect_wait_handle.result_received();
 }