Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1887013003:
Blimp: Add BlobChannelSender implementation (Closed)
|
Created:
4 years, 8 months ago by Kevin M Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@blobchannel-blobcache Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: BlobChannelSender and bindings interface.
This class is responsible for sending blobs to a remote receiver.
The transport-specific details are delegated to a bindings object,
the implementation of which is out of scope for this CL.
R=wez@chromium.org,nyquist@chromium.org
BUG=600719
Committed: https://crrev.com/66b30ed02476e009cfb27237cc217f05acf5096c
Cr-Commit-Position: refs/heads/master@{#391567}
Patch Set 1 #Patch Set 2 : Remove erroneous namespace #
Total comments: 28
Patch Set 3 : Wez feedback. #
Total comments: 15
Patch Set 4 : wez feedback #Patch Set 5 : re-upload with async blobcache base CL #Patch Set 6 : Sync-only! #
Total comments: 7
Patch Set 7 : wez feedback #Patch Set 8 : rebase #
Messages
Total messages: 28 (11 generated)
Description was changed from ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, (the implementation of which is out of scope for this CL.) R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, (the implementation of which is out of scope for this CL.) R=wez@chromium.org,nyquist@chromium.org BUG=600719 ==========
Description was changed from ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, (the implementation of which is out of scope for this CL.) R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ==========
lgtm https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:24: class BLIMP_NET_EXPORT BlobChannelSender { Should there be a short comment explaining this class?
https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_bindings.h (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_bindings.h:17: class BlobSenderBindings { This just looks like a BlobSender "delegate"; why "Bindings"? https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_bindings.h:21: const std::vector<uint8_t>& data) = 0; Provide typedefs for these so that BlobChannel/BlobCache code can use BlobId and BlobData/BlobContent/BlobBytes for better readability. Why use vector<uint8_t> rather than string? Isn't a string effectively just a vector<char> and so equivalent for our purposes? This interface seems very expensive; if the sender wants to "trickle" the blobs across to the receiver, for instance, then it has to accept a complete copy of the blob payload and store it internally, despite the cache itself already having a copy. If we add an explicit concept of a ref-counted read-only Blob then we can avoid unnecessary copying. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.cc (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.cc:36: cache_->Put(id, cache_item); What is the cache expected to do if it already contains item Id, but with different content? https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.cc:44: << base::HexEncode(id.data(), id.size()); Why is this only a DLOG(WARNING)? Seems worthy of DCHECK or DLOG(FATAL), since it indicates a coding error in the caller. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:13: #include "base/memory/weak_ptr.h" You don't seem to use WeakPtr. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:14: #include "base/observer_list.h" Nor observer lists. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:16: #include "blimp/net/blimp_message_processor.h" Nor BlimpMessageProcessor. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:26: BlobChannelSender(BlobCache* cache, BlobSenderBindings* bindings); 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...? https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:29: // Stores a blob inside the BlobCache. 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. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:30: // Must be called on the main thread. There is no "main thread", nor any concept of threads at all, as far as I can tell, in this class. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:42: // Used for suppressing redundant data transfers. 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? https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:43: // TODO(kmarshall): Replace with a Bloom filter. nit: Remove this TODO; we will _probably_ want to optimize this but there's more to it than just "replacing with a Bloom filter". https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:44: std::set<std::string> client_cache_state_; 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?
https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_bindings.h (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_bindings.h:17: class BlobSenderBindings { On 2016/04/15 17:15:16, Wez wrote: > This just looks like a BlobSender "delegate"; why "Bindings"? It is just a delegate. So, now it's a Delegate. :) Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_bindings.h:21: const std::vector<uint8_t>& data) = 0; On 2016/04/15 17:15:16, Wez wrote: > Provide typedefs for these so that BlobChannel/BlobCache code can use BlobId and > BlobData/BlobContent/BlobBytes for better readability. > > Why use vector<uint8_t> rather than string? Isn't a string effectively just a > vector<char> and so equivalent for our purposes? > > This interface seems very expensive; if the sender wants to "trickle" the blobs > across to the receiver, for instance, then it has to accept a complete copy of > the blob payload and store it internally, despite the cache itself already > having a copy. > > If we add an explicit concept of a ref-counted read-only Blob then we can avoid > unnecessary copying. Added "types.h" for standardization. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.cc (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.cc:36: cache_->Put(id, cache_item); On 2016/04/15 17:15:16, Wez wrote: > What is the cache expected to do if it already contains item Id, but with > different content? Drop it. We really don't want to support ID collisions, since it's supposed to be content-keyed in the first place. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.cc:44: << base::HexEncode(id.data(), id.size()); On 2016/04/15 17:15:16, Wez wrote: > Why is this only a DLOG(WARNING)? Seems worthy of DCHECK or DLOG(FATAL), since > it indicates a coding error in the caller. Yeee https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:13: #include "base/memory/weak_ptr.h" On 2016/04/15 17:15:16, Wez wrote: > You don't seem to use WeakPtr. Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:14: #include "base/observer_list.h" On 2016/04/15 17:15:17, Wez wrote: > Nor observer lists. Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:16: #include "blimp/net/blimp_message_processor.h" On 2016/04/15 17:15:16, Wez wrote: > Nor BlimpMessageProcessor. Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:24: class BLIMP_NET_EXPORT BlobChannelSender { On 2016/04/15 17:12:52, nyquist wrote: > Should there be a short comment explaining this class? Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:26: BlobChannelSender(BlobCache* cache, BlobSenderBindings* bindings); On 2016/04/15 17:15:16, Wez wrote: > 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...? Clarified ownership (or lack thereof.) Addressed Binding naming nit, is now "Delegate". https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:29: // Stores a blob inside the BlobCache. On 2016/04/15 17:15:16, Wez wrote: > 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. It puts the blob into the channel, where it can be delivered either by a locally-originating push, or a remote-originating pull. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:30: // Must be called on the main thread. On 2016/04/15 17:15:16, Wez wrote: > There is no "main thread", nor any concept of threads at all, as far as I can > tell, in this class. Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:42: // Used for suppressing redundant data transfers. On 2016/04/15 17:15:16, Wez wrote: > 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? Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:43: // TODO(kmarshall): Replace with a Bloom filter. On 2016/04/15 17:15:16, Wez wrote: > nit: Remove this TODO; we will _probably_ want to optimize this but there's more > to it than just "replacing with a Bloom filter". Done. https://codereview.chromium.org/1887013003/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:44: std::set<std::string> client_cache_state_; On 2016/04/15 17:15:16, Wez wrote: > 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? Done. "receiver_cache_contents_"
https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.cc (right): https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.cc:22: DCHECK(data.get()); nit: Just DCHECK(data) Do you also want to DCHECK that |id| is non-empty? https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:31: virtual void Deliver(const BlobId& id, BlobData data) = 0; nit: Suggest DeliverBob, in case the implementer delivers other things ;) https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:36: BlobChannelSender(BlobCache* cache, Delegate* delegate); Can/should the Sender own the cache object? https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:41: // Does nothing if there is already a blob |id| in the channel. As previously discussed, what if |data| is different for an existing |id|? Would it be cleaner to have the caller able to query whether an Id is already in the channel and _then_ Put() it iff it's not, so that we avoid the BlobData construction/copy overhead? https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:45: // have the blob. Sorry, still not clear from this interface why Put and Deliver are separate - why would I ever "put" something in the channel if I _don't_ want it to be "delivered"? https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:55: std::set<BlobId> receiver_cache_contents_; This won't work well if the Client-side has a purgeable cache, since we may no-op a Put(), thinking that the content is already there, only to discover that it is not because of purge - the Client will presumably report back that it is missing an entry, but by then we have no way of passing it the content, unless the caller provides it again? Should we allow the caller to express whether an item is relevant, and therefore should be retained locally, until the peer confirms that it has made use if it? e.g. have the call return a dummy object, or token, which it can retain to express continued interest in that Blob? Then if/when the Client confirms that it has processed the affected update you might relinquish interest in it, making it a high-priority candidate for discard.
https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.cc (right): https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.cc:22: DCHECK(data.get()); On 2016/04/19 00:01:57, Wez wrote: > nit: Just DCHECK(data) > > Do you also want to DCHECK that |id| is non-empty? Done. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:31: virtual void Deliver(const BlobId& id, BlobData data) = 0; On 2016/04/19 00:01:57, Wez wrote: > nit: Suggest DeliverBob, in case the implementer delivers other things ;) Done. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:36: BlobChannelSender(BlobCache* cache, Delegate* delegate); On 2016/04/19 00:01:57, Wez wrote: > Can/should the Sender own the cache object? Hmmm. I don't have a reason why not. Made |cache| and |delegate| unique_ptr. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:41: // Does nothing if there is already a blob |id| in the channel. On 2016/04/19 00:01:57, Wez wrote: > As previously discussed, what if |data| is different for an existing |id|? > Would it be cleaner to have the caller able to query whether an Id is already in > the channel and _then_ Put() it iff it's not, so that we avoid the BlobData > construction/copy overhead? |id| is content-keyed, so it's safe to assume that there shouldn't be collisions like this. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:45: // have the blob. On 2016/04/19 00:01:57, Wez wrote: > Sorry, still not clear from this interface why Put and Deliver are separate - > why would I ever "put" something in the channel if I _don't_ want it to be > "delivered"? As discussed offline - breaking this into different calls gives us the flexibility to specify when and how the data is delivered. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:55: std::set<BlobId> receiver_cache_contents_; On 2016/04/19 00:01:57, Wez wrote: > This won't work well if the Client-side has a purgeable cache, since we may > no-op a Put(), thinking that the content is already there, only to discover that > it is not because of purge - the Client will presumably report back that it is > missing an entry, but by then we have no way of passing it the content, unless > the caller provides it again? > > Should we allow the caller to express whether an item is relevant, and therefore > should be retained locally, until the peer confirms that it has made use if it? > e.g. have the call return a dummy object, or token, which it can retain to > express continued interest in that Blob? Then if/when the Client confirms that > it has processed the affected update you might relinquish interest in it, making > it a high-priority candidate for discard. We don't have the ability to control data retention in the client - the simple disk cache's retention policy is kind of a black box for us. That leaves us with snapshots of the client's cache contents which might be out-of-date for the intervals between eviction and notification. Allowing the client to make explicit requests in those instances satisfies those async edge cases.
Kay-o, it's sync only now!
kmarshall@chromium.org changed reviewers: + haibinlu@chromium.org
Adding haibin to reviewer list.
LGTM w/ nits https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:41: // Does nothing if there is already a blob |id| in the channel. On 2016/04/19 23:39:23, Kevin M wrote: > On 2016/04/19 00:01:57, Wez wrote: > > As previously discussed, what if |data| is different for an existing |id|? > > Would it be cleaner to have the caller able to query whether an Id is already > in > > the channel and _then_ Put() it iff it's not, so that we avoid the BlobData > > construction/copy overhead? > > |id| is content-keyed, so it's safe to assume that there shouldn't be collisions > like this. Could we just pass |data| and calculate the Id internally, then, eliminating any scope for the caller messing-up? That might work best if BlobDataPtr became BlobPtr and Blobs then include a cached Id field, set at construction time. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:45: // have the blob. On 2016/04/19 23:39:23, Kevin M wrote: > On 2016/04/19 00:01:57, Wez wrote: > > Sorry, still not clear from this interface why Put and Deliver are separate - > > why would I ever "put" something in the channel if I _don't_ want it to be > > "delivered"? > > As discussed offline - breaking this into different calls gives us the > flexibility to specify when and how the data is delivered. Acknowledged. https://codereview.chromium.org/1887013003/diff/40001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_sender.h:55: std::set<BlobId> receiver_cache_contents_; On 2016/04/19 23:39:23, Kevin M wrote: > On 2016/04/19 00:01:57, Wez wrote: > > This won't work well if the Client-side has a purgeable cache, since we may > > no-op a Put(), thinking that the content is already there, only to discover > that > > it is not because of purge - the Client will presumably report back that it is > > missing an entry, but by then we have no way of passing it the content, unless > > the caller provides it again? > > > > Should we allow the caller to express whether an item is relevant, and > therefore > > should be retained locally, until the peer confirms that it has made use if > it? > > e.g. have the call return a dummy object, or token, which it can retain to > > express continued interest in that Blob? Then if/when the Client confirms that > > it has processed the affected update you might relinquish interest in it, > making > > it a high-priority candidate for discard. > > We don't have the ability to control data retention in the client - the simple > disk cache's retention policy is kind of a black box for us. That leaves us with > snapshots of the client's cache contents which might be out-of-date for the > intervals between eviction and notification. Allowing the client to make > explicit requests in those instances satisfies those async edge cases. Acknowledged. https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_sender.cc (right): https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender.cc:48: receiver_cache_contents_.insert(id); nit: Suggest moving this to come immediately after the if() check against receiver_cache_contents, since logically the two belong together - if an item is in there then we know we already have delivered it, or are about to. https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender.h:41: void Put(const BlobId& id, BlobDataPtr data); nit: PutBlob, to match DeliverBlob below? https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender.h:51: // A set of the cache item IDs in the receiver's cache. nit: I'd suggest: "Used to track the item Ids in the receiver's cache. This may differ from the set of Ids in |cache_|, for instance if an Id hasn't yet been delivered, or has been evicted at the receiver. https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_sender_unittest.cc (right): https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender_unittest.cc:49: ~BlobChannelSenderTest() override {} Don't you need to delete the two StrickMocks?
https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_sender.cc (right): https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender.cc:48: receiver_cache_contents_.insert(id); On 2016/05/03 22:11:05, Wez wrote: > nit: Suggest moving this to come immediately after the if() check against > receiver_cache_contents, since logically the two belong together - if an item is > in there then we know we already have delivered it, or are about to. Done. https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_sender.h (right): https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender.h:51: // A set of the cache item IDs in the receiver's cache. On 2016/05/03 22:11:05, Wez wrote: > nit: I'd suggest: > > "Used to track the item Ids in the receiver's cache. This may differ from the > set of Ids in |cache_|, for instance if an Id hasn't yet been delivered, or has > been evicted at the receiver. Good suggestion, thx https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_sender_unittest.cc (right): https://codereview.chromium.org/1887013003/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_sender_unittest.cc:49: ~BlobChannelSenderTest() override {} On 2016/05/03 22:11:05, Wez wrote: > Don't you need to delete the two StrickMocks? Nope, blob_sender_ owns the objects themselves.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1887013003/#ps120001 (title: "wez feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887013003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887013003/120001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for blimp/common/blob_cache/mock_blob_cache.h:
While running git apply --index -3 -p1;
Falling back to three-way merge...
Applied patch to 'blimp/common/blob_cache/mock_blob_cache.h' with conflicts.
U blimp/common/blob_cache/mock_blob_cache.h
Patch: N blimp/common/blob_cache/mock_blob_cache.h
Index: blimp/common/blob_cache/mock_blob_cache.h
diff --git a/blimp/common/blob_cache/mock_blob_cache.h
b/blimp/common/blob_cache/mock_blob_cache.h
new file mode 100644
index
0000000000000000000000000000000000000000..36a3104c0a0ff5d0bc6c9bfd21f27f53d48d8bb3
--- /dev/null
+++ b/blimp/common/blob_cache/mock_blob_cache.h
@@ -0,0 +1,25 @@
+// 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_COMMON_BLOB_CACHE_MOCK_BLOB_CACHE_H_
+#define BLIMP_COMMON_BLOB_CACHE_MOCK_BLOB_CACHE_H_
+
+#include "blimp/common/blob_cache/blob_cache.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace blimp {
+
+class MockBlobCache : public BlobCache {
+ public:
+ MockBlobCache();
+ ~MockBlobCache() override;
+
+ MOCK_CONST_METHOD1(Contains, bool(const BlobId&));
+ MOCK_METHOD2(Put, void(const BlobId&, BlobDataPtr));
+ MOCK_CONST_METHOD1(Get, BlobDataPtr(const BlobId&));
+};
+
+} // namespace blimp
+
+#endif // BLIMP_COMMON_BLOB_CACHE_MOCK_BLOB_CACHE_H_
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by kmarshall@chromium.org
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/1887013003/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1887013003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1887013003/140001
Message was sent while issue was closed.
Description was changed from ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: BlobChannelSender and bindings interface. This class is responsible for sending blobs to a remote receiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. R=wez@chromium.org,nyquist@chromium.org BUG=600719 Committed: https://crrev.com/66b30ed02476e009cfb27237cc217f05acf5096c Cr-Commit-Position: refs/heads/master@{#391567} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/66b30ed02476e009cfb27237cc217f05acf5096c Cr-Commit-Position: refs/heads/master@{#391567} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
