[Media Router] Use DialMediaSinkService in MediaRouterMojoImpl

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

 // Copyright 2017 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 CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_DIAL_DIAL_MEDIA_SINK_SERVICE_H_
 #define CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_DIAL_DIAL_MEDIA_SINK_SERVICE_H_
 
 #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 {
 
 class DeviceDescriptionService;
 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;
 
   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
   // exists.
   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;
 
   // Called when description service successfully fetches and parses device
   // description XML. Restart |finish_timer_| if it is not running.
   void OnDeviceDescriptionAvailable(
       const DialDeviceData& device_data,
       const ParsedDialDeviceDescription& description_data);
 
   // Called when fails to fetch or parse device description XML.
   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
   // 3 seconds later. If |OnDeviceDescriptionAvailable()| is called after
   // |finish_timer_| expires, |finish_timer_| is restarted.
   std::unique_ptr<base::OneShotTimer> finish_timer_;
 
   std::unique_ptr<DeviceDescriptionService> description_service_;
 
   // Sorted sinks from current round of discovery.
   std::set<MediaSinkInternal> current_sinks_;
 
   // Sorted sinks sent to Media Router Provider in last FetchCompleted().
   std::set<MediaSinkInternal> mrp_sinks_;
 
   // Device data list from current round of discovery.
   DialRegistry::DeviceList current_devices_;
 
   scoped_refptr<net::URLRequestContextGetter> request_context_;
 };
 
 }  // namespace media_router
 
 #endif  // CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_DIAL_DIAL_MEDIA_SINK_SERVICE_H_