Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2300493002:
Move glue code of blob channel to blimp_client_context_impl. (Closed)
|
Created:
4 years, 3 months ago by xingliu Modified:
4 years, 3 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove glue code of blob channel to blimp_client_context_impl.
Previously it's in blimp/client/session/blimp_client_session for the app
code.
This CL wraps blob channel objects to a wrapper class. Image decode
error delegate is left in BlimpClientContextImpl.
BUG=NONE
Committed: https://crrev.com/ef5bd7ec533f7dd3bba3fdd2986f360ad68cc51e
Cr-Commit-Position: refs/heads/master@{#416322}
Patch Set 1 #Patch Set 2 : Comment wording polish. #Patch Set 3 : Rebase on latest upstream revision. #Patch Set 4 : Refactor the code a bit. #
Total comments: 15
Patch Set 5 : Fix based on review. #Patch Set 6 : Wrap blobc to new class. #Patch Set 7 : Polish comment, clean unused forward decl. #Patch Set 8 : More polish on comments. #
Total comments: 4
Patch Set 9 : Private the functions, interface has expose the method. #Patch Set 10 : Rename the class, polish comments. #Patch Set 11 : Better includes. #
Total comments: 10
Patch Set 12 : nits fixes. #Patch Set 13 : Merge conflicts. #
Messages
Total messages: 44 (28 generated)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, khushalsagar@chromium.org, nyquist@chromium.org
Move glue code for Blob channel to blimp_client_context_impl.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:96: SetImageGeneratorFromEncodedFactory(); I checked, we don't need to do this. skia calls out to the static only if you don't give a image deserializer when deserializing the SkPicture, which we always provide. So you can avoid adding it. :) https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:191: new HeliumBlobReceiverDelegate); nit: prefer MakeUnique over new Foo. For this and in the receiver below. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:192: blob_delegate_ = blob_delegate.get(); Do we actually need to cache this pointer? We only use it to register the feature, so would it be cleaner to do the thread_pipe_manager_->RegisterFeature for blob_delegate in SetupBlobChannel itself?
https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:96: SetImageGeneratorFromEncodedFactory(); On 2016/08/31 03:55:08, Khushal wrote: > I checked, we don't need to do this. skia calls out to the static only if you > don't give a image deserializer when deserializing the SkPicture, which we > always provide. So you can avoid adding it. :) Looked through the code quickly as well and it looks correct. w00t this would be great. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:185: thread_pipe_manager_->RegisterFeature(BlimpMessage::kBlobChannel, let's try to keep these in alphabetical order for the feature? https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:192: blob_delegate_ = blob_delegate.get(); On 2016/08/31 03:55:08, Khushal wrote: > Do we actually need to cache this pointer? We only use it to register the > feature, so would it be cleaner to do the thread_pipe_manager_->RegisterFeature > for blob_delegate in SetupBlobChannel itself? Would it be cleaner to expose a BlobChannelReceiver::delegate() method? That would make it so that we can still keep all of the registration together in the same method. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.h:92: // Setup blob channel. I do this a lot too, but maybe just remove this comment or elaborate a bit :).
https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:96: SetImageGeneratorFromEncodedFactory(); On 2016/08/31 05:40:57, David Trainor wrote: > On 2016/08/31 03:55:08, Khushal wrote: > > I checked, we don't need to do this. skia calls out to the static only if you > > don't give a image deserializer when deserializing the SkPicture, which we > > always provide. So you can avoid adding it. :) > > Looked through the code quickly as well and it looks correct. w00t this would > be great. I wonder, are there any times where we have to re-decode the data? I vaguely remember me having this here because they all of a sudden called us "out-of-the-blue", where we did not get the option to pass it in. But I can't seem to remember when that happened, or whether it will still happen.
https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:96: SetImageGeneratorFromEncodedFactory(); On 2016/09/01 00:26:23, nyquist wrote: > On 2016/08/31 05:40:57, David Trainor wrote: > > On 2016/08/31 03:55:08, Khushal wrote: > > > I checked, we don't need to do this. skia calls out to the static only if > you > > > don't give a image deserializer when deserializing the SkPicture, which we > > > always provide. So you can avoid adding it. :) > > > > Looked through the code quickly as well and it looks correct. w00t this would > > be great. > > I wonder, are there any times where we have to re-decode the data? I vaguely > remember me having this here because they all of a sudden called us > "out-of-the-blue", where we did not get the option to pass it in. But I can't > seem to remember when that happened, or whether it will still happen. I'll remove this. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:185: thread_pipe_manager_->RegisterFeature(BlimpMessage::kBlobChannel, On 2016/08/31 05:40:57, David Trainor wrote: > let's try to keep these in alphabetical order for the feature? Done. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:191: new HeliumBlobReceiverDelegate); On 2016/08/31 03:55:08, Khushal wrote: > nit: prefer MakeUnique over new Foo. For this and in the receiver below. Changed the first one. Didn't change the second one to use MakeUnique since it passed base type pointer. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:192: blob_delegate_ = blob_delegate.get(); On 2016/08/31 05:40:57, David Trainor wrote: > On 2016/08/31 03:55:08, Khushal wrote: > > Do we actually need to cache this pointer? We only use it to register the > > feature, so would it be cleaner to do the > thread_pipe_manager_->RegisterFeature > > for blob_delegate in SetupBlobChannel itself? > > Would it be cleaner to expose a BlobChannelReceiver::delegate() method? That > would make it so that we can still keep all of the registration together in the > same method. RegisterFeatures needs BlimpMessageProcessor*, HeliumBlobReceiverDelegate implements BlimpMessageProcessor and BlobChannelReceiver::Delegate, so if we doing this, need to static cast. Should I do it here? Should I wrap these objects into class like BlobChannelFeature? so it looks similar to other stuff. Right now these things are blimp/net code. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.h:92: // Setup blob channel. On 2016/08/31 05:40:57, David Trainor wrote: > I do this a lot too, but maybe just remove this comment or elaborate a bit :). Done.
https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:192: blob_delegate_ = blob_delegate.get(); On 2016/08/31 05:40:57, David Trainor wrote: > On 2016/08/31 03:55:08, Khushal wrote: > > Do we actually need to cache this pointer? We only use it to register the > > feature, so would it be cleaner to do the > thread_pipe_manager_->RegisterFeature > > for blob_delegate in SetupBlobChannel itself? > > Would it be cleaner to expose a BlobChannelReceiver::delegate() method? That > would make it so that we can still keep all of the registration together in the > same method. Yeah, that's cool too. Or just return the receiver from this method and pass it to the RegisterFeatures method. But why do we need the delegate interface at all? All the delegate is used for is to set the receiver, and feels like the code here should be able to hook that up? For lifetimes, you can make sure that you destroy the receiver before destroying the Helium object?
Description was changed from ========== Move glue code of blob channel to blimp_client_context_impl. Previously it's in blimp/client/session/blimp_client_session for the app code. This CL: 1. Moved the glue code to BlimpClientContextImpl. 2. Put skia initialize function in a static method in context impl. BUG=NONE ========== to ========== Move glue code of blob channel to blimp_client_context_impl. Previously it's in blimp/client/session/blimp_client_session for the app code. This CL wraps blob channel objects to a wrapper class. Image decode error delegate is left in BlimpClientContextImpl. BUG=NONE ==========
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Wrap 0.5 blob channel code to a new class. Decode error delegate is still context_impl, since it uses glue layer data, I think it makes some sense to leave it in context impl. https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/60001/blimp/client/core/blimp... blimp/client/core/blimp_client_context_impl.cc:192: blob_delegate_ = blob_delegate.get(); On 2016/09/01 02:10:10, Khushal wrote: > On 2016/08/31 05:40:57, David Trainor wrote: > > On 2016/08/31 03:55:08, Khushal wrote: > > > Do we actually need to cache this pointer? We only use it to register the > > > feature, so would it be cleaner to do the > > thread_pipe_manager_->RegisterFeature > > > for blob_delegate in SetupBlobChannel itself? > > > > Would it be cleaner to expose a BlobChannelReceiver::delegate() method? That > > would make it so that we can still keep all of the registration together in > the > > same method. > > Yeah, that's cool too. Or just return the receiver from this method and pass it > to the RegisterFeatures method. > > But why do we need the delegate interface at all? All the delegate is used for > is to set the receiver, and feels like the code here should be able to hook that > up? For lifetimes, you can make sure that you destroy the receiver before > destroying the Helium object? For test, they have a mock message processor.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.cc:216: io_thread_task_runner_->PostTask( Can you add a comment that we're currently just dropping the connection if we get any decode failures? https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/comp... File blimp/client/core/compositor/blob_manager.h (right): https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/comp... blimp/client/core/compositor/blob_manager.h:23: class BlobManager : public BlimpMessageProcessor { You're right... BlobChannelFeature makes sense for now.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixes according to review, and some misc polish. https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.cc:216: io_thread_task_runner_->PostTask( On 2016/09/02 00:56:47, David Trainor wrote: > Can you add a comment that we're currently just dropping the connection if we > get any decode failures? Done. https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/comp... File blimp/client/core/compositor/blob_manager.h (right): https://codereview.chromium.org/2300493002/diff/140001/blimp/client/core/comp... blimp/client/core/compositor/blob_manager.h:23: class BlobManager : public BlimpMessageProcessor { On 2016/09/02 00:56:47, David Trainor wrote: > You're right... BlobChannelFeature makes sense for now. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % default... and other nits! https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... File blimp/client/core/compositor/blob_channel_feature.cc (right): https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature.cc:25: base::WrapUnique(new InMemoryBlobCache), std::move(blob_delegate)); MakeUnique<InMemoryBlobCache>() https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature.cc:33: BlobChannelFeature::~BlobChannelFeature() {} = default b/c cooler https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... File blimp/client/core/compositor/blob_channel_feature_unittest.cc (right): https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature_unittest.cc:25: ~MockBlobChannelFeature() override {} = DEFAULT https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature_unittest.cc:27: BlobChannelReceiver* Receiver() { return blob_receiver_.get(); } Receiver -> receiver https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature_unittest.cc:35: BlobManagerTest() {} default x 2
Prepare to commit. https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... File blimp/client/core/compositor/blob_channel_feature.cc (right): https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature.cc:25: base::WrapUnique(new InMemoryBlobCache), std::move(blob_delegate)); On 2016/09/02 16:27:23, David Trainor wrote: > MakeUnique<InMemoryBlobCache>() Done. https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature.cc:33: BlobChannelFeature::~BlobChannelFeature() {} On 2016/09/02 16:27:23, David Trainor wrote: > = default b/c cooler Done. https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... File blimp/client/core/compositor/blob_channel_feature_unittest.cc (right): https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature_unittest.cc:25: ~MockBlobChannelFeature() override {} On 2016/09/02 16:27:23, David Trainor wrote: > = DEFAULT Done. https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature_unittest.cc:27: BlobChannelReceiver* Receiver() { return blob_receiver_.get(); } On 2016/09/02 16:27:23, David Trainor wrote: > Receiver -> receiver Done. https://codereview.chromium.org/2300493002/diff/200001/blimp/client/core/comp... blimp/client/core/compositor/blob_channel_feature_unittest.cc:35: BlobManagerTest() {} On 2016/09/02 16:27:23, David Trainor wrote: > default x 2 Done.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2300493002/#ps220001 (title: "nits fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2300493002/#ps240001 (title: "Merge conflicts.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Move glue code of blob channel to blimp_client_context_impl. Previously it's in blimp/client/session/blimp_client_session for the app code. This CL wraps blob channel objects to a wrapper class. Image decode error delegate is left in BlimpClientContextImpl. BUG=NONE ========== to ========== Move glue code of blob channel to blimp_client_context_impl. Previously it's in blimp/client/session/blimp_client_session for the app code. This CL wraps blob channel objects to a wrapper class. Image decode error delegate is left in BlimpClientContextImpl. BUG=NONE Committed: https://crrev.com/ef5bd7ec533f7dd3bba3fdd2986f360ad68cc51e Cr-Commit-Position: refs/heads/master@{#416322} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/ef5bd7ec533f7dd3bba3fdd2986f360ad68cc51e Cr-Commit-Position: refs/heads/master@{#416322} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
