Commit 578c90a6 authored by Jun Choi's avatar Jun Choi Committed by Commit Bot

Revert "Add web::NavigationContext::IsPlaceholderNavigation"

This reverts commit 7b0f2bb8.

Reason for revert: Fails multiple IOS builds including Builder ios-slimnav <https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-slimnav/191>

Link to sample failure logs : https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8929398908718170816/+/steps/ios_chrome_unittests__iPhone_6s_iOS_12.1_/0/logs/TextToSpeechListenerTest.ValidAudioDataTest/0


Original change's description:
> Add web::NavigationContext::IsPlaceholderNavigation
> 
> This is a step in multistep refactoring. The next steps will be:
> 
> 1.) web::NavigationContext::URL is never set to placeholder
>     URL and always represents navigation URL
> 2.) Same web::NavigationContext will be reused for placeholder
>     navigation to extend it's lifetime.
> 3.) WebStateObserver::DidFinishNavigation will be caller after
>     placeholder navigation is finished and will use original
>     web::NavigationContext passed to WebStateObserver::DidStartNavigation
> 
> This will partially fix crbug.com/903497 and will call
> WebStateObserver::DidFinishNavigation after committed URL actually
> changed.
> 
> Bug: 903497
> Change-Id: Ib6ba96d664ac352d038787aa74ac844d07282789
> Reviewed-on: https://chromium-review.googlesource.com/c/1331044
> Reviewed-by: Danyao Wang <danyao@chromium.org>
> Commit-Queue: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609398}

TBR=eugenebut@chromium.org,danyao@chromium.org

