Picture-in-Picture: remove/rename some methods in player/video element.

Mostly follow-ups from https://chromium-review.googlesource.com/c/chromium/src/+/1379049

Some methods became pointless, some had their meaning significantly
changed.

This CL also changes the timing of requestPictureInPicture() and exitPictureInPicture()
in order to be sync until the call to the service.

Bug: 919860, 930338
Change-Id: I437a060c2dd92021d514c4d6e2ffeb9f4642525b
Reviewed-on: https://chromium-review.googlesource.com/c/1409544
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#631517}
diff --git a/chrome/browser/apps/platform_apps/app_browsertest.cc b/chrome/browser/apps/platform_apps/app_browsertest.cc
index db83368..14db9cd 100644
--- a/chrome/browser/apps/platform_apps/app_browsertest.cc
+++ b/chrome/browser/apps/platform_apps/app_browsertest.cc
@@ -1420,10 +1420,7 @@
   ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
       web_contents, "exitPictureInPicture();", &result));
   EXPECT_TRUE(result);
-  // TODO(mlamouri): this check is relying on IPC timing. The timing will be
-  // fixed with
-  // https://chromium-review.googlesource.com/c/chromium/src/+/1409544
-  // EXPECT_FALSE(window_controller->GetWindowForTesting()->IsVisible());
+  EXPECT_FALSE(window_controller->GetWindowForTesting()->IsVisible());
 }
 
 }  // namespace extensions
diff --git a/content/renderer/media/stream/webmediaplayer_ms.cc b/content/renderer/media/stream/webmediaplayer_ms.cc
index 3b11c6d..87dd463 100644
--- a/content/renderer/media/stream/webmediaplayer_ms.cc
+++ b/content/renderer/media/stream/webmediaplayer_ms.cc
@@ -647,7 +647,7 @@
   delegate_->DidPlayerMutedStatusChange(delegate_id_, volume == 0.0);
 }
 
