Merge lp:~robert-ancell/mir/frontend-class-renaming into lp:~mir-team/mir/trunk

[Robert Ancell]

Refactor frontend code to use simpler and more consistent class naming

Rename some of the classes in the frontend to make it simpler to understand. This is based on my first impression of understanding the frontend code. While I could see the intention to make the transport layer (socket, binder) and protocol (protocol buffers) flexible I found this to confuse the classes and be overly abstracted.

1. Rename ProtobufSocketCommunicator to SocketCommicator

This is because the Protobuf part is confusing
 - It makes the relationship to SocketSession less clear.
 - The Protobuf prefix is not consistently applied, i.e. SessionMediator is also Protobuf dependent but doesn't use the prefix.
 - One prefix (Socket) is simpler than two (ProtobufSocket).

2. Rename ProtobufMessageProcessor to MessageProcessor

This is the last remaining "Protobuf" class so removing it seems ideal.
 - Removed the NullMessageProcessor since it was only used in one place - a simple NULL check replaces it.
 - message_process.h was overloaded with two classes (MessageProcessor and MessageSender) - Move MessageSender to its own header file and just use protobuf_message_processor.h as the new message_processor.h.

The one part that doesn't feel great now is the fact that there is a clear abstraction for MessageSender, but not for MessageProcessor (which is tied to protocol buffers). I didn't have a good name for using MessageProcessor as the interface and ?MessageProcessor as the implementation. We could use SocketMessageProcessor except the message processor is independent from the transport layer.

The use of "Processor" as a name seems a bit vague to me (i.e. most classes are "processing" something). I think it would be better to use names that are clearer inverses e.g. Sender/Receiver, Encoder/Decoder or Serializer/Deserializer.

(The last two points are worth discussing but I don't think need to be solved by this merge).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:550
http://jenkins.qa.ubuntu.com/job/mir-ci/271/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/220
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/272
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/272/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/80

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

i'm going to mark as 'needs information' but what I really mean is 'needs discussion'. I guess if the binder code that drove these abstractions is now gone, it would make sense for the abstractions to be reworked. The only part that I don't like is that we're getting rid of MessageProcessor's interface class, will make it harder to mock.

I guess overall I agree with the reasons and the substance for the changes, but don't see the test scenario that's driving the changes that go beyond the name changes (eg, making process_message public instead of private)

review: Needs Information

[Kevin DuBois (kdub)]

> I guess overall I agree with the reasons and the substance for the changes,
reasons for, and substance of, the changes

[Daniel van Vugt (vanvugt)]

Looks good to me. Tests are unchanged and coverage should slightly increase with this.

If we need to increase abstraction again later for more mocking, then that's an exercise for later.

review: Approve

[Daniel van Vugt (vanvugt)]

There is a possible argument that "Protobuf" prefixes are appropriate, should anyone want to add support for other protocols later. Though I'm not entirely sure the existing design is sufficient to support a clean replacement of the protocol layer like that. So simpler is better for now.

[Alan Griffiths (alan-griffiths)]

I'm happy with the motivation. Not so happy with some aspects of the proposed changes.

Dependencies should always be on interfaces, not concrete classes - so I think it wrong to conflate MessageProcessor and ProtobufMessageProcessor.

We've a rule in the codebase that we don't have null dependencies (and use null objects instead). I don't think we should be making an exception here just to avoid having a trivial NullMessageProcessor class.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

It's not a rule; it's a convention. And any convention which results in trivial un(der)used classes should probably be changed, or at least bent. As I said above; "If we need to increase abstraction again later for more mocking, then that's an exercise for later".

I feel that Mir has a growing problem with its code complexity out-pacing functionality. That's a particular concern for our ability to maintain and enhance the code. So any successful attempt to simplify things should surely be welcomed.

[Alexandros Frantzis (afrantzis)]

> There is a possible argument that "Protobuf" prefixes are appropriate, should anyone want to add support for other protocols later

It's also about descriptive naming. ProtobufSocketCommunicator is a communicator that uses protobuf over sockets, all the important information is in the name.

> The only part that I don't like is that we're getting rid of MessageProcessor's interface class, will make it harder to mock.

> Dependencies should always be on interfaces, not concrete classes - so I think it wrong to conflate MessageProcessor and ProtobufMessageProcessor.

The fact that we can make MessageProcessor concrete without any consequences, tells me that we are missing some tests where the interface is used (in SocketSession, e.g. message_processed_when_new_message_arrives), not that we don't need to have it as an interface.

> > We've a rule in the codebase that we don't have null dependencies (and use null objects instead).
> It's not a rule; it's a convention. And any convention which results in trivial un(der)used classes should probably be changed, or at least bent.

However you call it, the point is that there are reasons for it, we didn't just flip a coin and went with NullObjects. If we turn this around: "any convention which results in the circulation of null smart pointers should probably be changed, or at least bent." :) As with any engineering decision, there are trade-offs involved, and the decision was that NullObject trade-offs are worth it.

review: Needs Fixing

[Robert Ancell (robert-ancell)]

> We've a rule in the codebase that we don't have null dependencies (and use
> null objects instead). I don't think we should be making an exception here
> just to avoid having a trivial NullMessageProcessor class.

I wasn't aware that is the case, I'll work on fixing that. Can you please add this information to the style guide?

[Robert Ancell (robert-ancell)]

My motivation for this is gone. I will pick it up later if it returns :)

