pip2: Don't discard tabs that own a picture-in-picture window

This CL prevents tabs from being discarded when they have a video or document picture-in-picture window open. This prevents an issue where picture-in-picture windows can close unexpectedly. Bug: 1446327 Change-Id: I9d6c021a0f527fa86243ce2273d8d7b868c326bd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4582630 Reviewed-by: Francois Pierre Doray <fdoray@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Commit-Queue: Tommy Steimel <steimel@chromium.org> Cr-Commit-Position: refs/heads/main@{#1210398}
chromium · Oct 16, 2023 · 52b496d · 52b496d
1 parent e2db962
commit 52b496d
Show file tree

Hide file tree

Showing 13 changed files with 83 additions and 2 deletions.
diff --git a/chrome/browser/performance_manager/policies/page_discarding_helper.cc b/chrome/browser/performance_manager/policies/page_discarding_helper.cc
@@ -312,6 +312,11 @@ PageDiscardingHelper::CanDiscardResult PageDiscardingHelper::CanDiscard(
     return CanDiscardResult::kProtected;
   }
 
+  // Don't discard pages that are displaying content in picture-in-picture.
+  if (page_node->HasPictureInPicture()) {
+    return CanDiscardResult::kProtected;
+  }
+
   // Do not discard PDFs as they might contain entry that is not saved and they
   // don't remember their scrolling positions. See crbug.com/547286 and
   // crbug.com/65244.

diff --git a/chrome/browser/performance_manager/policies/page_discarding_helper_unittest.cc b/chrome/browser/performance_manager/policies/page_discarding_helper_unittest.cc
@@ -442,6 +442,13 @@ TEST_F(PageDiscardingHelperTest,
   EXPECT_TRUE(CanDiscard(page_node(), DiscardReason::EXTERNAL));
 }
 
+TEST_F(PageDiscardingHelperTest, TestCannotDiscardWithPictureInPicture) {
+  page_node()->SetHasPictureInPicture(true);
+  EXPECT_FALSE(CanDiscard(page_node(), DiscardReason::URGENT));
+  EXPECT_FALSE(CanDiscard(page_node(), DiscardReason::PROACTIVE));
+  EXPECT_TRUE(CanDiscard(page_node(), DiscardReason::EXTERNAL));
+}
+
 // Tests DiscardMultiplePages.
 
 TEST_F(PageDiscardingHelperTest, DiscardMultiplePagesNoCandidate) {

diff --git a/chrome/browser/resource_coordinator/decision_details.cc b/chrome/browser/resource_coordinator/decision_details.cc
@@ -38,6 +38,7 @@ const char* kDecisionFailureReasonStrings[] = {
     "Tab is currently holding an IndexedDB lock",
     "Tab has notification permission ",
     "Tab is a web application window",
+    "Tab is displaying content in picture-in-picture",
 };
 static_assert(std::size(kDecisionFailureReasonStrings) ==
                   static_cast<size_t>(DecisionFailureReason::MAX),
@@ -151,6 +152,9 @@ void PopulateFailureReason(
     case DecisionFailureReason::LIVE_WEB_APP:
       ukm->SetFailureLiveWebApp(1);
       break;
+    case DecisionFailureReason::LIVE_PICTURE_IN_PICTURE:
+      ukm->SetFailureLivePictureInPicture(1);
+      break;
     case DecisionFailureReason::MAX:
       NOTREACHED();
       break;

diff --git a/chrome/browser/resource_coordinator/decision_details.h b/chrome/browser/resource_coordinator/decision_details.h
@@ -86,6 +86,8 @@ enum class DecisionFailureReason : int32_t {
   LIVE_STATE_HAS_NOTIFICATIONS_PERMISSION,
   // The tab is a standalone desktop PWA window.
   LIVE_WEB_APP,
+  // The tab is displaying content in picture-in-picture.
+  LIVE_PICTURE_IN_PICTURE,
   // This must remain last.
   MAX,
 };

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
@@ -431,6 +431,11 @@ bool TabLifecycleUnitSource::TabLifecycleUnit::CanDiscard(
     decision_details->AddReason(DecisionFailureReason::LIVE_WEB_APP);
   }
 
+  if (web_contents()->HasPictureInPictureVideo() ||
+      web_contents()->HasPictureInPictureDocument()) {
+    decision_details->AddReason(DecisionFailureReason::LIVE_PICTURE_IN_PICTURE);
+  }
+
   if (decision_details->reasons().empty()) {
     decision_details->AddReason(
         DecisionSuccessReason::HEURISTIC_OBSERVED_TO_BE_SAFE);

diff --git a/chrome/browser/ui/webui/discards/graph_dump_impl.h b/chrome/browser/ui/webui/discards/graph_dump_impl.h
@@ -142,6 +142,9 @@ class DiscardsGraphDumpImpl : public discards::mojom::GraphDump,
   void OnIsAudibleChanged(
       const performance_manager::PageNode* page_node) override {}
   // Ignored.
+  void OnHasPictureInPictureChanged(
+      const performance_manager::PageNode* page_node) override {}
+  // Ignored.
   void OnLoadingStateChanged(
       const performance_manager::PageNode* page_node,
       performance_manager::PageNode::LoadingState previous_state) override {}

diff --git a/components/performance_manager/graph/page_node_impl.cc b/components/performance_manager/graph/page_node_impl.cc
@@ -63,6 +63,8 @@ PageNodeImpl::PageNodeImpl(const WebContentsProxy& contents_proxy,
       browser_context_id_(browser_context_id),
       is_visible_(initial_properties.Has(PagePropertyFlag::kIsVisible)),
       is_audible_(initial_properties.Has(PagePropertyFlag::kIsAudible)),
+      has_picture_in_picture_(
+          initial_properties.Has(PagePropertyFlag::kHasPictureInPicture)),
       page_state_(page_state) {
   // Nodes are created on the UI thread, then accessed on the PM sequence.
   // `weak_this_` can be returned from GetWeakPtrOnUIThread() and dereferenced
@@ -166,6 +168,11 @@ void PageNodeImpl::SetIsAudible(bool is_audible) {
   }
 }
 
+void PageNodeImpl::SetHasPictureInPicture(bool has_picture_in_picture) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  has_picture_in_picture_.SetAndMaybeNotify(this, has_picture_in_picture);
+}
+
 void PageNodeImpl::SetUkmSourceId(ukm::SourceId ukm_source_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   ukm_source_id_.SetAndMaybeNotify(this, ukm_source_id);
@@ -299,6 +306,11 @@ bool PageNodeImpl::is_audible() const {
   return is_audible_.value();
 }
 
+bool PageNodeImpl::has_picture_in_picture() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return has_picture_in_picture_.value();
+}
+
 PageNode::LoadingState PageNodeImpl::loading_state() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return loading_state_.value();
@@ -541,6 +553,11 @@ base::TimeDelta PageNodeImpl::GetTimeSinceLastVisibilityChange() const {
   return TimeSinceLastVisibilityChange();
 }
 
+bool PageNodeImpl::HasPictureInPicture() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return has_picture_in_picture();
+}
+
 bool PageNodeImpl::IsAudible() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return is_audible();

diff --git a/components/performance_manager/graph/page_node_impl.h b/components/performance_manager/graph/page_node_impl.h
@@ -37,8 +37,9 @@ class TabConnectednessAccess;
 enum class PagePropertyFlag {
   kIsVisible,  // initializes PageNode::IsVisible()
   kMin = kIsVisible,
-  kIsAudible,  // initializes PageNode::IsAudible()
-  kMax = kIsAudible,
+  kIsAudible,            // initializes PageNode::IsAudible()
+  kHasPictureInPicture,  // initializes PageNode::HasPictureInPicture()
+  kMax = kHasPictureInPicture,
 };
 using PagePropertyFlags = base::
     EnumSet<PagePropertyFlag, PagePropertyFlag::kMin, PagePropertyFlag::kMax>;
@@ -86,6 +87,7 @@ class PageNodeImpl
   void SetIsFocused(bool is_focused);
   void SetIsVisible(bool is_visible);
   void SetIsAudible(bool is_audible);
+  void SetHasPictureInPicture(bool has_picture_in_picture);
   void SetLoadingState(LoadingState loading_state);
   void SetUkmSourceId(ukm::SourceId ukm_source_id);
   void OnFaviconUpdated();
@@ -126,6 +128,7 @@ class PageNodeImpl
   bool is_focused() const;
   bool is_visible() const;
   bool is_audible() const;
+  bool has_picture_in_picture() const;
   LoadingState loading_state() const;
   ukm::SourceId ukm_source_id() const;
   LifecycleState lifecycle_state() const;
@@ -241,6 +244,7 @@ class PageNodeImpl
   bool IsFocused() const override;
   bool IsVisible() const override;
   base::TimeDelta GetTimeSinceLastVisibilityChange() const override;
+  bool HasPictureInPicture() const override;
   bool IsAudible() const override;
   absl::optional<base::TimeDelta> GetTimeSinceLastAudibleChange()
       const override;
@@ -361,6 +365,12 @@ class PageNodeImpl
   ObservedProperty::NotifiesOnlyOnChanges<bool,
                                           &PageNodeObserver::OnIsAudibleChanged>
       is_audible_ GUARDED_BY_CONTEXT(sequence_checker_){false};
+  // Whether or not the page is displaying content in picture-in-picture. Driven
+  // by browser instrumentation. Initialized on construction.
+  ObservedProperty::NotifiesOnlyOnChanges<
+      bool,
+      &PageNodeObserver::OnHasPictureInPictureChanged>
+      has_picture_in_picture_ GUARDED_BY_CONTEXT(sequence_checker_){false};
   // The loading state. This is driven by instrumentation in the browser
   // process.
   ObservedProperty::NotifiesOnlyOnChangesWithPreviousValue<

diff --git a/components/performance_manager/graph/page_node_impl_unittest.cc b/components/performance_manager/graph/page_node_impl_unittest.cc
@@ -273,6 +273,7 @@ class LenientMockObserver : public PageNodeImpl::Observer {
   MOCK_METHOD1(OnIsFocusedChanged, void(const PageNode*));
   MOCK_METHOD1(OnIsVisibleChanged, void(const PageNode*));
   MOCK_METHOD1(OnIsAudibleChanged, void(const PageNode*));
+  MOCK_METHOD1(OnHasPictureInPictureChanged, void(const PageNode*));
   MOCK_METHOD2(OnLoadingStateChanged,
                void(const PageNode*, PageNode::LoadingState));
   MOCK_METHOD1(OnUkmSourceIdChanged, void(const PageNode*));

diff --git a/components/performance_manager/performance_manager_tab_helper.cc b/components/performance_manager/performance_manager_tab_helper.cc
@@ -102,6 +102,10 @@ PerformanceManagerTabHelper::PerformanceManagerTabHelper(
   if (web_contents->IsCurrentlyAudible()) {
     initial_property_flags.Put(PagePropertyFlag::kIsAudible);
   }
+  if (web_contents->HasPictureInPictureVideo() ||
+      web_contents->HasPictureInPictureDocument()) {
+    initial_property_flags.Put(PagePropertyFlag::kHasPictureInPicture);
+  }
 
   // Create the page node.
   std::unique_ptr<PageData> page = std::make_unique<PageData>();
@@ -486,6 +490,14 @@ void PerformanceManagerTabHelper::DidUpdateFaviconURL(
                                 base::Unretained(primary_page_node())));
 }
 
+void PerformanceManagerTabHelper::MediaPictureInPictureChanged(
+    bool is_picture_in_picture) {
+  PerformanceManagerImpl::CallOnGraphImpl(
+      FROM_HERE, base::BindOnce(&PageNodeImpl::SetHasPictureInPicture,
+                                base::Unretained(primary_page_node()),
+                                is_picture_in_picture));
+}
+
 void PerformanceManagerTabHelper::OnWebContentsFocused(
     content::RenderWidgetHost* render_widget_host) {
   PerformanceManagerImpl::CallOnGraphImpl(

diff --git a/components/performance_manager/performance_manager_tab_helper.h b/components/performance_manager/performance_manager_tab_helper.h
@@ -89,6 +89,7 @@ class PerformanceManagerTabHelper
   void DidUpdateFaviconURL(
       content::RenderFrameHost* render_frame_host,
       const std::vector<blink::mojom::FaviconURLPtr>& candidates) override;
+  void MediaPictureInPictureChanged(bool is_picture_in_picture) override;
   void OnWebContentsFocused(
       content::RenderWidgetHost* render_widget_host) override;
   void OnWebContentsLostFocus(

diff --git a/components/performance_manager/public/graph/page_node.h b/components/performance_manager/public/graph/page_node.h
@@ -154,6 +154,10 @@ class PageNode : public Node {
   virtual absl::optional<base::TimeDelta> GetTimeSinceLastAudibleChange()
       const = 0;
 
+  // Returns true if this page is displaying content in a picture-in-picture
+  // window, false otherwise.
+  virtual bool HasPictureInPicture() const = 0;
+
   // Returns the page's loading state.
   virtual LoadingState GetLoadingState() const = 0;
 
@@ -312,6 +316,9 @@ class PageNodeObserver {
   // this property change.
   virtual void OnIsAudibleChanged(const PageNode* page_node) = 0;
 
+  // Invoked when the HasPictureInPicture property changes.
+  virtual void OnHasPictureInPictureChanged(const PageNode* page_node) = 0;
+
   // Invoked when the GetLoadingState property changes.
   virtual void OnLoadingStateChanged(const PageNode* page_node,
                                      PageNode::LoadingState previous_state) = 0;
@@ -398,6 +405,7 @@ class PageNode::ObserverDefaultImpl : public PageNodeObserver {
   void OnIsFocusedChanged(const PageNode* page_node) override {}
   void OnIsVisibleChanged(const PageNode* page_node) override {}
   void OnIsAudibleChanged(const PageNode* page_node) override {}
+  void OnHasPictureInPictureChanged(const PageNode* page_node) override {}
   void OnLoadingStateChanged(const PageNode* page_node,
                              PageNode::LoadingState previous_state) override {}
   void OnUkmSourceIdChanged(const PageNode* page_node) override {}

diff --git a/tools/metrics/ukm/ukm.xml b/tools/metrics/ukm/ukm.xml
@@ -24328,6 +24328,12 @@ be describing additional metrics about the same event.
       via feature policy.
     </summary>
   </metric>
+  <metric name="FailureLivePictureInPicture" enum="Boolean">
+    <summary>
+      Boolean indicating that the intervention was disallowed because the tab is
+      displaying content in picture-in-picture.
+    </summary>
+  </metric>
   <metric name="FailureLiveStateCapturing" enum="Boolean">
     <summary>
       Boolean indicating that the intervention was disallowed because the tab is