Combine PictureInPictureController::EnterPictureInPicture methods

There used to be a video EnterPictureInPicture method
and an element EnterPictureInPicture method. However,
this resulted in callers accidentally calling the wrong
overload since the video element is derived from element.
Therefore this combines the two methods to make it more
robust.

BUG=959778

Change-Id: Icc5272d6b3f667af6719fd4cd15ae3b660da13a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596220
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657359}

third_party/blink/renderer/core/frame/picture_in_picture_controller.h

@@ -49,12 +49,8 @@ class CORE_EXPORT PictureInPictureController
     kDisabledByAttribute,
   };
 
-  // Enter Picture-in-Picture for a video element and resolve promise if any.
-  virtual void EnterPictureInPicture(HTMLVideoElement*,
-                                     ScriptPromiseResolver*) = 0;
-
-  // Enter Picture-in-Picture for an element (except a video element) with
-  // options if any and resolve promise if any.
+  // Enter Picture-in-Picture for an element with options if any and resolve
+  // promise if any.
   virtual void EnterPictureInPicture(HTMLElement*,
                                      PictureInPictureOptions*,
                                      ScriptPromiseResolver*) = 0;

third_party/blink/renderer/core/html/media/video_wake_lock_test.cc

@@ -116,7 +116,8 @@ class VideoWakeLockTest : public PageTestBase {
 
   void SimulateEnterPictureInPicture() {
     PictureInPictureController::From(GetDocument())
-        .EnterPictureInPicture(Video(), nullptr);
+        .EnterPictureInPicture(Video(), nullptr /* options */,
+                               nullptr /* promise */);
 
     MakeGarbageCollected<WaitForEvent>(
         video_.Get(), event_type_names::kEnterpictureinpicture);

third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.cc

@@ -86,10 +86,13 @@ void MediaControlPictureInPictureButtonElement::DefaultEventHandler(
 
     DCHECK(MediaElement().IsHTMLVideoElement());
     HTMLVideoElement* video_element = &ToHTMLVideoElement(MediaElement());
-    if (PictureInPictureController::IsElementInPictureInPicture(video_element))
+    if (PictureInPictureController::IsElementInPictureInPicture(
+            video_element)) {
       controller.ExitPictureInPicture(video_element, nullptr);
-    else
-      controller.EnterPictureInPicture(video_element, nullptr);
+    } else {
+      controller.EnterPictureInPicture(video_element, nullptr /* options */,
+                                       nullptr /* promise */);
+    }
   }
 
   MediaControlInputElement::DefaultEventHandler(event);

third_party/blink/renderer/modules/picture_in_picture/DEPS

@@ -4,4 +4,5 @@ include_rules = [
     "+third_party/blink/renderer/modules/event_target_modules.h",
     "+third_party/blink/renderer/modules/modules_export.h",
     "+third_party/blink/renderer/modules/picture_in_picture",
+    "+third_party/blink/renderer/core/frame/web_local_frame_impl.h",
 ]

third_party/blink/renderer/modules/picture_in_picture/html_video_element_picture_in_picture.cc

@@ -28,7 +28,7 @@ ScriptPromise HTMLVideoElementPictureInPicture::requestPictureInPicture(
   auto promise = resolver->Promise();
 
   PictureInPictureControllerImpl::From(element.GetDocument())
-      .EnterPictureInPicture(&element, resolver);
+      .EnterPictureInPicture(&element, nullptr /* options */, resolver);
 
   return promise;
 }

third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.cc

@@ -111,11 +111,23 @@ PictureInPictureControllerImpl::IsElementAllowed(
 }
 
 void PictureInPictureControllerImpl::EnterPictureInPicture(
-    HTMLVideoElement* element,
+    HTMLElement* element,
+    PictureInPictureOptions* options,
     ScriptPromiseResolver* resolver) {
-  DCHECK(element->GetWebMediaPlayer());
+  if (!IsVideoElement(*element)) {
+    // TODO(https://crbug.com/953957): Support element level pip.
+    if (resolver)
+      resolver->Resolve();
+
+    return;
+  }
+
+  HTMLVideoElement* video_element = static_cast<HTMLVideoElement*>(element);
 
-  if (picture_in_picture_element_ == element) {
+  DCHECK(video_element->GetWebMediaPlayer());
+  DCHECK(!options);
+
+  if (picture_in_picture_element_ == video_element) {
     if (resolver)
       resolver->Resolve(picture_in_picture_window_);
 
@@ -125,31 +137,22 @@ void PictureInPictureControllerImpl::EnterPictureInPicture(
   if (!EnsureService())
     return;
 
-  if (element->DisplayType() == WebMediaPlayer::DisplayType::kFullscreen)
+  if (video_element->DisplayType() == WebMediaPlayer::DisplayType::kFullscreen)
     Fullscreen::ExitFullscreen(*GetSupplementable());
 
-  element->GetWebMediaPlayer()->OnRequestPictureInPicture();
+  video_element->GetWebMediaPlayer()->OnRequestPictureInPicture();
 
   picture_in_picture_service_->StartSession(
-      element->GetWebMediaPlayer()->GetDelegateId(),
-      element->GetWebMediaPlayer()->GetSurfaceId(),
-      element->GetWebMediaPlayer()->NaturalSize(),
-      ShouldShowPlayPauseButton(*element), ShouldShowMuteButton(*element),
+      video_element->GetWebMediaPlayer()->GetDelegateId(),
+      video_element->GetWebMediaPlayer()->GetSurfaceId(),
+      video_element->GetWebMediaPlayer()->NaturalSize(),
+      ShouldShowPlayPauseButton(*video_element),
+      ShouldShowMuteButton(*video_element),
       WTF::Bind(&PictureInPictureControllerImpl::OnEnteredPictureInPicture,
-                WrapPersistent(this), WrapPersistent(element),
+                WrapPersistent(this), WrapPersistent(video_element),
                 WrapPersistent(resolver)));
 }
 
-void PictureInPictureControllerImpl::EnterPictureInPicture(
-    HTMLElement* element,
-    PictureInPictureOptions* options,
-    ScriptPromiseResolver* resolver) {
-  DCHECK(!IsVideoElement(*element));
-
-  // TODO(https://crbug.com/953957): Support element level pip.
-  resolver->Resolve();
-}
-
 void PictureInPictureControllerImpl::OnEnteredPictureInPicture(
     HTMLVideoElement* element,
     ScriptPromiseResolver* resolver,
@@ -331,7 +334,8 @@ void PictureInPictureControllerImpl::PageVisibilityChanged() {
   // If page becomes hidden and entering Auto Picture-in-Picture is allowed,
   // enter Picture-in-Picture.
   if (GetSupplementable()->hidden() && IsEnterAutoPictureInPictureAllowed()) {
-    EnterPictureInPicture(AutoPictureInPictureElement(), nullptr /* promise */);
+    EnterPictureInPicture(AutoPictureInPictureElement(), nullptr /* options */,
+                          nullptr /* promise */);
   }
 }
 

third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.h

@@ -70,8 +70,6 @@ class MODULES_EXPORT PictureInPictureControllerImpl
   bool IsExitAutoPictureInPictureAllowed() const;
 
   // Implementation of PictureInPictureController.
-  void EnterPictureInPicture(HTMLVideoElement*,
-                             ScriptPromiseResolver*) override;
   void EnterPictureInPicture(HTMLElement*,
                              PictureInPictureOptions*,
                              ScriptPromiseResolver*) override;

third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_test.cc

@@ -9,7 +9,10 @@
 #include "third_party/blink/public/mojom/picture_in_picture/picture_in_picture.mojom-blink.h"
 #include "third_party/blink/public/platform/web_media_stream.h"
 #include "third_party/blink/public/platform/web_media_stream_track.h"
+#include "third_party/blink/public/web/web_media_player_action.h"
 #include "third_party/blink/renderer/core/dom/events/event.h"
+#include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
+#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
 #include "third_party/blink/renderer/core/html/media/html_media_test_helper.h"
 #include "third_party/blink/renderer/core/html/media/html_video_element.h"
 #include "third_party/blink/renderer/core/testing/page_test_base.h"
@@ -168,7 +171,8 @@ TEST_F(PictureInPictureControllerTest, EnterPictureInPictureFiresEvent) {
   EXPECT_CALL(Service(), SetDelegate(_));
 
   PictureInPictureControllerImpl::From(GetDocument())
-      .EnterPictureInPicture(Video(), nullptr);
+      .EnterPictureInPicture(Video(), nullptr /* options */,
+                             nullptr /* promise */);
 
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kEnterpictureinpicture);
@@ -192,12 +196,15 @@ TEST_F(PictureInPictureControllerTest, ExitPictureInPictureFiresEvent) {
   EXPECT_CALL(Service(), SetDelegate(_));
 
   PictureInPictureControllerImpl::From(GetDocument())
-      .EnterPictureInPicture(Video(), nullptr);
+      .EnterPictureInPicture(Video(), nullptr /* options */,
+                             nullptr /* promise */);
+
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kEnterpictureinpicture);
 
   PictureInPictureControllerImpl::From(GetDocument())
       .ExitPictureInPicture(Video(), nullptr);
+
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kLeavepictureinpicture);
 
@@ -217,7 +224,8 @@ TEST_F(PictureInPictureControllerTest, StartObserving) {
   EXPECT_CALL(Service(), SetDelegate(_));
 
   PictureInPictureControllerImpl::From(GetDocument())
-      .EnterPictureInPicture(Video(), nullptr);
+      .EnterPictureInPicture(Video(), nullptr /* options */,
+                             nullptr /* promise */);
 
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kEnterpictureinpicture);
@@ -243,7 +251,8 @@ TEST_F(PictureInPictureControllerTest, StopObserving) {
   EXPECT_CALL(Service(), SetDelegate(_));
 
   PictureInPictureControllerImpl::From(GetDocument())
-      .EnterPictureInPicture(Video(), nullptr);
+      .EnterPictureInPicture(Video(), nullptr /* options */,
+                             nullptr /* promise */);
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kEnterpictureinpicture);
 
@@ -270,7 +279,8 @@ TEST_F(PictureInPictureControllerTest, PlayPauseButton_InfiniteDuration) {
   EXPECT_CALL(Service(), SetDelegate(_));
 
   PictureInPictureControllerImpl::From(GetDocument())
-      .EnterPictureInPicture(Video(), nullptr);
+      .EnterPictureInPicture(Video(), nullptr /* options */,
+                             nullptr /* promise */);
 
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kEnterpictureinpicture);
@@ -293,7 +303,8 @@ TEST_F(PictureInPictureControllerTest, PlayPauseButton_MediaSource) {
   EXPECT_CALL(Service(), SetDelegate(_));
 
   PictureInPictureControllerImpl::From(GetDocument())
-      .EnterPictureInPicture(Video(), nullptr);
+      .EnterPictureInPicture(Video(), nullptr /* options */,
+                             nullptr /* promise */);
 
   MakeGarbageCollected<WaitForEvent>(Video(),
                                      event_type_names::kEnterpictureinpicture);
@@ -302,4 +313,23 @@ TEST_F(PictureInPictureControllerTest, PlayPauseButton_MediaSource) {
   test::RunPendingTasks();
 }
 
+TEST_F(PictureInPictureControllerTest, PerformMediaPlayerAction) {
+  frame_test_helpers::WebViewHelper helper;
+  helper.Initialize();
+
+  WebLocalFrameImpl* frame = helper.LocalMainFrame();
+  Document* document = frame->GetFrame()->GetDocument();
+
+  Persistent<HTMLVideoElement> video =
+      MakeGarbageCollected<HTMLVideoElement>(*document);
+  document->body()->AppendChild(video);
+
+  IntPoint bounds = video->BoundsInViewport().Center();
+
+  frame->PerformMediaPlayerAction(
+      WebPoint(bounds.X(), bounds.Y()),
+      WebMediaPlayerAction(WebMediaPlayerAction::Type::kPictureInPicture,
+                           true));
+}
+
 }  // namespace blink