=== modified file 'src/server/frontend/CMakeLists.txt'
--- src/server/frontend/CMakeLists.txt	2013-03-28 09:50:09 +0000
+++ src/server/frontend/CMakeLists.txt	2013-04-01 21:52:22 +0000
@@ -4,8 +4,7 @@
   session_mediator.cpp
   null_session_mediator_report.cpp
   null_message_processor_report.cpp
-  protobuf_message_processor.cpp
-  null_message_processor.cpp
+  message_processor.cpp
   surface_creation_parameters.cpp
 
   resource_cache.cpp
@@ -13,8 +12,8 @@
 )
 
 list(APPEND FRONTEND_SOURCES
-  protobuf_socket_communicator.cpp
-  make_protobuf_socket_communicator.cpp
+  socket_communicator.cpp
+  make_socket_communicator.cpp
   socket_session.cpp
 )
 

=== renamed file 'src/server/frontend/make_protobuf_socket_communicator.cpp' => 'src/server/frontend/make_socket_communicator.cpp'
--- src/server/frontend/make_protobuf_socket_communicator.cpp	2013-03-21 03:32:59 +0000
+++ src/server/frontend/make_socket_communicator.cpp	2013-04-01 21:52:22 +0000
@@ -18,7 +18,7 @@
 
 #include "mir/default_server_configuration.h"
 #include "mir/options/option.h"
-#include "protobuf_socket_communicator.h"
+#include "socket_communicator.h"
 
 namespace mf = mir::frontend;
 namespace mg = mir::graphics;
