Pull code associated with a Media PermissionRequest out of MediaStreamDevicesController

chrome/browser/media/webrtc/media_stream_devices_controller.h

Index: chrome/browser/media/webrtc/media_stream_devices_controller.h
diff --git a/chrome/browser/media/webrtc/media_stream_devices_controller.h b/chrome/browser/media/webrtc/media_stream_devices_controller.h
index acd6b4c6647d6521c3f48fce6e7f4355b08db6b4..bcf5ce08ec3b55a7fedfd8ce4732d1485525897c 100644
--- a/chrome/browser/media/webrtc/media_stream_devices_controller.h
+++ b/chrome/browser/media/webrtc/media_stream_devices_controller.h
@@ -33,19 +33,66 @@ namespace test {
 class MediaStreamDevicesControllerTestApi;
 }
 
-namespace internal {
-// Delegate showing permission prompts.
-class PermissionPromptDelegate {
+class MediaStreamDevicesController {
  public:
-  virtual void ShowPrompt(
-      bool user_gesture,
-      content::WebContents* web_contents,
-      std::unique_ptr<MediaStreamDevicesController> controller) = 0;
-};
-}
+  // This class is only needed until we unify the codepaths for permission
+  // requests. It can be removed once crbug.com/606138 is fixed.
+  class Request : public PermissionRequest {
+   public:
+    using PromptAnsweredCallback =
+        base::Callback<void(ContentSetting, bool /* persist */)>;

[Nico, 2017/04/24 13:51:53]

nit: spell this like `/*persist=*/bool` -- clang-format knows about that
convention, and it makes it clear that `/*persist=*/false` at a call site means
"set persist to false" (`false /*persist*/` could also mean "false here means we
persist").

Better yet, use enums instead of bools.

[raymes, 2017/04/26 00:28:14]

Thanks I didn't know that. That was the convention that I had always known and
seen used in Chrome but I'll keep this in mind for the future.

+
+    Request(Profile* profile,
+            bool is_asking_for_audio,
+            bool is_asking_for_video,
+            const GURL& security_origin,
+            PromptAnsweredCallback prompt_answered_callback);
+
+    ~Request() override;
+
+    bool IsAskingForAudio() const;
+    bool IsAskingForVideo() const;
+    base::string16 GetMessageText() const;
+
+    // PermissionRequest:
+    IconId GetIconId() const override;
+    base::string16 GetMessageTextFragment() const override;
+    GURL GetOrigin() const override;
+    void PermissionGranted() override;
+    void PermissionDenied() override;
+    void Cancelled() override;
+    void RequestFinished() override;
+    PermissionRequestType GetPermissionRequestType() const override;
+    bool ShouldShowPersistenceToggle() const override;
+
+   private:
+    Profile* profile_;
+
+    bool is_asking_for_audio_;
+    bool is_asking_for_video_;
+
+    GURL security_origin_;
+
+    PromptAnsweredCallback prompt_answered_callback_;
+
+    // Whether the prompt has been answered.
+    bool responded_;
+
+    DISALLOW_COPY_AND_ASSIGN(Request);
+  };
+
+  // This class is only needed interally and for tests. It can be removed once
+  // crbug.com/606138 is fixed. Delegate for showing permission prompts. It's
+  // only public because subclassing from a friend class doesn't work in gcc
+  // (see https://codereview.chromium.org/2768923003).
+  class PermissionPromptDelegate {

[Nico, 2017/04/24 13:51:53]

std::unique_ptr<PermissionPromptDelegate> delegate_; below deletes this class
through the base pointer, so it must have a virtual dtor, else the dtor of
subclasses won't be called. (Subclasses don't have dtors from what I can tell,
but it's still a latent bug.)

[raymes, 2017/04/26 00:28:14]

Thanks for catching this. Fixed in: https://codereview.chromium.org/2836093006# 

+   public:
+    virtual void ShowPrompt(
+        bool user_gesture,
+        content::WebContents* web_contents,
+        std::unique_ptr<MediaStreamDevicesController::Request> request) = 0;
+  };
 
-class MediaStreamDevicesController : public PermissionRequest {
- public:
   static void RequestPermissions(
       const content::MediaStreamRequest& request,
       const content::MediaResponseCallback& callback);
@@ -53,29 +100,20 @@ class MediaStreamDevicesController : public PermissionRequest {
   // Registers the prefs backing the audio and video policies.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-  ~MediaStreamDevicesController() override;
+  ~MediaStreamDevicesController();
 
   bool IsAskingForAudio() const;
   bool IsAskingForVideo() const;
-  base::string16 GetMessageText() const;
+
+  // Called when a permission prompt has been answered, with the |response| and
+  // whether the choice should be persisted.
+  void PromptAnswered(ContentSetting response, bool persist);
 
 #if defined(OS_ANDROID)
   // Called when the Android OS-level prompt is answered.
   void AndroidOSPromptAnswered(bool allowed);
 #endif  // defined(OS_ANDROID)
 
-  bool ShouldShowPersistenceToggle() const override;
-
-  // PermissionRequest:
-  IconId GetIconId() const override;
-  base::string16 GetMessageTextFragment() const override;
-  GURL GetOrigin() const override;
-  void PermissionGranted() override;
-  void PermissionDenied() override;
-  void Cancelled() override;
-  void RequestFinished() override;
-  PermissionRequestType GetPermissionRequestType() const override;
-
  private:
   friend class MediaStreamDevicesControllerTest;
   friend class test::MediaStreamDevicesControllerTestApi;
@@ -86,7 +124,7 @@ class MediaStreamDevicesController : public PermissionRequest {
   static void RequestPermissionsWithDelegate(
       const content::MediaStreamRequest& request,
       const content::MediaResponseCallback& callback,
-      internal::PermissionPromptDelegate* delegate);
+      PermissionPromptDelegate* delegate);
 
   MediaStreamDevicesController(content::WebContents* web_contents,
                                const content::MediaStreamRequest& request,
@@ -158,7 +196,7 @@ class MediaStreamDevicesController : public PermissionRequest {
   // audio/video devices was granted or not.
   content::MediaResponseCallback callback_;
 
-  std::unique_ptr<internal::PermissionPromptDelegate> delegate_;
+  std::unique_ptr<PermissionPromptDelegate> delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaStreamDevicesController);
 };