Revert "Fenced Frames: Navigations always replace the current entry"

This reverts commit 5e37873045b5e4c8cd4244480476a2b0c23b8114. Reason for revert: Appears to be breaking the tree https://ci.chromium.org/ui/p/chromium/builders/ci/Mac%20Builder%20(dbg)/202959/blamelist Original change's description: > Fenced Frames: Navigations always replace the current entry > > Background: > Fenced frames can navigate themselves but their history is not part of > the browser back/forward list as that could be a communication channel > from the fenced frame to the embedding page. This aligns with MPArch's > disjoint back/forward list for nested frame trees. (For shadowDOM, this > would be achieved with additional API level restrictions like > history.length always returning 1 etc.) > > Current Change: > This CL focuses on fenced frames to always have a replacement-only > navigation which was decided due to being a simpler model since it > doesn't imply that there's a hidden list of back/forward entries for > the nested page, only accessible via history APIs and not via the > back/forward buttons. This is also consistent with the iframes new > opt-in mode for disjoint session history as discussed in > whatwg/html#6501. > This change affects both shadowDOM and MPArch versions. > > Design: > https://docs.google.com/document/d/17rtX55WkxMcfh6ipuhP4mNULIVxUApvYt4ZYXfX2x-s/edit#heading=h.af2cik2j1rbs > > This CL includes browser tests to check the NavigationController's entry > count and > https://chromium-review.googlesource.com/c/chromium/src/+/3227344 > added WPTs for all the history API surface. > > > Bug: 1242533 > Change-Id: Ic574ee1bf87ce3a53dde7d280abaa46233d85b0d > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3216452 > Reviewed-by: Reilly Grant <reillyg@chromium.org> > Reviewed-by: Dave Tapuska <dtapuska@chromium.org> > Reviewed-by: Rakina Zata Amni <rakina@chromium.org> > Reviewed-by: Jeremy Roman <jbroman@chromium.org> > Reviewed-by: Lei Zhang <thestig@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Commit-Queue: Dave Tapuska <dtapuska@chromium.org> > Commit-Queue: Shivani Sharma <shivanisha@chromium.org> > Cr-Commit-Position: refs/heads/main@{#938761} Bug: 1242533 Change-Id: Ib946f629447de53164e1d335cf4b983e1fbdaa35 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3260236 Auto-Submit: anthonyvd <anthonyvd@chromium.org> Owners-Override: anthonyvd <anthonyvd@chromium.org> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#938770} NOKEYCHECK=True GitOrigin-RevId: 367479e4b169662f146b1053e9deaeedee035208
mjfroman · Nov 5, 2021 · 664fff6 · 664fff6
1 parent d7bab20
commit 664fff6
Show file tree

Hide file tree