@@ -31,7 +31,7 @@
         [&,this]() -> std::shared_ptr<mf::Communicator>
         {
             auto const threads = the_options()->get("ipc-thread-pool", 10);
-            return std::make_shared<mf::ProtobufSocketCommunicator>(
+            return std::make_shared<mf::SocketCommunicator>(
                 the_socket_file(),
                 the_ipc_factory(the_frontend_shell(), the_viewable_area(), the_buffer_allocator()),
                 threads);

=== renamed file 'src/server/frontend/protobuf_message_processor.cpp' => 'src/server/frontend/message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2013-03-22 03:33:00 +0000
+++ src/server/frontend/message_processor.cpp	2013-04-01 21:52:22 +0000
@@ -16,7 +16,7 @@
  * Authored by: Alan Griffiths <[e-mail]>
  */
 
-#include "protobuf_message_processor.h"
+#include "message_processor.h"
 #include "mir/frontend/message_processor_report.h"
 #include "mir/frontend/resource_cache.h"
 
@@ -26,7 +26,7 @@
 
 namespace mfd = mir::frontend::detail;
 
-mfd::ProtobufMessageProcessor::ProtobufMessageProcessor(
+mfd::MessageProcessor::MessageProcessor(
     MessageSender* sender,
     std::shared_ptr<protobuf::DisplayServer> const& display_server,
     std::shared_ptr<ResourceCache> const& resource_cache,
@@ -39,28 +39,28 @@
 }
 
 template<class ResultMessage>
-void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, ResultMessage* response)
-{
-    send_response(id, static_cast<google::protobuf::Message*>(response));
-}
-
-void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Buffer* response)
-{
-    const auto& fd = extract_fds_from(response);
-    send_response(id, static_cast<google::protobuf::Message*>(response));
-    sender->send_fds(fd);
-    resource_cache->free_resource(response);
-}
-
-void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Platform* response)
-{
-    const auto& fd = extract_fds_from(response);
-    send_response(id, static_cast<google::protobuf::Message*>(response));
-    sender->send_fds(fd);
-    resource_cache->free_resource(response);
-}
-
-void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Connection* response)
+void mfd::MessageProcessor::send_response(::google::protobuf::uint32 id, ResultMessage* response)
+{
+    send_response(id, static_cast<google::protobuf::Message*>(response));
+}
+
+void mfd::MessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Buffer* response)
+{
+    const auto& fd = extract_fds_from(response);
+    send_response(id, static_cast<google::protobuf::Message*>(response));
+    sender->send_fds(fd);
+    resource_cache->free_resource(response);
+}
+
+void mfd::MessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Platform* response)
+{
+    const auto& fd = extract_fds_from(response);
+    send_response(id, static_cast<google::protobuf::Message*>(response));
+    sender->send_fds(fd);
+    resource_cache->free_resource(response);
+}
+
+void mfd::MessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Connection* response)
 {
     const auto& fd = response->has_platform() ?
         extract_fds_from(response->mutable_platform()) :
@@ -71,7 +71,7 @@
     resource_cache->free_resource(response);
 }
 
-void mfd::ProtobufMessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Surface* response)
+void mfd::MessageProcessor::send_response(::google::protobuf::uint32 id, mir::protobuf::Surface* response)
 {
     const auto& fd = response->has_buffer() ?
         extract_fds_from(response->mutable_buffer()) :
@@ -83,7 +83,7 @@
 }
 
 template<class Response>
-std::vector<int32_t> mfd::ProtobufMessageProcessor::extract_fds_from(Response* response)
+std::vector<int32_t> mfd::MessageProcessor::extract_fds_from(Response* response)
 {
     std::vector<int32_t> fd(response->fd().data(), response->fd().data() + response->fd().size());
     response->clear_fd();
@@ -92,7 +92,7 @@
 }
 
 template<class ParameterMessage, class ResultMessage>
