Rewrote few prefs related pyauto tests as browser_tests.

chrome/browser/prefs/pref_functional_browsertest.cc

Index: chrome/browser/prefs/pref_functional_browsertest.cc
diff --git a/chrome/browser/prefs/pref_functional_browsertest.cc b/chrome/browser/prefs/pref_functional_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..87112e4f17e064c05d749127e4f9bc26260191cc
--- /dev/null
+++ b/chrome/browser/prefs/pref_functional_browsertest.cc
@@ -0,0 +1,180 @@
+// Copyright (c) 2013 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 <string>
+
+#include "base/path_service.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/download_test_observer.h"
+
+using content::BrowserContext;
+using content::DownloadManager;
+
+class PrefsFunctionalTest : public InProcessBrowserTest {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) {
+    PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_);
+  }

[Bernhard Bauer, 2013/08/01 11:38:42]

Nit: Newline before public/protected/private sections.

[pshenoy, 2013/08/01 17:26:44]

Done.

+ protected:
+  // Create a DownloadTestObserverTerminal that will wait for the
+  // specified number of downloads to finish.
+  content::DownloadTestObserver* CreateWaiter(

[Bernhard Bauer, 2013/08/01 11:38:42]

Who owns this object? The caller? If so, you can return a scoped_ptr.

[pshenoy, 2013/08/01 17:26:44]

I am using scoped_ptr in line# 69 where it is actually getting called. Please
suggest if that's enough?

[Bernhard Bauer, 2013/08/01 22:20:07]

You can also return a scoped_ptr here. The effect is the same, but it documents
that the caller takes ownership, and the compiler will verify it too.

[pshenoy, 2013/08/01 23:44:33]

Done.

+      Browser* browser, int num_downloads) {

[Bernhard Bauer, 2013/08/01 11:38:42]

Does this not fit on the previous line?

[pshenoy, 2013/08/01 17:26:44]

Done.

+    DownloadManager* download_manager =
+      BrowserContext::GetDownloadManager(browser->profile());

[Bernhard Bauer, 2013/08/01 11:38:42]

Indent two more spaces.

[pshenoy, 2013/08/01 17:26:44]

Done.

+    return new content::DownloadTestObserverTerminal(
+        download_manager, num_downloads,
+        content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
+  }
+  base::FilePath test_data_dir_;

[Bernhard Bauer, 2013/08/01 11:38:42]

Nit: A newline before this would be nice.

Hm, is this even used?

[pshenoy, 2013/08/01 17:26:44]

Done.

+};
+
+IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestDownloadDirPref) {
+  ASSERT_TRUE(test_server()->Start());
+
+  DownloadManager* download_manager =
+      BrowserContext::GetDownloadManager(browser()->profile());
+  base::FilePath download_dir =
+      (DownloadPrefs::FromDownloadManager(download_manager))->DownloadPath();
+  base::FilePath new_download_dir = download_dir.AppendASCII("my_downloads");
+  base::FilePath downloaded_pkg =
+      new_download_dir.AppendASCII("a_zip_file.zip");
+
+  // If the directory exists, delete it.
+  if (base::PathExists(new_download_dir)) {
+    base::DeleteFile(new_download_dir, true);
+  }
+
+  // Create the new downloads directory.
+  file_util::CreateDirectory(new_download_dir);
+  // Set pref to download in new_download_dir.
+  browser()->profile()->GetPrefs()->SetFilePath(
+      prefs::kDownloadDefaultDirectory,
+      new_download_dir);
+
+  // Create a downloads observer.
+  scoped_ptr<content::DownloadTestObserver> downloads_observer(
+      CreateWaiter(browser(), 1));
+  ui_test_utils::NavigateToURL(
+      browser(),
+      test_server()->GetURL("files/downloads/a_zip_file.zip"));
+  // Waits for the download to complete.
+  downloads_observer->WaitForFinished();
+  EXPECT_TRUE(base::PathExists(downloaded_pkg));
+}
+
+// Verify image content settings show or hide images.
+IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestImageContentSettings) {
+  ASSERT_TRUE(test_server()->Start());
+
+  ui_test_utils::NavigateToURL(
+      browser(),
+      test_server()->GetURL("files/settings/image_page.html"));
+
+  bool result = false;
+  std::string script =
+      "for (i=0; i < document.images.length; i++) {"
+      "  if ((document.images[i].naturalWidth != 0) &&"
+      "      (document.images[i].naturalHeight != 0)) {"
+      "    window.domAutomationController.send(true);"
+      "  }"
+      "}"
+      "window.domAutomationController.send(false);";
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      script,
+      &result));
+  EXPECT_TRUE(result);
+
+  base::DictionaryValue value;
+  value.SetInteger("images", 2);
+  browser()->profile()->GetPrefs()->Set(prefs::kDefaultContentSettings,
+                                        value);
+
+  ui_test_utils::NavigateToURL(
+      browser(),
+      test_server()->GetURL("files/settings/image_page.html"));
+
+  result = false;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      script,
+      &result));
+  EXPECT_FALSE(result);
+}
+
+// Verify enabling disbling javascript perfs work.

