Clean up thread handling in Blimp browser tests.

blimp/engine/mojo/blob_channel_service.h

 // 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_ENGINE_MOJO_BLOB_CHANNEL_SERVICE_H_
 #define BLIMP_ENGINE_MOJO_BLOB_CHANNEL_SERVICE_H_
 
 #include <string>
+#include <vector>
 
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
 #include "blimp/engine/mojo/blob_channel.mojom.h"
+#include "blimp/net/blob_channel/blob_channel_sender.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 
 namespace blimp {
-
-class BlobChannelSender;
-
 namespace engine {
 
 // Service for processing BlobChannel requests from the renderer.
-// Runs on the browser process.
+// Runs on the IO thread of the browser process.

[Wez, 2016/08/19 19:15:50]

nit: Is the caller responsible for calling it on the IO thread, or does it punt
work to the IO thread internally? Not clear from the context and less obvious
given the UI thread comments below.

[Kevin M, 2016/08/19 21:43:33]

Done.

+// Calls to the UI thread are executed using the UI thread's TaskRunner.

[Wez, 2016/08/19 19:15:50]

nit: This comment seems tautologous; how else would you call to the UI thread
but via a TaskRunner? :P

[Kevin M, 2016/08/19 21:43:33]

Acknowledged.

 class BlobChannelService : public mojom::BlobChannel {
  public:
-  // |blob_channel_sender| must outlive the Mojo connection.
-  explicit BlobChannelService(BlobChannelSender* blob_channel_sender);
+  // |blob_channel_sender|: The BlobChannelSender object which will receive
+  // blobs and answer delivery status queries.
+  // |task_runner|: The UI thread task runner.

[Wez, 2016/08/19 19:15:50]

Can we instead clarify _why_ the caller needs to provide the UI task runner?
What will it be used for?

[Kevin M, 2016/08/19 21:43:33]

Done.

+  BlobChannelService(
+      base::WeakPtr<BlobChannelSender> blob_channel_sender,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);

[Wez, 2016/08/19 19:15:50]

ui_task_runner?

Here and above, though, the desired property is that this is the task-runner
that the blob-channel-sender must be called on, so I'd suggest describing it in
those terms and naming it accordingly, rather than assuming the UI thread.

[Kevin M, 2016/08/19 21:43:33]

Done.

[Wez, 2016/08/20 00:40:24]

Is it necessary that this is all on the UI thread? Or should we call it
blob_channel_sender_task_runner or equivalent?

[Kevin M, 2016/08/22 19:46:56]

Done.

 
   ~BlobChannelService() override;
 
   // Factory method called by Mojo.
   // Binds |this| to the connection specified by |request|.
   void BindRequest(mojo::InterfaceRequest<mojom::BlobChannel> request);
 
  private:
+  std::vector<BlobChannelSender::CacheStateEntry> GetCachedBlobIdsOnUiThread();
+
+  // Called when the list of blob IDs arrives from the BlobChannelSender on the
+  // UI thread.

[Wez, 2016/08/19 19:15:50]

IIUC they "arrive" only because we request them, so I think it'd be clearer to
say "Used to receive the results of BlobChannelSender::GetCachedBlobs()
asynchronously from the BlobChannelSender's thread" or similar.

Suggest renaming to OnGetCachedBlobsCompleted().

[Kevin M, 2016/08/19 21:43:33]

Done.

+  void ReceivedBlobIds(
+      const BlobChannelService::GetCachedBlobIdsCallback& response_callback,
+      const std::vector<BlobChannelSender::CacheStateEntry>& ids);
+
   // BlobChannel implementation.
   void GetCachedBlobIds(
       const GetCachedBlobIdsCallback& response_callback) override;
   void PutBlob(const std::string& id,
                mojo::ScopedSharedBufferHandle data,
                uint32_t size) override;
   void DeliverBlob(const std::string& id) override;
 
   mojo::BindingSet<mojom::BlobChannel> bindings_;
 
   // Sender object which will receive the blobs passed over the Mojo service.
-  BlobChannelSender* blob_channel_sender_;
+  base::WeakPtr<BlobChannelSender> blob_channel_sender_;
+
+  // UI thread TaskRunner.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(BlobChannelService);
 };
 
 }  // namespace engine
 }  // namespace blimp
 
 #endif  // BLIMP_ENGINE_MOJO_BLOB_CHANNEL_SERVICE_H_