Skip to content

GitLab

T
tangled
Commit 352492fd authored by Danyao Wang's avatar Danyao Wang Committed by Commit Bot
Browse files
Options

[ios] Always notify WebStateObservers on navigation item title change. 

The short circuit skips WebStateImpl->OnTitleChanged() incorrectly on
back/forward navigation because although the title in the navigation
item doesn't need changing, the visible navigation item has changed.

This only affects navigating to native content from web content because
web state title for web content is updated separately in
|-webViewTitleDidChange|.

Bug: 864601
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ifd74f87aff0276394def154a8410b3dd1ce6c1c8
Reviewed-on: https://chromium-review.googlesource.com/1148693
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: default avatarKurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577967}
parent d6287383
Hide whitespace changes
Inline Side-by-side
Showing with 109 additions and 6 deletions
ios/web/web_state/navigation_and_load_callbacks_inttest.mm
View file @ 352492fd
......@@ -499,6 +499,7 @@ class WebStateObserverMock : public WebStateObserver {
MOCK_METHOD1(DidStopLoading, void(WebState*));
MOCK_METHOD2(PageLoaded, void(WebState*, PageLoadCompletionStatus));
MOCK_METHOD1(DidChangeBackForwardState, void(WebState*));
MOCK_METHOD1(TitleWasSet, void(WebState*));
void WebStateDestroyed(WebState* web_state) override { NOTREACHED(); }
private:
......@@ -609,6 +610,7 @@ TEST_P(NavigationAndLoadCallbacksTest, NewPageNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -642,6 +644,7 @@ TEST_P(NavigationAndLoadCallbacksTest, EnableWebUsageTwice) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -710,6 +713,7 @@ TEST_P(NavigationAndLoadCallbacksTest, WebPageReloadNavigation) {
EXPECT_CALL(*decider_, ShouldAllowResponse(_, /*for_main_frame=*/true))
.WillOnce(Return(true));
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -734,6 +738,7 @@ TEST_P(NavigationAndLoadCallbacksTest, WebPageReloadNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyReloadFinishedContext(web_state(), url, &context, &nav_id,
true /* is_web_page */));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -768,6 +773,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ReloadWithUserAgentType) {
EXPECT_CALL(*decider_, ShouldAllowResponse(_, /*for_main_frame=*/true))
.WillOnce(Return(true));
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -788,6 +794,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ReloadWithUserAgentType) {
.WillOnce(Return(true));
// TODO(crbug.com/798836): verify the correct User-Agent header is sent.
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -824,6 +831,7 @@ TEST_P(NavigationAndLoadCallbacksTest, UserInitiatedHashChangeNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -902,6 +910,7 @@ TEST_P(NavigationAndLoadCallbacksTest, RendererInitiatedHashChangeNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -956,6 +965,7 @@ TEST_P(NavigationAndLoadCallbacksTest, StateNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1011,6 +1021,7 @@ TEST_P(NavigationAndLoadCallbacksTest, NativeContentNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewNativePageFinishedContext(web_state(), url, &context,
&nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1026,6 +1037,7 @@ TEST_P(NavigationAndLoadCallbacksTest, NativeContentReload) {
// No ShouldAllowRequest/ShouldAllowResponse callbacks for native content
// navigations.
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1051,6 +1063,78 @@ TEST_P(NavigationAndLoadCallbacksTest, NativeContentReload) {
/*check_for_repost=*/false);
}
// Tests WasTitleSet callback triggered when navigating back to native content
// from web content.
TEST_P(NavigationAndLoadCallbacksTest, GoBackToNativeContent) {
// Load a native content URL.
GURL url(url::SchemeHostPort(kTestNativeContentScheme, "ui", 0).Serialize());
EXPECT_CALL(observer_, DidStartLoading(web_state()));
EXPECT_CALL(observer_, DidStartNavigation(web_state(), _));
// No ShouldAllowRequest/ShouldAllowResponse callbacks for native content
// navigations.
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
[provider_ setController:content_ forURL:url];
ASSERT_TRUE(LoadUrl(url));
// Load a web navigation.
const GURL web_url = test_server_->GetURL("/echo");
// Perform new page navigation.
NavigationContext* context = nullptr;
int32_t nav_id = 0;
EXPECT_CALL(observer_, DidStartLoading(web_state()));
WebStatePolicyDecider::RequestInfo expected_request_info(
ui::PageTransition::PAGE_TRANSITION_CLIENT_REDIRECT, web_url,
/*target_main_frame=*/true, /*has_user_gesture=*/false);
EXPECT_CALL(*decider_,
ShouldAllowRequest(_, RequestInfoMatch(expected_request_info)))
.WillOnce(Return(true));
EXPECT_CALL(observer_, DidStartNavigation(web_state(), _))
.WillOnce(VerifyPageStartedContext(
web_state(), web_url, ui::PageTransition::PAGE_TRANSITION_TYPED,
&context, &nav_id));
EXPECT_CALL(*decider_, ShouldAllowResponse(_, /*for_main_frame=*/true))
.WillOnce(Return(true));
if (GetWebClient()->IsSlimNavigationManagerEnabled()) {
EXPECT_CALL(observer_, DidChangeBackForwardState(web_state()));
}
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), web_url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
ASSERT_TRUE(LoadUrl(web_url));
// Going back to native content should trigger TitleWasSet.
if (GetWebClient()->IsSlimNavigationManagerEnabled()) {
EXPECT_CALL(observer_, DidChangeBackForwardState(web_state())).Times(2);
// TODO(crbug.com/867095): fix this extra callback triggered by placeholder
// URL load.
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
}
EXPECT_CALL(observer_, DidStartLoading(web_state()));
EXPECT_CALL(observer_, DidStartNavigation(web_state(), _));
// No ShouldAllowRequest/ShouldAllowResponse callbacks for native content
// navigations.
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
// This call is required to make sure WebStateObservers update their cached
// version of current title.
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
ASSERT_TRUE(ExecuteBlockAndWaitForLoad(url, ^{
navigation_manager()->GoBack();
}));
}
// Tests successful navigation to a new page with post HTTP method.
TEST_P(NavigationAndLoadCallbacksTest, UserInitiatedPostNavigation) {
// Prior to iOS 11, POST navigation is implemented as an XMLHttpRequest in
......@@ -1086,6 +1170,7 @@ TEST_P(NavigationAndLoadCallbacksTest, UserInitiatedPostNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state())).Times(2);
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1117,6 +1202,7 @@ TEST_P(NavigationAndLoadCallbacksTest, UserInitiatedPostNavigation) {
.WillOnce(VerifyPostFinishedContext(
web_state(), url, /*has_user_gesture=*/true, &context, &nav_id,
/*renderer_initiated=*/false));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1146,6 +1232,7 @@ TEST_P(NavigationAndLoadCallbacksTest, RendererInitiatedPostNavigation) {
EXPECT_CALL(*decider_, ShouldAllowResponse(_, /*for_main_frame=*/true))
.WillOnce(Return(true));
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1176,6 +1263,7 @@ TEST_P(NavigationAndLoadCallbacksTest, RendererInitiatedPostNavigation) {
.WillOnce(VerifyPostFinishedContext(
web_state(), action, /*has_user_gesture=*/false, &context, &nav_id,
/*renderer_initiated=*/true));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1201,6 +1289,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ReloadPostNavigation) {
EXPECT_CALL(*decider_, ShouldAllowResponse(_, /*for_main_frame=*/true))
.WillOnce(Return(true));
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1223,6 +1312,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ReloadPostNavigation) {
EXPECT_CALL(observer_, DidChangeBackForwardState(web_state()));
}
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1264,6 +1354,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ReloadPostNavigation) {
.WillOnce(VerifyPostFinishedContext(
web_state(), action, /*has_user_gesture=*/true, &context, &nav_id,
reload_is_renderer_initiated));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1299,6 +1390,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ForwardPostNavigation) {
EXPECT_CALL(*decider_, ShouldAllowResponse(_, /*for_main_frame=*/true))
.WillOnce(Return(true));
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1321,6 +1413,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ForwardPostNavigation) {
EXPECT_CALL(observer_, DidChangeBackForwardState(web_state()));
}
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1353,6 +1446,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ForwardPostNavigation) {
.WillOnce(Return(true));
}
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1384,6 +1478,7 @@ TEST_P(NavigationAndLoadCallbacksTest, ForwardPostNavigation) {
.WillOnce(VerifyPostFinishedContext(
web_state(), action, /*has_user_gesture=*/true, &context, &nav_id,
/*renderer_initiated=*/false));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1425,6 +1520,7 @@ TEST_P(NavigationAndLoadCallbacksTest, RedirectNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(
web_state(), redirect_url, kExpectedMimeType, &context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::SUCCESS));
......@@ -1488,6 +1584,7 @@ TEST_P(NavigationAndLoadCallbacksTest, FLAKY_FailedLoad) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(web_state(), url, /*mime_type=*/"",
&context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
EXPECT_CALL(observer_,
PageLoaded(web_state(), PageLoadCompletionStatus::FAILURE));
......@@ -1588,6 +1685,7 @@ TEST_P(NavigationAndLoadCallbacksTest, StopFinishedNavigation) {
EXPECT_CALL(observer_, DidFinishNavigation(web_state(), _))
.WillOnce(VerifyNewPageFinishedContext(web_state(), url, /*mime_type=*/"",
&context, &nav_id));
EXPECT_CALL(observer_, TitleWasSet(web_state()));
EXPECT_CALL(observer_, DidStopLoading(web_state()));
test::LoadUrl(web_state(), url);
......
ios/web/web_state/ui/crw_web_controller.mm
View file @ 352492fd
......@@ -1290,9 +1290,6 @@ GURL URLEscapedForHistory(const GURL& url) {
return;
base::string16 newTitle = base::SysNSStringToUTF16(title);
if (item->GetTitle() == newTitle)
return;
item->SetTitle(newTitle);
// TODO(crbug.com/546218): See if this can be removed; it's not clear that
// other platforms send this (tab sync triggers need to be compared against
......
ios/web/web_state/ui/crw_web_controller_unittest.mm
View file @ 352492fd
......@@ -1089,10 +1089,18 @@ TEST_P(CRWWebControllerTitleTest, TitleChange) {
scoped_observer.Add(web_state());
ASSERT_EQ(0, observer.title_change_count());
// Expect TitleWasSet callback after the page is loaded.
int initial_title_change_count = 0;
if (web::GetWebClient()->IsSlimNavigationManagerEnabled()) {
// WKBasedNavigationManager produces an extra call to TitleWasSet because it
// loads New Tab Page in web view.
initial_title_change_count += 1;
}
// Expect TitleWasSet callback after the page is loaded and due to WKWebView
// title change KVO.
LoadHtml(@"<title>Title1</title>");
EXPECT_EQ("Title1", base::UTF16ToUTF8(web_state()->GetTitle()));
EXPECT_EQ(1, observer.title_change_count());
EXPECT_EQ(initial_title_change_count + 2, observer.title_change_count());
// Expect at least one more TitleWasSet callback after changing title via
// JavaScript. On iOS 10 WKWebView fires 3 callbacks after JS excucution
......@@ -1101,7 +1109,7 @@ TEST_P(CRWWebControllerTitleTest, TitleChange) {
// Fix expecteation when WKWebView stops sending extra KVO calls.
ExecuteJavaScript(@"window.document.title = 'Title2';");
EXPECT_EQ("Title2", base::UTF16ToUTF8(web_state()->GetTitle()));
EXPECT_GE(observer.title_change_count(), 2);
EXPECT_GE(observer.title_change_count(), initial_title_change_count + 3);
};
// Tests that fragment change navigations use title from the previous page.
......
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