Commit 949f7d47 authored by Justin Cohen's avatar Justin Cohen Committed by Commit Bot

[ios] Fix kRestoreNavigationTime histogram.

kRestoreNavigationTime was a timer created by WKBasedNavigationManagerImpl::Restore
and stopped when session restoration begins navigating to its final URL.

Unfortunately, WKBasedNavigationManagerImpl::Restore is called for every restored
WebState on startup.  Actual network access doesn't happen until a tab is selected. That
means if you have 10 tabs, background/kill/restore, navigate a bit, and then change tabs,
that second tab will have had a timer running since the app started.

Instead, start kRestoreNavigationTime when ios/web begins loading the restore page, and
finish the timer when ios/web begins loading the real page.  This should more correctly
tell how much time SlimNav restoration adds.

Bug: 1005044
Change-Id: I35fee36bb50ec627d7455d6215669a92a2b4bc6e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808355
Auto-Submit: Justin Cohen <justincohen@chromium.org>
Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Reviewed-by: default avatarGauthier Ambard <gambard@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697757}
parent 66e13e0a
......@@ -568,6 +568,8 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
}
self.webStateImpl->OnNavigationStarted(context);
self.webStateImpl->GetNavigationManagerImpl().OnNavigationStarted(
webViewURL);
return;
}
......@@ -618,8 +620,7 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
return;
}
self.webStateImpl->GetNavigationManagerImpl()
.OnRendererInitiatedNavigationStarted(webViewURL);
self.webStateImpl->GetNavigationManagerImpl().OnNavigationStarted(webViewURL);
// When a client-side redirect occurs while an interstitial warning is
// displayed, clear the warning and its navigation item, so that a new
......
......@@ -36,7 +36,7 @@ class LegacyNavigationManagerImpl : public NavigationManagerImpl {
void SetSessionController(CRWSessionController* session_controller) override;
void InitializeSession() override;
void OnNavigationItemsPruned(size_t pruned_item_count) override;
void OnRendererInitiatedNavigationStarted(const GURL& url) override;
void OnNavigationStarted(const GURL& url) override;
void OnNavigationItemCommitted() override;
CRWSessionController* GetSessionController() const override;
void AddTransientItem(const GURL& url) override;
......
......@@ -59,8 +59,7 @@ void LegacyNavigationManagerImpl::OnNavigationItemCommitted() {
delegate_->OnNavigationItemCommitted(item);
}
void LegacyNavigationManagerImpl::OnRendererInitiatedNavigationStarted(
const GURL& url) {}
void LegacyNavigationManagerImpl::OnNavigationStarted(const GURL& url) {}
CRWSessionController* LegacyNavigationManagerImpl::GetSessionController()
const {
......
......@@ -94,8 +94,8 @@ class NavigationManagerImpl : public NavigationManager {
virtual void OnNavigationItemsPruned(size_t pruned_item_count) = 0;
virtual void OnNavigationItemCommitted() = 0;
// Called when renderer-initiated navigation has started.
virtual void OnRendererInitiatedNavigationStarted(const GURL& url) = 0;
// Called when a navigation has started.
virtual void OnNavigationStarted(const GURL& url) = 0;
// Prepares for the deletion of WKWebView such as caching necessary data.
virtual void DetachFromWebView();
......
......@@ -1881,8 +1881,7 @@ TEST_P(NavigationManagerTest, ReloadWithUserAgentTypeOnIntenalUrl) {
->GetPendingItemInCurrentOrRestoredSession()
->SetVirtualURL(virtual_url);
[mock_wk_list_ setCurrentURL:base::SysUTF8ToNSString(url.spec())];
navigation_manager()->OnRendererInitiatedNavigationStarted(
GURL("http://www.1.com/virtual"));
navigation_manager()->OnNavigationStarted(GURL("http://www.1.com/virtual"));
navigation_manager()->ReloadWithUserAgentType(UserAgentType::DESKTOP);
......@@ -2089,14 +2088,14 @@ TEST_P(NavigationManagerTest, Restore) {
EXPECT_TRUE(pending_url.SchemeIsFile());
EXPECT_EQ("restore_session.html", pending_url.ExtractFileName());
EXPECT_EQ("http://www.url.com/0", pending_item->GetVirtualURL());
navigation_manager()->OnNavigationStarted(pending_url);
// Simulate the end effect of loading the restore session URL in web view.
pending_item->SetURL(urls[1]);
[mock_wk_list_ setCurrentURL:@"http://www.url.com/1"
backListURLs:@[ @"http://www.url.com/0" ]
forwardListURLs:@[ @"http://www.url.com/2" ]];
navigation_manager()->OnRendererInitiatedNavigationStarted(
GURL("http://www.url.com/2"));
navigation_manager()->OnNavigationStarted(GURL("http://www.url.com/2"));
}
ASSERT_TRUE(WaitUntilConditionOrTimeout(kWaitForPageLoadTimeout, ^{
......
......@@ -94,7 +94,7 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
void InitializeSession() override;
void OnNavigationItemsPruned(size_t pruned_item_count) override;
void OnNavigationItemCommitted() override;
void OnRendererInitiatedNavigationStarted(const GURL& url) override;
void OnNavigationStarted(const GURL& url) override;
void DetachFromWebView() override;
CRWSessionController* GetSessionController() const override;
void AddTransientItem(const GURL& url) override;
......
......@@ -98,14 +98,17 @@ void WKBasedNavigationManagerImpl::OnNavigationItemCommitted() {
}
}
void WKBasedNavigationManagerImpl::OnRendererInitiatedNavigationStarted(
const GURL& url) {
if (!wk_navigation_util::IsRestoreSessionUrl(url) &&
is_restore_session_in_progress_) {
// Session restoration navigations are rendered-initiated.
void WKBasedNavigationManagerImpl::OnNavigationStarted(const GURL& url) {
if (!is_restore_session_in_progress_)
return;
GURL target_url;
if (wk_navigation_util::IsRestoreSessionUrl(url) &&
!web::wk_navigation_util::ExtractTargetURL(url, &target_url)) {
restoration_timer_ = std::make_unique<base::ElapsedTimer>();
} else if (!wk_navigation_util::IsRestoreSessionUrl(url)) {
is_restore_session_in_progress_ = false;
DCHECK(restoration_timer_);
UMA_HISTOGRAM_TIMES(kRestoreNavigationTime, restoration_timer_->Elapsed());
restoration_timer_.reset();
......@@ -715,7 +718,6 @@ void WKBasedNavigationManagerImpl::UnsafeRestore(
// committed item, because a restored session has no pending or transient
// item.
is_restore_session_in_progress_ = true;
restoration_timer_ = std::make_unique<base::ElapsedTimer>();
if (last_committed_item_index > -1)
restored_visible_item_ = std::move(items[last_committed_item_index]);
......
......@@ -607,6 +607,7 @@ TEST_P(WebStateTest, RestorePageTitles) {
CRWSessionStorage* session_storage = [[CRWSessionStorage alloc] init];
session_storage.itemStorages = item_storages;
auto web_state = WebState::CreateWithStorageSession(params, session_storage);
web_state->SetKeepRenderProcessAlive(true);
NavigationManager* navigation_manager = web_state->GetNavigationManager();
// TODO(crbug.com/873729): The session will not be restored until
// LoadIfNecessary call. Fix the bug and remove extra call.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment