Skip to content

Commit

Permalink
Provide WebContents::CreateParams to tab helpers.
Browse files Browse the repository at this point in the history
With browser-side navigation enabled, navigations in new foreground tabs are
considered to have started in the background. This is a change in behavior.
This causes page load metrics to incorrectly consider these page loads to have
started in the background, which skews all of Chrome's page load metrics
(PageLoad.* in UMA).

clamy explains:
"""
the background/foreground issue seems to be due to the ordering of calls in
chrome/browser/ui/browser_navigator.cc when we create a new tab.
What happens is:
- we first create a WebContents and ask it to navigate
(https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?type=cs&l=557).
This result in the creation of a NavigationHandle, so we fire DidStartNavigation.
In the current architecture, we fire DidStartNavigation after receiving
 DidStartProvisionalLoad.
- then we insert the WebContents in the tab strip
(https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?type=cs&l=589).
This fires WebContentsImpl::WasShown.
"""

This patch implements Nasko's proposed fix to expose the WebContents
CreateParams to interested WebContentsObservers, page load metrics's
MetricsWebContentsObserver being the first consumer.

We don't have the CreateParams in all places where a WebContents is passed to
TabHelpers, so we wrap the CreateParams in a base::Optional<> and provide
CreateParams in cases where they are available.

BUG=725347

