Revert "[bfcache] Add notRestoredReasons field in PerformanceNavigati…

…onTiming" This reverts commit e1af9facf232a11871b0c668b90ec17c5a5d5e16. Reason for revert: Likely cause for multiple builder failures. For example, see https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ASan%20Tests%20(sandboxed)/93643/overview Original change's description: > [bfcache] Add notRestoredReasons field in PerformanceNavigationTiming > > This CL adds back/forward cache not restored reasons in Performance > NavigationTiming API. > It's proposed here: > w3c/navigation-timing#171 > Explainer: https://github.com/rubberyuzu/bfcache-not-retored-reason/blob/main/NotRestoredReason.md > > This CL does two things: > - exposes not restored reasons to PerformanceNavigationTiming API > - adds WPT > > The WPT have to run on all platforms because this API is intended to > collect bfcache metrics from the wild, regardless of the platforms. > > LOW_COVERAGE_REASON=Adding coverage for new field. Just missing the tests for the existing fields > > Bug: 1349228 > Change-Id: I8dd96a60188bdff94a21c4e4e34cacf181a823db > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3835534 > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Kentaro Hara <haraken@chromium.org> > Reviewed-by: Weizhong Xia <weizhong@google.com> > Commit-Queue: Yuzu Saijo <yuzus@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1054800} Bug: 1349228 Change-Id: Iab0fed375560bb0b09bf4e8ea18b80c5d88b1375 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3935464 Owners-Override: Emily Shack <emshack@google.com> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Kentaro Hara <haraken@chromium.org> Auto-Submit: Emily Shack <emshack@chromium.org> Cr-Commit-Position: refs/heads/main@{#1055015} NOKEYCHECK=True GitOrigin-RevId: f48af8c89a823a0f20f1eb5c51871a0c8860d97f
mjfroman · Oct 5, 2022 · 7b60cbf · 7b60cbf
1 parent 55964af
commit 7b60cbf
Show file tree

Hide file tree

Showing 18 changed files with 24 additions and 446 deletions.
diff --git a/blink/renderer/core/frame/local_frame.cc b/blink/renderer/core/frame/local_frame.cc
@@ -594,8 +594,6 @@ bool LocalFrame::DetachImpl(FrameDetachType type) {
   if (text_fragment_handler_)
     text_fragment_handler_->DidDetachDocumentOrFrame();
 
-  not_restored_reasons_.reset();
-
   DCHECK(!view_->IsAttached());
   Client()->WillBeDetached();
 
@@ -3316,40 +3314,4 @@ void LocalFrame::MaybeUpdateWindowControlsOverlayWithNewZoomLevel() {
 }
 #endif  // !BUILDFLAG(IS_ANDROID)
 
-void LocalFrame::SetNotRestoredReasons(
-    mojom::blink::BackForwardCacheNotRestoredReasonsPtr not_restored_reasons) {
-  // Back/forward cache is only enabled for outermost main frame.
-  DCHECK(IsOutermostMainFrame());
-  not_restored_reasons_ = mojo::Clone(not_restored_reasons);
-}
-
-const mojom::blink::BackForwardCacheNotRestoredReasonsPtr&
-LocalFrame::GetNotRestoredReasons() {
-  // Back/forward cache is only enabled for the outermost main frames, and the
-  // web exposed API returns non-null values only for the outermost main frames.
-  DCHECK(IsOutermostMainFrame());
-  return not_restored_reasons_;
-}
-
-bool LocalFrame::HasBlockingReasons() {
-  DCHECK(IsOutermostMainFrame());
-  if (!not_restored_reasons_)
-    return false;
-  return HasBlockingReasonsHelper(not_restored_reasons_);
-}
-
-bool LocalFrame::HasBlockingReasonsHelper(
-    const mojom::blink::BackForwardCacheNotRestoredReasonsPtr& not_restored) {
-  if (not_restored->blocked)
-    return true;
-  if (not_restored->same_origin_details) {
-    for (const auto& child : not_restored->same_origin_details->children) {
-      if (HasBlockingReasonsHelper(child))
-        return true;
-    }
-    return false;
-  }
-  return false;
-}
-
 }  // namespace blink
