Scrape search definitions from forms that have onsubmit handlers. The scraping is done after submit events are handled by the page DOM so doing this is safe.

Adds test infrastructure for determining that scraping occurs on submit:
- allow testserver to be configured to serve pages from / on the server
- provide a ui test util that navigates and waits for N subsequent redirections/navigations before returning control to the test to handle automated submission

Eric, please review the test server changes.
Scott, please look over everything else.
Review URL: http://codereview.chromium.org/62145

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@13444 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 210 insertions, 31 deletions

diff --git a/chrome/browser/search_engines/template_url_scraper_unittest.cc b/chrome/browser/search_engines/template_url_scraper_unittest.cc
new file mode 100644
index 0000000..dc02852
--- /dev/null
+++ b/chrome/browser/search_engines/template_url_scraper_unittest.cc
@@ -0,0 +1,88 @@
+// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this
+// source code is governed by a BSD-style license that can be found in the
+// LICENSE file.
+
+#include "chrome/browser/browser.h"
+#include "chrome/browser/profile.h"
+#include "chrome/browser/search_engines/template_url_model.h"
+#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
+#include "chrome/common/notification_registrar.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+#include "net/base/host_resolver_unittest.h"
+#include "net/base/net_util.h"
+
+namespace {
+class TemplateURLScraperTest : public InProcessBrowserTest {
+ public:
+  TemplateURLScraperTest() {
+  }
+
+ protected:
+  virtual void ConfigureHostMapper(net::RuleBasedHostMapper* host_mapper) {
+    InProcessBrowserTest::ConfigureHostMapper(host_mapper);
+    // We use foo.com in our tests.
+    host_mapper->AddRule("*.foo.com", "localhost");
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TemplateURLScraperTest);
+};
+
+class TemplateURLModelLoader : public NotificationObserver {
+ public:
+  explicit TemplateURLModelLoader(TemplateURLModel* model) : model_(model) {
+    registrar_.Add(this, NotificationType::TEMPLATE_URL_MODEL_LOADED,
+                   Source<TemplateURLModel>(model));
+    model_->Load();
+    ui_test_utils::RunMessageLoop();
+  }
+
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+    if (type == NotificationType::TEMPLATE_URL_MODEL_LOADED &&
+        Source<TemplateURLModel>(source).ptr() == model_) {
+      MessageLoop::current()->Quit();
+    }
+  }
+
+ private:
+  NotificationRegistrar registrar_;
+
+  TemplateURLModel* model_;
+
+  DISALLOW_COPY_AND_ASSIGN(TemplateURLModelLoader);
+};
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(TemplateURLScraperTest, ScrapeWithOnSubmit) {
+  TemplateURLModel* template_urls = browser()->profile()->GetTemplateURLModel();
+  TemplateURLModelLoader loader(template_urls);
+
+  std::vector<const TemplateURL*> all_urls = template_urls->GetTemplateURLs();
+
+  // We need to substract the default pre-populated engines that the profile is
+  // set up with.
+  size_t default_index = 0;
+  std::vector<TemplateURL*> prepopulate_urls;
+  TemplateURLPrepopulateData::GetPrepopulatedEngines(
+      browser()->profile()->GetPrefs(),
+      &prepopulate_urls,
+      &default_index);
+
+  EXPECT_EQ(prepopulate_urls.size(), all_urls.size());
+
+  scoped_refptr<HTTPTestServer> server(
+      HTTPTestServer::CreateServerWithFileRootURL(
+          L"chrome/test/data/template_url_scraper/submit_handler", L"/",
+          g_browser_process->io_thread()->message_loop()));
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      browser(), GURL("http://www.foo.com:1337/"), 2);
+
+  all_urls = template_urls->GetTemplateURLs();
+  EXPECT_EQ(1, all_urls.size() - prepopulate_urls.size());
+}
diff --git a/chrome/test/data/template_url_scraper/submit_handler/index.html b/chrome/test/data/template_url_scraper/submit_handler/index.html
new file mode 100644
index 0000000..2e0b04f
--- /dev/null
+++ b/chrome/test/data/template_url_scraper/submit_handler/index.html
@@ -0,0 +1,24 @@
+<html>
+<head>
+<title>Submit handler TemplateURL scraping test</title>
+<script>
+
+function submit_handler() {
+  document.getElementById("data").value = "test_data";
+}
+
+function submit_form() {
+  document.forms[0].submit();
+}
+
+</script>
+</head>
+
+<body onload="submit_form();">
+<form action="http://www.foo.com:1337" onsubmit="submit_handler();">
+<input type="hidden" id="data" />
+<input type="text" name="q" />
+<input type="submit" />
+</form>
+</body>
+</html>
diff --git a/chrome/test/in_process_browser_test.cc b/chrome/test/in_process_browser_test.cc
index 39114ee..6699144 100644
--- a/chrome/test/in_process_browser_test.cc
+++ b/chrome/test/in_process_browser_test.cc
@@ -125,12 +125,7 @@ void InProcessBrowserTest::SetUp() {
 
   scoped_refptr<net::RuleBasedHostMapper> host_mapper(
       new net::RuleBasedHostMapper());
-  // TODO(sky): Don't make a real dns lookup here or simulate a failing
-  // lookup.
-  host_mapper->AllowDirectLookup("*.google.com");
-  // See http://en.wikipedia.org/wiki/Web_Proxy_Autodiscovery_Protocol
-  // We don't want the test code to use it.
-  host_mapper->AddSimulatedFailure("wpad");
+  ConfigureHostMapper(host_mapper.get());
   net::ScopedHostMapper scoped_host_mapper(host_mapper.get());
   BrowserMain(params);
 }
