[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl

chrome/browser/media/router/discovery/dial/dial_media_sink_service.h

Index: chrome/browser/media/router/discovery/dial/dial_media_sink_service.h
diff --git a/chrome/browser/media/router/discovery/dial/dial_media_sink_service.h b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.h
index 86f72c5beba6118eb17052fe0f245a9b357fa052..723ccb661ff851d89111767a1018f653bd2759fe 100644
--- a/chrome/browser/media/router/discovery/dial/dial_media_sink_service.h
+++ b/chrome/browser/media/router/discovery/dial/dial_media_sink_service.h
@@ -8,10 +8,12 @@
 #include <memory>
 #include <set>
 
+#include "base/memory/ref_counted.h"
 #include "chrome/browser/media/router/discovery/dial/device_description_service.h"
 #include "chrome/browser/media/router/discovery/dial/dial_registry.h"
 #include "chrome/common/media_router/discovery/media_sink_internal.h"
 #include "chrome/common/media_router/discovery/media_sink_service.h"
+#include "content/public/browser/browser_thread.h"
 
 namespace media_router {
 
@@ -20,13 +22,16 @@ class DialRegistry;
 
 // A service which can be used to start background discovery and resolution of
 // DIAL devices (Smart TVs, Game Consoles, etc.).
-// This class is not thread safe. All methods must be called from the IO thread.
-class DialMediaSinkService : public MediaSinkService,
+// This class is thread safe. It is created on UI thread and destroyed on IO

[Kevin M, 2017/05/03 21:07:27]

* If it was threadsafe then one could use it on any thread, which is the
opposite of how this is implemented? If that's the case, then what about using
"thread affine" instead of "thread safe"?

* Add comments regarding thread affinity to the method calls themselves.

[zhaobin, 2017/05/04 23:52:35]

Methods in this class can be invoked on both UI and IO thread so it is thread
safe? Remove the second half to avoid confusion.

+// thread.
+class DialMediaSinkService : public base::RefCountedThreadSafe<

[Kevin M, 2017/05/03 21:07:27]

totally low pri nit: move RefCountedThreadSafe to the end of the list for
readability. IMO it's the least interesting override from a reader's POV -
they're going to be more interested in the base classes that affect the class'
method signatures.

[zhaobin, 2017/05/04 23:52:35]

Done.

+                                 DialMediaSinkService,
+                                 content::BrowserThread::DeleteOnIOThread>,
+                             public MediaSinkService,
                              public DialRegistry::Observer {
  public:
   DialMediaSinkService(const OnSinksDiscoveredCallback& callback,
                        net::URLRequestContextGetter* request_context);
-  ~DialMediaSinkService() override;
 
   // MediaSinkService implementation
   void Start() override;
@@ -34,6 +39,9 @@ class DialMediaSinkService : public MediaSinkService,
   void Stop();

[Kevin M, 2017/05/03 21:07:27]

Move non-overrides above overrides.

[zhaobin, 2017/05/04 23:52:35]

Done.

 
  protected:
+  ~DialMediaSinkService() override;
+
+  // Returns instance of device registry.

[Kevin M, 2017/05/03 21:07:27]

Document why this is protected virtual? (testing, right?)

Nit: it might be better to set these via ForTest() injection methods instead of
defining test subclasses (composition preferred over inheritance)

[zhaobin, 2017/05/04 23:52:35]

Done.

   virtual DialRegistry* dial_registry();
 
   // Returns instance of device description service. Create a new one if none
@@ -41,13 +49,26 @@ class DialMediaSinkService : public MediaSinkService,
   virtual DeviceDescriptionService* GetDescriptionService();
 
  private:
+  friend struct content::BrowserThread::DeleteOnThread<
+      content::BrowserThread::IO>;
+  friend class base::DeleteHelper<DialMediaSinkService>;
   friend class DialMediaSinkServiceTest;
   FRIEND_TEST_ALL_PREFIXES(DialMediaSinkServiceTest, TestStart);
-  FRIEND_TEST_ALL_PREFIXES(DialMediaSinkServiceTest, TestFetchCompleted);
+  FRIEND_TEST_ALL_PREFIXES(DialMediaSinkServiceTest,
+                           TestFetchCompleted_SameSink);
   FRIEND_TEST_ALL_PREFIXES(DialMediaSinkServiceTest, TestIsDifferent);
   FRIEND_TEST_ALL_PREFIXES(DialMediaSinkServiceTest,
                            TestOnDeviceDescriptionAvailable);
 
+  // Start discovery on IO thread.
+  void StartOnIOThread();
+
+  // Stop discovery on IO thread.
+  void StopOnIOThread();
+
+  // Called when |finish_timer_| expires.
+  void OnFetchCompletedOnUIThread();
+
   // api::dial::DialRegistry::Observer implementation
   void OnDialDeviceEvent(const DialRegistry::DeviceList& devices) override;
   void OnDialError(DialRegistry::DialErrorCode type) override;
@@ -62,7 +83,7 @@ class DialMediaSinkService : public MediaSinkService,
   void OnDeviceDescriptionError(const DialDeviceData& device,
                                 const std::string& error_message);
 
-  // Called when |finish_timer_| expires.
+  // Called when |finish_timer_| expires and post task on UI thread.
   void OnFetchCompleted();
 
   // Timer for finishing fetching. Starts in |OnDialDeviceEvent()|, and expires