diff --git a/blink/renderer/core/frame/local_frame.h b/blink/renderer/core/frame/local_frame.h
@@ -43,7 +43,6 @@
 #include "third_party/blink/public/common/frame/frame_ad_evidence.h"
 #include "third_party/blink/public/common/frame/transient_allow_fullscreen.h"
 #include "third_party/blink/public/common/tokens/tokens.h"
-#include "third_party/blink/public/mojom/back_forward_cache_not_restored_reasons.mojom-blink.h"
 #include "third_party/blink/public/mojom/blob/blob_url_store.mojom-blink-forward.h"
 #include "third_party/blink/public/mojom/devtools/devtools_agent.mojom-blink-forward.h"
 #include "third_party/blink/public/mojom/devtools/inspector_issue.mojom-blink-forward.h"
@@ -605,15 +604,6 @@ class CORE_EXPORT LocalFrame final
   mojom::blink::BackForwardCacheControllerHost&
   GetBackForwardCacheControllerHostRemote();
 
-  // Sets back/forward cache NotRestoredReasons for this frame. Only set for
-  // outermost main frame.
-  void SetNotRestoredReasons(
-      mojom::blink::BackForwardCacheNotRestoredReasonsPtr);
-  const mojom::blink::BackForwardCacheNotRestoredReasonsPtr&
-  GetNotRestoredReasons();
-  // Returns if the saved NotRestoredReasons has any blocking reasons.
-  bool HasBlockingReasons();
-
   const AtomicString& GetReducedAcceptLanguage() const {
     return reduced_accept_language_;
   }
@@ -881,10 +871,6 @@ class CORE_EXPORT LocalFrame final
                                     String& clip_html,
                                     gfx::Rect& clip_rect);
 
-  // Helper function for |HasBlockingReasons()|.
-  bool HasBlockingReasonsHelper(
-      const mojom::blink::BackForwardCacheNotRestoredReasonsPtr&);
-
 #if !BUILDFLAG(IS_ANDROID)
   void SetTitlebarAreaDocumentStyleEnvironmentVariables() const;
   void MaybeUpdateWindowControlsOverlayWithNewZoomLevel();
@@ -957,9 +943,6 @@ class CORE_EXPORT LocalFrame final
 
   mojom::blink::ViewportIntersectionState intersection_state_;
 
-  // Only set for outermost main frame.
-  mojom::blink::BackForwardCacheNotRestoredReasonsPtr not_restored_reasons_;
-
   // Per-frame URLLoader factory.
   std::unique_ptr<WebURLLoaderFactory> url_loader_factory_;
 