-void mfd::ProtobufMessageProcessor::invoke(
+void mfd::MessageProcessor::invoke(
     void (protobuf::DisplayServer::*function)(
         ::google::protobuf::RpcController* controller,
         const ParameterMessage* request,
@@ -108,7 +108,7 @@
     {
         std::unique_ptr<google::protobuf::Closure> callback(
             google::protobuf::NewPermanentCallback(this,
-                &ProtobufMessageProcessor::send_response,
+                &MessageProcessor::send_response,
                 invocation.id(),
                 &result_message));
 
@@ -125,7 +125,7 @@
     }
 }
 
-void mfd::ProtobufMessageProcessor::send_response(
+void mfd::MessageProcessor::send_response(
     ::google::protobuf::uint32 id,
     google::protobuf::Message* response)
 {
@@ -142,7 +142,7 @@
     sender->send(buffer2);
 }
 
-bool mfd::ProtobufMessageProcessor::dispatch(mir::protobuf::wire::Invocation const& invocation)
+bool mfd::MessageProcessor::dispatch(mir::protobuf::wire::Invocation const& invocation)
 {
     report->received_invocation(display_server.get(), invocation.id(), invocation.method_name());
 
@@ -207,7 +207,7 @@
 }
 
 
-bool mfd::ProtobufMessageProcessor::process_message(std::istream& msg)
+bool mfd::MessageProcessor::process_message(std::istream& msg)
 {
     try
     {

=== renamed file 'src/server/frontend/protobuf_message_processor.h' => 'src/server/frontend/message_processor.h'
--- src/server/frontend/protobuf_message_processor.h	2013-03-22 03:33:00 +0000
+++ src/server/frontend/message_processor.h	2013-04-01 21:52:22 +0000
@@ -17,10 +17,10 @@
  */
 
 
-#ifndef MIR_FRONTEND_PROTOBUF_MESSAGE_PROCESSOR_H_
-#define MIR_FRONTEND_PROTOBUF_MESSAGE_PROCESSOR_H_
+#ifndef MIR_FRONTEND_MESSAGE_PROCESSOR_H_
+#define MIR_FRONTEND_MESSAGE_PROCESSOR_H_
 
-#include "message_processor.h"
+#include "message_sender.h"
 
 #include "mir_protobuf.pb.h"
 #include "mir_protobuf_wire.pb.h"
@@ -42,14 +42,16 @@
 namespace detail
 {
 
-struct ProtobufMessageProcessor : MessageProcessor
+struct MessageProcessor
 {
-    ProtobufMessageProcessor(
+    MessageProcessor(
         MessageSender* sender,
         std::shared_ptr<protobuf::DisplayServer> const& display_server,
         std::shared_ptr<ResourceCache> const& resource_cache,
         std::shared_ptr<MessageProcessorReport> const& report);
 
+    bool process_message(std::istream& msg);
+
 private:
     void send_response(::google::protobuf::uint32 id, google::protobuf::Message* response);
 
@@ -75,8 +77,6 @@
     template<class Response>
     std::vector<int32_t> extract_fds_from(Response* response);
 
-    bool process_message(std::istream& msg);
-
     bool dispatch(mir::protobuf::wire::Invocation const& invocation);
 
     template<class ParameterMessage, class ResultMessage>
@@ -97,4 +97,4 @@
 }
 }
 
-#endif /* PROTOBUF_MESSAGE_PROCESSOR_H_ */
+#endif /* MIR_FRONTEND_MESSAGE_PROCESSOR_H_ */

=== renamed file 'src/server/frontend/message_processor.h' => 'src/server/frontend/message_sender.h'
--- src/server/frontend/message_processor.h	2013-03-13 04:54:15 +0000
+++ src/server/frontend/message_sender.h	2013-04-01 21:52:22 +0000
@@ -17,8 +17,8 @@
  */
 
 
-#ifndef MIR_FRONTEND_MESSAGE_PROCESSOR_H_
-#define MIR_FRONTEND_MESSAGE_PROCESSOR_H_
+#ifndef MIR_FRONTEND_MESSAGE_SENDER_H_
+#define MIR_FRONTEND_MESSAGE_SENDER_H_
 
 #include <vector>
 #include <memory>
@@ -41,26 +41,10 @@
     MessageSender(MessageSender const&) = delete;
     MessageSender& operator=(MessageSender const&) = delete;
 };
-
-struct MessageProcessor
-{
-    virtual bool process_message(std::istream& msg) = 0;
-protected:
-    MessageProcessor() = default;
-    ~MessageProcessor() = default;
-    MessageProcessor(MessageProcessor const&) = delete;
-    MessageProcessor& operator=(MessageProcessor const&) = delete;
-};
-
-struct NullMessageProcessor : MessageProcessor
-{
-    bool process_message(std::istream&);
-};
-
-}
-}
-}
-
-
-
-#endif /* PROTOBUF_MESSAGE_PROCESSOR_H_ */
+}
+}
+}
+
+
+
+#endif /* MIR_FRONTEND_MESSAGE_SENDER_H_ */

