Clean up thread handling in Blimp browser tests.

blimp/engine/mojo/blob_channel_service.h

Index: blimp/engine/mojo/blob_channel_service.h
diff --git a/blimp/engine/mojo/blob_channel_service.h b/blimp/engine/mojo/blob_channel_service.h
index a137a0694087e56ab08a372acd940d50c251cd55..e364f6596a6d3499b588f8634b6809126d2122db 100644
--- a/blimp/engine/mojo/blob_channel_service.h
+++ b/blimp/engine/mojo/blob_channel_service.h
@@ -6,22 +6,28 @@
 #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;
 
@@ -30,6 +36,14 @@ class BlobChannelService : public mojom::BlobChannel {
   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;
@@ -41,7 +55,12 @@ class BlobChannelService : public mojom::BlobChannel {
   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);
 };