diff --git a/blink/renderer/core/frame/web_local_frame_impl.cc b/blink/renderer/core/frame/web_local_frame_impl.cc
@@ -3041,43 +3041,30 @@ void WebLocalFrameImpl::SetSessionStorageArea(
 
 void WebLocalFrameImpl::SetNotRestoredReasons(
     const mojom::BackForwardCacheNotRestoredReasonsPtr& not_restored_reasons) {
-  GetFrame()->SetNotRestoredReasons(
-      ConvertNotRestoredReasons(not_restored_reasons));
+  not_restored_reasons_ =
+      not_restored_reasons.is_null()
+          ? mojom::BackForwardCacheNotRestoredReasonsPtr(nullptr)
+          : not_restored_reasons->Clone();
 }
 
 bool WebLocalFrameImpl::HasBlockingReasons() {
-  return GetFrame()->HasBlockingReasons();
-}
-
-const mojom::blink::BackForwardCacheNotRestoredReasonsPtr&
-WebLocalFrameImpl::GetNotRestoredReasons() {
-  return GetFrame()->GetNotRestoredReasons();
-}
-
-mojom::blink::BackForwardCacheNotRestoredReasonsPtr
-WebLocalFrameImpl::ConvertNotRestoredReasons(
-    const mojom::BackForwardCacheNotRestoredReasonsPtr& reasons_to_copy) {
-  mojom::blink::BackForwardCacheNotRestoredReasonsPtr not_restored_reasons;
-  if (!reasons_to_copy.is_null()) {
-    not_restored_reasons =
-        mojom::blink::BackForwardCacheNotRestoredReasons::New();
-    not_restored_reasons->blocked = reasons_to_copy->blocked;
-    auto details = mojom::blink::SameOriginBfcacheNotRestoredDetails::New();
-    if (reasons_to_copy->same_origin_details) {
-      details->id = reasons_to_copy->same_origin_details->id.c_str();
-      details->name = reasons_to_copy->same_origin_details->name.c_str();
-      details->src = reasons_to_copy->same_origin_details->src.c_str();
-      details->url = reasons_to_copy->same_origin_details->url.c_str();
-      for (const auto& reason : reasons_to_copy->same_origin_details->reasons) {
-        details->reasons.push_back(reason.c_str());
-      }
-      for (const auto& child : reasons_to_copy->same_origin_details->children) {
-        details->children.push_back(ConvertNotRestoredReasons(child));
-      }
+  if (!not_restored_reasons_)
+    return false;
+  return HasBlockingReasonsHelper(not_restored_reasons_);
+}
+
+bool WebLocalFrameImpl::HasBlockingReasonsHelper(
+    const mojom::BackForwardCacheNotRestoredReasonsPtr& not_restored) {
+  if (not_restored->blocked)
+    return true;
+  if (not_restored->same_origin_details) {
+    for (const auto& child : not_restored->same_origin_details->children) {
+      if (HasBlockingReasonsHelper(child))
+        return true;
     }
-    not_restored_reasons->same_origin_details = std::move(details);
+    return false;
   }
-  return not_restored_reasons;
+  return not_restored->blocked;
 }
 
 void WebLocalFrameImpl::AddHitTestOnTouchStartCallback(

diff --git a/blink/renderer/core/frame/web_local_frame_impl.h b/blink/renderer/core/frame/web_local_frame_impl.h
@@ -386,9 +386,6 @@ class CORE_EXPORT WebLocalFrameImpl final
   // Returns if the current frame's NotRestoredReasons has any blocking reasons.
   bool HasBlockingReasons() override;
 
-  const mojom::blink::BackForwardCacheNotRestoredReasonsPtr&
-  GetNotRestoredReasons();
-
   void InitializeCoreFrame(
       Page&,
       FrameOwner*,
@@ -558,6 +555,10 @@ class CORE_EXPORT WebLocalFrameImpl final
   // Sets the local core frame and registers destruction observers.
   void SetCoreFrame(LocalFrame*);
 
+  // Helper function for |HasBlockingReasons()|.
+  bool HasBlockingReasonsHelper(
+      const mojom::BackForwardCacheNotRestoredReasonsPtr&);
+
   // Inherited from WebFrame, but intentionally hidden: it never makes sense
   // to call these on a WebLocalFrameImpl.
   bool IsWebLocalFrame() const override;
@@ -610,11 +611,6 @@ class CORE_EXPORT WebLocalFrameImpl final
       network::mojom::blink::WebSandboxFlags sandbox_flags =
           network::mojom::blink::WebSandboxFlags::kNone);
 
-  // This function converts mojom::BackForwardCacheNotRestoredReasonsPtr to
-  // mojom::blink::BackForwardCacheNotRestoredReasonsPtr.
-  mojom::blink::BackForwardCacheNotRestoredReasonsPtr ConvertNotRestoredReasons(
-      const mojom::BackForwardCacheNotRestoredReasonsPtr& reasons_struct);
-
   WebLocalFrameClient* client_;
 
   // TODO(dcheng): Inline this field directly rather than going through Member.

diff --git a/blink/renderer/core/timing/performance_navigation_timing.cc b/blink/renderer/core/timing/performance_navigation_timing.cc
@@ -9,7 +9,6 @@
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/document_timing.h"
 #include "third_party/blink/renderer/core/frame/local_dom_window.h"
-#include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/loader/document_load_timing.h"
 #include "third_party/blink/renderer/core/loader/document_loader.h"
 #include "third_party/blink/renderer/core/performance_entry_names.h"
@@ -311,54 +310,6 @@ DOMHighResTimeStamp PerformanceNavigationTiming::duration() const {
   return loadEventEnd();
 }
 
-ScriptValue PerformanceNavigationTiming::notRestoredReasons(
-    ScriptState* script_state) const {
-  DocumentLoader* loader = GetDocumentLoader();
-  if (!loader || !loader->GetFrame()->IsOutermostMainFrame())
-    return ScriptValue::CreateNull(script_state->GetIsolate());
-
-  // TODO(crbug.com/1370954): Save NotRestoredReasons in Document instead of
-  // Frame.
-  return NotRestoredReasonsBuilder(script_state,
-                                   loader->GetFrame()->GetNotRestoredReasons());
-}
-
-ScriptValue PerformanceNavigationTiming::NotRestoredReasonsBuilder(
-    ScriptState* script_state,
-    const mojom::blink::BackForwardCacheNotRestoredReasonsPtr& reasons) const {
-  if (!reasons)
-    return ScriptValue::CreateNull(script_state->GetIsolate());
-  V8ObjectBuilder builder(script_state);
-  builder.AddBoolean("blocked", reasons->blocked);
-  builder.AddString("url", AtomicString(reasons->same_origin_details
-                                            ? reasons->same_origin_details->url
-                                            : ""));
-  builder.AddString("src", AtomicString(reasons->same_origin_details
-                                            ? reasons->same_origin_details->src
-                                            : ""));
-  builder.AddString("id", AtomicString(reasons->same_origin_details
-                                           ? reasons->same_origin_details->id
-                                           : ""));
-  builder.AddString("name",
-                    AtomicString(reasons->same_origin_details
-                                     ? reasons->same_origin_details->name
-                                     : ""));
-  Vector<AtomicString> reason_strings;
-  Vector<v8::Local<v8::Value>> children_result;
-  if (reasons->same_origin_details) {
-    for (const auto& reason : reasons->same_origin_details->reasons) {
-      reason_strings.push_back(reason);
-    }
-    for (const auto& child : reasons->same_origin_details->children) {
-      children_result.push_back(
-          NotRestoredReasonsBuilder(script_state, child).V8Value());
-    }
-  }
-  builder.Add("reasons", reason_strings);
-  builder.Add("children", children_result);
-  return builder.GetScriptValue();
-}
-
 void PerformanceNavigationTiming::BuildJSONValue(
     V8ObjectBuilder& builder) const {
   PerformanceResourceTiming::BuildJSONValue(builder);
@@ -379,11 +330,5 @@ void PerformanceNavigationTiming::BuildJSONValue(
         "activationStart",
         PerformanceNavigationTimingActivationStart::activationStart(*this));
   }
-
-  if (RuntimeEnabledFeatures::BackForwardCacheNotRestoredReasonsEnabled(
-          ExecutionContext::From(builder.GetScriptState()))) {
-    builder.Add("notRestoredReasons",
-                notRestoredReasons(builder.GetScriptState()));
-  }
 }
 }  // namespace blink
diff --git a/blink/renderer/core/timing/performance_navigation_timing.h b/blink/renderer/core/timing/performance_navigation_timing.h
@@ -5,7 +5,6 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_TIMING_PERFORMANCE_NAVIGATION_TIMING_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_TIMING_PERFORMANCE_NAVIGATION_TIMING_H_
 
-#include "third_party/blink/public/mojom/back_forward_cache_not_restored_reasons.mojom-blink.h"
 #include "third_party/blink/public/web/web_navigation_type.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/dom/dom_high_res_time_stamp.h"
@@ -56,7 +55,6 @@ class CORE_EXPORT PerformanceNavigationTiming final
   DOMHighResTimeStamp loadEventEnd() const;
   AtomicString type() const;
   uint16_t redirectCount() const;
-  ScriptValue notRestoredReasons(ScriptState* script_state) const;
 
   // PerformanceResourceTiming overrides:
   DOMHighResTimeStamp fetchStart() const override;
@@ -90,10 +88,6 @@ class CORE_EXPORT PerformanceNavigationTiming final
   AtomicString AlpnNegotiatedProtocol() const override;
   AtomicString ConnectionInfo() const override;
 
-  ScriptValue NotRestoredReasonsBuilder(
-      ScriptState* script_state,
-      const mojom::blink::BackForwardCacheNotRestoredReasonsPtr& reasons) const;
-
   scoped_refptr<ResourceTimingInfo> resource_timing_info_;
 };
 }  // namespace blink

