Revert "[metrics] Start loading visible tabs before creating FirstWebContentsProfiler."

This reverts commit 47b6798d
("https://chromium-review.googlesource.com/c/chromium/src/+/1976500).

This commit is suspected to have affected the 95th percentile of
SessionRestore.ForegroundTabFirstPaint4 on Mac. Reverting it
temporarily to verify that hypothesis.

TBR=sky@chromium.org

Bug: 1035419, 1020549, 1022492
Change-Id: Ide1787a64b28c9f731ee1cb4f8bf0c451bb072e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2062692Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742609}

chrome/browser/metrics/first_web_contents_profiler.cc

@@ -55,6 +55,10 @@ enum class FinishReason {
   kMaxValue = kAbandonNoInitiallyVisibleContent
 };
 
+// Per documentation in navigation_request.cc, a navigation id is guaranteed
+// nonzero.
+constexpr int64_t kInvalidNavigationId = 0;
+
 void RecordFinishReason(FinishReason finish_reason) {
   base::UmaHistogramEnumeration("Startup.FirstWebContents.FinishReason",
                                 finish_reason);
@@ -81,6 +85,9 @@ class FirstWebContentsProfiler : public content::WebContentsObserver {
   // Logs |finish_reason| to UMA and deletes this FirstWebContentsProfiler.
   void FinishedCollectingMetrics(FinishReason finish_reason);
 
+  // The first NavigationHandle id observed by this.
+  int64_t first_navigation_id_ = kInvalidNavigationId;
+
   // Whether a main frame navigation finished since this was created.
   bool did_finish_first_navigation_ = false;
 
@@ -96,11 +103,6 @@ FirstWebContentsProfiler::FirstWebContentsProfiler(
     : content::WebContentsObserver(web_contents),
       memory_pressure_listener_(base::BindRepeating(
           &startup_metric_utils::OnMemoryPressureBeforeFirstNonEmptyPaint)) {
-  // FirstWebContentsProfiler is created before the main MessageLoop starts
-  // running. At that time, any visible WebContents should have a pending
-  // NavigationEntry, i.e. should have dispatched DidStartNavigation() but not
-  // DidFinishNavigation().
-  DCHECK(web_contents->GetController().GetPendingEntry());
 }
 
 void FirstWebContentsProfiler::DidStartNavigation(
@@ -111,11 +113,22 @@ void FirstWebContentsProfiler::DidStartNavigation(
     return;
   }
 
-  // FirstWebContentsProfiler is created after DidStartNavigation() has been
-  // dispatched for the first top-level navigation. If another
-  // DidStartNavigation() is received, it means that a new navigation was
-  // initiated.
-  FinishedCollectingMetrics(FinishReason::kAbandonNewNavigation);
+  // TODO(https://crbug.com/1035419): Ensure that all visible tabs start
+  // navigating before FirstWebContentsProfiler creation and always handle
+  // a top-level DidStartNavigation() as a new navigation.
+  // Upcoming CL: https://crrev.com/c/chromium/src/+/1976500
+
+  if (first_navigation_id_ != kInvalidNavigationId) {
+    // Abandon if this is not the first observed top-level navigation.
+    DCHECK_NE(first_navigation_id_, navigation_handle->GetNavigationId());
+    FinishedCollectingMetrics(FinishReason::kAbandonNewNavigation);
+    return;
+  }
+
+  DCHECK(!did_finish_first_navigation_);
+
+  // Keep track of the first top-level navigation id observed by this.
+  first_navigation_id_ = navigation_handle->GetNavigationId();
 }
 
 void FirstWebContentsProfiler::DidFinishNavigation(
@@ -137,10 +150,21 @@ void FirstWebContentsProfiler::DidFinishNavigation(
     return;
   }
 
-  // It is not possible to get a second top-level DidFinishNavigation() without
-  // first having a DidStartNavigation(), which would have deleted |this|.
-  DCHECK(!did_finish_first_navigation_);
+  if (first_navigation_id_ == kInvalidNavigationId) {
+    // Keep track of the first top-level navigation id observed by this.
+    //
+    // Note: FirstWebContentsProfiler may be created before or after
+    // DidStartNavigation() is dispatched for the first navigation, which is why
+    // |first_navigation_id_| may be set in DidStartNavigation() or
+    // DidFinishNavigation().
+    first_navigation_id_ = navigation_handle->GetNavigationId();
+  } else if (first_navigation_id_ != navigation_handle->GetNavigationId()) {
+    // Abandon if this is not the first observed top-level navigation.
+    FinishedCollectingMetrics(FinishReason::kAbandonNewNavigation);
+    return;
+  }
 
+  DCHECK(!did_finish_first_navigation_);
   did_finish_first_navigation_ = true;
 
   startup_metric_utils::RecordFirstWebContentsMainNavigationStart(

chrome/browser/ui/browser_tabrestore.cc

@@ -21,11 +21,9 @@
 #include "components/sessions/content/content_serialized_navigation_builder.h"
 #include "components/tab_groups/tab_group_id.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/restore_type.h"
 #include "content/public/browser/session_storage_namespace.h"
 #include "content/public/browser/web_contents.h"
-#include "ui/gfx/geometry/size.h"
 
 using content::NavigationEntry;
 using content::RestoreType;
@@ -91,32 +89,6 @@ std::unique_ptr<WebContents> CreateRestoredTab(
   return web_contents;
 }
 
-// Start loading a restored tab after adding it to its browser, if visible.
-//
-// Without this, loading starts when
-// WebContentsImpl::UpdateWebContentsVisibility(VISIBLE) is invoked, which
-// happens at a different time on Mac vs. other desktop platform due to a
-// different windowing system. Starting to load here ensures consistent behavior
-// across desktop platforms and allows FirstWebContentsProfiler to have strict
-// cross-platform expectations about events it observes.
-void LoadRestoredTabIfVisible(Browser* browser,
-                              content::WebContents* web_contents) {
-  if (web_contents->GetVisibility() != content::Visibility::VISIBLE)
-    return;
-
-  DCHECK_EQ(browser->tab_strip_model()->GetActiveWebContents(), web_contents);
-  // A layout should already have been performed to determine the contents size.
-  // The contents size should not be empty, unless the browser size and restored
-  // size are also empty.
-  DCHECK(!browser->window()->GetContentsSize().IsEmpty() ||
-         (browser->window()->GetBounds().IsEmpty() &&
-          browser->window()->GetRestoredBounds().IsEmpty()));
-  DCHECK_EQ(GetWebContentsSize(web_contents),
-            browser->window()->GetContentsSize());
-
-  web_contents->GetController().LoadIfNecessary();
-}
-
 }  // namespace
 
 WebContents* AddRestoredTab(
@@ -133,11 +105,10 @@ WebContents* AddRestoredTab(
     content::SessionStorageNamespace* session_storage_namespace,
     const std::string& user_agent_override,
     bool from_session_restore) {
-  const bool initially_hidden = !select || browser->window()->IsMinimized();
   std::unique_ptr<WebContents> web_contents = CreateRestoredTab(
       browser, navigations, selected_navigation, extension_app_id,
       from_last_session, last_active_time, session_storage_namespace,
-      user_agent_override, initially_hidden, from_session_restore);
+      user_agent_override, !select, from_session_restore);
 
   int add_types = select ? TabStripModel::ADD_ACTIVE : TabStripModel::ADD_NONE;
   if (pin) {
@@ -155,16 +126,24 @@ WebContents* AddRestoredTab(
                                                      group.value());
   }
 
-  if (initially_hidden) {
+  if (select) {
+    if (
+#if defined(OS_MACOSX)
+        // Activating a window on another space causes the system to switch to
+        // that space. Since the session restore process shows and activates
+        // windows itself, activating windows here should be safe to skip.
+        // Cautiously apply only to macOS, for now (https://crbug.com/1019048).
+        !from_session_restore &&
+#endif
+        !browser->window()->IsMinimized())
+      browser->window()->Activate();
+  } else {
     // We set the size of the view here, before Blink does its initial layout.
     // If we don't, the initial layout of background tabs will be performed
     // with a view width of 0, which may cause script outputs and anchor link
     // location calculations to be incorrect even after a new layout with
     // proper view dimensions. TabStripModel::AddWebContents() contains similar
     // logic.
-    //
-    // TODO(https://crbug.com/1040221): There should be a way to ask the browser
-    // to perform a layout so that size of the hidden WebContents is right.
     gfx::Size size = browser->window()->GetContentsSize();
     // Fallback to the restore bounds if it's empty as the window is not shown
     // yet and the bounds may not be available on all platforms.
@@ -172,28 +151,11 @@ WebContents* AddRestoredTab(
       size = browser->window()->GetRestoredBounds().size();
     ResizeWebContents(raw_web_contents, gfx::Rect(size));
     raw_web_contents->WasHidden();
-  } else {
-    const bool should_activate =
-#if defined(OS_MACOSX)
-        // Activating a window on another space causes the system to switch to
-        // that space. Since the session restore process shows and activates
-        // windows itself, activating windows here should be safe to skip.
-        // Cautiously apply only to macOS, for now (https://crbug.com/1019048).
-        !from_session_restore;
-#else
-        true;
-#endif
-    if (should_activate)
-      browser->window()->Activate();
   }
-
   SessionService* session_service =
       SessionServiceFactory::GetForProfileIfExisting(browser->profile());
   if (session_service)
     session_service->TabRestored(raw_web_contents, pin);
-
-  LoadRestoredTabIfVisible(browser, raw_web_contents);
-
   return raw_web_contents;
 }
 
@@ -220,9 +182,6 @@ WebContents* ReplaceRestoredTab(
       insertion_index + 1, std::move(web_contents),
       TabStripModel::ADD_ACTIVE | TabStripModel::ADD_INHERIT_OPENER);
   tab_strip->CloseWebContentsAt(insertion_index, TabStripModel::CLOSE_NONE);
-
-  LoadRestoredTabIfVisible(browser, raw_web_contents);
-
   return raw_web_contents;
 }
 

chrome/browser/ui/browser_tabrestore_browsertest.cc

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/browser_tabrestore.h"
-
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/ui/browser.h"
@@ -16,7 +14,6 @@
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/sessions/core/tab_restore_service.h"
-#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
@@ -83,17 +80,12 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
   Browser* browser = active_browser_list->get(0);
   RecentTabsSubMenuModel menu(nullptr, browser);
   menu.ExecuteCommand(RecentTabsSubMenuModel::GetFirstRecentTabsCommandId(), 0);
-  // There should be 3 restored tabs in the new browser. The active tab should
-  // be loading.
-  EXPECT_EQ(2u, active_browser_list->size());
-  Browser* restored_browser = active_browser_list->get(1);
-  EXPECT_EQ(3, restored_browser->tab_strip_model()->count());
-  EXPECT_TRUE(restored_browser->tab_strip_model()
-                  ->GetActiveWebContents()
-                  ->GetController()
-                  .GetPendingEntry());
   AwaitTabsReady(&queue, 2);
 
+  // There should be 3 restored tabs in the new browser.
+  EXPECT_EQ(2u, active_browser_list->size());
+  browser = active_browser_list->get(1);
+  EXPECT_EQ(3, browser->tab_strip_model()->count());
   // For the two test tabs we've just received "READY" DOM message.
   // But there won't be such message from the "about:blank" tab.
   // And it is possible that TabLoader hasn't loaded it yet.
@@ -101,7 +93,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
   // CheckVisbility on "about:blank".
   {
     content::WebContents* about_blank_contents =
-        restored_browser->tab_strip_model()->GetWebContentsAt(0);
+        browser->tab_strip_model()->GetWebContentsAt(0);
     EXPECT_EQ("about:blank", about_blank_contents->GetURL().spec());
     if (about_blank_contents->IsLoading() ||
         about_blank_contents->GetController().NeedsReload()) {
@@ -114,53 +106,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
   }
 
   // The middle tab only should have visible disposition.
-  CheckVisbility(restored_browser->tab_strip_model(), 1);
-}
-
-// Expect a selected restored tab to start loading synchronously.
-//
-// Previously, on Mac, a selected restored tab only started loading when a
-// native message indicated that the window was visible. On other platforms,
-// it started loading synchronously. https://crbug.com/1022492
-IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest,
-                       SelectedRestoredTabStartsLoading) {
-  sessions::SerializedNavigationEntry navigation_entry;
-  navigation_entry.set_index(0);
-  navigation_entry.set_virtual_url(GURL(url::kAboutBlankURL));
-
-  std::vector<sessions::SerializedNavigationEntry> navigations;
-  navigations.push_back(navigation_entry);
-
-  content::WebContents* web_contents = chrome::AddRestoredTab(
-      browser(), navigations, /* tab_index=*/1, /* selected_navigation=*/0,
-      /* extension_app_id=*/std::string(), /* raw_group_id=*/base::nullopt,
-      /* select=*/true, /* pin=*/false, /* from_last_session=*/true,
-      /* last_active_time=*/base::TimeTicks::Now(),
-      /* storage_namespace=*/nullptr,
-      /* user_agent_override=*/std::string(), /* from_session_restore=*/true);
-
-  EXPECT_TRUE(web_contents->GetController().GetPendingEntry());
-}
-
-// Expect a *non* selected restored tab to *not* start loading synchronously.
-IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest,
-                       NonSelectedRestoredTabDoesNotStartsLoading) {
-  sessions::SerializedNavigationEntry navigation_entry;
-  navigation_entry.set_index(0);
-  navigation_entry.set_virtual_url(GURL(url::kAboutBlankURL));
-
-  std::vector<sessions::SerializedNavigationEntry> navigations;
-  navigations.push_back(navigation_entry);
-
-  content::WebContents* web_contents = chrome::AddRestoredTab(
-      browser(), navigations, /* tab_index=*/1, /* selected_navigation=*/0,
-      /* extension_app_id=*/std::string(), /* raw_group_id=*/base::nullopt,
-      /* select=*/false, /* pin=*/false, /* from_last_session=*/true,
-      /* last_active_time=*/base::TimeTicks::Now(),
-      /* storage_namespace=*/nullptr,
-      /* user_agent_override=*/std::string(), /* from_session_restore=*/true);
-
-  EXPECT_FALSE(web_contents->GetController().GetPendingEntry());
+  CheckVisbility(browser->tab_strip_model(), 1);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, DelegateRestoreTabDisposition) {

chrome/browser/ui/web_contents_sizer.cc

@@ -6,13 +6,11 @@
 
 #include "build/build_config.h"
 #include "content/public/browser/web_contents.h"
-#include "ui/gfx/geometry/size.h"
 
 #if defined(USE_AURA)
 #include "ui/aura/window.h"
 #elif defined(OS_ANDROID)
 #include "content/public/browser/render_widget_host_view.h"
-#include "ui/android/view_android.h"
 #endif
 
 void ResizeWebContents(content::WebContents* web_contents,
@@ -24,19 +22,5 @@ void ResizeWebContents(content::WebContents* web_contents,
   content::RenderWidgetHostView* view = web_contents->GetRenderWidgetHostView();
   if (view)
     view->SetBounds(new_bounds);
-#else
-#error "ResizeWebContents not implemented for this platform"
-#endif
-}
-
-gfx::Size GetWebContentsSize(content::WebContents* web_contents) {
-#if defined(USE_AURA)
-  aura::Window* window = web_contents->GetNativeView();
-  return window->bounds().size();
-#elif defined(OS_ANDROID)
-  ui::ViewAndroid* view_android = web_contents->GetNativeView();
-  return view_android->bounds().size();
-#else
-#error "GetWebContentsSize not implemented for this platform"
 #endif
 }

chrome/browser/ui/web_contents_sizer.h

@@ -11,14 +11,10 @@ class WebContents;
 
 namespace gfx {
 class Rect;
-class Size;
 }
 
 // A platform-agnostic function to resize a WebContents.
 void ResizeWebContents(content::WebContents* web_contents,
                        const gfx::Rect& bounds);
 
-// A platform-agnostic function to get the size of a WebContents.
-gfx::Size GetWebContentsSize(content::WebContents* web_contents);
-
 #endif  // CHROME_BROWSER_UI_WEB_CONTENTS_SIZER_H_

chrome/browser/ui/web_contents_sizer.mm

@@ -7,7 +7,6 @@
 #import <Cocoa/Cocoa.h>
 
 #include "content/public/browser/web_contents.h"
-#include "ui/gfx/geometry/size.h"
 
 void ResizeWebContents(content::WebContents* web_contents,
                        const gfx::Rect& new_bounds) {
@@ -23,9 +22,3 @@ void ResizeWebContents(content::WebContents* web_contents,
                  new_bounds.size().height());
   [view setFrame:new_wcv_frame];
 }
-
-gfx::Size GetWebContentsSize(content::WebContents* web_contents) {
-  NSView* view = web_contents->GetNativeView().GetNativeNSView();
-  NSRect frame = [view frame];
-  return gfx::Size(NSWidth(frame), NSHeight(frame));
-}