[metrics] Start loading visible tabs before creating FirstWebContentsProfiler.

On Windows, a visible restored tab starts loading in the following stack,
before FirstWebContentsProfiler is created:

  content::NavigationControllerImpl::LoadIfNecessary
  content::NavigationControllerImpl::SetActive
  ...
  content::WebContentsImpl::UpdateWebContentsVisibility
  content::WebContentsViewAura::UpdateWebContentsVisibility
  ...
  BrowserView::Show
  SessionRestoreImpl::ShowBrowser
  SessionRestoreImpl::RestoreTab
  SessionRestoreImpl::RestoreTabsToBrowser
  SessionRestoreImpl::ProcessSessionWindows
  SessionRestoreImpl::ProcessSessionWindowsAndNotify
  SessionRestoreImpl::Restore
  SessionRestore::RestoreSession
  ...
  StartupBrowserCreator::Start
  ...
  content::BrowserMainLoop::PreMainMessageLoopRun

On Mac, it starts loading in the following stack, after
FirstWebContentsProfiler is created:

  content::NavigationControllerImpl::LoadIfNecessary
  content::NavigationControllerImpl::SetActive
  ...
  content::WebContentsImpl::UpdateWebContentsVisibility
  content::WebContentsViewMac::OnWindowVisibilityChanged
  -[WebContentsViewCocoa updateWebContentsVisibility]
  -[WebContentsViewCocoa windowChangedOcclusionState:]
  ...
  base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run
  ...
  ChromeBrowserMainParts::MainMessageLoopRun

This prevents FirstWebContentsProfiler from having strict cross-platform
expectations about events it observes. With this CL, we ensure that visible
restored tab start loading in the following stack on all platforms:

  content::NavigationControllerImpl::LoadIfNecessary
  chrome::`anonymous namespace'::CreateRestoredTab
  chrome::AddRestoredTab
  ...
  SessionRestoreImpl::Restore
  SessionRestore::RestoreSession
  ...
  StartupBrowserCreator::Start
  ...
  content::BrowserMainLoop::PreMainMessageLoopRun

This allows us to enforce strict cross-platform expectations about events
observed by FirstWebContentsProfiler, and also have the nice benefits that
navigation isn't delayed until we get a window visibility update during a
Mac session restore.

Bug: 1035419, 1020549, 1022492
Change-Id: I26f1a1a7cfa73cf95a3bdb32a83bf2dc1722d3f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1976500Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731969}

chrome/browser/metrics/first_web_contents_profiler.cc

@@ -51,10 +51,6 @@ 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);
@@ -79,9 +75,6 @@ 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;
 
@@ -90,7 +83,13 @@ class FirstWebContentsProfiler : public content::WebContentsObserver {
 
 FirstWebContentsProfiler::FirstWebContentsProfiler(
     content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {}
+    : content::WebContentsObserver(web_contents) {
+  // 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(
     content::NavigationHandle* navigation_handle) {
@@ -100,22 +99,11 @@ void FirstWebContentsProfiler::DidStartNavigation(
     return;
   }
 
-  // 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();
+  // 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);
 }
 
 void FirstWebContentsProfiler::DidFinishNavigation(
@@ -137,21 +125,10 @@ void FirstWebContentsProfiler::DidFinishNavigation(
     return;
   }
 
-  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;
-  }
-
+  // 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_);
+
   did_finish_first_navigation_ = true;
 
   startup_metric_utils::RecordFirstWebContentsMainNavigationStart(

chrome/browser/ui/browser_tabrestore.cc

@@ -21,9 +21,11 @@
 #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;
@@ -89,6 +91,32 @@ 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(
@@ -105,10 +133,11 @@ 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, !select, from_session_restore);
+      user_agent_override, initially_hidden, from_session_restore);
 
   int add_types = select ? TabStripModel::ADD_ACTIVE : TabStripModel::ADD_NONE;
   if (pin) {
@@ -126,24 +155,16 @@ WebContents* AddRestoredTab(
                                                      group.value());
   }
 
-  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 {
+  if (initially_hidden) {
     // 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.
@@ -151,11 +172,28 @@ 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;
 }
 
@@ -182,6 +220,9 @@ 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,6 +2,8 @@
 // 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"
@@ -14,6 +16,7 @@
 #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"
@@ -80,12 +83,17 @@ 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.
@@ -93,7 +101,7 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
   // CheckVisbility on "about:blank".
   {
     content::WebContents* about_blank_contents =
-        browser->tab_strip_model()->GetWebContentsAt(0);
+        restored_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()) {
@@ -106,7 +114,53 @@ IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, RecentTabsMenuTabDisposition) {
   }
 
   // The middle tab only should have visible disposition.
-  CheckVisbility(browser->tab_strip_model(), 1);
+  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());
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserTabRestoreTest, DelegateRestoreTabDisposition) {

chrome/browser/ui/web_contents_sizer.cc

@@ -6,11 +6,13 @@
 
 #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,
@@ -22,5 +24,19 @@ 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,10 +11,14 @@ 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,6 +7,7 @@
 #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) {
@@ -22,3 +23,9 @@ 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));
+}