[Bernhard Bauer, 2013/08/01 11:38:42]

"Verify that enabling/disabling Javascript in prefs works."

[pshenoy, 2013/08/01 17:26:44]

Done.

+IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestJavascriptEnableDisable) {
+  ASSERT_TRUE(test_server()->Start());
+
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kWebKitJavascriptEnabled));
+  ui_test_utils::NavigateToURL(
+      browser(),
+      test_server()->GetURL("files/javaScriptTitle.html"));
+  EXPECT_EQ(ASCIIToUTF16("Title from script javascript enabled"),
+            browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kWebKitJavascriptEnabled,
+                                               false);
+  ui_test_utils::NavigateToURL(
+      browser(),
+      test_server()->GetURL("files/javaScriptTitle.html"));
+  EXPECT_EQ(ASCIIToUTF16("This is html title"),
+            browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
+}
+
+// Verify DNS prefetching pref.
+IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestNetworkPredictionEnabledPref) {
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kNetworkPredictionEnabled));
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kNetworkPredictionEnabled,
+                                               false);
+  chrome::Exit();

[Bernhard Bauer, 2013/08/01 11:38:42]

Hm, I'm confused -- this is an in-process test, right? So what does
chrome::Exit() do then?

[pshenoy, 2013/08/01 17:26:44]

I'm confused too :-) I wanted to close all browser windows and chrome::Exit()
actually did it for me. I am not sure how do we simulate restarting the browser
within browser_test.

Note: I tried running the test without "chrome::Exit() and chrome::NewWindow()"
and it works fine. Please suggest if there is a better way of handling this.

[Bernhard Bauer, 2013/08/01 22:20:07]

Well, yes. The test sets a preference and then reads it again. Whether you close
browser windows or not doesn't have any effect on that.
It depends on what you want to test. If you want to test that preferences are
preserved across browser restarts, you will need to start the browser twice,
which you can't do in a single test. One thing you could do would be to split up
the test into two: one test that sets the preference, possibly commits the
pending write, and then verifies that the Preferences file has been written out
to disk, and one test that initially makes sure the Preferences file exists on
disk, then starts the browser and verifies that the preference has been read.

Another thing to test (which TestSessionRestoreShowBookmarkBar seems to do) is
to test the *effect* of the preferences, by setting a preference and then
verifying the resulting behavior. That depends on the preference of course.

[pshenoy, 2013/08/01 23:44:33]

I have removed 'chrome::Exit() and chrome::NewWindow()' for these tests as it
has no effect.

Just curious. How do we preserve preferences across tests in browser_tests? As I
understand (I have very limited knowledge about browser_tests) each browser_test
launches browser with a clean profile. So am not sure how we can make use of the
preference set by one test in another. 

[Bernhard Bauer, 2013/08/02 08:46:12]

Use a PRE_ prefix:
http://www.chromium.org/developers/testing/browser-tests#TOC-Tests-Spanning-R...

Alternatively, use two tests: One that verifies that the preference is set in
the Preferences file on disk, and one that writes out that Preferences file
before starting the browser. Those can then run independently.

[pshenoy, 2013/08/02 16:36:17]

Thank you so much for the information.

+  chrome::NewWindow(browser());
+
+  EXPECT_FALSE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kNetworkPredictionEnabled));
+}
+
+// Verify toolbar buttons prefs.
+IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest, TestToolbarButtonsPref) {

[Bernhard Bauer, 2013/08/01 11:38:42]

This doesn't really test much else than the previous test.

[pshenoy, 2013/08/01 17:26:44]

Done.

+  EXPECT_FALSE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kShowHomeButton));
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kShowHomeButton, true);
+
+  chrome::Exit();
+  chrome::NewWindow(browser());
+
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kShowHomeButton));
+}
+
+// Verify restore for bookmark bar visibility.
+IN_PROC_BROWSER_TEST_F(PrefsFunctionalTest,
+                       TestSessionRestoreShowBookmarkBar) {
+  EXPECT_FALSE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kShowBookmarkBar));
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kShowBookmarkBar, true);
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kShowBookmarkBar));
+
+  chrome::Exit();
+  chrome::NewWindow(browser());
+
+  EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kShowBookmarkBar));
+  EXPECT_EQ(BookmarkBar::SHOW, browser()->bookmark_bar_state());
+}