Partial reland 1/2: [metrics] Start loading visible tabs before creating FirstWebContentsProfiler.

This is a partial reland of https://crrev.com/c/chromium/src/+/2062692.
We confirmed that the CL affected startup metrics on Mac. However, our
assessment is that the change is correct (restored tabs are painted
before non-restored tabs more often). As a precaution, the
original CL is split in two:
- Start loading restored tabs earlier (this CL)
- Enforce stricter requirements in FirstWebContentsProfiler (http://crrev.com/c/chromium/src/+/2097368)

Original change description:

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: I2964de553c0195ba94a35dc41d17c4a28cc5d05d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2095970
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749327}

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,21 @@ void ResizeWebContents(content::WebContents* web_contents,
   content::RenderWidgetHostView* view = web_contents->GetRenderWidgetHostView();
   if (view)
     view->SetBounds(new_bounds);
+#else
+// The Mac implementation is in web_contents_sizer.mm.
+#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
+// The Mac implementation is in web_contents_sizer.mm.
+#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));
+}