Enable accessibility testing for the bookmark browser test.

chrome/test/base/in_process_browser_test.h

Index: chrome/test/base/in_process_browser_test.h
diff --git a/chrome/test/base/in_process_browser_test.h b/chrome/test/base/in_process_browser_test.h
index 07710d2743a09f7e4cd643ef36e46d2969a5358a..1aed707e77c2eab6fe25c1bba5f4c3fd968b5062 100644
--- a/chrome/test/base/in_process_browser_test.h
+++ b/chrome/test/base/in_process_browser_test.h
@@ -9,6 +9,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_base.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -104,6 +105,9 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   void TearDown() override;
 
  protected:
+  // Constant for accessibility audit.
+  static const std::string kExpectedAccessibilityResults;

[Jay Civelli, 2015/02/05 23:29:09]

Does it need to be part of the class?

[hcarmona, 2015/02/06 22:50:53]

With the other change to make the parameter for the RunAccessibilityChecks into
an error message it's no longer necessary to have this constant at all.

+
   // Returns the browser created by CreateBrowser.
   Browser* browser() const { return browser_; }
 
@@ -148,6 +152,16 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   // the navigation to complete, and show the browser's window.
   void AddBlankTabAndShow(Browser* browser);
 
+  // Disables running of accessibility tests for a particular test case. This is
+  // only meaningful when called from IN_PROC_BROWSER_TEST_F to override the
+  // value of run_accessibility_checks_.
+  void DisableAccessibilityChecks();

[Jay Civelli, 2015/02/05 23:29:09]

How about having only 1 method?
SetAccessibilityCheckEnabled(bool enabled)

[hcarmona, 2015/02/06 22:50:53]

I think it makes sense. Collapsed both of these into 1 method.

+
+  // Enables running of accessibility tests for a particular test case. This is
+  // only meaningful when called from IN_PROC_BROWSER_TEST_F to override the
+  // value of run_accessibility_checks_.
+  void EnableAccessibilityChecks();
+
 #if !defined OS_MACOSX
   // Return a CommandLine object that is used to relaunch the browser_test
   // binary as a browser process. This function is deliberately not defined on
@@ -178,6 +192,15 @@ class InProcessBrowserTest : public content::BrowserTestBase {
     multi_desktop_test_ = multi_desktop_test;
   }
 
+  // This should be called in a sub-class constructor to enable/disable
+  // accessibility testing for all test cases.
+  void set_run_accessibility_checks(bool run_accessibility_checks) {

[Jay Civelli, 2015/02/05 23:29:09]

I am a bit confused by this.
You can either enable it always (in which case RunAccessibilityChecks is called
by the test framework for you for all test cases) or on a test case bases by
calling RunAccessibilityChecks from your test.
Did I get it right?

[hcarmona, 2015/02/06 22:50:53]

I think you're right and this is too confusing. I've simplified this a bit by
only having one method now. This method can be called in SetUpOnMainThread to
enable a11y audit for all tests but can still be called from the test body to
enable/disable only one test if necessary.

A test shouldn't need to call RunAccessibilityChecks because they can just
enable it. It's here so that it can be tested in
InProcessAccessibilityBrowsertest.

[Jay Civelli, 2015/02/07 01:11:12]

This looks much better, thanks!

+    run_accessibility_checks_ = run_accessibility_checks;
+  }
+
+  // Will run accessibility checks and fail the test if there are any issues.
+  void RunAccessibilityChecks(std::string *test_result);

[Jay Civelli, 2015/02/05 23:29:09]

std::string*

[hcarmona, 2015/02/06 22:50:53]

Done.

+
  private:
   // Creates a user data directory for the test if one is needed. Returns true
   // if successful.
@@ -190,6 +213,12 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   // with an in-process test.
   void PrepareTestCommandLine(base::CommandLine* command_line);
 
+  // This method loads the javascript accessibility library.
+  // Returns false on loading error. Accessibility audit will fail if library is

[Jay Civelli, 2015/02/05 23:29:09]

Should it just assert if it fails to load?

[hcarmona, 2015/02/06 22:50:53]

Only called one place. Moved this method's contents into RunAccessibilityChecks.

+  // not loaded.
+  bool LoadAccessibilityLibrary(content::WebContents* web_contents)
+      WARN_UNUSED_RESULT;
+
   // Browser created from CreateBrowser.
   Browser* browser_;
 
@@ -207,6 +236,13 @@ class InProcessBrowserTest : public content::BrowserTestBase {
   // not ensure that Browsers are only created on the tested desktop).
   bool multi_desktop_test_;
 
+  // True if the accessibility tests should be run.
+  bool run_accessibility_checks_;
+
+  // True if the accessibility test should run for a particular test case.
+  // This is reset for every test case.
+  bool run_accessibility_checks_for_test_case_;
+
 #if defined(OS_MACOSX)
   base::mac::ScopedNSAutoreleasePool* autorelease_pool_;
 #endif  // OS_MACOSX