Commit e7574ffd authored by Ernesto García's avatar Ernesto García Committed by Commit Bot

omnibox: Support url unelide for samedoc navigation

Many websites rely on same doc navigation to render
different views using pushState/replaceState API, which
was originally not considered to unelide URL.
This CL enables uneliding for same document navigations
with the following considerations:
- If the same doc navigation is just a fragment navigation
(e.g.) from /foo to /foo#bar, URL should remain in its
initial state.
- If the same doc navigation is not a fragment navigation
(e.g.) from /foo to /foo/bar, URL should be unelided.
Also, added tests to check both scenarios, including
GetPreviousURL mock in the mock_navigation_handle.

Bug: 1096160
Change-Id: Ic051699c9fdb34eaa16e0c586672aae6d8d98b9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335675Reviewed-by: default avatarTommy Li <tommycli@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarEmily Stark <estark@chromium.org>
Commit-Queue: Ernesto García <ernestognw@google.com>
Cr-Commit-Position: refs/heads/master@{#794869}
parent 80991231
......@@ -1820,7 +1820,17 @@ void OmniboxViewViews::DidFinishNavigation(
if (!navigation->IsInMainFrame())
return;
if (navigation->IsSameDocument()) {
// Same-document navigations should be treated with the following
// considerations:
// - If the same-document navigation was triggered by a fragment navigation,
// the current elision/unelision state shouldn't be altered, since it always
// remains in the same page, keeping location.pathname
// - If the same-document navigation was triggered by the use of history
// pushState/replaceState API, we should unelide and reset state to support
// the same behaviour in websites that rely on same-document navigation to
// render different views as if it were 'normal' navigation
if (navigation->IsSameDocument() &&
navigation->GetPreviousURL().EqualsIgnoringRef(navigation->GetURL())) {
if (!IsURLEligibleForSimplifiedDomainEliding())
return;
......
......@@ -2362,6 +2362,43 @@ TEST_P(OmniboxViewViewsHideOnInteractionTest, SameDocNavigations) {
ASSERT_TRUE(elide_animation);
EXPECT_FALSE(elide_animation->IsAnimating());
}
// On same-document main-frame fragment navigation, the URL should remain
// elided to the simplified domain.
{
content::MockNavigationHandle navigation;
navigation.set_is_same_document(true);
navigation.set_previous_url(GURL(kSimplifiedDomainDisplayUrl));
navigation.set_url(
GURL(kSimplifiedDomainDisplayUrl + base::ASCIIToUTF16("#foobar")));
omnibox_view()->DidFinishNavigation(&navigation);
ASSERT_NO_FATAL_FAILURE(ExpectElidedToSimplifiedDomain(
omnibox_view(), kSimplifiedDomainDisplayUrlScheme,
kSimplifiedDomainDisplayUrlSubdomain,
kSimplifiedDomainDisplayUrlHostnameAndScheme,
kSimplifiedDomainDisplayUrlPath, ShouldElideToRegistrableDomain()));
OmniboxViewViews::ElideAnimation* elide_animation =
omnibox_view()->GetElideAfterInteractionAnimationForTesting();
ASSERT_TRUE(elide_animation);
EXPECT_FALSE(elide_animation->IsAnimating());
}
// On same-document main-frame non-fragment navigation, the URL shouldn't
// remain elided to the simplified domain.
{
content::MockNavigationHandle navigation;
navigation.set_is_same_document(true);
navigation.set_previous_url(GURL(kSimplifiedDomainDisplayUrl));
navigation.set_url(
GURL(kSimplifiedDomainDisplayUrl + base::ASCIIToUTF16("/foobar")));
omnibox_view()->DidFinishNavigation(&navigation);
ASSERT_NO_FATAL_FAILURE(ExpectUnelidedFromSimplifiedDomain(
render_text, gfx::Range(kSimplifiedDomainDisplayUrlScheme.size(),
kSimplifiedDomainDisplayUrl.size())));
OmniboxViewViews::ElideAnimation* elide_animation =
omnibox_view()->GetElideAfterInteractionAnimationForTesting();
EXPECT_FALSE(elide_animation);
}
}
// Tests that in the hide-on-interaction field trial, a same-document navigation
......
......@@ -28,6 +28,7 @@ class MockNavigationHandle : public NavigationHandle {
// NavigationHandle implementation:
int64_t GetNavigationId() override { return navigation_id_; }
const GURL& GetURL() override { return url_; }
const GURL& GetPreviousURL() override { return previous_url_; }
SiteInstance* GetStartingSiteInstance() override {
return starting_site_instance_;
}
......@@ -74,7 +75,6 @@ class MockNavigationHandle : public NavigationHandle {
MOCK_METHOD0(HasSubframeNavigationEntryCommitted, bool());
MOCK_METHOD0(DidReplaceEntry, bool());
MOCK_METHOD0(ShouldUpdateHistory, bool());
MOCK_METHOD0(GetPreviousURL, const GURL&());
MOCK_METHOD0(GetSocketAddress, net::IPEndPoint());
const net::HttpRequestHeaders& GetRequestHeaders() override {
return request_headers_;
......@@ -133,6 +133,9 @@ class MockNavigationHandle : public NavigationHandle {
MOCK_METHOD0(GetIsOverridingUserAgent, bool());
void set_url(const GURL& url) { url_ = url; }
void set_previous_url(const GURL& previous_url) {
previous_url_ = previous_url;
}
void set_starting_site_instance(SiteInstance* site_instance) {
starting_site_instance_ = site_instance;
}
......@@ -188,6 +191,7 @@ class MockNavigationHandle : public NavigationHandle {
private:
int64_t navigation_id_;
GURL url_;
GURL previous_url_;
SiteInstance* starting_site_instance_ = nullptr;
WebContents* web_contents_ = nullptr;
GURL base_url_for_data_url_;
......
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