=== removed file 'src/server/frontend/null_message_processor.cpp'
--- src/server/frontend/null_message_processor.cpp	2013-03-13 04:54:15 +0000
+++ src/server/frontend/null_message_processor.cpp	1970-01-01 00:00:00 +0000
@@ -1,26 +0,0 @@
-/*
- * Copyright © 2012 Canonical Ltd.
- *
- * This program is free software: you can redistribute it and/or modify it
- * under the terms of the GNU Lesser General Public License version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Alan Griffiths <[e-mail]>
- */
-
-#include "message_processor.h"
-
-namespace mfd = mir::frontend::detail;
-
-bool mfd::NullMessageProcessor::process_message(std::istream& )
-{
-    return false;
-}

=== renamed file 'src/server/frontend/protobuf_socket_communicator.cpp' => 'src/server/frontend/socket_communicator.cpp'
--- src/server/frontend/protobuf_socket_communicator.cpp	2013-03-22 03:33:00 +0000
+++ src/server/frontend/socket_communicator.cpp	2013-04-01 21:52:22 +0000
@@ -16,8 +16,8 @@
  * Authored by: Thomas Guest <[e-mail]>
  */
 
-#include "protobuf_socket_communicator.h"
-#include "protobuf_message_processor.h"
+#include "socket_communicator.h"
+#include "message_processor.h"
 #include "socket_session.h"
 
 #include "mir/frontend/protobuf_ipc_factory.h"
@@ -31,7 +31,7 @@
 namespace ba = boost::asio;
 
 
-mf::ProtobufSocketCommunicator::ProtobufSocketCommunicator(
+mf::SocketCommunicator::SocketCommunicator(
     std::string const& socket_file,
     std::shared_ptr<ProtobufIpcFactory> const& ipc_factory,
     int threads)
@@ -45,14 +45,14 @@
     start_accept();
 }
 
