Blimp: Add BlobChannelSender implementation

blimp/net/blob_channel/blob_channel_sender.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_BLOB_CHANNEL_BLOB_CHANNEL_SENDER_H_
+#define BLIMP_NET_BLOB_CHANNEL_BLOB_CHANNEL_SENDER_H_
+
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"

[Wez, 2016/04/15 17:15:16]

You don't seem to use WeakPtr.

[Kevin M, 2016/04/15 22:42:57]

Done.

+#include "base/observer_list.h"

[Wez, 2016/04/15 17:15:17]

Nor observer lists.

[Kevin M, 2016/04/15 22:42:57]

Done.

+#include "base/threading/thread_checker.h"
+#include "blimp/net/blimp_message_processor.h"

[Wez, 2016/04/15 17:15:16]

Nor BlimpMessageProcessor.

[Kevin M, 2016/04/15 22:42:57]

Done.

+#include "blimp/net/blimp_net_export.h"
+
+namespace blimp {
+
+class BlobCache;
+class BlobSenderBindings;
+
+class BLIMP_NET_EXPORT BlobChannelSender {

[nyquist, 2016/04/15 17:12:52]

Should there be a short comment explaining this class?

[Kevin M, 2016/04/15 22:42:57]

Done.

+ public:
+  BlobChannelSender(BlobCache* cache, BlobSenderBindings* bindings);

[Wez, 2016/04/15 17:15:16]

Ownership of bare-pointer parameters needs to be clear:
- Who owns the BlobCache?
- Who owns the BlobSenderBindings?

Naming nits:
- Why is this BlobSenderBindings rather than BlobSender::Delegate, for example?
- Why is this class BlobChannelSender but the delegate BlobSender...?

[Kevin M, 2016/04/15 22:42:57]

Clarified ownership (or lack thereof.)

Addressed Binding naming nit, is now "Delegate".

+  ~BlobChannelSender();
+
+  // Stores a blob inside the BlobCache.

[Wez, 2016/04/15 17:15:16]

That's an implementation detail, surely?  Conceptually this ensures that the
peer receiver will receive blob |id| eventually, while Push forces the blob to
be transmitted in its entirety before returning?

If you're not going to have the BlobCache and channel sender/receiver inherent
from a pair of putter/getter interfaces then we should give these methods more
descriptive names, e.g. QueueBlob and FlushBlob.

[Kevin M, 2016/04/15 22:42:57]

It puts the blob into the channel, where it can be delivered either by a
locally-originating push, or a remote-originating pull.

+  // Must be called on the main thread.

[Wez, 2016/04/15 17:15:16]

There is no "main thread", nor any concept of threads at all, as far as I can
tell, in this class.

[Kevin M, 2016/04/15 22:42:57]

Done.

+  void Put(const std::string& id, std::unique_ptr<std::vector<uint8_t>> data);
+
+  // Sends the blob |id| to the remote side.
+  // Must be called on the main thread.
+  void Push(const std::string& id);
+
+ private:
+  BlobCache* cache_;
+  BlobSenderBindings* bindings_;
+
+  // A listing of the cache items in the receiver's cache.
+  // Used for suppressing redundant data transfers.

[Wez, 2016/04/15 17:15:16]

This tells me what it's for, but not what it actually contains, nor why it is
distinct from the BlobCache itself; is it e.g. a set of Ids of items that are in
the cache?

[Kevin M, 2016/04/15 22:42:57]

Done.

+  // TODO(kmarshall): Replace with a Bloom filter.

[Wez, 2016/04/15 17:15:16]

nit: Remove this TODO; we will _probably_ want to optimize this but there's more
to it than just "replacing with a Bloom filter".

[Kevin M, 2016/04/15 22:42:57]

Done.

+  std::set<std::string> client_cache_state_;

[Wez, 2016/04/15 17:15:16]

This name doesn't help me understand what this set actually contains, nor why it
is distinct from |cache_| above.  Should it be called cache_index_ or
cache_contents_, for example?

[Kevin M, 2016/04/15 22:42:57]

Done. "receiver_cache_contents_"

+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlobChannelSender);
+};
+
+}  // namespace blimp
+
+#endif  // BLIMP_NET_BLOB_CHANNEL_BLOB_CHANNEL_SENDER_H_