Showing 11 changed files with 9 additions and 61 deletions.
diff --git a/blink/public/web/web_view.h b/blink/public/web/web_view.h
@@ -101,7 +101,6 @@ class WebView {
   // |is_prerendering| defines whether the page is being prerendered by the
   // Prerender2 feature (see content/browser/prerender/README.md).
   // [is_inside_portal] defines whether the page is inside_portal.
-  // [is_fenced_frame] defines whether the page is for a fenced frame.
   // |compositing_enabled| dictates whether accelerated compositing should be
   // enabled for the page. It must be false if no clients are provided, or if a
   // LayerTreeView will not be set for the WebWidget.
@@ -126,7 +125,6 @@ class WebView {
       bool is_hidden,
       bool is_prerendering,
       bool is_inside_portal,
-      bool is_fenced_frame,
       bool compositing_enabled,
       bool widgets_never_composited,
       WebView* opener,

diff --git a/blink/renderer/core/exported/web_view_impl.cc b/blink/renderer/core/exported/web_view_impl.cc
@@ -448,7 +448,6 @@ WebView* WebView::Create(
     bool is_hidden,
     bool is_prerendering,
     bool is_inside_portal,
-    bool is_fenced_frame,
     bool compositing_enabled,
     bool widgets_never_composited,
     WebView* opener,
@@ -461,7 +460,7 @@ WebView* WebView::Create(
       client,
       is_hidden ? mojom::blink::PageVisibilityState::kHidden
                 : mojom::blink::PageVisibilityState::kVisible,
-      is_prerendering, is_inside_portal, is_fenced_frame, compositing_enabled,
+      is_prerendering, is_inside_portal, compositing_enabled,
       widgets_never_composited, To<WebViewImpl>(opener), std::move(page_handle),
       agent_group_scheduler, session_storage_namespace_id,
       std::move(page_base_background_color));
@@ -472,7 +471,6 @@ WebViewImpl* WebViewImpl::Create(
     mojom::blink::PageVisibilityState visibility,
     bool is_prerendering,
     bool is_inside_portal,
-    bool is_fenced_frame,
     bool compositing_enabled,
     bool widgets_never_composited,
     WebViewImpl* opener,
@@ -483,7 +481,7 @@ WebViewImpl* WebViewImpl::Create(
   // Take a self-reference for WebViewImpl that is released by calling Close(),
   // then return a raw pointer to the caller.
   auto web_view = base::AdoptRef(new WebViewImpl(
-      client, visibility, is_prerendering, is_inside_portal, is_fenced_frame,
+      client, visibility, is_prerendering, is_inside_portal,
       compositing_enabled, widgets_never_composited, opener,
       std::move(page_handle), agent_group_scheduler,
       session_storage_namespace_id, std::move(page_base_background_color)));
@@ -546,7 +544,6 @@ WebViewImpl::WebViewImpl(
     mojom::blink::PageVisibilityState visibility,
     bool is_prerendering,
     bool is_inside_portal,
-    bool is_fenced_frame,
     bool does_composite,
     bool widgets_never_composited,
     WebViewImpl* opener,
@@ -582,11 +579,6 @@ WebViewImpl::WebViewImpl(
   // page.
   SetInsidePortal(is_inside_portal);
 
-  if (is_fenced_frame && features::IsFencedFramesEnabled() &&
-      features::IsFencedFramesMPArchBased()) {
-    page_->SetIsMainFrameFencedFrameRoot();
-  }
-
   // When not compositing, keep the Page in the loop so that it will paint all
   // content into the root layer, as multiple layers can only be used when
   // compositing them together later.

diff --git a/blink/renderer/core/exported/web_view_impl.h b/blink/renderer/core/exported/web_view_impl.h
@@ -117,7 +117,6 @@ class CORE_EXPORT WebViewImpl final : public WebView,
       mojom::blink::PageVisibilityState visibility,
       bool is_prerendering,
       bool is_inside_portal,
-      bool is_fenced_frame,
       bool compositing_enabled,
       bool widgets_never_composited,
       WebViewImpl* opener,
@@ -650,7 +649,6 @@ class CORE_EXPORT WebViewImpl final : public WebView,
       mojom::blink::PageVisibilityState visibility,
       bool is_prerendering,
       bool is_inside_portal,
-      bool is_fenced_frame,
       bool does_composite,
       bool widgets_never_composite,
       WebViewImpl* opener,

diff --git a/blink/renderer/core/exported/web_view_test.cc b/blink/renderer/core/exported/web_view_test.cc
@@ -489,7 +489,6 @@ TEST_F(WebViewTest, SetBaseBackgroundColorBeforeMainFrame) {
                       /*is_hidden=*/false,
                       /*is_prerendering=*/false,
                       /*is_inside_portal=*/false,
-                      /*is_fenced_frame=*/false,
                       /*compositing_enabled=*/true,
                       /*widgets_never_composited=*/false,
                       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),
@@ -2744,7 +2743,6 @@ TEST_F(WebViewTest, ClientTapHandlingNullWebViewClient) {
   WebViewImpl* web_view = To<WebViewImpl>(WebView::Create(
       /*client=*/nullptr, /*is_hidden=*/false, /*is_prerendering=*/false,
       /*is_inside_portal=*/false,
-      /*is_fenced_frame=*/false,
       /*compositing_enabled=*/false,
       /*widgets_never_composited=*/false,
       /*opener=*/nullptr, mojo::NullAssociatedReceiver(),

diff --git a/blink/renderer/core/frame/frame_test_helpers.cc b/blink/renderer/core/frame/frame_test_helpers.cc
@@ -621,7 +621,6 @@ void WebViewHelper::InitializeWebView(TestWebViewClient* web_view_client,
                       /*is_hidden=*/false,
                       /*is_prerendering=*/false,
                       /*is_inside_portal=*/false,
-                      /*is_fenced_frame=*/false,
                       /*compositing_enabled=*/true,
                       /*widgets_never_composited=*/false,
                       /*opener=*/opener, mojo::NullAssociatedReceiver(),

diff --git a/blink/renderer/core/frame/local_frame.cc b/blink/renderer/core/frame/local_frame.cc
@@ -512,27 +512,12 @@ bool LocalFrame::NavigationShouldReplaceCurrentHistoryEntry(
 }
 
 bool LocalFrame::ShouldMaintainTrivialSessionHistory() const {
-  // This should be kept in sync with
+  // TODO(https://crbug.com/1197384): We may have to add fenced frames. This
+  // should be kept in sync with NavigationControllerImpl version,
   // NavigationControllerImpl::ShouldMaintainTrivialSessionHistory.
   //
   // TODO(mcnee): Similarly, we need to restrict orphaned contexts.
-  return GetPage()->InsidePortal() || GetDocument()->IsPrerendering() ||
-         IsInFencedFrameTree();
-}
-
-bool LocalFrame::IsInFencedFrameTree() const {
-  if (!blink::features::IsFencedFramesEnabled())
-    return false;
-
-  switch (blink::features::kFencedFramesImplementationTypeParam.Get()) {
-    case blink::features::FencedFramesImplementationType::kMPArch:
-      return GetPage()->IsMainFrameFencedFrameRoot();
-    case blink::features::FencedFramesImplementationType::kShadowDOM:
-      return Tree().Top(FrameTreeBoundary::kFenced) !=
-             Tree().Top(FrameTreeBoundary::kIgnoreFence);
-    default:
-      return false;
-  }
+  return GetPage()->InsidePortal() || GetDocument()->IsPrerendering();
 }
 
 bool LocalFrame::DetachImpl(FrameDetachType type) {

diff --git a/blink/renderer/core/frame/local_frame.h b/blink/renderer/core/frame/local_frame.h
@@ -804,13 +804,6 @@ class CORE_EXPORT LocalFrame final : public Frame,
       const FrameLoadRequest& request,
       WebFrameLoadType frame_load_type);
 
-  // Returns false if fenced frames are disabled. Returns true if the
-  // feature is enabled and if |this| or any of its ancestor nodes is a
-  // fenced frame. For MPArch returns the value of
-  // Page::IsMainFrameFencedFrameRoot and for shadowDOM returns true, if
-  // the FrameTree that this frame is in is not the outermost FrameTree.
-  bool IsInFencedFrameTree() const;
-
   std::unique_ptr<FrameScheduler> frame_scheduler_;
 
   // Holds all PauseSubresourceLoadingHandles allowing either |this| to delete

diff --git a/blink/renderer/core/page/page.cc b/blink/renderer/core/page/page.cc
@@ -1079,14 +1079,6 @@ bool Page::InsidePortal() const {
   return inside_portal_;
 }
 
-void Page::SetIsMainFrameFencedFrameRoot() {
-  is_fenced_frame_tree_ = true;
-}
-
-bool Page::IsMainFrameFencedFrameRoot() const {
-  return is_fenced_frame_tree_;
-}
-
 void Page::SetMediaFeatureOverride(const AtomicString& media_feature,
                                    const String& value) {
   if (!media_feature_overrides_) {

diff --git a/blink/renderer/core/page/page.h b/blink/renderer/core/page/page.h
@@ -385,11 +385,6 @@ class CORE_EXPORT Page final : public GarbageCollected<Page>,
   // Fully invalidate paint of all local frames in this page.
   void InvalidatePaint();
 
-  // Should be invoked when the main frame of this frame tree is a fenced frame.
-  void SetIsMainFrameFencedFrameRoot();
-  // Returns if the main frame of this frame tree is a fenced frame.
-  bool IsMainFrameFencedFrameRoot() const;
-
  private:
   friend class ScopedPagePauser;
 
@@ -511,11 +506,6 @@ class CORE_EXPORT Page final : public GarbageCollected<Page>,
   // prerender activation; it does not go from false to true.
   bool is_prerendering_ = false;
 
-  // Whether the the Page's main document is a Fenced Frame document. This is
-  // only set for the MPArch implementation and is true when the corresponding
-  // browser side FrameTree has the FrameTree::Type of kFencedFrame.
-  bool is_fenced_frame_tree_ = false;
-
   mojom::blink::TextAutosizerPageInfo web_text_autosizer_page_info_;
 
   WebScopedVirtualTimePauser history_navigation_virtual_time_pauser_;

diff --git a/blink/renderer/modules/peerconnection/webrtc_audio_renderer_test.cc b/blink/renderer/modules/peerconnection/webrtc_audio_renderer_test.cc
@@ -144,7 +144,6 @@ class MAYBE_WebRtcAudioRendererTest : public testing::Test {
             /*is_hidden=*/false,
             /*is_prerendering=*/false,
             /*is_inside_portal=*/false,
-            /*is_fenced_frame=*/false,
             /*compositing_enabled=*/false,
             /*widgets_never_composited=*/false,
             /*opener=*/nullptr,

diff --git a/...d_frame/history-length-fenced-navigations-replace-do-not-contribute-to-joint-expected.txt b/...d_frame/history-length-fenced-navigations-replace-do-not-contribute-to-joint-expected.txt
@@ -0,0 +1,4 @@
+This is a testharness.js-based test.
+FAIL All fenced navigations should be replace-only and not contribute to joint session history assert_equals: Navigations inside of a fenced frame are always replacement navigations and never increase `history.length` inside the fence: top-level-fenced-frame expected "PASS > top-level-fenced-frame" but got "FAIL > top-level-fenced-frame history.length: 10"
+Harness: the test ran to completion.
+