Blimp: Add BlobChannel Helium messages and delegate impls.

Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving implementation to BlobChannelReceiverImpl.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719
Committed: https://crrev.com/9dced9059d886d9fecdd3316db84ac9a931d3cc7
Cr-Commit-Position: refs/heads/master@{#396271}

[Kevin M, 2016-05-10 23:07:06]

Description was changed from

==========
Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable
following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving
implementation to BlobChannelReceiverImpl.
* Add new BlobChannelMessage.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719
==========

to

==========
Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable
following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving
implementation to BlobChannelReceiverImpl.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719
==========

[haibinlu, 2016-05-12 18:20:10]

https://codereview.chromium.org/1970463004/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message.cc (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message.cc:98: std::unique_ptr<BlimpMessage>
CreateBlimpMessage(
add a unit test case?

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
File blimp/net/blob_channel/blob_channel_receiver.cc (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_receiver.cc:27: BlobDataPtr data) {
DCHECK not needed?

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_receiver.cc:42: DCHECK(delegate_);
ditto

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_receiver.h:53: class BLIMP_NET_EXPORT
BlobChannelReceiverImpl : public BlobChannelReceiver {
shall this be in blob_channel_receiver_impl.h?

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
File blimp/net/blob_channel/helium_blob_channel_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:45: class
HeliumBlobChannelTest : public testing::Test {
why prefix it with "helium"?

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:55:
HeliumBlobReceiverDelegate receiver_delegate_;
Can this be a unique_ptr so that  BlimpMessageProxy can be removed?

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
File blimp/net/blob_channel/helium_blob_receiver_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:16: // Receives and
processes incoming blob messages in Helium format.
comment about "Helium format"?

[Kevin M, 2016-05-13 18:08:40]

kmarshall@chromium.org changed reviewers:
+ wez@chromium.org
- haibinlu@chromium.org

[Kevin M, 2016-05-13 18:08:40]

kmarshall@chromium.org changed required reviewers:
+ wez@chromium.org

[Kevin M, 2016-05-13 18:08:50]

+R wez@ (no longer OOO)

[Kevin M, 2016-05-13 18:40:22]

kmarshall@chromium.org changed reviewers:
+ haibinlu@chromium.org

[Kevin M, 2016-05-13 18:40:24]

https://codereview.chromium.org/1970463004/diff/1/blimp/common/create_blimp_m...
File blimp/common/create_blimp_message.cc (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/common/create_blimp_m...
blimp/common/create_blimp_message.cc:98: std::unique_ptr<BlimpMessage>
CreateBlimpMessage(
On 2016/05/12 18:20:09, haibinlu wrote:
> add a unit test case?

Done.

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
File blimp/net/blob_channel/blob_channel_receiver.cc (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_receiver.cc:27: BlobDataPtr data) {
On 2016/05/12 18:20:09, haibinlu wrote:
> DCHECK not needed?

Done.

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_receiver.cc:42: DCHECK(delegate_);
On 2016/05/12 18:20:09, haibinlu wrote:
> ditto

Done.

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/blob...
blimp/net/blob_channel/blob_channel_receiver.h:53: class BLIMP_NET_EXPORT
BlobChannelReceiverImpl : public BlobChannelReceiver {
On 2016/05/12 18:20:09, haibinlu wrote:
> shall this be in blob_channel_receiver_impl.h?

IMO it's not necessary - in practice, there will only be one non-test impl
class. The interface separation is there only for the benefit of test code.

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
File blimp/net/blob_channel/helium_blob_channel_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:45: class
HeliumBlobChannelTest : public testing::Test {
On 2016/05/12 18:20:09, haibinlu wrote:
> why prefix it with "helium"?

These tests are Helium-specific; they're not general BlobChannel tests. Since
test suite names occupy the same namespace, a prefix seems necessary.

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:55:
HeliumBlobReceiverDelegate receiver_delegate_;
On 2016/05/12 18:20:09, haibinlu wrote:
> Can this be a unique_ptr so that  BlimpMessageProxy can be removed?

The receiver takes delegates as unique_ptrs, so those delegates can't also be
owned here. The proxy allows the receiver to own a Delegate object, with the
underlying delegate impl living elsewhere.

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
File blimp/net/blob_channel/helium_blob_receiver_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/1/blimp/net/blob_channel/heli...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:16: // Receives and
processes incoming blob messages in Helium format.
On 2016/05/12 18:20:09, haibinlu wrote:
> comment about "Helium format"?

Renamed to BlimpMessage.

[haibinlu, 2016-05-13 21:06:17]

lgtm

[Kevin M, 2016-05-16 18:00:45]

kmarshall@chromium.org changed required reviewers:
- wez@chromium.org

[Kevin M, 2016-05-16 22:36:06]

wez: friendly ping!

[Kevin M, 2016-05-20 18:32:56]

pingz

[Wez, 2016-05-20 21:46:18]

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/blob_cache...
File blimp/common/blob_cache/blob_cache.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:18: using BlobDataPtr = scoped_refptr<const
BlobData>;
This seems peculiar - I think it's worth an explanatory comment to clarify what
the intended usage of the BlobData and BlobDataPtr types is, here.

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/create_bli...
File blimp/common/create_blimp_message.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/create_bli...
blimp/common/create_blimp_message.h:71: std::unique_ptr<BlobChannelMessage>
blob_channel_message);
This differs from the pattern we're using for other messages; I'd prefer that we
follow the existing pattern and have a separate CL to change everything to this
style, if that's what we want.

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/create_bli...
File blimp/common/create_blimp_message_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/create_bli...
blimp/common/create_blimp_message_unittest.cc:99: EXPECT_NE(nullptr, message);
nit: EXPECT_TRUE(message) or even ASSERT_TRUE(message), since the other
expectations will crash if !message.

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/logging.cc...
blimp/common/logging.cc:256: LogFields* output) const override {
nit: Want to log the payload size as well?

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:21: optional bytes payload = 3;
nit: Want to use oneof here, and define a TransferBlob message type?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.cc (left):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.cc:38:
delegate_->SetReceiver(nullptr);
Did you mean to remove the SetReceiver(nullptr) call from the dtor?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:39: BlobChannelReceiver();
This is a pure interface, so no need for ctor to be declared.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:40: virtual
~BlobChannelReceiver();
Pure interface, so no-op virtual dtor can be declared in-line.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:52: // Receives blobs from a
remote sender.
And does what with them? :)

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:53: class BLIMP_NET_EXPORT
BlobChannelReceiverImpl : public BlobChannelReceiver {
Rather than declare this implementation in the header, why not add a static
Create() method to the Receiver interface, and move this into the cc?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_channel_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:26: // Proxies
ProcessMessage calls to an un-ownedBlimpMessageProcessor* raw
nit: Missing some whitespace?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:28: // the underlying
message processor object living elsewhere.
In other tests we handle this by allocating the object and storing a bare
pointer to it in the test, then passing it off to the necessary sub-component -
do we really need this proxy class?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:81: // Verifies that the
Receiver understands messages sent by the Sender.
Rather than having this be helium_blob_channel_unittest.cc, I'd split it into
helium_blob_sender_delegate_unittest.cc and a separate receiver one; then both
would use static test message data to verify that the sender sends correct data
and that the receiver can process data, and one or other of them could then
include the round-trip test as well.

Having unittest.cc files that don't match the name of the code files they're
testing makes spotting coverage gaps much harder!

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.cc:27: BlobChannelMessage*
blob_msg = message->mutable_blob_channel();
nit: Comment here to explain why you take a mutable pointer to the channel
message.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.cc:42: // during the call
to OnBlobReceived().
I think the point here is that we don't store it to a BlobDataPtr because that
marks the BlobData as const?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:8: #include <memory>
Is this just needed for unique_ptr?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:16: // Receives and
processes incoming blob messages in BlimpMessage format.
Not clear from this description what it does with them, besides read them - do
they disappear into the abyss, for example?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:27: const
net::CompletionCallback& callback) override;
Looks like you need to include net/completion_callback.h?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_sender_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:15: void
DoNothingCompletionCallback(int) {}
Is there no common no-op callback? e.g. what happens if you just pass
net::CompletionCallback()..?

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:24:
DCHECK(outgoing_processor_);
nit: You're going to deref that later on, so you don't really need the DCHECK.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:38: callback.Run(net::OK);
Don't return OK if it's not-implemented; if we find code starts calling it then
it'll generate log spam rather than crashing or disconnecting.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_sender_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.h:26: // Sets the message
processor which will process the output of |this|.
nit: "... processor to which blob messages will be sent."?

[Kevin M, 2016-05-23 20:48:08]

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/blob_cache...
File blimp/common/blob_cache/blob_cache.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/blob_cache...
blimp/common/blob_cache/blob_cache.h:18: using BlobDataPtr = scoped_refptr<const
BlobData>;
On 2016/05/20 21:46:17, Wez wrote:
> This seems peculiar - I think it's worth an explanatory comment to clarify
what
> the intended usage of the BlobData and BlobDataPtr types is, here.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/create_bli...
File blimp/common/create_blimp_message.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/create_bli...
blimp/common/create_blimp_message.h:71: std::unique_ptr<BlobChannelMessage>
blob_channel_message);
On 2016/05/20 21:46:17, Wez wrote:
> This differs from the pattern we're using for other messages; I'd prefer that
we
> follow the existing pattern and have a separate CL to change everything to
this
> style, if that's what we want.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/logging.cc...
blimp/common/logging.cc:256: LogFields* output) const override {
On 2016/05/20 21:46:17, Wez wrote:
> nit: Want to log the payload size as well?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:21: optional bytes payload = 3;
On 2016/05/20 21:46:17, Wez wrote:
> nit: Want to use oneof here, and define a TransferBlob message type?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.cc (left):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.cc:38:
delegate_->SetReceiver(nullptr);
On 2016/05/20 21:46:17, Wez wrote:
> Did you mean to remove the SetReceiver(nullptr) call from the dtor?

Nope, it was just unnecessary, since |delegate_| is owned by |this| and will be
destroyed immediately anyways.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:39: BlobChannelReceiver();
On 2016/05/20 21:46:17, Wez wrote:
> This is a pure interface, so no need for ctor to be declared.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:40: virtual
~BlobChannelReceiver();
On 2016/05/20 21:46:17, Wez wrote:
> Pure interface, so no-op virtual dtor can be declared in-line.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:52: // Receives blobs from a
remote sender.
On 2016/05/20 21:46:17, Wez wrote:
> And does what with them? :)

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:53: class BLIMP_NET_EXPORT
BlobChannelReceiverImpl : public BlobChannelReceiver {
On 2016/05/20 21:46:17, Wez wrote:
> Rather than declare this implementation in the header, why not add a static
> Create() method to the Receiver interface, and move this into the cc?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_channel_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:26: // Proxies
ProcessMessage calls to an un-ownedBlimpMessageProcessor* raw
On 2016/05/20 21:46:17, Wez wrote:
> nit: Missing some whitespace?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:28: // the underlying
message processor object living elsewhere.
On 2016/05/20 21:46:17, Wez wrote:
> In other tests we handle this by allocating the object and storing a bare
> pointer to it in the test, then passing it off to the necessary sub-component
-
> do we really need this proxy class?

Um, derp, don't know why I went down this path. Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:81: // Verifies that the
Receiver understands messages sent by the Sender.
On 2016/05/20 21:46:17, Wez wrote:
> Rather than having this be helium_blob_channel_unittest.cc, I'd split it into
> helium_blob_sender_delegate_unittest.cc and a separate receiver one; then both
> would use static test message data to verify that the sender sends correct
data
> and that the receiver can process data, and one or other of them could then
> include the round-trip test as well.
> 
> Having unittest.cc files that don't match the name of the code files they're
> testing makes spotting coverage gaps much harder!

Will do this in a followup patch.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.cc:27: BlobChannelMessage*
blob_msg = message->mutable_blob_channel();
On 2016/05/20 21:46:18, Wez wrote:
> nit: Comment here to explain why you take a mutable pointer to the channel
> message.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.cc:42: // during the call
to OnBlobReceived().
On 2016/05/20 21:46:18, Wez wrote:
> I think the point here is that we don't store it to a BlobDataPtr because that
> marks the BlobData as const?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:8: #include <memory>
On 2016/05/20 21:46:18, Wez wrote:
> Is this just needed for unique_ptr?

Yes.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:16: // Receives and
processes incoming blob messages in BlimpMessage format.
On 2016/05/20 21:46:18, Wez wrote:
> Not clear from this description what it does with them, besides read them - do
> they disappear into the abyss, for example?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:27: const
net::CompletionCallback& callback) override;
On 2016/05/20 21:46:18, Wez wrote:
> Looks like you need to include net/completion_callback.h?

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_sender_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:15: void
DoNothingCompletionCallback(int) {}
On 2016/05/20 21:46:18, Wez wrote:
> Is there no common no-op callback? e.g. what happens if you just pass
> net::CompletionCallback()..?

Can't find any no-op *completion callbacks*.

Running a net::CompletionCallback() results in a segmentation fault.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:24:
DCHECK(outgoing_processor_);
On 2016/05/20 21:46:18, Wez wrote:
> nit: You're going to deref that later on, so you don't really need the DCHECK.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:38: callback.Run(net::OK);
On 2016/05/20 21:46:18, Wez wrote:
> Don't return OK if it's not-implemented; if we find code starts calling it
then
> it'll generate log spam rather than crashing or disconnecting.

Done.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_sender_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.h:26: // Sets the message
processor which will process the output of |this|.
On 2016/05/20 21:46:18, Wez wrote:
> nit: "... processor to which blob messages will be sent."?

Done.

[Wez, 2016-05-24 01:18:40]

Looking good.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_channel_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_channel_unittest.cc:81: // Verifies that the
Receiver understands messages sent by the Sender.
On 2016/05/23 20:48:07, Kevin M wrote:
> On 2016/05/20 21:46:17, Wez wrote:
> > Rather than having this be helium_blob_channel_unittest.cc, I'd split it
into
> > helium_blob_sender_delegate_unittest.cc and a separate receiver one; then
both
> > would use static test message data to verify that the sender sends correct
> data
> > and that the receiver can process data, and one or other of them could then
> > include the round-trip test as well.
> > 
> > Having unittest.cc files that don't match the name of the code files they're
> > testing makes spotting coverage gaps much harder!
> 
> Will do this in a followup patch.

Acknowledged.

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_sender_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/20001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_sender_delegate.cc:15: void
DoNothingCompletionCallback(int) {}
On 2016/05/23 20:48:08, Kevin M wrote:
> On 2016/05/20 21:46:18, Wez wrote:
> > Is there no common no-op callback? e.g. what happens if you just pass
> > net::CompletionCallback()..?
> 
> Can't find any no-op *completion callbacks*.
> 
> Running a net::CompletionCallback() results in a segmentation fault.

*sadcodepanda*

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/logging.cc...
blimp/common/logging.cc:267: output);
break; explicitly here, don't just drop-through.

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/logging.cc...
blimp/common/logging.cc:268: default:  // unknown
Here and elsewhere(for follow-up CL), we should be using the ONEOF_FOO_NOT_SET
value rather than providing a default clause, so that when new cases are added
the compiler will catch the logger if it isn't updated to match.

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:12: optional string blob_id = 2;
nit: You don't think blob_id is likely to be common w/ other messages?

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:21: class Delegate {
Re-reviewing the HeliumBlobReceiverDelegate I was finding this Delegate
arrangement very strange.

The Delegate as you have it here is really a base implementation of a delegate,
and all that it actually does is accept the BlobChannelReceiver and store it, so
the subclass can call OnBlobReceived.

It would be cleaner to have this be a pure interface and let
HeliumBlobReceiverDelegate store the receiver and call it, I think.

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.cc:42: // The data is not
stored as a BlobDataPtr because its contents are const.
Confusing; I though the point was that we need its contents to be *non* const
right now?

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:18: // passes them to an
attached BlobChannelReceiver.
It's still not clear how the BlobChannelReceiver becomes attached; this comment
needs to explain that the caller must call
BlobChannelReceiver::Delegate::SetReceiver() on this object before handing it
messages to process.

[Kevin M, 2016-05-25 00:06:33]

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/logging.cc...
blimp/common/logging.cc:267: output);
On 2016/05/24 01:18:40, Wez wrote:
> break; explicitly here, don't just drop-through.

Done.

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/logging.cc...
blimp/common/logging.cc:268: default:  // unknown
On 2016/05/24 01:18:40, Wez wrote:
> Here and elsewhere(for follow-up CL), we should be using the ONEOF_FOO_NOT_SET
> value rather than providing a default clause, so that when new cases are added
> the compiler will catch the logger if it isn't updated to match.

Done, added todo to top of .cc.

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:12: optional string blob_id = 2;
On 2016/05/24 01:18:40, Wez wrote:
> nit: You don't think blob_id is likely to be common w/ other messages?

I was thinking I could make promote it provided that it makes sense for the use
case. For get, we might want to specify multiple IDs at once. Cache digest
upload would use multiple IDs or some kind of efficiently packed digest/Bloom
filter.

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:21: class Delegate {
On 2016/05/24 01:18:40, Wez wrote:
> Re-reviewing the HeliumBlobReceiverDelegate I was finding this Delegate
> arrangement very strange.
> 
> The Delegate as you have it here is really a base implementation of a
delegate,
> and all that it actually does is accept the BlobChannelReceiver and store it,
so
> the subclass can call OnBlobReceived.
> 
> It would be cleaner to have this be a pure interface and let
> HeliumBlobReceiverDelegate store the receiver and call it, I think.

Done.

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.cc (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.cc:42: // The data is not
stored as a BlobDataPtr because its contents are const.
On 2016/05/24 01:18:40, Wez wrote:
> Confusing; I though the point was that we need its contents to be *non* const
> right now?

I think the first two lines summarize what it is I'm trying to do. Removed the
last line because it's confusing.

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
File blimp/net/blob_channel/helium_blob_receiver_delegate.h (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/net/blob_channel/...
blimp/net/blob_channel/helium_blob_receiver_delegate.h:18: // passes them to an
attached BlobChannelReceiver.
On 2016/05/24 01:18:40, Wez wrote:
> It's still not clear how the BlobChannelReceiver becomes attached; this
comment
> needs to explain that the caller must call
> BlobChannelReceiver::Delegate::SetReceiver() on this object before handing it
> messages to process.

Done.

[Wez, 2016-05-25 02:47:44]

LGTM w/ nits.

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/40001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:12: optional string blob_id = 2;
On 2016/05/25 00:06:33, Kevin M wrote:
> On 2016/05/24 01:18:40, Wez wrote:
> > nit: You don't think blob_id is likely to be common w/ other messages?
> 
> I was thinking I could make promote it provided that it makes sense for the
use
> case. For get, we might want to specify multiple IDs at once. Cache digest
> upload would use multiple IDs or some kind of efficiently packed digest/Bloom
> filter.

Acknowledged.

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/logging.cc...
blimp/common/logging.cc:25: // explicit coverage of all enum values, including
TYPE_NOT_SET.
nit: Let's file a bug for that cleanup, rather than leaving a TODO?

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_integration_test.cc (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_integration_test.cc:53: class
NullBlobReceiverDelegate : public BlobChannelReceiver::Delegate {
nit: Seems more natural to declare this in a top-level anonymous namespace?

It should also have DISALLOW_COPY_AND_ASSIGN and all that jazz.

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:22: public:
Need virtual dtor now!

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver_unittest.cc:34: class
NullBlobReceiverDelegate : public BlobChannelReceiver::Delegate {
nit: You could move this into test_common.h since we use it elsewhere.

However, consider replacing it with a MockBlobReceiverDelegate - that way you
can verify that it gets called to set/unset the Receiver correctly in the tests
below!

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/mock_blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/mock_blob_channel_receiver.h:23: };
DISALLOW_COPY_AND_ASSIGN here too?

[nyquist, 2016-05-25 20:16:01]

lgtm

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:13: optional bytes payload = 1;
Is there any reason why payload is id = 1 and not first in the list? If there
is, could you add a comment to that?

[Kevin M, 2016-05-25 20:23:38]

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/logging.cc
File blimp/common/logging.cc (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/logging.cc...
blimp/common/logging.cc:25: // explicit coverage of all enum values, including
TYPE_NOT_SET.
On 2016/05/25 02:47:44, Wez wrote:
> nit: Let's file a bug for that cleanup, rather than leaving a TODO?

Done, crbug.com/614748

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/proto/blob...
File blimp/common/proto/blob_channel.proto (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/common/proto/blob...
blimp/common/proto/blob_channel.proto:13: optional bytes payload = 1;
On 2016/05/25 20:16:01, nyquist wrote:
> Is there any reason why payload is id = 1 and not first in the list? If there
> is, could you add a comment to that?

No good reason, fixed.

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_integration_test.cc (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_integration_test.cc:53: class
NullBlobReceiverDelegate : public BlobChannelReceiver::Delegate {
On 2016/05/25 02:47:44, Wez wrote:
> nit: Seems more natural to declare this in a top-level anonymous namespace?
> 
> It should also have DISALLOW_COPY_AND_ASSIGN and all that jazz.

Done.

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver.h:22: public:
On 2016/05/25 02:47:44, Wez wrote:
> Need virtual dtor now!

Done.

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/blob_channel_receiver_unittest.cc:34: class
NullBlobReceiverDelegate : public BlobChannelReceiver::Delegate {
On 2016/05/25 02:47:44, Wez wrote:
> nit: You could move this into test_common.h since we use it elsewhere.
> 
> However, consider replacing it with a MockBlobReceiverDelegate - that way you
> can verify that it gets called to set/unset the Receiver correctly in the
tests
> below!

Moved to mock_blob_receiver.h, added mock expectations to verify setting.

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
File blimp/net/blob_channel/mock_blob_channel_receiver.h (right):

https://codereview.chromium.org/1970463004/diff/60001/blimp/net/blob_channel/...
blimp/net/blob_channel/mock_blob_channel_receiver.h:23: };
On 2016/05/25 02:47:44, Wez wrote:
> DISALLOW_COPY_AND_ASSIGN here too?

Done.

[Kevin M, 2016-05-25 20:23:50]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-05-25 20:23:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, nyquist@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1970463004/#ps80001
(title: "wez, nyquist feedback")

[commit-bot: I haz the power, 2016-05-25 20:24:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970463004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970463004/80001

[commit-bot: I haz the power, 2016-05-25 20:29:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 20:29:24]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Kevin M, 2016-05-25 23:42:33]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-05-25 23:42:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, wez@chromium.org, haibinlu@chromium.org
Link to the patchset: https://codereview.chromium.org/1970463004/#ps100001
(title: "fix BUILD.gn")

[commit-bot: I haz the power, 2016-05-25 23:43:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970463004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970463004/100001

[Kevin M, 2016-05-26 17:24:32]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-05-26 17:24:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, wez@chromium.org, haibinlu@chromium.org
Link to the patchset: https://codereview.chromium.org/1970463004/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-26 17:24:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970463004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970463004/120001

[commit-bot: I haz the power, 2016-05-26 20:33:08]

Description was changed from

==========
Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable
following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving
implementation to BlobChannelReceiverImpl.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719
==========

to

==========
Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable
following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving
implementation to BlobChannelReceiverImpl.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719
==========

[commit-bot: I haz the power, 2016-05-26 20:33:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-26 20:34:14]

Description was changed from

==========
Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable
following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving
implementation to BlobChannelReceiverImpl.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719
==========

to

==========
Blimp: Add BlobChannel Helium messages and delegate impls.

* Add BlobChannelMessage protobuf definition.
* Add prettyprint logging for BlobChannelMessage.
* Add CreateBlimpMessage specialization for BlobChannelMessage.
* Add targeted unit tests for new Helium delegates.
* Move BlobData constness into BlobDataPtr, so that BlobData objects are mutable
following their creation (necessary for passing payload buffers w/o copies).
* Refactor CreateBlobDataPtr into common location.
* Turn BlobChannelReceiver into an abstract interface for testing, moving
implementation to BlobChannelReceiverImpl.

R=haibinlu@chromium.org,nyquist@chromium.org
CC=wez@chromium.org
BUG=600719

Committed: https://crrev.com/9dced9059d886d9fecdd3316db84ac9a931d3cc7
Cr-Commit-Position: refs/heads/master@{#396271}
==========

[commit-bot: I haz the power, 2016-05-26 20:34:16]

Patchset 7 (id:??) landed as
https://crrev.com/9dced9059d886d9fecdd3316db84ac9a931d3cc7
Cr-Commit-Position: refs/heads/master@{#396271}