-void mf::ProtobufSocketCommunicator::start_accept()
+void mf::SocketCommunicator::start_accept()
 {
     auto const& socket_session = std::make_shared<mfd::SocketSession>(
         io_service,
         next_id(),
         connected_sessions);
 
-    auto session = std::make_shared<detail::ProtobufMessageProcessor>(
+    auto session = std::make_shared<detail::MessageProcessor>(
         socket_session.get(),
         ipc_factory->make_ipc_server(),
         ipc_factory->resource_cache(),
@@ -63,13 +63,13 @@
     acceptor.async_accept(
         socket_session->get_socket(),
         boost::bind(
-            &ProtobufSocketCommunicator::on_new_connection,
+            &SocketCommunicator::on_new_connection,
             this,
             socket_session,
             ba::placeholders::error));
 }
 
-int mf::ProtobufSocketCommunicator::next_id()
+int mf::SocketCommunicator::next_id()
 {
     int id = next_session_id.load();
     while (!next_session_id.compare_exchange_weak(id, id + 1)) std::this_thread::yield();
@@ -77,7 +77,7 @@
 }
 
 
-void mf::ProtobufSocketCommunicator::start()
+void mf::SocketCommunicator::start()
 {
     auto run_io_service = boost::bind(&ba::io_service::run, &io_service);
 
@@ -87,7 +87,7 @@
     }
 }
 
-mf::ProtobufSocketCommunicator::~ProtobufSocketCommunicator()
+mf::SocketCommunicator::~SocketCommunicator()
 {
     io_service.stop();
 
@@ -104,7 +104,7 @@
     std::remove(socket_file.c_str());
 }
 
-void mf::ProtobufSocketCommunicator::on_new_connection(
+void mf::SocketCommunicator::on_new_connection(
     std::shared_ptr<mfd::SocketSession> const& session,
     const boost::system::error_code& ec)
 {

=== renamed file 'src/server/frontend/protobuf_socket_communicator.h' => 'src/server/frontend/socket_communicator.h'
--- src/server/frontend/protobuf_socket_communicator.h	2013-03-21 03:32:59 +0000
+++ src/server/frontend/socket_communicator.h	2013-04-01 21:52:22 +0000
@@ -16,8 +16,8 @@
  * Authored by: Thomas Guest <[e-mail]>
  */
 
-#ifndef MIR_FRONTEND_PROTOBUF_ASIO_COMMUNICATOR_H_
-#define MIR_FRONTEND_PROTOBUF_ASIO_COMMUNICATOR_H_
+#ifndef MIR_FRONTEND_SOCKET_COMMUNICATOR_H_
+#define MIR_FRONTEND_SOCKET_COMMUNICATOR_H_
 
 #include "connected_sessions.h"
 
@@ -51,16 +51,16 @@
 struct SocketSession;
 }
 
-class ProtobufSocketCommunicator : public Communicator
+class SocketCommunicator : public Communicator
 {
 public:
     // Create communicator based on Boost asio and Google protobufs
     // using the supplied socket.
-    explicit ProtobufSocketCommunicator(
+    explicit SocketCommunicator(
         const std::string& socket_file,
         std::shared_ptr<ProtobufIpcFactory> const& ipc_factory,
         int threads);
-    ~ProtobufSocketCommunicator();
+    ~SocketCommunicator();
     void start();
 
 private:
@@ -80,4 +80,4 @@
 }
 }
 
-#endif // MIR_FRONTEND_PROTOBUF_ASIO_COMMUNICATOR_H_
+#endif // MIR_FRONTEND_SOCKET_COMMUNICATOR_H_

=== modified file 'src/server/frontend/socket_session.cpp'
--- src/server/frontend/socket_session.cpp	2013-03-22 15:11:46 +0000
+++ src/server/frontend/socket_session.cpp	2013-04-01 21:52:22 +0000
@@ -89,7 +89,7 @@
 
     std::istream msg(&message);
 
-    if (processor->process_message(msg))
+    if (!processor || processor->process_message(msg))
     {
         read_next_message();
     }

=== modified file 'src/server/frontend/socket_session.h'
--- src/server/frontend/socket_session.h	2013-03-13 04:54:15 +0000
+++ src/server/frontend/socket_session.h	2013-04-01 21:52:22 +0000
@@ -40,8 +40,7 @@
         std::shared_ptr<ConnectedSessions<SocketSession>> const& connected_sessions) :
         socket(io_service),
         id_(id_),
-        connected_sessions(connected_sessions),
-        processor(std::make_shared<NullMessageProcessor>()) {}
+        connected_sessions(connected_sessions) {}
 
     int id() const { return id_; }
 

=== modified file 'tests/mir_test_doubles/test_protobuf_socket_server.cpp'
--- tests/mir_test_doubles/test_protobuf_socket_server.cpp	2013-03-13 08:09:52 +0000
+++ tests/mir_test_doubles/test_protobuf_socket_server.cpp	2013-04-01 21:52:22 +0000
@@ -17,7 +17,7 @@
  */
 
 #include "mir_test/test_protobuf_server.h"
-#include "src/server/frontend/protobuf_socket_communicator.h"
+#include "src/server/frontend/socket_communicator.h"
 
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
@@ -34,5 +34,5 @@
 
 std::shared_ptr<mf::Communicator> mt::TestProtobufServer::make_communicator(const std::string& socket_name, std::shared_ptr<frontend::ProtobufIpcFactory> const& factory)
 {
-    return std::make_shared<mf::ProtobufSocketCommunicator>(socket_name, factory, 10);
+    return std::make_shared<mf::SocketCommunicator>(socket_name, factory, 10);
 }