summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorpkasting@chromium.org <pkasting@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2011-10-14 20:42:49 +0000
committerpkasting@chromium.org <pkasting@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2011-10-14 20:42:49 +0000
commita1e20c2d98f590f2a48af3b3a1f2530f7af2b42e (patch)
treea2ec95f80c9501b333c8283921df41a0ce704fab
parent8bc38d2964d1a6477bee5e6e880efe3f1e027d8e (diff)
downloadchromium_src-a1e20c2d98f590f2a48af3b3a1f2530f7af2b42e.zip
chromium_src-a1e20c2d98f590f2a48af3b3a1f2530f7af2b42e.tar.gz
chromium_src-a1e20c2d98f590f2a48af3b3a1f2530f7af2b42e.tar.bz2
When users navigate to an intranet host that has not been previously typed, treat it as a typed navigation.
This fixes two different problems: (1) Due to scheme-stripping, a user who navigated to e.g. "host/path" via a link or bookmark, for a not-previously-typed host, would erroneously get a search page instead of a navigation when reloading via hitting enter or when editing the path to be something else. We could instead fix this by being more conservative about scheme-stripping, but that is uglier UI, a bigger change, and doesn't solve problem (2). (2) Users who navigated to intranet hosts (with or without a path or other URL components) via link clicks/bookmarks/etc. would later try to type them and be frustrated that the omnibox would do a search and then show the accidental search infobar. This change makes the omnibox more aggressive about learning that such hostnames are valid intranet hosts even before users type them, if they've at least visited them. There are four issues here: (1) Because in incognito mode we never add navigations to any history DBs, users there won't see the above benefits. In fact, users there will struggle to use (new) intranet hosts in general. I filed bug 100271 about this. (2) The omnibox will now be more likely to inline-autocomplete intranet URLs you haven't actually typed before. After thinking for a while I don't think this will have a large effect either way especially as intranet URLs are often short (and thus it's less likely users will try to search or navigate to a pure prefix of one). (3) Heavy intranet users will have a larger in-memory DB. I don't think this effect will be dramatic since it's just one URL per unique hostname, and there's no real way around this anyway. (4) The additional checking in AddPage() could make navigation slower. I don't think this ought to happen because most navigations will fail the RCDS check for "no known TLD" (which ought to be fast, as that code is called by things like cookie management), so they'll never even check the database. If this turns out to be wrong, then we'll need to fall back on something like the solution in bug 100271. BUG=94806 TEST=On a clean profile, add a bookmark for some intranet host (without navigating there), with or without a path. Click on the bookmark. Then try to modify the path and check that the omnibox still wants to navigate, not search. Review URL: http://codereview.chromium.org/8286011 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@105565 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r--chrome/browser/autocomplete/history_url_provider.cc21
-rw-r--r--chrome/browser/history/history_backend.cc25
-rw-r--r--chrome/browser/history/history_unittest.cc85
-rw-r--r--chrome/browser/history/url_database.cc24
-rw-r--r--chrome/browser/history/url_database.h8
5 files changed, 135 insertions, 28 deletions
diff --git a/chrome/browser/autocomplete/history_url_provider.cc b/chrome/browser/autocomplete/history_url_provider.cc
index d644d66..6d9f25b 100644
--- a/chrome/browser/autocomplete/history_url_provider.cc
+++ b/chrome/browser/autocomplete/history_url_provider.cc
@@ -400,7 +400,7 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
// for more results than we need, of every prefix type, in hopes this will
// give us far more than enough to work with. CullRedirects() will then
// reduce the list to the best kMaxMatches results.
- db->AutocompleteForPrefix(i->prefix + params->input.text(),
+ db->AutocompleteForPrefix(UTF16ToUTF8(i->prefix + params->input.text()),
kMaxMatches * 2, (backend == NULL), &url_matches);
for (URLRowVector::const_iterator j(url_matches.begin());
j != url_matches.end(); ++j) {
@@ -750,22 +750,11 @@ bool HistoryURLProvider::CanFindIntranetURL(
!LowerCaseEqualsASCII(input.scheme(), chrome::kHttpScheme) ||
!input.parts().host.is_nonempty())
return false;
- const string16 host(input.text().substr(input.parts().host.begin,
- input.parts().host.len));
- if (net::RegistryControlledDomainService::GetRegistryLength(
- UTF16ToUTF8(host), false) != 0)
+ const std::string host(UTF16ToUTF8(
+ input.text().substr(input.parts().host.begin, input.parts().host.len)));
+ if (net::RegistryControlledDomainService::GetRegistryLength(host, false) != 0)
return false;
- std::vector<history::URLRow> dummy;
- for (history::Prefixes::const_iterator i(prefixes_.begin());
- i != prefixes_.end(); ++i) {
- if ((i->num_components == 1) &&
- (db->AutocompleteForPrefix(i->prefix + host + ASCIIToUTF16("/"), 1,
- true, &dummy) ||
- db->AutocompleteForPrefix(i->prefix + host + ASCIIToUTF16(":"), 1,
- true, &dummy)))
- return true;
- }
- return false;
+ return db->IsTypedHost(host);
}
bool HistoryURLProvider::PromoteMatchForInlineAutocomplete(
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index f942672..bee9c8c 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -92,7 +92,7 @@ static const int kArchiveDaysThreshold = 90;
// Converts from PageUsageData to MostVisitedURL. |redirects| is a
// list of redirects for this URL. Empty list means no redirects.
MostVisitedURL MakeMostVisitedURL(const PageUsageData& page_data,
- const RedirectList& redirects) {
+ const RedirectList& redirects) {
MostVisitedURL mv;
mv.url = page_data.GetURL();
mv.title = page_data.GetTitle();
@@ -406,7 +406,28 @@ void HistoryBackend::AddPage(scoped_refptr<HistoryAddPageArgs> request) {
bool is_keyword_generated =
(transition == content::PAGE_TRANSITION_KEYWORD_GENERATED);
- if (request->redirects.size() <= 1) {
+ // If the user is navigating to a not-previously-typed intranet hostname,
+ // change the transition to TYPED so that the omnibox will learn that this is
+ // a known host.
+ bool has_redirects = request->redirects.size() > 1;
+ if (content::PageTransitionIsMainFrame(request->transition) &&
+ (transition != content::PAGE_TRANSITION_TYPED) && !is_keyword_generated) {
+ const GURL& origin_url(has_redirects ?
+ request->redirects[0] : request->url);
+ if (origin_url.SchemeIs(chrome::kHttpScheme) ||
+ origin_url.SchemeIs(chrome::kHttpsScheme) ||
+ origin_url.SchemeIs(chrome::kFtpScheme)) {
+ std::string host(origin_url.host());
+ if ((net::RegistryControlledDomainService::GetRegistryLength(
+ host, false) == 0) && !db_->IsTypedHost(host)) {
+ transition = content::PAGE_TRANSITION_TYPED;
+ request->transition = content::PageTransitionFromInt(transition |
+ content::PageTransitionGetQualifier(request->transition));
+ }
+ }
+ }
+
+ if (!has_redirects) {
// The single entry is both a chain start and end.
content::PageTransition t = content::PageTransitionFromInt(
request->transition |
diff --git a/chrome/browser/history/history_unittest.cc b/chrome/browser/history/history_unittest.cc
index fa15325..d5ce21a 100644
--- a/chrome/browser/history/history_unittest.cc
+++ b/chrome/browser/history/history_unittest.cc
@@ -469,8 +469,8 @@ TEST_F(HistoryTest, AddRedirect) {
ASSERT_TRUE(history->Init(history_dir_, NULL));
const char* first_sequence[] = {
- "http://first.page/",
- "http://second.page/"};
+ "http://first.page.com/",
+ "http://second.page.com/"};
int first_count = arraysize(first_sequence);
history::RedirectList first_redirects;
for (int i = 0; i < first_count; i++)
@@ -515,7 +515,7 @@ TEST_F(HistoryTest, AddRedirect) {
// so we pass in a CLIENT_REDIRECT qualifier to mock that behavior.
history::RedirectList second_redirects;
second_redirects.push_back(first_redirects[1]);
- second_redirects.push_back(GURL("http://last.page/"));
+ second_redirects.push_back(GURL("http://last.page.com/"));
history->AddPage(second_redirects[1], MakeFakeHost(1), 1,
second_redirects[0],
static_cast<content::PageTransition>(
@@ -534,10 +534,87 @@ TEST_F(HistoryTest, AddRedirect) {
EXPECT_EQ(1, query_url_row_.visit_count());
ASSERT_EQ(1U, query_url_visits_.size());
EXPECT_EQ(content::PAGE_TRANSITION_CLIENT_REDIRECT |
- content::PAGE_TRANSITION_CHAIN_END, query_url_visits_[0].transition);
+ content::PAGE_TRANSITION_CHAIN_END,
+ query_url_visits_[0].transition);
EXPECT_EQ(second_visit, query_url_visits_[0].referring_visit);
}
+TEST_F(HistoryTest, MakeIntranetURLsTyped) {
+ scoped_refptr<HistoryService> history(new HistoryService);
+ history_service_ = history;
+ ASSERT_TRUE(history->Init(history_dir_, NULL));
+
+ // Add a non-typed visit to an intranet URL on an unvisited host. This should
+ // get promoted to a typed visit.
+ const GURL test_url("http://intranet_host/path");
+ history->AddPage(test_url, NULL, 0, GURL(), content::PAGE_TRANSITION_LINK,
+ history::RedirectList(), history::SOURCE_BROWSED, false);
+ EXPECT_TRUE(QueryURL(history, test_url));
+ EXPECT_EQ(1, query_url_row_.visit_count());
+ EXPECT_EQ(1, query_url_row_.typed_count());
+ ASSERT_EQ(1U, query_url_visits_.size());
+ EXPECT_EQ(content::PAGE_TRANSITION_TYPED,
+ content::PageTransitionStripQualifier(query_url_visits_[0].transition));
+
+ // Add more visits on the same host. None of these should be promoted since
+ // there is already a typed visit.
+
+ // Different path.
+ const GURL test_url2("http://intranet_host/different_path");
+ history->AddPage(test_url2, NULL, 0, GURL(), content::PAGE_TRANSITION_LINK,
+ history::RedirectList(), history::SOURCE_BROWSED, false);
+ EXPECT_TRUE(QueryURL(history, test_url2));
+ EXPECT_EQ(1, query_url_row_.visit_count());
+ EXPECT_EQ(0, query_url_row_.typed_count());
+ ASSERT_EQ(1U, query_url_visits_.size());
+ EXPECT_EQ(content::PAGE_TRANSITION_LINK,
+ content::PageTransitionStripQualifier(query_url_visits_[0].transition));
+
+ // No path.
+ const GURL test_url3("http://intranet_host/");
+ history->AddPage(test_url3, NULL, 0, GURL(), content::PAGE_TRANSITION_LINK,
+ history::RedirectList(), history::SOURCE_BROWSED, false);
+ EXPECT_TRUE(QueryURL(history, test_url3));
+ EXPECT_EQ(1, query_url_row_.visit_count());
+ EXPECT_EQ(0, query_url_row_.typed_count());
+ ASSERT_EQ(1U, query_url_visits_.size());
+ EXPECT_EQ(content::PAGE_TRANSITION_LINK,
+ content::PageTransitionStripQualifier(query_url_visits_[0].transition));
+
+ // Different scheme.
+ const GURL test_url4("https://intranet_host/");
+ history->AddPage(test_url4, NULL, 0, GURL(), content::PAGE_TRANSITION_LINK,
+ history::RedirectList(), history::SOURCE_BROWSED, false);
+ EXPECT_TRUE(QueryURL(history, test_url4));
+ EXPECT_EQ(1, query_url_row_.visit_count());
+ EXPECT_EQ(0, query_url_row_.typed_count());
+ ASSERT_EQ(1U, query_url_visits_.size());
+ EXPECT_EQ(content::PAGE_TRANSITION_LINK,
+ content::PageTransitionStripQualifier(query_url_visits_[0].transition));
+
+ // Different transition.
+ const GURL test_url5("http://intranet_host/another_path");
+ history->AddPage(test_url5, NULL, 0, GURL(),
+ content::PAGE_TRANSITION_AUTO_BOOKMARK,
+ history::RedirectList(), history::SOURCE_BROWSED, false);
+ EXPECT_TRUE(QueryURL(history, test_url5));
+ EXPECT_EQ(1, query_url_row_.visit_count());
+ EXPECT_EQ(0, query_url_row_.typed_count());
+ ASSERT_EQ(1U, query_url_visits_.size());
+ EXPECT_EQ(content::PAGE_TRANSITION_AUTO_BOOKMARK,
+ content::PageTransitionStripQualifier(query_url_visits_[0].transition));
+
+ // Original URL.
+ history->AddPage(test_url, NULL, 0, GURL(), content::PAGE_TRANSITION_LINK,
+ history::RedirectList(), history::SOURCE_BROWSED, false);
+ EXPECT_TRUE(QueryURL(history, test_url));
+ EXPECT_EQ(2, query_url_row_.visit_count());
+ EXPECT_EQ(1, query_url_row_.typed_count());
+ ASSERT_EQ(2U, query_url_visits_.size());
+ EXPECT_EQ(content::PAGE_TRANSITION_LINK,
+ content::PageTransitionStripQualifier(query_url_visits_[1].transition));
+}
+
TEST_F(HistoryTest, Typed) {
scoped_refptr<HistoryService> history(new HistoryService);
history_service_ = history;
diff --git a/chrome/browser/history/url_database.cc b/chrome/browser/history/url_database.cc
index 35fb59b..834ece4 100644
--- a/chrome/browser/history/url_database.cc
+++ b/chrome/browser/history/url_database.cc
@@ -291,7 +291,7 @@ bool URLDatabase::InitIconMappingEnumeratorForEverything(
return true;
}
-bool URLDatabase::AutocompleteForPrefix(const string16& prefix,
+bool URLDatabase::AutocompleteForPrefix(const std::string& prefix,
size_t max_results,
bool typed_only,
std::vector<history::URLRow>* results) {
@@ -323,11 +323,10 @@ bool URLDatabase::AutocompleteForPrefix(const string16& prefix,
// followed by the maximum character size. Use 8-bit strings for everything
// so we can be sure sqlite is comparing everything in 8-bit mode. Otherwise,
// it will have to convert strings either to UTF-8 or UTF-16 for comparison.
- std::string prefix_utf8(UTF16ToUTF8(prefix));
- std::string end_query(prefix_utf8);
+ std::string end_query(prefix);
end_query.push_back(std::numeric_limits<unsigned char>::max());
- statement.BindString(0, prefix_utf8);
+ statement.BindString(0, prefix);
statement.BindString(1, end_query);
statement.BindInt(2, static_cast<int>(max_results));
@@ -340,6 +339,23 @@ bool URLDatabase::AutocompleteForPrefix(const string16& prefix,
return !results->empty();
}
+bool URLDatabase::IsTypedHost(const std::string& host) {
+ const char* schemes[] = {
+ chrome::kHttpScheme,
+ chrome::kHttpsScheme,
+ chrome::kFtpScheme
+ };
+ std::vector<history::URLRow> dummy;
+ for (size_t i = 0; i < arraysize(schemes); ++i) {
+ std::string scheme_and_host(schemes[i]);
+ scheme_and_host += chrome::kStandardSchemeSeparator + host;
+ if (AutocompleteForPrefix(scheme_and_host + '/', 1, true, &dummy) ||
+ AutocompleteForPrefix(scheme_and_host + ':', 1, true, &dummy))
+ return true;
+ }
+ return false;
+}
+
bool URLDatabase::FindShortestURLFromBase(const std::string& base,
const std::string& url,
int min_visits,
diff --git a/chrome/browser/history/url_database.h b/chrome/browser/history/url_database.h
index 08c43d8..837a957 100644
--- a/chrome/browser/history/url_database.h
+++ b/chrome/browser/history/url_database.h
@@ -168,12 +168,16 @@ class URLDatabase {
// sorted by typed count, then by visit count, then by visit date (most recent
// first) up to the given maximum number. If |typed_only| is true, only urls
// that have been typed once are returned. For caller convenience, returns
- // whether any results were found. Called by HistoryURLProvider.
- bool AutocompleteForPrefix(const string16& prefix,
+ // whether any results were found.
+ bool AutocompleteForPrefix(const std::string& prefix,
size_t max_results,
bool typed_only,
std::vector<URLRow>* results);
+ // Returns true if the database holds some past typed navigation to a URL on
+ // the provided hostname.
+ bool IsTypedHost(const std::string& host);
+
// Tries to find the shortest URL beginning with |base| that strictly
// prefixes |url|, and has minimum visit_ and typed_counts as specified.
// If found, fills in |info| and returns true; otherwise returns false,