Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1891083002:
Blimp: Add BlobChannelReceiver and bindings interface. (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-sender Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: Implement BlobChannelReceiver.
This class is responsible for routing blob reads to locally cached
entries, or if they aren't available, issuing requests for the data to
a remote BlobChannelReceiver.
The transport-specific details are provided by a delegate object,
the implementation of which is out of scope for this CL.
Add a BlobCache mock class.
R=wez@chromium.org,nyquist@chromium.org
BUG=600719
Committed: https://crrev.com/eaac155c0dd775c2b295c94b1b3ed85dab453908
Cr-Commit-Position: refs/heads/master@{#391394}
Patch Set 1 #Patch Set 2 : moar bindings commentz! #
Total comments: 9
Patch Set 3 : wez feedback #Patch Set 4 : git rm *bindings* #
Total comments: 12
Patch Set 5 : Sync only! #
Total comments: 21
Patch Set 6 : wez feedback #
Total comments: 4
Patch Set 7 : wez feedback #
Total comments: 10
Patch Set 8 : wez feedback #
Messages
Total messages: 25 (8 generated)
https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_bindings.h (right): https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_bindings.h:27: class Delegate { Why is this callback wrapped in a class and called a delegate, while the BlobChannelReceiver just has a callback? https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:32: BlobChannelReceiver(BlobCache* cache, BlobReceiverBindings* bindings); See comments on BlobChannelSender re the naming & structure of these APIs. https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:38: // |callback| is discarded. Do we need the synchronous path? Or can we just have the asynchronous path but require that the current impl call the callback before returning? https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:41: // Can be called on any thread. Holds |lock_|. This object isn't ref-counted; how can it "be called on any thread"? Is the caller required to ensure some special lifetime constraints of this class?
https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_bindings.h (right): https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_bindings.h:27: class Delegate { On 2016/04/15 17:27:43, Wez wrote: > Why is this callback wrapped in a class and called a delegate, while the > BlobChannelReceiver just has a callback? The delegate dispatches events for a single client; the BlobChannelReceiver needs to fan out events to >1 clients (multiple frames can request the same blob concurrently). https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:32: BlobChannelReceiver(BlobCache* cache, BlobReceiverBindings* bindings); On 2016/04/15 17:27:43, Wez wrote: > See comments on BlobChannelSender re the naming & structure of these APIs. Done. https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:38: // |callback| is discarded. On 2016/04/15 17:27:43, Wez wrote: > Do we need the synchronous path? Or can we just have the asynchronous path but > require that the current impl call the callback before returning? The synchronous path is pretty essential for getting bits out to Skia - the caller gets the image bits synchronously and we don't have the ability to slip them in asynchronously at arbitrary points after rendering has completed. https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:41: // Can be called on any thread. Holds |lock_|. On 2016/04/15 17:27:43, Wez wrote: > This object isn't ref-counted; how can it "be called on any thread"? Is the > caller required to ensure some special lifetime constraints of this class? This is meant to support concurrent calls by a number of raster working threads. I'm not sure if we have guarantees that the raster threads will finish execution before the BlimpClientSession is torn down, but it _seems_ like a safe bet? O_o
https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.cc (right): https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:16: BlobChannelReceiver::Delegate::~Delegate() {} Your API contract is that Delegate MUST out-live the BlobChannelReceiver, so you can DCHECK(!receiver.isValid()) during destruction to ensure that. However, you don't seem to need a WeakPtr here at all; you can just have BlobChannelReceiver call SetReceiver(nullptr) during destruction and avoid that extra overhead. https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:48: return cache_->Get(id); See below; like Put(), cache_->Get() seems likely to need to be either always- or at least sometimes- asynchronous, in general? https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:53: delegate_->Request(id); Minimize the amount of code inside the Lock - set a local bool to indicate whether to dispatch the request, and dispatch it after releasing the lock (this is safe since you only ever dispatch a single request per-Id). https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:71: cache_->Put(id, data); In general adding a blob to the cache will be time-consuming & may involve I/O (e.g. to persist to disk), so will be asynchronous - should we be calling Put() while we're holding the lock? https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:75: for (auto callback_it = callbacks_for_id.first; nit: callback_it is actually a more confusing name than e.g. it alone would be. Why can't you just use std::vector::assign to copy the range into pending_callbacks, though? https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:59: BlobChannelReceiver(BlobCache* cache, Delegate* delegate); Remind me why the receiver can't just "own" the cache object? https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:68: // Can be called on any thread. Holds |lock_|. Holds lock_ is an internal implementation detail. I think what you want to say is that this is thread-safe? https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:69: BlobData Get(const BlobId& id, const BlobReceivedCallback& callback); What is BlobData? I don't see it defined anywhere in this CL, nor the existing codebase. https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:84: // Guards concurrent access to |cache_| and |active_read_callbacks_|. Concurrent access by whom? The delegates call the BlobChannelReceiver via a WeakPtr, so those calls must all happen on the thread on which BlobChannelReceiver is going to be torn down.
https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/20001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:41: // Can be called on any thread. Holds |lock_|. On 2016/04/15 23:53:44, Kevin M wrote: > On 2016/04/15 17:27:43, Wez wrote: > > This object isn't ref-counted; how can it "be called on any thread"? Is the > > caller required to ensure some special lifetime constraints of this class? > > This is meant to support concurrent calls by a number of raster working threads. > I'm not sure if we have guarantees that the raster threads will finish execution > before the BlimpClientSession is torn down, but it _seems_ like a safe bet? O_o Best to state such assumptions up-front! https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.cc (right): https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:48: return cache_->Get(id); On 2016/04/18 22:40:40, Wez wrote: > See below; like Put(), cache_->Get() seems likely to need to be either always- > or at least sometimes- asynchronous, in general? Having this in here also means that the cache implementation needs to be thread-clean (i.e. OK to call from different threads, even though it's not fully thread-safe). https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:53: delegate_->Request(id); On 2016/04/18 22:40:40, Wez wrote: > Minimize the amount of code inside the Lock - set a local bool to indicate > whether to dispatch the request, and dispatch it after releasing the lock (this > is safe since you only ever dispatch a single request per-Id). Note that calling delegate_->Request() here means that the Delegate is being called on multiple threads, and that it's being called on caller's raster threads, neither of which seem desirable - it would be cleaner to PostTask the Request() call back to the BlobChannelReceiver's home thread.
https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/60001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:79: Delegate* delegate_ = nullptr; nit: You have no default ctor for this class, so no need for in-line init.
PTAL. Code is now sync-only, we can add async in a later CL when an async transport and/or async cache are implemented.
Description was changed from ========== Blimp: Add BlobChannelReceiver and bindings interface. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. 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: Add BlobChannelReceiver and bindings interface. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ==========
Description was changed from ========== Blimp: Add BlobChannelReceiver and bindings interface. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ==========
Description was changed from ========== Blimp: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are delegated to a bindings object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are provided by a delegate object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ==========
https://codereview.chromium.org/1891083002/diff/80001/blimp/common/blob_cache... File blimp/common/blob_cache/mock_blob_cache.h (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/common/blob_cache... blimp/common/blob_cache/mock_blob_cache.h:21: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.cc (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:15: BlobChannelReceiver::Delegate::~Delegate() {} nit: You can still DCHECK(!receiver.isValid()) during destruction to ensure BlobChannelReceiver did the right thing. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:59: // Guards against concurrent access to |cache_| and |active_read_callbacks_|. There is no |active_read_callbacks_| here? Concurrent access by whom? Are you suggesting that Get() is called from a different thread (or threads?) to OnBlobReceived()? If that's somehow a valid usage of this class then comment on the thread-safety properties in a class comment. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:26: return new BlobData(input); nit: Use make_scoped_refptr(...) here. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:33: class MockBlobReceiverDelegate : public BlobChannelReceiver::Delegate { Is there any value in this being a mock, since there is nothing in it to mock? https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:55: TEST_F(BlobChannelReceiverTest, TestGetFromCache) { Don't include Test in a test's name; we already know it's a test, 'cos the fixture name ends in Test. ;) Name it based on what it tests, e.g. this might be GetKnownBlob. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:61: ASSERT_NE(result.get(), nullptr); nit: No need for this, since dereferencing result will crash if it's null. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:65: TEST_F(BlobChannelReceiverTest, TestGetFromTransport) { This isn't really getting-from-transport, but testing that OnBlobReceived() has the desired effect on the cache, right? https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:72: auto result = blob_receiver_->Get(kBlobId1); If you used a real BlobCache instead of a mock then this test would make more sense; you'd call OnBlobReceived() and verify that Get() returns the correct result. You'd also try Get() before OnBlobReceived() to verify that the cache is clean beforehand. The preceding test would instead look like: ASSERT_FALSE(cache.Contains) cache.Put EXPECT_EQ(receiver.Get)
https://codereview.chromium.org/1891083002/diff/80001/blimp/common/blob_cache... File blimp/common/blob_cache/mock_blob_cache.h (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/common/blob_cache... blimp/common/blob_cache/mock_blob_cache.h:21: }; On 2016/04/21 00:55:02, Wez wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.cc (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.cc:15: BlobChannelReceiver::Delegate::~Delegate() {} On 2016/04/21 00:55:02, Wez wrote: > nit: You can still DCHECK(!receiver.isValid()) during destruction to ensure > BlobChannelReceiver did the right thing. Done. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:59: // Guards against concurrent access to |cache_| and |active_read_callbacks_|. On 2016/04/21 00:55:02, Wez wrote: > There is no |active_read_callbacks_| here? > > Concurrent access by whom? Are you suggesting that Get() is called from a > different thread (or threads?) to OnBlobReceived()? > > If that's somehow a valid usage of this class then comment on the thread-safety > properties in a class comment. Removed obsolete comment. Yeah, this needs to allow concurrent access from a number of render threads. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:26: return new BlobData(input); On 2016/04/21 00:55:02, Wez wrote: > nit: Use make_scoped_refptr(...) here. Used BlobDataPtr instead, so that we're consistent with use of typedefs. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:33: class MockBlobReceiverDelegate : public BlobChannelReceiver::Delegate { On 2016/04/21 00:55:02, Wez wrote: > Is there any value in this being a mock, since there is nothing in it to mock? It will eventually have mock methods, once we have add client Get(). But for now, yeah - it's not a mock, so I'll call it a NullBlobReceiverDelegate. I use the subclass for promoting OnBlobReceived to public visibility. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:55: TEST_F(BlobChannelReceiverTest, TestGetFromCache) { On 2016/04/21 00:55:02, Wez wrote: > Don't include Test in a test's name; we already know it's a test, 'cos the > fixture name ends in Test. ;) Name it based on what it tests, e.g. this might > be GetKnownBlob. Done. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:61: ASSERT_NE(result.get(), nullptr); On 2016/04/21 00:55:02, Wez wrote: > nit: No need for this, since dereferencing result will crash if it's null. Acknowledged. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:65: TEST_F(BlobChannelReceiverTest, TestGetFromTransport) { On 2016/04/21 00:55:02, Wez wrote: > This isn't really getting-from-transport, but testing that OnBlobReceived() has > the desired effect on the cache, right? Yes, renamed. https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:72: auto result = blob_receiver_->Get(kBlobId1); On 2016/04/21 00:55:02, Wez wrote: > If you used a real BlobCache instead of a mock then this test would make more > sense; you'd call OnBlobReceived() and verify that Get() returns the correct > result. You'd also try Get() before OnBlobReceived() to verify that the cache > is clean beforehand. > > The preceding test would instead look like: > > ASSERT_FALSE(cache.Contains) > cache.Put > EXPECT_EQ(receiver.Get) Done.
https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:59: // Guards against concurrent access to |cache_| and |active_read_callbacks_|. On 2016/04/21 21:24:08, Kevin M wrote: > On 2016/04/21 00:55:02, Wez wrote: > > There is no |active_read_callbacks_| here? > > > > Concurrent access by whom? Are you suggesting that Get() is called from a > > different thread (or threads?) to OnBlobReceived()? > > > > If that's somehow a valid usage of this class then comment on the > thread-safety > > properties in a class comment. > > Removed obsolete comment. > > Yeah, this needs to allow concurrent access from a number of render threads. If it's a resource shared across multiple threads then should it be ref-counted? Does it need to be deleted on a specific thread, e.g. the one on which it was created? https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:72: auto result = blob_receiver_->Get(kBlobId1); On 2016/04/21 21:24:08, Kevin M wrote: > On 2016/04/21 00:55:02, Wez wrote: > > If you used a real BlobCache instead of a mock then this test would make more > > sense; you'd call OnBlobReceived() and verify that Get() returns the correct > > result. You'd also try Get() before OnBlobReceived() to verify that the cache > > is clean beforehand. > > > > The preceding test would instead look like: > > > > ASSERT_FALSE(cache.Contains) > > cache.Put > > EXPECT_EQ(receiver.Get) > > Done. After suggesting this, I realised that an advantage of the mock approach is that you can verify that Put() gets called exactly-once. You might consider mixing mock + in-memory impl so that you have mock-able methods you can verify get called directly, but which also forward-on to the "real" underlying impl. On the other hand the test is so simple that your previous mock-based approach was pretty simple. Up to you! https://codereview.chromium.org/1891083002/diff/100001/blimp/common/blob_cach... File blimp/common/blob_cache/mock_blob_cache.h (right): https://codereview.chromium.org/1891083002/diff/100001/blimp/common/blob_cach... blimp/common/blob_cache/mock_blob_cache.h:13: class MockBlobCache : public BlobCache { Looks like this whole mock is unused in this CL now; either revert to using the mock (if you think verifying exactly-once call to Put is valuable) or remove this? https://codereview.chromium.org/1891083002/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver.h:20: // Can be accessed concurrently from any thread. That's a very strong statement for a not-ref-counted class. I think you meant to put this comment on Get(), with a stern warning about the caller being responsible for making sure that the instance is not deleted while any threads might want to call Get() on it.
https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/80001/blimp/net/blob_channel/... blimp/net/blob_channel/blob_channel_receiver.h:59: // Guards against concurrent access to |cache_| and |active_read_callbacks_|. > If it's a resource shared across multiple threads then should it be ref-counted? We don't need to. We can safely assume that the raster threads will be torn down before the AtExitManager is invoked (blimp_main =owns=> client session => compositor feature => compositor manager => threads). > Does it need to be deleted on a specific thread, e.g. the one on which it was > created? Not currently, but I don't know about the future - that probably depends on the requirements of the BlobCache impl? https://codereview.chromium.org/1891083002/diff/100001/blimp/common/blob_cach... File blimp/common/blob_cache/mock_blob_cache.h (right): https://codereview.chromium.org/1891083002/diff/100001/blimp/common/blob_cach... blimp/common/blob_cache/mock_blob_cache.h:13: class MockBlobCache : public BlobCache { On 2016/04/21 22:52:50, Wez wrote: > Looks like this whole mock is unused in this CL now; either revert to using the > mock (if you think verifying exactly-once call to Put is valuable) or remove > this? I'll revert to using the mock. https://codereview.chromium.org/1891083002/diff/100001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/100001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver.h:20: // Can be accessed concurrently from any thread. On 2016/04/21 22:52:50, Wez wrote: > That's a very strong statement for a not-ref-counted class. > > I think you meant to put this comment on Get(), with a stern warning about the > caller being responsible for making sure that the instance is not deleted while > any threads might want to call Get() on it. Done.
kmarshall@chromium.org changed reviewers: + haibinlu@chromium.org - nyquist@chromium.org
Adding haibin to reviewer list
LGTM w/ nits. https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver.cc (right): https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver.cc:34: DCHECK(delegate_); nit: Next line de-refs delegate_, so no need to DCHECK it here - next line will crash reliably if it's null. https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver.h:62: base::Lock lock_; nit: Rename this cache_lock_, so it's clear that it only protects access to cache_? https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right): https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:22: const char kBlobId1[] = "blob-1"; nit: Can this and the payload just be kBlobId and kBlobPayload, since we don't seem to have any other Ids? https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:62: ASSERT_NE(result.get(), nullptr); nit: These should be ASSERT_OP(expected, actual), to get the right print-out on failure. I take it these don't work as ASSERT_TRUE(result)? https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:72: EXPECT_CALL(*cache_, Get(kBlobId1)).WillOnce(Return(payload)); Don't mix EXPECT_CALL in with actual calls into the code like this - it has potential undefined behaviour - set up expectations first and then make invocations to run the test.
https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver.cc (right): https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver.cc:34: DCHECK(delegate_); On 2016/05/03 21:32:20, Wez wrote: > nit: Next line de-refs delegate_, so no need to DCHECK it here - next line will > crash reliably if it's null. Done. https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver.h (right): https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver.h:62: base::Lock lock_; On 2016/05/03 21:32:20, Wez wrote: > nit: Rename this cache_lock_, so it's clear that it only protects access to > cache_? Done. https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... File blimp/net/blob_channel/blob_channel_receiver_unittest.cc (right): https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:22: const char kBlobId1[] = "blob-1"; On 2016/05/03 21:32:20, Wez wrote: > nit: Can this and the payload just be kBlobId and kBlobPayload, since we don't > seem to have any other Ids? Done. https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:62: ASSERT_NE(result.get(), nullptr); On 2016/05/03 21:32:20, Wez wrote: > nit: These should be ASSERT_OP(expected, actual), to get the right print-out on > failure. I take it these don't work as ASSERT_TRUE(result)? Done. https://codereview.chromium.org/1891083002/diff/120001/blimp/net/blob_channel... blimp/net/blob_channel/blob_channel_receiver_unittest.cc:72: EXPECT_CALL(*cache_, Get(kBlobId1)).WillOnce(Return(payload)); On 2016/05/03 21:32:20, Wez wrote: > Don't mix EXPECT_CALL in with actual calls into the code like this - it has > potential undefined behaviour - set up expectations first and then make > invocations to run the test. Done.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1891083002/#ps140001 (title: "wez feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1891083002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1891083002/140001
Message was sent while issue was closed.
Description was changed from ========== Blimp: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are provided by a delegate object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are provided by a delegate object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. 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: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are provided by a delegate object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 ========== to ========== Blimp: Implement BlobChannelReceiver. This class is responsible for routing blob reads to locally cached entries, or if they aren't available, issuing requests for the data to a remote BlobChannelReceiver. The transport-specific details are provided by a delegate object, the implementation of which is out of scope for this CL. Add a BlobCache mock class. R=wez@chromium.org,nyquist@chromium.org BUG=600719 Committed: https://crrev.com/eaac155c0dd775c2b295c94b1b3ed85dab453908 Cr-Commit-Position: refs/heads/master@{#391394} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/eaac155c0dd775c2b295c94b1b3ed85dab453908 Cr-Commit-Position: refs/heads/master@{#391394} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
