Move glue code of blob channel to blimp_client_context_impl.

blimp/client/core/blimp_client_context_impl.cc

 // 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.
 
 #include "blimp/client/core/blimp_client_context_impl.h"
 
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "blimp/client/core/compositor/decoding_image_generator.h"
 #include "blimp/client/core/contents/blimp_contents_impl.h"
 #include "blimp/client/core/contents/blimp_contents_manager.h"
 #include "blimp/client/core/contents/ime_feature.h"
 #include "blimp/client/core/contents/navigation_feature.h"
 #include "blimp/client/core/contents/tab_control_feature.h"
 #include "blimp/client/core/session/cross_thread_network_event_observer.h"
 #include "blimp/client/public/blimp_client_context_delegate.h"
+#include "blimp/common/blob_cache/in_memory_blob_cache.h"
+#include "blimp/net/blob_channel/blob_channel_receiver.h"
+#include "blimp/net/blob_channel/helium_blob_receiver_delegate.h"
+#include "third_party/skia/include/core/SkGraphics.h"
 
 #if defined(OS_ANDROID)
 #include "blimp/client/core/android/blimp_client_context_impl_android.h"
 #endif  // OS_ANDROID
 
 namespace blimp {
 namespace client {
 
 namespace {
+
+SkImageGenerator* CreateImageGenerator(SkData* data) {
+  return blimp::client::DecodingImageGenerator::create(data);
+}
+
 const char kDefaultAssignerUrl[] =
     "https://blimp-pa.googleapis.com/v1/assignment";
 }  // namespace
 
+// static
+void BlimpClientContextImpl::SetImageGeneratorFromEncodedFactory() {
+  SkGraphics::SetImageGeneratorFromEncodedFactory(CreateImageGenerator);
+}
+
 // This function is declared in //blimp/client/public/blimp_client_context.h,
 // and either this function or the one in
 // //blimp/client/core/dummy_blimp_client_context.cc should be linked in to
 // any binary using BlimpClientContext::Create.
 // static
 BlimpClientContext* BlimpClientContext::Create(
     scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner) {
 #if defined(OS_ANDROID)
   return new BlimpClientContextImplAndroid(io_thread_task_runner,
@@ skipping 20 matching lines @@
       weak_factory_(this) {
   net_components_.reset(new ClientNetworkComponents(
       base::MakeUnique<CrossThreadNetworkEventObserver>(
           weak_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get())));
 
   // The |thread_pipe_manager_| must be set up correctly before features are
   // registered.
   thread_pipe_manager_ = base::MakeUnique<ThreadPipeManager>(
       io_thread_task_runner_, net_components_->GetBrowserConnectionHandler());
 
+  SetupBlobChannel();
+
   RegisterFeatures();
 
   // Initialize must only be posted after the calls features have been
   // registered.
   io_thread_task_runner_->PostTask(
       FROM_HERE, base::Bind(&ClientNetworkComponents::Initialize,
                             base::Unretained(net_components_.get())));
+
+  SetImageGeneratorFromEncodedFactory();

[Khushal, 2016/08/31 03:55:08]

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. :)

[David Trainor- moved to gerrit, 2016/08/31 05:40:57]

Looked through the code quickly as well and it looks correct.  w00t this would
be great.

[nyquist, 2016/09/01 00:26:23]

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.

[xingliu, 2016/09/01 02:08:09]

I'll remove this.

 }
 
 BlimpClientContextImpl::~BlimpClientContextImpl() {
   io_thread_task_runner_->DeleteSoon(FROM_HERE, net_components_.release());
 }
 
 void BlimpClientContextImpl::SetDelegate(BlimpClientContextDelegate* delegate) {
   delegate_ = delegate;
 }
 
@@ skipping 68 matching lines @@
   // Register features' message senders and receivers.
   ime_feature_->set_outgoing_message_processor(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kIme,
                                             ime_feature_.get()));
   navigation_feature_->set_outgoing_message_processor(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kNavigation,
                                             navigation_feature_.get()));
   tab_control_feature_->set_outgoing_message_processor(
       thread_pipe_manager_->RegisterFeature(BlimpMessage::kTabControl,
                                             tab_control_feature_.get()));
+  thread_pipe_manager_->RegisterFeature(BlimpMessage::kBlobChannel,

[David Trainor- moved to gerrit, 2016/08/31 05:40:57]

let's try to keep these in alphabetical order for the feature?

[xingliu, 2016/09/01 02:08:10]

Done.

+                                        blob_delegate_);
+}
+
+void BlimpClientContextImpl::SetupBlobChannel() {
+  std::unique_ptr<HeliumBlobReceiverDelegate> blob_delegate(
+      new HeliumBlobReceiverDelegate);

[Khushal, 2016/08/31 03:55:08]

nit: prefer MakeUnique over new Foo. For this and in the receiver below.

[xingliu, 2016/09/01 02:08:09]

Changed the first one. Didn't change the second one to use MakeUnique since it
passed base type pointer.

+  blob_delegate_ = blob_delegate.get();

[Khushal, 2016/08/31 03:55:08]

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?

[David Trainor- moved to gerrit, 2016/08/31 05:40:57]

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.

[xingliu, 2016/09/01 02:08:10]

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.

[Khushal, 2016/09/01 02:10:10]

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?

[xingliu, 2016/09/01 21:03:34]

For test, they have a mock message processor.

+  blob_receiver_ = BlobChannelReceiver::Create(
+      base::WrapUnique(new InMemoryBlobCache), std::move(blob_delegate));
+  blob_image_processor_.set_blob_receiver(blob_receiver_.get());
+  blob_image_processor_.set_error_delegate(this);
+}
+
+void BlimpClientContextImpl::OnImageDecodeError() {
+  io_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &BrowserConnectionHandler::DropCurrentConnection,
+          base::Unretained(net_components_->GetBrowserConnectionHandler())));
 }
 
 void BlimpClientContextImpl::CreateIdentitySource() {
   identity_source_ = base::MakeUnique<IdentitySource>(
       delegate_, base::Bind(&BlimpClientContextImpl::ConnectToAssignmentSource,
                             base::Unretained(this)));
 }
 
 }  // namespace client
 }  // namespace blimp