@@ -221,3 +216,11 @@ void InProcessBrowserTest::RunTestOnMainThreadLoop() {
 
   MessageLoopForUI::current()->Quit();
 }
+
+void InProcessBrowserTest::ConfigureHostMapper(
+    net::RuleBasedHostMapper* host_mapper) {
+  host_mapper->AllowDirectLookup("*.google.com");
+  // See http://en.wikipedia.org/wiki/Web_Proxy_Autodiscovery_Protocol
+  // We don't want the test code to use it.
+  host_mapper->AddSimulatedFailure("wpad");
+}
diff --git a/chrome/test/in_process_browser_test.h b/chrome/test/in_process_browser_test.h
index 89de9b9..457516e 100644
--- a/chrome/test/in_process_browser_test.h
+++ b/chrome/test/in_process_browser_test.h
@@ -12,6 +12,9 @@
 
 class Browser;
 class Profile;
+namespace net {
+class RuleBasedHostMapper;
+}
 
 // Base class for tests wanting to bring up a browser in the unit test process.
 // Writing tests with InProcessBrowserTest is slightly different than that of
@@ -64,6 +67,11 @@ class InProcessBrowserTest : public testing::Test, public NotificationObserver {
   // Override this rather than TestBody.
   virtual void RunTestOnMainThread() = 0;
 
+  // Allows subclasses to configure the host mapper. By default this blocks
+  // requests to google.com as Chrome pings that on startup and we don't want to
+  // do that during testing.
+  virtual void ConfigureHostMapper(net::RuleBasedHostMapper* host_mapper);
+
   // Starts an HTTP server.
   HTTPTestServer* StartHTTPServer();
 
diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc
index 47e3ef2..6af1091 100644
--- a/chrome/test/ui_test_utils.cc
+++ b/chrome/test/ui_test_utils.cc
@@ -20,8 +20,11 @@ namespace {
 // Used to block until a navigation completes.
 class NavigationNotificationObserver : public NotificationObserver {
  public:
-  explicit NavigationNotificationObserver(NavigationController* controller)
-      : navigation_started_(false) {
+  NavigationNotificationObserver(NavigationController* controller,
+                                 int number_of_navigations)
+      : navigation_started_(false),
+        navigations_completed_(0),
+        number_of_navigations_(number_of_navigations) {
     registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
                    Source<NavigationController>(controller));
     registrar_.Add(this, NotificationType::LOAD_START,
@@ -38,7 +41,8 @@ class NavigationNotificationObserver : public NotificationObserver {
         type == NotificationType::LOAD_START) {
       navigation_started_ = true;
     } else if (type == NotificationType::LOAD_STOP) {
-      if (navigation_started_) {
+      if (navigation_started_ &&
+          ++navigations_completed_ == number_of_navigations_) {
         navigation_started_ = false;
         MessageLoopForUI::current()->Quit();
       }
@@ -51,6 +55,12 @@ class NavigationNotificationObserver : public NotificationObserver {
   // If true the navigation has started.
   bool navigation_started_;
 
+  // The number of navigations that have been completed.
+  int navigations_completed_;
+
+  // The number of navigations to wait for.
+  int number_of_navigations_;
+
   DISALLOW_COPY_AND_ASSIGN(NavigationNotificationObserver);
 };
 
@@ -66,15 +76,26 @@ void RunMessageLoop() {
 }
 
 void WaitForNavigation(NavigationController* controller) {
-  NavigationNotificationObserver observer(controller);
+  WaitForNavigations(controller, 1);
+}
+
+void WaitForNavigations(NavigationController* controller,
+                        int number_of_navigations) {
+  NavigationNotificationObserver observer(controller, number_of_navigations);
 }
 
 void NavigateToURL(Browser* browser, const GURL& url) {
+  NavigateToURLBlockUntilNavigationsComplete(browser, url, 1);
+}
+
+void NavigateToURLBlockUntilNavigationsComplete(Browser* browser,
+                                                const GURL& url,
+                                                int number_of_navigations) {
   NavigationController* controller =
       browser->GetSelectedTabContents()->controller();
   browser->OpenURLFromTab(browser->GetSelectedTabContents(), url, GURL(),
                           CURRENT_TAB, PageTransition::TYPED);
-  WaitForNavigation(controller);
+  WaitForNavigations(controller, number_of_navigations);
 }
 
 }  // namespace ui_test_utils
diff --git a/chrome/test/ui_test_utils.h b/chrome/test/ui_test_utils.h
index f0433b0..ced32d2 100644
--- a/chrome/test/ui_test_utils.h
+++ b/chrome/test/ui_test_utils.h
@@ -21,10 +21,20 @@ void RunMessageLoop();
 // the navigation finishes.
 void WaitForNavigation(NavigationController* controller);
 
+// Waits for |controller| to complete a navigation. This blocks until
+// the specified number of navigations complete.
+void WaitForNavigations(NavigationController* controller,
+                        int number_of_navigations);
+
 // Navigates the selected tab of |browser| to |url|, blocking until the
 // navigation finishes.
 void NavigateToURL(Browser* browser, const GURL& url);
 
+// Navigates the selected tab of |browser| to |url|, blocking until the
+// number of navigations specified complete.
+void NavigateToURLBlockUntilNavigationsComplete(Browser* browser,
+                                                const GURL& url,
+                                                int number_of_navigations);
 }
 
 #endif  // CHROME_TEST_UI_TEST_UTILS_H_
diff --git a/chrome/test/unit/unittests.vcproj b/chrome/test/unit/unittests.vcproj
index 4a5d3cd..fb35266 100644
--- a/chrome/test/unit/unittests.vcproj
+++ b/chrome/test/unit/unittests.vcproj
@@ -704,6 +704,10 @@
 				>
 			</File>
 			<File
+				RelativePath="..\..\browser\search_engines\template_url_scraper_unittest.cc"
+				>
+			</File>
+			<File
 				RelativePath="..\..\browser\search_engines\template_url_unittest.cc"
 				>
 			</File>
diff --git a/net/base/ssl_client_socket_unittest.cc b/net/base/ssl_client_socket_unittest.cc
index 7c7b170..2399466 100644
--- a/net/base/ssl_client_socket_unittest.cc
+++ b/net/base/ssl_client_socket_unittest.cc
@@ -27,21 +27,21 @@ class SSLClientSocketTest : public PlatformTest {
   void StartOKServer() {
     bool success = server_.Start(net::TestServerLauncher::ProtoHTTP,
         server_.kHostName, server_.kOKHTTPSPort,
-        FilePath(), server_.GetOKCertPath());
+        FilePath(), server_.GetOKCertPath(), std::wstring());
     ASSERT_TRUE(success);
   }
 
   void StartMismatchedServer() {
     bool success = server_.Start(net::TestServerLauncher::ProtoHTTP,
         server_.kMismatchedHostName, server_.kOKHTTPSPort,
-        FilePath(), server_.GetOKCertPath());
+        FilePath(), server_.GetOKCertPath(), std::wstring());
     ASSERT_TRUE(success);
   }
 
   void StartExpiredServer() {
     bool success = server_.Start(net::TestServerLauncher::ProtoHTTP,
         server_.kHostName, server_.kBadHTTPSPort,
-        FilePath(), server_.GetExpiredCertPath());
+        FilePath(), server_.GetExpiredCertPath(), std::wstring());
     ASSERT_TRUE(success);
   }
 
diff --git a/net/base/ssl_test_util.cc b/net/base/ssl_test_util.cc
index 71edb6a..a3fe3b9 100644
--- a/net/base/ssl_test_util.cc
+++ b/net/base/ssl_test_util.cc
@@ -152,7 +152,8 @@ void TestServerLauncher::SetPythonPath() {
 bool TestServerLauncher::Start(Protocol protocol,
                                const std::string& host_name, int port,
                                const FilePath& document_root,
-                               const FilePath& cert_path) {
+                               const FilePath& cert_path,
+                               const std::wstring& file_root_url) {
   if (!cert_path.value().empty()) {
     if (!LoadTestRootCert())
       return false;
@@ -198,6 +199,11 @@ bool TestServerLauncher::Start(Protocol protocol,
     command_line.append(cert_path.ToWStringHack());
     command_line.append(L"\"");
   }
+  if (!file_root_url.empty()) {
+    command_line.append(L" --file-root-url=\"");
+    command_line.append(file_root_url);
+    command_line.append(L"\"");
+  }
 
   if (!base::LaunchApp(command_line, false, true, &process_handle_)) {
     LOG(ERROR) << "Failed to launch " << command_line;
diff --git a/net/base/ssl_test_util.h b/net/base/ssl_test_util.h
index b75453b..54449ee 100644
--- a/net/base/ssl_test_util.h
+++ b/net/base/ssl_test_util.h
@@ -39,12 +39,15 @@ class TestServerLauncher {
   // Start src/net/tools/testserver/testserver.py and
   // ask it to serve the given protocol.
   // If protocol is HTTP, and cert_path is not empty, serves HTTPS.
+  // file_root_url specifies the root url on the server that documents will be
+  // served out of. This is /files/ by default.
   // Returns true on success, false if files not found or root cert
   // not trusted.
   bool Start(Protocol protocol,
              const std::string& host_name, int port,
              const FilePath& document_root,
-             const FilePath& cert_path);
+             const FilePath& cert_path,
+             const std::wstring& file_root_url);
 
   // Stop the server started by Start().
   bool Stop();
diff --git a/net/tools/testserver/testserver.py b/net/tools/testserver/testserver.py
index 5784059..9ab77a8 100644
--- a/net/tools/testserver/testserver.py
+++ b/net/tools/testserver/testserver.py
@@ -565,13 +565,15 @@ class TestPageHandler(BaseHTTPServer.BaseHTTPRequestHandler):
     """This handler sends the contents of the requested file.  Wow, it's like
     a real webserver!"""
 
-    prefix='/files/'
+    prefix = self.server.file_root_url
     if not self.path.startswith(prefix):
       return False
 
     file = self.path[len(prefix):]
     entries = file.split('/');
     path = os.path.join(self.server.data_dir, *entries)
+    if os.path.isdir(path):
+      path = os.path.join(path, 'index.html')
 
     if not os.path.isfile(path):
       print "File not found " + file + " full path:" + path
@@ -1047,6 +1049,7 @@ def main(options, args):
       print 'HTTP server started on port %d...' % port
 
     server.data_dir = MakeDataDir()
+    server.file_root_url = options.file_root_url
     MakeDumpDir(server.data_dir)
 
   # means FTP Server
@@ -1102,6 +1105,8 @@ if __name__ == '__main__':
                            help='Specify that https should be used, specify '
                            'the path to the cert containing the private key '
                            'the server should use')
+  option_parser.add_option('', '--file-root-url', default='/files/',
+                           help='Specify a root URL for files served.')
   options, args = option_parser.parse_args()
 
   sys.exit(main(options, args))
diff --git a/net/url_request/url_request_unittest.h b/net/url_request/url_request_unittest.h
index 88b83a6..e661153 100644
--- a/net/url_request/url_request_unittest.h
+++ b/net/url_request/url_request_unittest.h
@@ -260,20 +260,22 @@ class BaseTestServer : public base::RefCounted<BaseTestServer> {
   bool Start(net::TestServerLauncher::Protocol protocol,
              const std::string& host_name, int port,
              const FilePath& document_root,
-             const FilePath& cert_path) {
+             const FilePath& cert_path,
+             const std::wstring& file_root_url) {
     std::string blank;
     return Start(protocol, host_name, port, document_root, cert_path,
-                 blank, blank);
+                 file_root_url, blank, blank);
   }
 
   bool Start(net::TestServerLauncher::Protocol protocol,
              const std::string& host_name, int port,
              const FilePath& document_root,
              const FilePath& cert_path,
+             const std::wstring& file_root_url,
              const std::string& url_user,
              const std::string& url_password) {
     if (!launcher_.Start(protocol,
-        host_name, port, document_root, cert_path))
+        host_name, port, document_root, cert_path, file_root_url))
       return false;
 
     std::string scheme;
@@ -338,13 +340,20 @@ class HTTPTestServer : public BaseTestServer {
   static scoped_refptr<HTTPTestServer> CreateServer(
       const std::wstring& document_root,
       MessageLoop* loop) {
+    return CreateServerWithFileRootURL(document_root, std::wstring(), loop);
+  }
+
+  static scoped_refptr<HTTPTestServer> CreateServerWithFileRootURL(
+      const std::wstring& document_root,
+      const std::wstring& file_root_url,
+      MessageLoop* loop) {
     scoped_refptr<HTTPTestServer> test_server = new HTTPTestServer();
     test_server->loop_ = loop;
     FilePath no_cert;
     FilePath docroot = FilePath::FromWStringHack(document_root);
     if (!test_server->Start(net::TestServerLauncher::ProtoHTTP,
                             kDefaultHostName, kHTTPDefaultPort,
-                            docroot, no_cert)) {
+                            docroot, no_cert, file_root_url)) {
       return NULL;
     }
     return test_server;
@@ -438,7 +447,7 @@ class HTTPSTestServer : public HTTPTestServer {
     if (!test_server->Start(net::TestServerLauncher::ProtoHTTP,
         net::TestServerLauncher::kHostName,
         net::TestServerLauncher::kOKHTTPSPort,
-        docroot, certpath)) {
+        docroot, certpath, std::wstring())) {
       return NULL;
     }
     return test_server;
@@ -454,7 +463,7 @@ class HTTPSTestServer : public HTTPTestServer {
     if (!test_server->Start(net::TestServerLauncher::ProtoHTTP,
         net::TestServerLauncher::kMismatchedHostName,
         net::TestServerLauncher::kOKHTTPSPort,
-        docroot, certpath)) {
+        docroot, certpath, std::wstring())) {
       return NULL;
     }
     return test_server;
@@ -469,7 +478,7 @@ class HTTPSTestServer : public HTTPTestServer {
     if (!test_server->Start(net::TestServerLauncher::ProtoHTTP,
         net::TestServerLauncher::kHostName,
         net::TestServerLauncher::kBadHTTPSPort,
-        docroot, certpath)) {
+        docroot, certpath, std::wstring())) {
       return NULL;
     }
     return test_server;
@@ -484,7 +493,7 @@ class HTTPSTestServer : public HTTPTestServer {
     FilePath docroot = FilePath::FromWStringHack(document_root);
     FilePath certpath = FilePath::FromWStringHack(cert_path);
     if (!test_server->Start(net::TestServerLauncher::ProtoHTTP,
-        host_name, port, docroot, certpath)) {
+        host_name, port, docroot, certpath, std::wstring())) {
       return NULL;
     }
     return test_server;
@@ -517,7 +526,7 @@ class FTPTestServer : public BaseTestServer {
     FilePath docroot = FilePath::FromWStringHack(document_root);
     FilePath no_cert;
     if (!test_server->Start(net::TestServerLauncher::ProtoFTP,
-        kDefaultHostName, kFTPDefaultPort, docroot, no_cert,
+        kDefaultHostName, kFTPDefaultPort, docroot, no_cert, std::wstring(),
         url_user, url_password)) {
       return NULL;
     }
diff --git a/webkit/glue/searchable_form_data.cc b/webkit/glue/searchable_form_data.cc
index 3c787b2..6187897 100644
--- a/webkit/glue/searchable_form_data.cc
+++ b/webkit/glue/searchable_form_data.cc
@@ -320,10 +320,8 @@ SearchableFormData* SearchableFormData::Create(WebCore::HTMLFormElement* form) {
   if (frame == NULL)
     return NULL;
 
-  // Only consider forms that GET data, do not have script for onsubmit, and
-  // the action targets an http page.
-  if (!IsFormMethodGet(form) || FormHasOnSubmit(form) ||
-      !IsHTTPFormSubmit(form))
+  // Only consider forms that GET data and the action targets an http page.
+  if (!IsFormMethodGet(form) || !IsHTTPFormSubmit(form))
     return NULL;
 
   Vector<char> enc_string;
diff --git a/webkit/glue/unittest_test_server.h b/webkit/glue/unittest_test_server.h
index c8e622b..583abf9 100644
--- a/webkit/glue/unittest_test_server.h
+++ b/webkit/glue/unittest_test_server.h
@@ -26,7 +26,7 @@ class UnittestTestServer : public HTTPTestServer {
     FilePath no_cert;
     FilePath docroot = FilePath::FromWStringHack(L"webkit/data");
     if (!test_server->Start(net::TestServerLauncher::ProtoHTTP,
-        "localhost", 1337, docroot, no_cert)) {
+        "localhost", 1337, docroot, no_cert, std::wstring())) {
       delete test_server;
       return NULL;
     }