-void WebMediaPlayerMS::EnterPictureInPicture() {
+void WebMediaPlayerMS::OnRequestPictureInPicture() {
   if (!bridge_)
     ActivateSurfaceLayerForVideo();
 
@@ -655,12 +655,6 @@
   DCHECK(bridge_->GetSurfaceId().is_valid());
 }
 
-void WebMediaPlayerMS::ExitPictureInPicture() {
-  // Internal cleanups.
-  // TODO(mlamouri): remove the need for this.
-  OnPictureInPictureModeEnded();
-}
-
 void WebMediaPlayerMS::SetPictureInPictureCustomControls(
     const std::vector<blink::PictureInPictureControlInfo>& controls) {
   delegate_->DidSetPictureInPictureCustomControls(delegate_id_, controls);
diff --git a/content/renderer/media/stream/webmediaplayer_ms.h b/content/renderer/media/stream/webmediaplayer_ms.h
index 3fcc581..7b95d0e 100644
--- a/content/renderer/media/stream/webmediaplayer_ms.h
+++ b/content/renderer/media/stream/webmediaplayer_ms.h
@@ -116,8 +116,7 @@
   void Seek(double seconds) override;
   void SetRate(double rate) override;
   void SetVolume(double volume) override;
-  void EnterPictureInPicture() override;
-  void ExitPictureInPicture() override;
+  void OnRequestPictureInPicture() override;
   void SetPictureInPictureCustomControls(
       const std::vector<blink::PictureInPictureControlInfo>&) override;
   void SetSinkId(
diff --git a/content/renderer/media_capture_from_element/html_video_element_capturer_source_unittest.cc b/content/renderer/media_capture_from_element/html_video_element_capturer_source_unittest.cc
index 510dff2..2df724d 100644
--- a/content/renderer/media_capture_from_element/html_video_element_capturer_source_unittest.cc
+++ b/content/renderer/media_capture_from_element/html_video_element_capturer_source_unittest.cc
@@ -50,8 +50,7 @@
   void Seek(double seconds) override {}
   void SetRate(double) override {}
   void SetVolume(double) override {}
-  void EnterPictureInPicture() override {}
-  void ExitPictureInPicture() override {}
+  void OnRequestPictureInPicture() override {}
   void SetPictureInPictureCustomControls(
       const std::vector<blink::PictureInPictureControlInfo>&) override {}
   blink::WebTimeRanges Buffered() const override {
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index 7dcdd0b..d05c969 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -902,7 +902,7 @@
   UpdatePlayState();
 }
 
-void WebMediaPlayerImpl::EnterPictureInPicture() {
+void WebMediaPlayerImpl::OnRequestPictureInPicture() {
   if (!surface_layer_for_video_enabled_)
     ActivateSurfaceLayerForVideo();
 
@@ -910,12 +910,6 @@
   DCHECK(bridge_->GetSurfaceId().is_valid());
 }
 
-void WebMediaPlayerImpl::ExitPictureInPicture() {
-  // Internal cleanups.
-  // TODO(mlamouri): remove the need for this.
-  OnPictureInPictureModeEnded();
-}
-
 void WebMediaPlayerImpl::SetPictureInPictureCustomControls(
     const std::vector<blink::PictureInPictureControlInfo>& controls) {
   delegate_->DidSetPictureInPictureCustomControls(delegate_id_, controls);
diff --git a/media/blink/webmediaplayer_impl.h b/media/blink/webmediaplayer_impl.h
index 7183daa..06ede31 100644
--- a/media/blink/webmediaplayer_impl.h
+++ b/media/blink/webmediaplayer_impl.h
@@ -120,8 +120,7 @@
   void Seek(double seconds) override;
   void SetRate(double rate) override;
   void SetVolume(double volume) override;
-  void EnterPictureInPicture() override;
-  void ExitPictureInPicture() override;
+  void OnRequestPictureInPicture() override;
   void SetPictureInPictureCustomControls(
       const std::vector<blink::PictureInPictureControlInfo>&) override;
   void SetSinkId(
diff --git a/third_party/blink/public/platform/web_media_player.h b/third_party/blink/public/platform/web_media_player.h
index 3808082..820549f 100644
--- a/third_party/blink/public/platform/web_media_player.h
+++ b/third_party/blink/public/platform/web_media_player.h
@@ -150,12 +150,10 @@
   virtual void SetRate(double) = 0;
   virtual void SetVolume(double) = 0;
 
-  // Enter Picture-in-Picture and notifies Blink with window size
-  // when video successfully enters Picture-in-Picture.
-  // TODO(mlamouri): rename to "OnRequestPictureInPicture".
-  virtual void EnterPictureInPicture() = 0;
-  // Exit Picture-in-Picture and notifies Blink when it's done.
-  virtual void ExitPictureInPicture() = 0;
+  // The associated media element is going to enter Picture-in-Picture. This
+  // method should make sure the player is set up for this and has a SurfaceId
+  // as it will be needed.
+  virtual void OnRequestPictureInPicture() = 0;
   // Assign custom controls to the Picture-in-Picture window.
   virtual void SetPictureInPictureCustomControls(
       const std::vector<PictureInPictureControlInfo>&) = 0;
diff --git a/third_party/blink/renderer/core/html/media/html_media_element.h b/third_party/blink/renderer/core/html/media/html_media_element.h
index 0d0e45c..75753e3 100644
--- a/third_party/blink/renderer/core/html/media/html_media_element.h
+++ b/third_party/blink/renderer/core/html/media/html_media_element.h
@@ -371,6 +371,7 @@
   friend class ContextMenuControllerTest;
   friend class MediaElementFillingViewportTest;
   friend class VideoWakeLockTest;
+  friend class PictureInPictureControllerTest;
 
   void ResetMediaPlayerAndMediaSource();
 
diff --git a/third_party/blink/renderer/core/html/media/html_video_element.cc b/third_party/blink/renderer/core/html/media/html_video_element.cc
index 43565f1..6b58137 100644
--- a/third_party/blink/renderer/core/html/media/html_video_element.cc
+++ b/third_party/blink/renderer/core/html/media/html_video_element.cc
@@ -663,14 +663,6 @@
          PictureInPictureController::Status::kEnabled;
 }
 
-void HTMLVideoElement::enterPictureInPicture() {
-  if (DisplayType() == WebMediaPlayer::DisplayType::kFullscreen)
-    Fullscreen::ExitFullscreen(GetDocument());
-
-  if (GetWebMediaPlayer())
-    GetWebMediaPlayer()->EnterPictureInPicture();
-}
-
 void HTMLVideoElement::SendCustomControlsToPipWindow() {
   // TODO(sawtelle): Allow setting controls multiple times for a video, even
   // when not active, https://crbug.com/869133
diff --git a/third_party/blink/renderer/core/html/media/html_video_element.h b/third_party/blink/renderer/core/html/media/html_video_element.h
index b8313c2..9db2894 100644
--- a/third_party/blink/renderer/core/html/media/html_video_element.h
+++ b/third_party/blink/renderer/core/html/media/html_video_element.h
@@ -175,7 +175,6 @@
 
   void MediaRemotingStarted(const WebString& remote_device_friendly_name) final;
   bool SupportsPictureInPicture() const final;
-  void enterPictureInPicture();
   void SendCustomControlsToPipWindow();
   void PictureInPictureStopped() final;
   void PictureInPictureControlClicked(const WebString& control_id) final;
diff --git a/third_party/blink/renderer/core/html/media/video_wake_lock_test.cc b/third_party/blink/renderer/core/html/media/video_wake_lock_test.cc
index bc01a5e..785b17a 100644
--- a/third_party/blink/renderer/core/html/media/video_wake_lock_test.cc
+++ b/third_party/blink/renderer/core/html/media/video_wake_lock_test.cc
@@ -53,6 +53,12 @@
   mojo::Binding<mojom::blink::PictureInPictureService> binding_;
 };
 
+class VideoWakeLockMediaPlayer final : public EmptyWebMediaPlayer {
+ public:
+  ReadyState GetReadyState() const final { return kReadyStateHaveMetadata; }
+  bool HasVideo() const final { return true; }
+};
+
 class VideoWakeLockFrameClient : public test::MediaStubLocalFrameClient {
  public:
   static VideoWakeLockFrameClient* Create(
@@ -109,7 +115,7 @@
   void SetUp() override {
     PageTestBase::SetupPageWithClients(
         nullptr, VideoWakeLockFrameClient::Create(
-                     std::make_unique<EmptyWebMediaPlayer>()));
+                     std::make_unique<VideoWakeLockMediaPlayer>()));
 
     service_manager::InterfaceProvider::TestApi test_api(
         GetFrame().Client()->GetInterfaceProvider());
@@ -119,6 +125,7 @@
                            WTF::Unretained(&pip_service_)));
 
     video_ = HTMLVideoElement::Create(GetDocument());
+    video_->SetReadyState(HTMLMediaElement::ReadyState::kHaveMetadata);
     video_wake_lock_ = MakeGarbageCollected<VideoWakeLock>(*video_.Get());
 
     GetPage().SetIsHidden(false, true);
diff --git a/third_party/blink/renderer/modules/picture_in_picture/document_picture_in_picture.cc b/third_party/blink/renderer/modules/picture_in_picture/document_picture_in_picture.cc
index cfd09a8..0cb6b586 100644
--- a/third_party/blink/renderer/modules/picture_in_picture/document_picture_in_picture.cc
+++ b/third_party/blink/renderer/modules/picture_in_picture/document_picture_in_picture.cc
@@ -45,14 +45,8 @@
   ScriptPromise promise = resolver->Promise();
 
   DCHECK(IsHTMLVideoElement(picture_in_picture_element));
-  document.GetTaskRunner(TaskType::kMediaElementEvent)
-      ->PostTask(
-          FROM_HERE,
-          WTF::Bind(
-              &PictureInPictureControllerImpl::ExitPictureInPicture,
-              WrapPersistent(&controller),
-              WrapPersistent(ToHTMLVideoElement(picture_in_picture_element)),
-              WrapPersistent(resolver)));
+  controller.ExitPictureInPicture(
+      ToHTMLVideoElement(picture_in_picture_element), resolver);
   return promise;
 }
 
diff --git a/third_party/blink/renderer/modules/picture_in_picture/html_video_element_picture_in_picture.cc b/third_party/blink/renderer/modules/picture_in_picture/html_video_element_picture_in_picture.cc
index 486889e..cac49b5 100644
--- a/third_party/blink/renderer/modules/picture_in_picture/html_video_element_picture_in_picture.cc
+++ b/third_party/blink/renderer/modules/picture_in_picture/html_video_element_picture_in_picture.cc
@@ -95,13 +95,7 @@
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
   ScriptPromise promise = resolver->Promise();
 
-  document.GetTaskRunner(TaskType::kMediaElementEvent)
-      ->PostTask(
-          FROM_HERE,
-          WTF::Bind(&PictureInPictureControllerImpl::EnterPictureInPicture,
-                    WrapPersistent(&controller), WrapPersistent(&element),
-                    WrapPersistent(resolver)));
-
+  controller.EnterPictureInPicture(&element, resolver);
   return promise;
 }
 
diff --git a/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.cc b/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.cc
index 3b96446..4da05d4 100644
--- a/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.cc
+++ b/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.cc
@@ -16,6 +16,7 @@
 #include "third_party/blink/renderer/core/dom/events/event.h"
 #include "third_party/blink/renderer/core/events/picture_in_picture_control_event.h"
 #include "third_party/blink/renderer/core/frame/settings.h"
+#include "third_party/blink/renderer/core/fullscreen/fullscreen.h"
 #include "third_party/blink/renderer/core/html/media/html_video_element.h"
 #include "third_party/blink/renderer/modules/picture_in_picture/enter_picture_in_picture_event.h"
 #include "third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_window.h"
@@ -99,6 +100,8 @@
 void PictureInPictureControllerImpl::EnterPictureInPicture(
     HTMLVideoElement* element,
     ScriptPromiseResolver* resolver) {
+  DCHECK(element->GetWebMediaPlayer());
+
   if (picture_in_picture_element_ == element) {
     if (resolver)
       resolver->Resolve(picture_in_picture_window_);
@@ -106,13 +109,14 @@
     return;
   }
 
-  element->enterPictureInPicture();
-
-  DCHECK(element->GetWebMediaPlayer());
-
   if (!EnsureService())
     return;
 
+  if (element->DisplayType() == WebMediaPlayer::DisplayType::kFullscreen)
+    Fullscreen::ExitFullscreen(*GetSupplementable());
+
+  element->GetWebMediaPlayer()->OnRequestPictureInPicture();
+
   picture_in_picture_service_->StartSession(
       element->GetWebMediaPlayer()->GetDelegateId(),
       element->GetWebMediaPlayer()->GetSurfaceId(),
@@ -131,7 +135,7 @@
     HTMLVideoElement* element,
     ScriptPromiseResolver* resolver,
     const WebSize& picture_in_picture_window_size) {
-  if (IsElementAllowed(*element) == Status::kDisabledByAttribute) {
+  if (IsElementAllowed(*element) != Status::kEnabled) {
     if (resolver) {
       resolver->Reject(
           DOMException::Create(DOMExceptionCode::kInvalidStateError, ""));
@@ -173,9 +177,6 @@
 void PictureInPictureControllerImpl::ExitPictureInPicture(
     HTMLVideoElement* element,
     ScriptPromiseResolver* resolver) {
-  if (element->GetWebMediaPlayer())
-    element->GetWebMediaPlayer()->ExitPictureInPicture();
-
   if (!EnsureService())
     return;
 
diff --git a/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_test.cc b/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_test.cc
index 05239c5..10b7da8 100644
--- a/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_test.cc
+++ b/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_test.cc
@@ -120,7 +120,6 @@
   DISALLOW_COPY_AND_ASSIGN(PictureInPictureControllerFrameClient);
 };
 
-// TODO: can probably be removed.
 class PictureInPictureControllerPlayer : public EmptyWebMediaPlayer {
  public:
   PictureInPictureControllerPlayer() = default;
@@ -132,6 +131,9 @@
     return EmptyWebMediaPlayer::Duration();
   }
 
+  ReadyState GetReadyState() const final { return kReadyStateHaveMetadata; }
+  bool HasVideo() const final { return true; }
+
   void set_infinity_duration(bool value) { infinity_duration_ = value; }
 
  private:
@@ -155,6 +157,7 @@
                            WTF::Unretained(&mock_service_)));
 
     video_ = HTMLVideoElement::Create(GetDocument());
+    video_->SetReadyState(HTMLMediaElement::ReadyState::kHaveMetadata);
     layer_ = cc::Layer::Create();
     video_->SetCcLayerForTesting(layer_.get());
 
diff --git a/third_party/blink/renderer/platform/testing/empty_web_media_player.h b/third_party/blink/renderer/platform/testing/empty_web_media_player.h
index fea2a40..8e268e9 100644
--- a/third_party/blink/renderer/platform/testing/empty_web_media_player.h
+++ b/third_party/blink/renderer/platform/testing/empty_web_media_player.h
@@ -28,8 +28,7 @@
   void Seek(double seconds) override {}
   void SetRate(double) override {}
   void SetVolume(double) override {}
-  void EnterPictureInPicture() override {}
-  void ExitPictureInPicture() override {}
+  void OnRequestPictureInPicture() override {}
   void SetPictureInPictureCustomControls(
       const std::vector<PictureInPictureControlInfo>&) override {}
   SurfaceLayerMode GetVideoSurfaceLayerMode() const override {
diff --git a/third_party/blink/web_tests/external/wpt/picture-in-picture/disable-picture-in-picture.html b/third_party/blink/web_tests/external/wpt/picture-in-picture/disable-picture-in-picture.html
index 31e0d12..5075c01 100644
--- a/third_party/blink/web_tests/external/wpt/picture-in-picture/disable-picture-in-picture.html
+++ b/third_party/blink/web_tests/external/wpt/picture-in-picture/disable-picture-in-picture.html
@@ -47,7 +47,9 @@
   return requestPictureInPictureWithTrustedClick(video)
   .then(() => {
     video.disablePictureInPicture = true;
-    assert_equals(document.pictureInPictureElement, null);
+    video.addEventListener('leavepictureinpicture', t.step_func(() => {
+      assert_equals(document.pictureInPictureElement, null);
+    }));
   });
 }, 'pictureInPictureElement is unset if disablePictureInPicture becomes true');
 
diff --git a/third_party/blink/web_tests/media/picture-in-picture/clear-after-request.html b/third_party/blink/web_tests/media/picture-in-picture/clear-after-request.html
new file mode 100644
index 0000000..8b0b86d
--- /dev/null
+++ b/third_party/blink/web_tests/media/picture-in-picture/clear-after-request.html
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<title>No crash when resetting player just after requesting PIP</title>
+<script src="../../resources/testharness.js"></script>
+<script src="../../resources/testharnessreport.js"></script>
+<video></video>
+<script>
+async_test(t => {
+  if (!document.pictureInPictureEnabled)
+    t.done();
+
+  const video = document.querySelector('video');
+  video.src = '../content/test.ogv';
+
+  video.addEventListener('click', t.step_func_done(() => {
+    video.requestPictureInPicture();
+    video.src = '';
+    // Do not crash.
+  }), { once: true });
+
+  video.play().then(t.step_func(() => {
+    const bounds = video.getBoundingClientRect();
+
+    chrome.gpuBenchmarking.pointerActionSequence([{
+      source: 'mouse',
+      actions: [
+        { name: 'pointerDown',
+          x: bounds.left + bounds.width / 2,
+          y: bounds.top + bounds.height / 2
+        },
+        { name: 'pointerUp' }
+      ]
+    }]);
+  }));
+});
+</script>