Skip to content

Commit

Permalink
Reland: PiP 2.0: Assume the feature is enabled on the browser side
Browse files Browse the repository at this point in the history
This is a reland of crrev.com/c/4198715. The CL had a merge issue where
another test was added in a separate CL that landed simulataneously and
caused a compilation issue. The old description is copied below:

Due to the way origin trials work, on the browser side we need to just
assume that the origin trial is enabled on the renderer that requests a
new document pip window.

(cherry picked from commit 08091e0)

Bug: 1410552
Change-Id: Icbc27fb1c591fa97275d2a6ff9f71f3acbae422a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4200080
Auto-Submit: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Mike Wasserman <msw@chromium.org>
Reviewed-by: Mike Wasserman <msw@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1098185}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4209811
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5563@{#65}
Cr-Branched-From: 3ac59a6-refs/heads/main@{#1097615}
  • Loading branch information
steimelchrome authored and Chromium LUCI CQ committed Jan 31, 2023
1 parent 7d92827 commit 92fc3e4
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 33 deletions.
7 changes: 0 additions & 7 deletions chrome/browser/ui/browser_navigator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,7 @@
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/site_isolation_policy.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/content_features.h"
#include "extensions/buildflags/buildflags.h"
#include "third_party/blink/public/common/features.h"
#include "ui/display/display.h"
#include "ui/display/screen.h"
#include "url/url_constants.h"
Expand Down Expand Up @@ -265,11 +263,6 @@ std::pair<Browser*, int> GetBrowserAndTabForDisposition(
return {GetOrCreateBrowser(profile, params.user_gesture), -1};
case WindowOpenDisposition::NEW_PICTURE_IN_PICTURE:
#if !BUILDFLAG(IS_ANDROID)
if (!base::FeatureList::IsEnabled(
blink::features::kDocumentPictureInPictureAPI)) {
return {nullptr, -1};
}

// Picture in picture windows may not be opened by other picture in
// picture windows.
if (params.browser->is_type_picture_in_picture())
Expand Down
29 changes: 3 additions & 26 deletions chrome/browser/ui/browser_navigator_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "chrome/app/chrome_command_ids.h"
Expand Down Expand Up @@ -44,15 +43,13 @@
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/bindings_policy.h"
#include "content/public/common/content_features.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/test_frame_navigation_observer.h"
#include "content/public/test/test_navigation_observer.h"
#include "content/public/test/test_utils.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "services/network/public/cpp/resource_request_body.h"
#include "third_party/blink/public/common/features.h"

#if BUILDFLAG(ENABLE_CAPTIVE_PORTAL_DETECTION)
#include "components/captive_portal/content/captive_portal_tab_helper.h"
Expand Down Expand Up @@ -1859,15 +1856,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, SubFrameNavigationUIData) {
}
#endif

// Helper class to enable picture in picture V2 for those tests that need it.
// Once the feature is enabled permanently, these can be merged back to
// BrowserNavigatorTest instead.
class BrowserNavigatorWithPictureInPictureTest : public BrowserNavigatorTest {
base::test::ScopedFeatureList scoped_feature_list_{
blink::features::kDocumentPictureInPictureAPI};
};

IN_PROC_BROWSER_TEST_F(BrowserNavigatorWithPictureInPictureTest,
IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
Disposition_PictureInPicture_Open) {
// Create the params for the PiP request.
auto pip_options = blink::mojom::PictureInPictureWindowOptions::New();
Expand Down Expand Up @@ -1895,7 +1884,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorWithPictureInPictureTest,
EXPECT_DOUBLE_EQ(0.5, aspect_ratio);
}

IN_PROC_BROWSER_TEST_F(BrowserNavigatorWithPictureInPictureTest,
IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
Disposition_PictureInPicture_OpenWithWidthAndHeight) {
// Give both an aspect ratio and a width/height that don't match. The
// width/height should take precedence.
Expand All @@ -1918,7 +1907,7 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorWithPictureInPictureTest,
EXPECT_EQ(500, override_bounds.height());
}

IN_PROC_BROWSER_TEST_F(BrowserNavigatorWithPictureInPictureTest,
IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
Disposition_PictureInPicture_CantFromAnotherPip) {
// Make sure that attempting to open a picture in picture window from a
// picture in picture window fails.
Expand All @@ -1931,16 +1920,4 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorWithPictureInPictureTest,
EXPECT_EQ(params.browser, nullptr);
}

IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest,
Disposition_PictureInPicture_FeatureMustBeEnabled) {
// Creating a picture in picture window should not work if the feature is off.
ASSERT_FALSE(base::FeatureList::IsEnabled(
blink::features::kDocumentPictureInPictureAPI));
NavigateParams params(MakeNavigateParams(browser()));
params.disposition = WindowOpenDisposition::NEW_PICTURE_IN_PICTURE;
Navigate(&params);

EXPECT_EQ(params.browser, nullptr);
}

} // namespace

0 comments on commit 92fc3e4

Please sign in to comment.