[Libreoffice-commits] online.git: 2 commits - loolwsd/LOOLProtocol.hpp loolwsd/test

[Tor Lillqvist]

 loolwsd/LOOLProtocol.hpp |    6 +++++-
 loolwsd/test/helpers.hpp |   16 ++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

New commits:
commit 53b718844fd2c0b6366055cf6fc4d018cf6c339b
Author: Tor Lillqvist <tml at collabora.com>
Date:   Tue Oct 18 16:23:33 2016 +0300

    Add a FIXME for some sillyness
    
    One thing works mainly by accident, another is inefficient.

diff --git a/loolwsd/test/helpers.hpp b/loolwsd/test/helpers.hpp
index 9e3756e..ab2b1ba 100644
--- a/loolwsd/test/helpers.hpp
+++ b/loolwsd/test/helpers.hpp
@@ -188,6 +188,22 @@ std::vector<char> getResponseMessage(Poco::Net::WebSocket& ws, const std::string
                 auto message = LOOLProtocol::getAbbreviatedMessage(response);
                 if (bytes > 0 && (flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) != Poco::Net::WebSocket::FRAME_OP_CLOSE)
                 {
+                    // FIXME: This is wrong in two ways:
+
+                    // 1) The message variable is the return value from getAbbreviatedMessage(),
+                    // That is a string that is intended for human-readable logging only. It is not
+                    // intended to be used for actually decoding the protocol. Sure, at the moment
+                    // it happens to work, but is still wrong.
+
+                    // 2) Using std::string::find() like this is silly. If message does not start
+                    // with prefix, the find() function will search through the whole buffer. That
+                    // is a waste of cycles. Use the LOOLProtocol functions to manipulate and
+                    // inspect LOOL protocol frames, that is what they are there
+                    // for. getFirstToken() should be quite efficient, and it doesn't incur the
+                    // (potential) overhead of setting up a StringTokenizer. Or, if this is actually
+                    // used to look for not just a first token, then introduce a startsWith()
+                    // function.
+
                     if (message.find(prefix) == 0)
                     {
                         std::cerr << name << "Got " << bytes << " bytes: " << message << std::endl;
commit b0c870cbeb77b69c5bf1046646b5253b411a546a
Author: Tor Lillqvist <tml at collabora.com>
Date:   Tue Oct 18 15:51:29 2016 +0300

    Expand documentation for getAbbreviatedMessage()
    
    It has always been the intent that getAbbreviatedMessage() is for
    producing human-readbale nice output for logging purposes only. But
    this has not been mentioned, so (naturally) the function has been
    misused. Sigh.

diff --git a/loolwsd/LOOLProtocol.hpp b/loolwsd/LOOLProtocol.hpp
index 6120e0c..3857a4e 100644
--- a/loolwsd/LOOLProtocol.hpp
+++ b/loolwsd/LOOLProtocol.hpp
@@ -133,7 +133,11 @@ namespace LOOLProtocol
         return getFirstLine(message.data(), message.size());
     }
 
-    /// Returns an abbereviation of the message (the first line, indicating truncation).
+    /// Returns an abbereviation of the message (the first line, indicating truncation). We assume
+    /// that it adhers to the LOOL protocol, i.e. that there is always a first (or only) line that
+    /// is in printable UTF-8. I.e. no encoding of binary bytes is done. The format of the result is
+    /// not guaranteed to be stable. It is to be used for logging purposes only, not for decoding
+    /// protocol frames.
     inline
     std::string getAbbreviatedMessage(const char *message, const int length)
     {