diff --git a/blink/renderer/core/timing/performance_navigation_timing.idl b/blink/renderer/core/timing/performance_navigation_timing.idl
@@ -23,6 +23,5 @@ enum NavigationType {
     readonly attribute DOMHighResTimeStamp loadEventEnd;
     readonly attribute NavigationType      type;
     readonly attribute unsigned short      redirectCount;
-    [RuntimeEnabled=BackForwardCacheNotRestoredReasons, CallWith=ScriptState] readonly attribute object notRestoredReasons;
     [CallWith=ScriptState, ImplementedAs=toJSONForBinding] object toJSON();
 };
diff --git a/blink/web_tests/NeverFixTests b/blink/web_tests/NeverFixTests
@@ -1954,7 +1954,3 @@ crbug.com/1293679 virtual/pending-beacon/external/wpt/pending_beacon/* [ Pass ]
 # Experimental JS shared memory features
 crbug.com/1351118 wpt_internal/js/shared_memory/* [ Skip ]
 crbug.com/1351118 virtual/js-shared-memory/wpt_internal/js/shared_memory/* [ Pass ]
-
-# Feature is not yet launched, so skip the base.
-crbug.com/1326344 external/wpt/performance-timeline/not-restored-reasons/* [ Skip ]
-crbug.com/1326344 virtual/not-restored-reasons/external/wpt/performance-timeline/not-restored-reasons/* [ Pass ]
diff --git a/blink/web_tests/VirtualTestSuites b/blink/web_tests/VirtualTestSuites
@@ -1313,11 +1313,5 @@
     "platforms": ["Linux", "Mac", "Win"],
     "bases": ["external/wpt/custom-elements/scoped-registry"],
     "args": ["--disable-blink-features=ScopedCustomElementRegistry"]
-  },
-  {
-    "prefix": "not-restored-reasons",
-    "platforms": ["Linux", "Mac", "Win"],
-    "bases": ["external/wpt/performance-timeline/not-restored-reasons/"],
-    "args": ["--enable-features=BackForwardCacheSendNotRestoredReasons"]
   }
 ]
diff --git a/...l/wpt/html/browsers/browsing-the-web/remote-context-helper-tests/resources/test-helper.js b/...l/wpt/html/browsers/browsing-the-web/remote-context-helper-tests/resources/test-helper.js
@@ -46,40 +46,3 @@ async function assertHeaderIsAsExpected(
         return res.headers.get(headerName);
       }, [headerName]), 'header is set');
 }
-
-async function  assertNotRestoredReasonsEquals(
-  remoteContextHelper, blocked, url, src, id, name, reasons, children) {
-  let result = await remoteContextHelper.executeScript(() => {
-    return performance.getEntriesByType('navigation')[0].notRestoredReasons;
-  });
-  assertReasonsStructEquals(result, blocked, url, src, id, name, reasons, children);
-}
-
-function assertReasonsStructEquals(result, blocked, url, src, id, name, reasons, children) {
-  assert_equals(result.blocked, blocked);
-  assert_equals(result.url, url);
-  assert_equals(result.src, src);
-  assert_equals(result.id, id);
-  assert_equals(result.name, name);
-  // Reasons should match.
-  assert_equals(result.reasons.length, reasons.length);
-  reasons.sort();
-  result.reasons.sort();
-  for (let i=0; i<reasons.length; i++) {
-    assert_equals(result.reasons[i], reasons[i]);
-  }
-  // Children should match.
-  assert_equals(result.children.length, children.length);
-  children.sort();
-  result.children.sort();
-  for (let j=0; j<children.length; j++) {
-    assertReasonsStructEquals(result.children[0],
-      children[0].blocked,
-      children[0].url,
-      children[0].src,
-      children[0].id,
-      children[0].name,
-      children[0].reasons,
-      children[0].children);
-  }
-}
diff --git a/...performance-timeline/not-restored-reasons/performance-navigation-timing-bfcache.window.js b/...performance-timeline/not-restored-reasons/performance-navigation-timing-bfcache.window.js