Change-Id: I8d7c9b66d752a792761d95ccf348f045eaa9de03
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 903497
Reviewed-on: https://chromium-review.googlesource.com/c/1343351Reviewed-by: default avatarJun Choi <hongjunchoi@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609494}
parent c8fc00a7
...@@ -197,7 +197,6 @@ source_set("unit_tests") { ...@@ -197,7 +197,6 @@ source_set("unit_tests") {
"//ios/web/public/test", "//ios/web/public/test",
"//ios/web/public/test/fakes", "//ios/web/public/test/fakes",
"//ios/web/test/fakes:fakes", "//ios/web/test/fakes:fakes",
"//ios/web/web_state:navigation_context",
"//net", "//net",
"//testing/gmock", "//testing/gmock",
"//testing/gtest", "//testing/gtest",
......
...@@ -7,7 +7,6 @@ specific_include_rules = { ...@@ -7,7 +7,6 @@ specific_include_rules = {
], ],
"^tab_unittest\.mm$": [ "^tab_unittest\.mm$": [
"+ios/web/web_state/ui/crw_web_controller.h", "+ios/web/web_state/ui/crw_web_controller.h",
"+ios/web/web_state/navigation_context_impl.h",
"+ios/web/navigation/navigation_manager_impl.h", "+ios/web/navigation/navigation_manager_impl.h",
"+ios/web/web_state/web_state_impl.h", "+ios/web/web_state/web_state_impl.h",
"+ios/web/test/fakes/crw_fake_back_forward_list.h", "+ios/web/test/fakes/crw_fake_back_forward_list.h",
......
...@@ -48,7 +48,6 @@ ...@@ -48,7 +48,6 @@
#import "ios/web/public/test/fakes/fake_navigation_context.h" #import "ios/web/public/test/fakes/fake_navigation_context.h"
#include "ios/web/public/test/test_web_thread_bundle.h" #include "ios/web/public/test/test_web_thread_bundle.h"
#import "ios/web/test/fakes/crw_fake_back_forward_list.h" #import "ios/web/test/fakes/crw_fake_back_forward_list.h"
#import "ios/web/web_state/navigation_context_impl.h"
#import "ios/web/web_state/ui/crw_web_controller.h" #import "ios/web/web_state/ui/crw_web_controller.h"
#import "ios/web/web_state/web_state_impl.h" #import "ios/web/web_state/web_state_impl.h"
#import "net/base/mac/url_conversions.h" #import "net/base/mac/url_conversions.h"
...@@ -238,13 +237,9 @@ class TabTest : public BlockCleanupTest, ...@@ -238,13 +237,9 @@ class TabTest : public BlockCleanupTest,
NSString* title) { NSString* title) {
DCHECK_EQ(tab_.webState, web_state_impl_.get()); DCHECK_EQ(tab_.webState, web_state_impl_.get());
std::unique_ptr<web::NavigationContextImpl> context1 = web::FakeNavigationContext context1;
web::NavigationContextImpl::CreateNavigationContext( context1.SetUrl(user_url);
web_state_impl_.get(), user_url, web_state_impl_->OnNavigationStarted(&context1);
/*has_user_gesture=*/true,
ui::PageTransition::PAGE_TRANSITION_AUTO_BOOKMARK,
/*is_renderer_initiated=*/true);
web_state_impl_->OnNavigationStarted(context1.get());
web::Referrer empty_referrer; web::Referrer empty_referrer;
web_state_impl_->GetNavigationManagerImpl().AddPendingItem( web_state_impl_->GetNavigationManagerImpl().AddPendingItem(
...@@ -252,13 +247,9 @@ class TabTest : public BlockCleanupTest, ...@@ -252,13 +247,9 @@ class TabTest : public BlockCleanupTest,
web::NavigationInitiationType::RENDERER_INITIATED, web::NavigationInitiationType::RENDERER_INITIATED,
web::NavigationManager::UserAgentOverrideOption::INHERIT); web::NavigationManager::UserAgentOverrideOption::INHERIT);
std::unique_ptr<web::NavigationContextImpl> context2 = web::FakeNavigationContext context2;
web::NavigationContextImpl::CreateNavigationContext( context2.SetUrl(redirect_url);
web_state_impl_.get(), redirect_url, web_state_impl_->OnNavigationStarted(&context2);
/*has_user_gesture=*/true,
ui::PageTransition::PAGE_TRANSITION_AUTO_BOOKMARK,
/*is_renderer_initiated=*/true);
web_state_impl_->OnNavigationStarted(context2.get());
if (GetParam() == NavigationManagerChoice::WK_BASED) { if (GetParam() == NavigationManagerChoice::WK_BASED) {
[fake_wk_list_ [fake_wk_list_
...@@ -266,9 +257,9 @@ class TabTest : public BlockCleanupTest, ...@@ -266,9 +257,9 @@ class TabTest : public BlockCleanupTest,
} }
web_state_impl_->GetNavigationManagerImpl().CommitPendingItem(); web_state_impl_->GetNavigationManagerImpl().CommitPendingItem();
context2->SetHasCommitted(true); context2.SetHasCommitted(true);
web_state_impl_->UpdateHttpResponseHeaders(redirect_url); web_state_impl_->UpdateHttpResponseHeaders(redirect_url);
web_state_impl_->OnNavigationFinished(context2.get()); web_state_impl_->OnNavigationFinished(&context2);
web_state_impl_->SetIsLoading(true); web_state_impl_->SetIsLoading(true);
base::string16 new_title = base::SysNSStringToUTF16(title); base::string16 new_title = base::SysNSStringToUTF16(title);
......
...@@ -58,7 +58,6 @@ source_set("wk_web_view_security_util") { ...@@ -58,7 +58,6 @@ source_set("wk_web_view_security_util") {
source_set("navigation_context") { source_set("navigation_context") {
deps = [ deps = [
"//base", "//base",
"//ios/web/navigation:core",
"//ios/web/public", "//ios/web/public",
] ]
......
...@@ -85,10 +85,6 @@ class NavigationContextImpl : public NavigationContext { ...@@ -85,10 +85,6 @@ class NavigationContextImpl : public NavigationContext {
bool IsNativeContentPresented() const; bool IsNativeContentPresented() const;
void SetIsNativeContentPresented(bool is_native_content_presented); void SetIsNativeContentPresented(bool is_native_content_presented);
// true if this navigation context is a placeholder navigation.
bool IsPlaceholderNavigation() const;
void SetPlaceholderNavigation(bool flag);
private: private:
NavigationContextImpl(WebState* web_state, NavigationContextImpl(WebState* web_state,
const GURL& url, const GURL& url,
...@@ -113,7 +109,6 @@ class NavigationContextImpl : public NavigationContext { ...@@ -113,7 +109,6 @@ class NavigationContextImpl : public NavigationContext {
bool is_loading_error_page_ = false; bool is_loading_error_page_ = false;
bool is_loading_html_string_ = false; bool is_loading_html_string_ = false;
bool is_native_content_presented_ = false; bool is_native_content_presented_ = false;
bool is_placeholder_navigation_ = false;
DISALLOW_COPY_AND_ASSIGN(NavigationContextImpl); DISALLOW_COPY_AND_ASSIGN(NavigationContextImpl);
}; };
......
...@@ -175,14 +175,6 @@ void NavigationContextImpl::SetIsNativeContentPresented( ...@@ -175,14 +175,6 @@ void NavigationContextImpl::SetIsNativeContentPresented(
is_native_content_presented_ = is_native_content_presented; is_native_content_presented_ = is_native_content_presented;
} }
bool NavigationContextImpl::IsPlaceholderNavigation() const {
return is_placeholder_navigation_;
}
void NavigationContextImpl::SetPlaceholderNavigation(bool flag) {
is_placeholder_navigation_ = flag;
}
NavigationContextImpl::NavigationContextImpl(WebState* web_state, NavigationContextImpl::NavigationContextImpl(WebState* web_state,
const GURL& url, const GURL& url,
bool has_user_gesture, bool has_user_gesture,
......
This diff is collapsed.
...@@ -46,7 +46,7 @@ class BrowserState; ...@@ -46,7 +46,7 @@ class BrowserState;
struct ContextMenuParams; struct ContextMenuParams;
struct FaviconURL; struct FaviconURL;
struct LoadCommittedDetails; struct LoadCommittedDetails;
class NavigationContextImpl; class NavigationContext;
class NavigationManager; class NavigationManager;
class SessionCertificatePolicyCacheImpl; class SessionCertificatePolicyCacheImpl;
class WebInterstitialImpl; class WebInterstitialImpl;
...@@ -78,12 +78,12 @@ class WebStateImpl : public WebState, public NavigationManagerDelegate { ...@@ -78,12 +78,12 @@ class WebStateImpl : public WebState, public NavigationManagerDelegate {
void SetWebController(CRWWebController* web_controller); void SetWebController(CRWWebController* web_controller);
// Notifies the observers that a navigation has started. // Notifies the observers that a navigation has started.
void OnNavigationStarted(web::NavigationContextImpl* context); void OnNavigationStarted(web::NavigationContext* context);
// Notifies the observers that a navigation has finished. For same-document // Notifies the observers that a navigation has finished. For same-document
// navigations notifies the observers about favicon URLs update using // navigations notifies the observers about favicon URLs update using
// candidates received in OnFaviconUrlUpdated. // candidates received in OnFaviconUrlUpdated.
void OnNavigationFinished(web::NavigationContextImpl* context); void OnNavigationFinished(web::NavigationContext* context);
// Called when current window's canGoBack / canGoForward state was changed. // Called when current window's canGoBack / canGoForward state was changed.
void OnBackForwardStateChanged(); void OnBackForwardStateChanged();
......
...@@ -735,27 +735,23 @@ void WebStateImpl::TakeSnapshot(CGRect rect, SnapshotCallback callback) { ...@@ -735,27 +735,23 @@ void WebStateImpl::TakeSnapshot(CGRect rect, SnapshotCallback callback) {
}]; }];
} }
void WebStateImpl::OnNavigationStarted(web::NavigationContextImpl* context) { void WebStateImpl::OnNavigationStarted(web::NavigationContext* context) {
// Navigation manager loads internal URLs to restore session history and // Navigation manager loads internal URLs to restore session history and
// create back-forward entries for Native View and WebUI. Do not trigger // create back-forward entries for Native View and WebUI. Do not trigger
// external callbacks. // external callbacks.
if (context->IsPlaceholderNavigation() || if (wk_navigation_util::IsWKInternalUrl(context->GetUrl()))
wk_navigation_util::IsRestoreSessionUrl(context->GetUrl())) {
return; return;
}
for (auto& observer : observers_) for (auto& observer : observers_)
observer.DidStartNavigation(this, context); observer.DidStartNavigation(this, context);
} }
void WebStateImpl::OnNavigationFinished(web::NavigationContextImpl* context) { void WebStateImpl::OnNavigationFinished(web::NavigationContext* context) {
// Navigation manager loads internal URLs to restore session history and // Navigation manager loads internal URLs to restore session history and
// create back-forward entries for Native View and WebUI. Do not trigger // create back-forward entries for Native View and WebUI. Do not trigger
// external callbacks. // external callbacks.
if (context->IsPlaceholderNavigation() || if (wk_navigation_util::IsWKInternalUrl(context->GetUrl()))
wk_navigation_util::IsRestoreSessionUrl(context->GetUrl())) {
return; return;
}
for (auto& observer : observers_) for (auto& observer : observers_)
observer.DidFinishNavigation(this, context); observer.DidFinishNavigation(this, context);
......
...@@ -415,7 +415,7 @@ TEST_P(WebStateImplTest, ObserverTest) { ...@@ -415,7 +415,7 @@ TEST_P(WebStateImplTest, ObserverTest) {
// Test that DidFinishNavigation() is called. // Test that DidFinishNavigation() is called.
ASSERT_FALSE(observer->did_finish_navigation_info()); ASSERT_FALSE(observer->did_finish_navigation_info());
const GURL url("http://test"); const GURL url("http://test");
std::unique_ptr<NavigationContextImpl> context = std::unique_ptr<web::NavigationContext> context =
NavigationContextImpl::CreateNavigationContext( NavigationContextImpl::CreateNavigationContext(
web_state_.get(), url, /*has_user_gesture=*/true, web_state_.get(), url, /*has_user_gesture=*/true,
ui::PageTransition::PAGE_TRANSITION_AUTO_BOOKMARK, ui::PageTransition::PAGE_TRANSITION_AUTO_BOOKMARK,
...@@ -505,27 +505,22 @@ TEST_P(WebStateImplTest, ObserverTest) { ...@@ -505,27 +505,22 @@ TEST_P(WebStateImplTest, ObserverTest) {
// Tests that placeholder navigations are not visible to WebStateObservers. // Tests that placeholder navigations are not visible to WebStateObservers.
TEST_P(WebStateImplTest, PlaceholderNavigationNotExposedToObservers) { TEST_P(WebStateImplTest, PlaceholderNavigationNotExposedToObservers) {
TestWebStateObserver observer(web_state_.get()); TestWebStateObserver observer(web_state_.get());
GURL placeholder_url = FakeNavigationContext context;
wk_navigation_util::CreatePlaceholderUrlForUrl(GURL("chrome://newtab")); context.SetUrl(
std::unique_ptr<NavigationContextImpl> context = wk_navigation_util::CreatePlaceholderUrlForUrl(GURL("chrome://newtab")));
NavigationContextImpl::CreateNavigationContext(
web_state_.get(), placeholder_url,
/*has_user_gesture=*/true,
ui::PageTransition::PAGE_TRANSITION_AUTO_BOOKMARK,
/*is_renderer_initiated=*/true);
context->SetPlaceholderNavigation(true);
// Test that OnPageLoaded() is not called. // Test that OnPageLoaded() is not called.
web_state_->OnPageLoaded(placeholder_url, /*load_success=*/true); web_state_->OnPageLoaded(context.GetUrl(), true /* load_success */);
EXPECT_FALSE(observer.load_page_info()); EXPECT_FALSE(observer.load_page_info());
web_state_->OnPageLoaded(placeholder_url, /*load_success=*/false); web_state_->OnPageLoaded(context.GetUrl(), false /* load_success */);
EXPECT_FALSE(observer.load_page_info()); EXPECT_FALSE(observer.load_page_info());
// Test that OnNavigationStarted() is not called. // Test that OnNavigationStarted() is not called.
web_state_->OnNavigationStarted(context.get()); web_state_->OnNavigationStarted(&context);
EXPECT_FALSE(observer.did_start_navigation_info()); EXPECT_FALSE(observer.did_start_navigation_info());
// Test that OnNavigationFinished() is not called. // Test that OnNavigationFinished() is not called.
web_state_->OnNavigationFinished(context.get()); web_state_->OnNavigationFinished(&context);
EXPECT_FALSE(observer.did_finish_navigation_info()); EXPECT_FALSE(observer.did_finish_navigation_info());
} }
...@@ -671,12 +666,8 @@ TEST_P(WebStateImplTest, GlobalObserverTest) { ...@@ -671,12 +666,8 @@ TEST_P(WebStateImplTest, GlobalObserverTest) {
// Test that DidStartNavigation() is called. // Test that DidStartNavigation() is called.
EXPECT_FALSE(observer->did_start_navigation_called()); EXPECT_FALSE(observer->did_start_navigation_called());
std::unique_ptr<NavigationContextImpl> context = FakeNavigationContext context;
NavigationContextImpl::CreateNavigationContext( web_state_->OnNavigationStarted(&context);
web_state_.get(), GURL::EmptyGURL(), /*has_user_gesture=*/true,
ui::PageTransition::PAGE_TRANSITION_AUTO_BOOKMARK,
/*is_renderer_initiated=*/true);
web_state_->OnNavigationStarted(context.get());
EXPECT_TRUE(observer->did_start_navigation_called()); EXPECT_TRUE(observer->did_start_navigation_called());
// Test that WebStateDidStartLoading() is called. // Test that WebStateDidStartLoading() is called.
...@@ -936,13 +927,9 @@ TEST_P(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) { ...@@ -936,13 +927,9 @@ TEST_P(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) {
auto observer = std::make_unique<TestWebStateObserver>(web_state_.get()); auto observer = std::make_unique<TestWebStateObserver>(web_state_.get());
// No callback if icons has not been fetched yet. // No callback if icons has not been fetched yet.
std::unique_ptr<NavigationContextImpl> context = FakeNavigationContext context;
NavigationContextImpl::CreateNavigationContext( context.SetIsSameDocument(true);
web_state_.get(), GURL::EmptyGURL(), web_state_->OnNavigationFinished(&context);
/*has_user_gesture=*/false, ui::PageTransition::PAGE_TRANSITION_LINK,
/*is_renderer_initiated=*/false);
context->SetIsSameDocument(true);
web_state_->OnNavigationFinished(context.get());
EXPECT_FALSE(observer->update_favicon_url_candidates_info()); EXPECT_FALSE(observer->update_favicon_url_candidates_info());
// Callback is called when icons were fetched. // Callback is called when icons were fetched.
...@@ -955,7 +942,7 @@ TEST_P(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) { ...@@ -955,7 +942,7 @@ TEST_P(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) {
// Callback is now called after same-document navigation. // Callback is now called after same-document navigation.
observer = std::make_unique<TestWebStateObserver>(web_state_.get()); observer = std::make_unique<TestWebStateObserver>(web_state_.get());
web_state_->OnNavigationFinished(context.get()); web_state_->OnNavigationFinished(&context);
ASSERT_TRUE(observer->update_favicon_url_candidates_info()); ASSERT_TRUE(observer->update_favicon_url_candidates_info());
ASSERT_EQ(1U, ASSERT_EQ(1U,
observer->update_favicon_url_candidates_info()->candidates.size()); observer->update_favicon_url_candidates_info()->candidates.size());
...@@ -972,14 +959,14 @@ TEST_P(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) { ...@@ -972,14 +959,14 @@ TEST_P(WebStateImplTest, FaviconUpdateForSameDocumentNavigations) {
// Document change navigation does not call callback. // Document change navigation does not call callback.
observer = std::make_unique<TestWebStateObserver>(web_state_.get()); observer = std::make_unique<TestWebStateObserver>(web_state_.get());
context->SetIsSameDocument(false); context.SetIsSameDocument(false);
web_state_->OnNavigationFinished(context.get()); web_state_->OnNavigationFinished(&context);
EXPECT_FALSE(observer->update_favicon_url_candidates_info()); EXPECT_FALSE(observer->update_favicon_url_candidates_info());
// Previous candidates were invalidated by the document change. No callback // Previous candidates were invalidated by the document change. No callback
// if icons has not been fetched yet. // if icons has not been fetched yet.
context->SetIsSameDocument(true); context.SetIsSameDocument(true);
web_state_->OnNavigationFinished(context.get()); web_state_->OnNavigationFinished(&context);
EXPECT_FALSE(observer->update_favicon_url_candidates_info()); EXPECT_FALSE(observer->update_favicon_url_candidates_info());
} }
......
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