Note that a side effect of this change is that we now will be able to raise events from transports since transports now will have a (smart) pointer to the connection_impl instance.
Before we would create factories in the `connection` class and pass references to them when creating the `connection_impl` instance. In the long run this won't work for at least couple of reasons:
- transports will have to have a reference to the `connection_impl` to be able to let connection know about events (e.g. message received, disconnected, error etc). As a result to create a transport a (smart) pointer to the `connection_impl` will have to be passed to the `transport_factory.create()` method. However this would require making the `connection_impl type` (and all types it uses) public because the factories are members of a public (i.e. the `connection`) type. An alternative is to use uniqe pointers which will be set to default factories in the `connection_impl` itself. Tests will still be able to inject their own implementation. An additional advantage of using pointers is that now we can hide a few types that were only exposed because we had references in the connection class and since the references are gone the types don't have to exposed anymore.
- because transports will have to have a back pointer to a `connection_impl` instance it will be possible that the `connection_impl` instance will outlive the `connection` object itself (think about tasks that captured a pointer to `connection_impl` and that running after the `connection` object goes out of scope). In this scenario the transport/`connection_impl` may want to use factories while they were already destroyed because they were members of an object that has already been destroyed. This would result in undefined behavior and most likely a crash. Moving factories to the `connection_impl` fixes this issue (note: currently the `connection` type holds a raw pointer to a `connection_impl` instance which is `delete`d in the `connection`'s dtor; this will have to be changed (most likely by changing the raw pointer to a smart pointer) to prevent from trying to use an already disposed pointer in transports - same as above)
As a result of this change we are 'polluting' the code base with smart pointers but the public surface is cleaner than before.
Capturing `this` pointer in task lambdas is problematic because the task may run after the object gets out of scope or is destructed. The websocket transport tasks were rewritten to not capture `this` pointer anymore. In the future in cases where we need to be able to access members of `this` we will pass it as a smart pointer.
casablanca websocket_client returns websocket_incoming_message. However because there is no way to set the websocket_incoming_message response contents externally testing scenarios where messages were received was not possible. To make the code testable we would have to change the template type to something that returns pplx::task<utility::string_t> instead of pplx::task<websocket_incoming_message> which meant we could not use the casablanca websocket_client directly as the template type which was the main point of using the template in the first place. As a result the static polymorphism (i.e. template) is being replaced with runtime polymorphism (base pure virtual class).
- moving url and querystring from connection to connection_impl
- marking ctors with 1 mandatory parameter as explicit to prevent from implicit conversions
- moving implementations from headers to cpp files
No functional changes
Note: these are just basic changes for the already exisiting Release|Win32 configuration. we will need to rethink and consolidate the build when we start buildning for other architectures/platforms
moozzyk
Pawel Kadluczka