Blimp: define HeliumTransport/HeliumStream interfaces.

blimp/net/helium/helium_stream.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BLIMP_NET_HELIUM_HELIUM_STREAM_H_
+#define BLIMP_NET_HELIUM_HELIUM_STREAM_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "blimp/net/helium/helium_result.h"
+
+namespace blimp {
+
+class HeliumMessage;
+
+// Abstract base class for HeliumMessage-oriented transport streams.
+// Details about how the HeliumStream is bound to the network layer are handled
+// by subclasses of HeliumStream.
+class HeliumStream {

[Wez, 2016/10/08 01:02:29]

Can we break this into a interface and a separate base-class, plz?

[Kevin M, 2016/10/11 20:02:37]

Done.

+ public:
+  class Delegate {
+   public:
+    // Invoked when the HeliumStream is disconnected.
+    //
+    // An observer which owns |this| may delete the HeliumStream in response to
+    // receiving an OnStreamDisconnected() notification. The deletion method
+    // must be asynchronously posted to occur *after* the OnStreamDisconnected()
+    // handler, not during.

[Wez, 2016/10/08 01:02:29]

nit: Could we arrange things to make this safe?

If we switched to a traditional ReceiveMessage() interface instead of a Delegate
then we could signal disconnects by having that return an EOF value. The
Delegate approach seems more complex by comparison.

[Kevin M, 2016/10/11 20:02:37]

I've been getting a lot of mixed signals from a lot of parties about using a
delegate vs. a ReceiveMessage callback. Matt and Garrett are in favor of the
delegate approach. I'm rather on the fence. When Matt pointed out that we should
slurp data from the pipe as fast as possible as a matter of policy so that we
don't create any pushback signals, I thought that my big argument for going with
a callback was rendered moot.

My inclination is to leave it as-is since IMO the matter of additional
complexity is relative/debatable...

[Garrett Casto, 2016/10/11 20:27:03]

To clarify my stance, I was originally for the delegate but after talking with
both Kevin and Matt, I basically think that it's a toss up. Flow control doesn't
seem like a motivating factor and the overall complexity seems about the same in
both cases. Somewhere there is a loop that is going to buffer data since we
don't want to apply multiple changesets at the same time (assuming that in some
cases changeset application is async).

At this point it feels like we should implement something and we can figure
refine the interface as necessary.

As is, it seems like there are two issues though:
- OnMessageReceived() doesn't identify the stream in any way, which seems like
something that the Sync layer would want. The original design for this used a
callback so that the Sync layer could choose whatever identifier it wanted for
each Stream.
- I think that you would want to specify a HeliumResult in
OnStreamDisconnected() (or as another parameter to OnMessageRecieved()) since
there may be different reasons for disconnection and the caller may react to
them differently.

+    virtual void OnStreamDisconnected(HeliumStream* stream) = 0;
+
+    // Called when a message is read from the HeliumStream.
+    virtual void OnMessageReceived(std::unique_ptr<HeliumMessage> message) = 0;
+  };
+
+  HeliumStream();
+  virtual ~HeliumStream();
+
+  // Sends |helium_message| over the Stream. |callback| is invoked when the
+  // message is sent (or otherwise moved to the low-level write buffers),
+  // which signals the caller that it is clear to send another message.
+  //
+  // The caller is responsible for ensuring that only one outstanding
+  // SendMessage() call is made at a time.
+  virtual void SendMessage(
+      std::unique_ptr<HeliumMessage> helium_message,
+      const base::Callback<void(HeliumResult)>& callback) = 0;
+
+  void SetDelegate(std::unique_ptr<Delegate> delegate);
+
+ protected:
+  Delegate* delegate() { return delegate_.get(); }
+
+ private:
+  std::unique_ptr<Delegate> delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(HeliumStream);
+};
+
+}  // namespace blimp
+
+#endif  // BLIMP_NET_HELIUM_HELIUM_STREAM_H_