Review-Url: https://codereview.chromium.org/2894973002
Cr-Commit-Position: refs/heads/master@{#474895}
  • Loading branch information
bmcquade authored and Commit bot committed May 26, 2017
1 parent a669662 commit 42b7206
Show file tree
Hide file tree
Showing 28 changed files with 174 additions and 93 deletions.
3 changes: 2 additions & 1 deletion android_webview/browser/aw_web_contents_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ void AwWebContentsDelegate::WebContentsCreated(
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) {
content::WebContents* new_contents,
const base::Optional<content::WebContents::CreateParams>& create_params) {
AwContentsIoThreadClient::RegisterPendingContents(new_contents);
}

Expand Down
15 changes: 9 additions & 6 deletions android_webview/browser/aw_web_contents_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,15 @@ class AwWebContentsDelegate

void NavigationStateChanged(content::WebContents* source,
content::InvalidateTypes changed_flags) override;
void WebContentsCreated(content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) override;
void WebContentsCreated(
content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents,
const base::Optional<content::WebContents::CreateParams>& create_params)
override;

void CloseContents(content::WebContents* source) override;
void ActivateContents(content::WebContents* contents) override;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/android/tab_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ TabAndroid* TabAndroid::GetNativeTab(JNIEnv* env, const JavaRef<jobject>& obj) {
void TabAndroid::AttachTabHelpers(content::WebContents* web_contents) {
DCHECK(web_contents);

TabHelpers::AttachTabHelpers(web_contents);
TabHelpers::AttachTabHelpers(web_contents, base::nullopt);
}

TabAndroid::TabAndroid(JNIEnv* env, const JavaRef<jobject>& obj)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ void TabWebContentsDelegateAndroid::AddNewContents(
// Can't create a new contents for the current tab - invalid case.
DCHECK_NE(disposition, WindowOpenDisposition::CURRENT_TAB);

TabHelpers::AttachTabHelpers(new_contents);
TabHelpers::AttachTabHelpers(new_contents, base::nullopt);

JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobject> obj = GetJavaDelegate(env);
Expand Down
14 changes: 8 additions & 6 deletions chrome/browser/devtools/devtools_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1047,12 +1047,14 @@ void DevToolsWindow::AddNewContents(WebContents* source,
}
}

void DevToolsWindow::WebContentsCreated(WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
WebContents* new_contents) {
void DevToolsWindow::WebContentsCreated(
WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
WebContents* new_contents,
const base::Optional<content::WebContents::CreateParams>& create_params) {
if (target_url.SchemeIs(content::kChromeDevToolsScheme) &&
target_url.path().rfind("toolbox.html") != std::string::npos) {
CHECK(can_dock_);
Expand Down
16 changes: 10 additions & 6 deletions chrome/browser/devtools/devtools_window.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "chrome/browser/devtools/devtools_contents_resizing_strategy.h"
#include "chrome/browser/devtools/devtools_toggle_action.h"
#include "chrome/browser/devtools/devtools_ui_bindings.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_delegate.h"
#include "content/public/browser/web_contents_observer.h"

Expand Down Expand Up @@ -288,12 +289,15 @@ class DevToolsWindow : public DevToolsUIBindings::Delegate,
const gfx::Rect& initial_rect,
bool user_gesture,
bool* was_blocked) override;
void WebContentsCreated(content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) override;
void WebContentsCreated(
content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents,
const base::Optional<content::WebContents::CreateParams>& create_params)
override;
void CloseContents(content::WebContents* source) override;
void ContentsZoomChange(bool zoom_in) override;
void BeforeUnloadFired(content::WebContents* tab,
Expand Down
14 changes: 12 additions & 2 deletions chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h"
#include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
#include "chrome/browser/page_load_metrics/page_load_tracker.h"
#include "chrome/browser/prerender/prerender_contents.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/page_load_metrics/page_load_metrics_messages.h"
#include "chrome/common/page_load_metrics/page_load_timing.h"
Expand Down Expand Up @@ -69,24 +70,33 @@ UserInitiatedInfo CreateUserInitiatedInfo(

MetricsWebContentsObserver::MetricsWebContentsObserver(
content::WebContents* web_contents,
const base::Optional<content::WebContents::CreateParams>& create_params,
std::unique_ptr<PageLoadMetricsEmbedderInterface> embedder_interface)
: content::WebContentsObserver(web_contents),
in_foreground_(false),
in_foreground_(create_params ? !create_params->initially_hidden : false),
embedder_interface_(std::move(embedder_interface)),
has_navigated_(false),
page_load_metrics_binding_(web_contents, this) {
// Prerender's CreateParams erroneously reports that it is not initially
// hidden, so we manually override visibility state for prerender.
const bool is_prerender =
prerender::PrerenderContents::FromWebContents(web_contents) != nullptr;
if (is_prerender)
in_foreground_ = false;

RegisterInputEventObserver(web_contents->GetRenderViewHost());
}

// static
MetricsWebContentsObserver* MetricsWebContentsObserver::CreateForWebContents(
content::WebContents* web_contents,
const base::Optional<content::WebContents::CreateParams>& create_params,
std::unique_ptr<PageLoadMetricsEmbedderInterface> embedder_interface) {
DCHECK(web_contents);

MetricsWebContentsObserver* metrics = FromWebContents(web_contents);
if (!metrics) {
metrics = new MetricsWebContentsObserver(web_contents,
metrics = new MetricsWebContentsObserver(web_contents, create_params,
std::move(embedder_interface));
web_contents->SetUserData(UserDataKey(), base::WrapUnique(metrics));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ class MetricsWebContentsObserver
// Note that the returned metrics is owned by the web contents.
static MetricsWebContentsObserver* CreateForWebContents(
content::WebContents* web_contents,
const base::Optional<content::WebContents::CreateParams>& create_params,
std::unique_ptr<PageLoadMetricsEmbedderInterface> embedder_interface);
MetricsWebContentsObserver(
content::WebContents* web_contents,
const base::Optional<content::WebContents::CreateParams>& create_params,
std::unique_ptr<PageLoadMetricsEmbedderInterface> embedder_interface);
~MetricsWebContentsObserver() override;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class MetricsWebContentsObserverTest : public ChromeRenderViewHostTestHarness {
// calling AttachObserver() again. Otherwise they will use-after-free the
// observer_.
observer_ = MetricsWebContentsObserver::CreateForWebContents(
web_contents(), base::WrapUnique(embedder_interface_));
web_contents(), base::nullopt, base::WrapUnique(embedder_interface_));
observer_->WasShown();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ void PageLoadMetricsObserverTestHarness::SetUp() {
SetContents(CreateTestWebContents());
NavigateAndCommit(GURL("http://www.google.com"));
observer_ = MetricsWebContentsObserver::CreateForWebContents(
web_contents(),
web_contents(), base::nullopt,
base::MakeUnique<TestPageLoadMetricsEmbedderInterface>(this));
web_contents()->WasShown();
}
Expand Down
25 changes: 25 additions & 0 deletions chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,31 @@ IN_PROC_BROWSER_TEST_P(PageLoadMetricsBrowserTest, NewPage) {
EXPECT_FALSE(NoPageLoadMetricsRecorded());
}

IN_PROC_BROWSER_TEST_P(PageLoadMetricsBrowserTest, NewPageInNewForegroundTab) {
ASSERT_TRUE(embedded_test_server()->Start());

// The IPCTypeVerifier watches for IPCs in the main web contents. This test
// navigates in a new web contents, so we need to indicate that we expect no
// timing updates in the main web contents.
ExpectNoTimingUpdates();

chrome::NavigateParams params(browser(),
embedded_test_server()->GetURL("/title1.html"),
ui::PAGE_TRANSITION_LINK);
params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
chrome::Navigate(&params);
auto waiter = base::MakeUnique<PageLoadMetricsWaiter>(params.target_contents);
waiter->AddMainFrameExpectation(TimingField::LOAD_EVENT);
waiter->Wait();

// Due to crbug.com/725347, with browser side navigation enabled, navigations
// in new tabs were recorded as starting in the background. Here we verify
// that navigations initiated in a new tab are recorded as happening in the
// foreground.
histogram_tester_.ExpectTotalCount(internal::kHistogramLoad, 1);
histogram_tester_.ExpectTotalCount(internal::kBackgroundHistogramLoad, 0);
}

IN_PROC_BROWSER_TEST_P(PageLoadMetricsBrowserTest, NoPaintForEmptyDocument) {
ASSERT_TRUE(embedded_test_server()->Start());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,11 @@ bool PageLoadMetricsEmbedder::IsNewTabPageUrl(const GURL& url) {
} // namespace

void InitializePageLoadMetricsForWebContents(
content::WebContents* web_contents) {
content::WebContents* web_contents,
const base::Optional<content::WebContents::CreateParams>& create_params) {
page_load_metrics::MetricsWebContentsObserver::CreateForWebContents(
web_contents, base::MakeUnique<PageLoadMetricsEmbedder>(web_contents));
web_contents, create_params,
base::MakeUnique<PageLoadMetricsEmbedder>(web_contents));
}

} // namespace chrome
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ class WebContents;
namespace chrome {

void InitializePageLoadMetricsForWebContents(
content::WebContents* web_contents);
content::WebContents* web_contents,
const base::Optional<content::WebContents::CreateParams>& create_params);

} // namespace chrome

Expand Down
12 changes: 8 additions & 4 deletions chrome/browser/prerender/prerender_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,11 @@ void PrerenderContents::StartPrerendering(

prerendering_has_started_ = true;

prerender_contents_.reset(CreateWebContents(session_storage_namespace));
TabHelpers::AttachTabHelpers(prerender_contents_.get());
content::WebContents::CreateParams create_params =
WebContents::CreateParams(profile_);
prerender_contents_.reset(
CreateWebContents(create_params, session_storage_namespace));
TabHelpers::AttachTabHelpers(prerender_contents_.get(), create_params);
content::WebContentsObserver::Observe(prerender_contents_.get());

// Tag the prerender contents with the task manager specific prerender tag, so
Expand Down Expand Up @@ -447,13 +450,14 @@ void PrerenderContents::OnRenderViewHostCreated(
}

WebContents* PrerenderContents::CreateWebContents(
const content::WebContents::CreateParams& create_params,
SessionStorageNamespace* session_storage_namespace) {
// TODO(ajwong): Remove the temporary map once prerendering is aware of
// multiple session storage namespaces per tab.
content::SessionStorageNamespaceMap session_storage_namespace_map;
session_storage_namespace_map[std::string()] = session_storage_namespace;
return WebContents::CreateWithSessionStorage(
WebContents::CreateParams(profile_), session_storage_namespace_map);
return WebContents::CreateWithSessionStorage(create_params,
session_storage_namespace_map);
}

void PrerenderContents::NotifyPrerenderStart() {
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/prerender/prerender_contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "chrome/common/prerender_types.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/common/referrer.h"
#include "mojo/public/cpp/bindings/binding.h"
Expand All @@ -38,7 +39,6 @@ class ProcessMetrics;
namespace content {
class RenderViewHost;
class SessionStorageNamespace;
class WebContents;
}

namespace history {
Expand Down Expand Up @@ -281,6 +281,7 @@ class PrerenderContents : public content::NotificationObserver,
}

content::WebContents* CreateWebContents(
const content::WebContents::CreateParams& create_params,
content::SessionStorageNamespace* session_storage_namespace);

PrerenderMode prerender_mode_;
Expand Down
16 changes: 9 additions & 7 deletions chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1695,17 +1695,19 @@ bool Browser::ShouldCreateWebContents(
return true;
}

void Browser::WebContentsCreated(WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
WebContents* new_contents) {
void Browser::WebContentsCreated(
WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
WebContents* new_contents,
const base::Optional<WebContents::CreateParams>& create_params) {
// Adopt the WebContents now, so all observers are in place, as the network
// requests for its initial navigation will start immediately. The WebContents
// will later be inserted into this browser using Browser::Navigate via
// AddNewContents.
TabHelpers::AttachTabHelpers(new_contents);
TabHelpers::AttachTabHelpers(new_contents, create_params);

// Make the tab show up in the task manager.
task_manager::WebContentsTags::CreateForTabContents(new_contents);
Expand Down
15 changes: 9 additions & 6 deletions chrome/browser/ui/browser.h
Original file line number Diff line number Diff line change
Expand Up @@ -635,12 +635,15 @@ class Browser : public TabStripModelObserver,
const GURL& target_url,
const std::string& partition_id,
content::SessionStorageNamespace* session_storage_namespace) override;
void WebContentsCreated(content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) override;
void WebContentsCreated(
content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents,
const base::Optional<content::WebContents::CreateParams>& create_params)
override;
void RendererUnresponsive(
content::WebContents* source,
const content::WebContentsUnresponsiveState& unresponsive_state) override;
Expand Down
9 changes: 6 additions & 3 deletions chrome/browser/ui/browser_navigator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,10 @@ using content::WebContents;

class BrowserNavigatorWebContentsAdoption {
public:
static void AttachTabHelpers(content::WebContents* contents) {
TabHelpers::AttachTabHelpers(contents);
static void AttachTabHelpers(
content::WebContents* contents,
const base::Optional<WebContents::CreateParams>& create_params) {
TabHelpers::AttachTabHelpers(contents, create_params);

// Make the tab show up in the task manager.
task_manager::WebContentsTags::CreateForTabContents(contents);
Expand Down Expand Up @@ -381,7 +383,8 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
// New tabs can have WebUI URLs that will make calls back to arbitrary
// tab helpers, so the entire set of tab helpers needs to be set up
// immediately.
BrowserNavigatorWebContentsAdoption::AttachTabHelpers(target_contents);
BrowserNavigatorWebContentsAdoption::AttachTabHelpers(target_contents,
create_params);
#if BUILDFLAG(ENABLE_EXTENSIONS)
extensions::TabHelper::FromWebContents(target_contents)->
SetExtensionAppById(params.extension_app_id);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/browser_tab_strip_model_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ Browser* BrowserTabStripModelDelegate::CreateNewStripWithContents(

void BrowserTabStripModelDelegate::WillAddWebContents(
content::WebContents* contents) {
TabHelpers::AttachTabHelpers(contents);
TabHelpers::AttachTabHelpers(contents, base::nullopt);

// Make the tab show up in the task manager.
task_manager::WebContentsTags::CreateForTabContents(contents);
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/ui/tab_helpers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ const char kTabContentsAttachedTabHelpersUserDataKey[] =
} // namespace

// static
void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
void TabHelpers::AttachTabHelpers(
WebContents* web_contents,
const base::Optional<WebContents::CreateParams>& create_params) {
// If already adopted, nothing to be done.
base::SupportsUserData::Data* adoption_tag =
web_contents->GetUserData(&kTabContentsAttachedTabHelpersUserDataKey);
Expand Down Expand Up @@ -202,7 +204,7 @@ void TabHelpers::AttachTabHelpers(WebContents* web_contents) {
MixedContentSettingsTabHelper::CreateForWebContents(web_contents);
NavigationCorrectionTabObserver::CreateForWebContents(web_contents);
NavigationMetricsRecorder::CreateForWebContents(web_contents);
chrome::InitializePageLoadMetricsForWebContents(web_contents);
chrome::InitializePageLoadMetricsForWebContents(web_contents, create_params);
PermissionRequestManager::CreateForWebContents(web_contents);
PopupBlockerTabHelper::CreateForWebContents(web_contents);
PrefsTabHelper::CreateForWebContents(web_contents);
Expand Down
Loading

0 comments on commit 42b7206

Please sign in to comment.