media: Simplify MediaPermissionDispatcher.

content/renderer/media/media_permission_dispatcher.h

Index: content/renderer/media/media_permission_dispatcher.h
diff --git a/content/renderer/media/media_permission_dispatcher.h b/content/renderer/media/media_permission_dispatcher.h
index 701afe6d088987a5c94754a1a0cd396ab1a13f11..4af23e5b09b76a3e41d8d22e115e182f43f09c4b 100644
--- a/content/renderer/media/media_permission_dispatcher.h
+++ b/content/renderer/media/media_permission_dispatcher.h
@@ -2,46 +2,62 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Base class for MediaPermission implementations in content. This class allows
-// multiple pending PermissionService calls by managing callbacks for its
-// subclasses. This class is not thread safe and should only be used on one
-// thread.
-
 #ifndef CONTENT_RENDERER_MEDIA_MEDIA_PERMISSION_DISPATCHER_H_
 #define CONTENT_RENDERER_MEDIA_MEDIA_PERMISSION_DISPATCHER_H_
 
 #include <map>
 
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/threading/thread_checker.h"
 #include "content/common/content_export.h"
+#include "content/common/permission_service.mojom.h"
+#include "content/public/renderer/render_frame_observer.h"
 #include "media/base/media_permission.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}
+
 namespace content {
 
-class CONTENT_EXPORT MediaPermissionDispatcher : public media::MediaPermission {
+// MediaPermission implementation.
+class CONTENT_EXPORT MediaPermissionDispatcher : public media::MediaPermission,
+                                                 public RenderFrameObserver {
  public:
-  MediaPermissionDispatcher();
+  explicit MediaPermissionDispatcher(RenderFrame* render_frame);
   ~MediaPermissionDispatcher() override;
 
- protected:
-  // Register callbacks for PermissionService calls.
-  uint32_t RegisterCallback(const PermissionStatusCB& permission_status_cb);
-
-  // Deliver the permission status |granted| to the callback identified by
-  // |request_id|.
-  void DeliverResult(uint32_t request_id, bool granted);
-
-  base::ThreadChecker& thread_checker() { return thread_checker_; }
+  // media::MediaPermission implementation.
+  // Note: Can be called on any thread. The |permission_status_cb| will always
+  // be fired on the thread where these methods are called.
+  void HasPermission(Type type,
+                     const GURL& security_origin,
+                     const PermissionStatusCB& permission_status_cb) override;
+  void RequestPermission(
+      Type type,
+      const GURL& security_origin,
+      const PermissionStatusCB& permission_status_cb) override;
 
  private:
   // Map of request IDs and pending PermissionStatusCBs.
   typedef std::map<uint32_t, PermissionStatusCB> RequestMap;
 
-  base::ThreadChecker thread_checker_;
+  // Register PermissionStatusCBs. Returns |request_id| that can be used to make
+  // PermissionService calls.
+  uint32_t RegisterCallback(const PermissionStatusCB& permission_status_cb);
+
+  // Callback for |permission_service_| calls.
+  void OnPermissionStatus(uint32_t request_id, PermissionStatus status);
 
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   uint32_t next_request_id_;
   RequestMap requests_;
+  PermissionServicePtr permission_service_;
+
+  // Bound to the |task_runner_|.

[Wez, 2015/11/25 02:16:20]

This doesn't know anything about "the |task_runner_|", so this is misleading.

If you want a comment to remind you that it can only be de-referenced on that
task runner's thread then I'd suggest something more explanatory like "Used to
safely post MediaPermission calls for execution on  |task_runner_|."

However, as previously noted, the "Can be called on any thread" comment earlier
on is highly suspect - there's nothing in this implementation that hints as to
why code on other threads can safely assume that this object is still valid when
call it in the first place.

[xhwang, 2015/11/25 07:06:03]

Done.
Updated the comment. Again ISTM the caller need to make sure |this| is valid
when
the call is made.

guoweis / sergeyu: It seems the permission is passed in
PeerConnectionDependencyFactory::CreatePeerConnection() which is RenderFrame
specific. Does that mean the peer connection created is owned by
RenderFrameImpl? What does the lifetime look like here? How does things work
when the P2P code needs to check permission only to find that the permission
service is already dead?

[guoweis_left_chromium, 2015/11/25 15:38:54]

+tommi to the review. RTCPeerConnection is eventually owned by the javascript
world although the exact relationship b/w it and RenderFrame is not clear to me.
Tommi, do you know if there is any situation that PC could outlive the
associated RenderFrame? We currently get a MediaPermission for mic/camera
permission check which is owned by RenderFrame. Can it still hold a pointer to
an invalid MediaPermission if the RenderFrame is deleted?

+  base::WeakPtr<MediaPermissionDispatcher> weak_this_;

[Wez, 2015/11/25 02:16:19]

nit: Convention elsewhere is to call these |weak_ptr_|.

[xhwang, 2015/11/25 07:06:03]

Done.

+
+  base::WeakPtrFactory<MediaPermissionDispatcher> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaPermissionDispatcher);
 };