[sql] sql::ScopedErrorIgnorer rename to sql::test::ScopedErrorExpecter

sql/connection.h

Index: sql/connection.h
diff --git a/sql/connection.h b/sql/connection.h
index 8e8931f2e67d0dbc576987830015fa108b3797e4..5c955af8e27c6b11e077da500fbfd6d0bea7b7cb 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -39,6 +39,7 @@ class Statement;
 // To allow some test classes to be friended.
 namespace test {
 class ScopedCommitHook;
+class ScopedErrorExpecter;
 class ScopedScalarFunction;
 class ScopedMockTimeSource;
 }
@@ -474,15 +475,15 @@ class SQL_EXPORT Connection {
   //   SELECT type, name, tbl_name, sql FROM sqlite_master ORDER BY 1, 2, 3, 4;
   std::string GetSchema() const;
 
-  // Clients which provide an error_callback don't see the
-  // error-handling at the end of OnSqliteError().  Expose to allow
-  // those clients to work appropriately with ScopedErrorIgnorer in
-  // tests.
-  static bool ShouldIgnoreSqliteError(int error);
+  // Clients which provide an error_callback don't see the error-handling at the
+  // end of OnSqliteError().  Expose to allow those clients to work for tests
+  // using ScopedErrorExpecter.  |true| if the error is expected.

[Mark P, 2016/05/23 19:53:27]

I'm having trouble understanding this comment.  I think it's because the current
flow is confusing.  In particular, if you have an error callback, why can't you
simply remember the error and record it in OnSqliteError after dispatching the
callback?  In other words, why do you need to return immediately?

I mainly ask because this current design requires people who supply callbacks to
have this code at the end of their callback.  This is unnecessary and easy to
forget:
 if (!sql::Connection::IsExpectedSqliteError(extended_error))
    DLOG(FATAL) << db->GetErrorMessage();

[Scott Hess - ex-Googler, 2016/06/03 00:02:11]

This is the same concern you had elsewhere about doing the DLOG(FATAL) after the
code attempted to recover the database.  The code can't do the DLOG(FATAL)
before calling the error callback, because then the callback won't get the
chance to fix it.  It can't do the DLOG(FATAL) after calling the error callback,
because the callback may have fixed things.

+  static bool IsExpectedSqliteError(int error);
 
-  // Additionally ignores errors which are unlikely to be caused by problems
-  // with the syntax of a SQL statement, or problems with the database schema.
-  static bool ShouldIgnoreSqliteCompileError(int error);
+  // Return |true| in cases where Extend IsExpectedSqliteError() returns |true|,

[Mark P, 2016/05/23 19:53:27]

Extend?
Is this an errant word?  If so, delete it.  If not, correct / explain it better.

[Scott Hess - ex-Googler, 2016/06/03 00:02:11]

Done.  Errant word.

+  // and also in cases which are likely to be caused by the state of the
+  // database, rather than the statement being compiled.

[Mark P, 2016/05/23 19:53:27]

This comment is confusing.  It's imply that this returns true if
IsExpectedSqliteError() returns true or the dababase is corrupted.  It's almost
saying, unless IsExpectedSqliteError() returns true, if there's a compile error
here, I don't care.  Yet the function name implies that this is only about
compile errors.

[Scott Hess - ex-Googler, 2016/06/03 00:02:11]

I dug back into the history of this a bit.  It looks like there were two issues
which seemed related but which weren't.  One was that a browser test started
hitting a SQLITE_BUSY error (which means someone else is using the database). 
Normally that should never happen, but this is in a somewhat confusing piece of
diagnostics code which I have not fully detangled.  There were additional
SQLITE_CORRUPT/SQLITE_NOTADB (depending) errors in related unit tests, which I
thought stemmed from the same issue, so they were solved in the same place.

The goal of the call was to have a firm error output in case of SQL syntax
errors, even if the module has an error callback (which generally can suppress
errors).  Currently the code tries to say "Oh, these errors can be ignored",
I'll change it to just look for SQLITE_ERROR, which AFAICT is the only error
which matters for this use case.  Some other errors could plausibly flag static
issues to address, perhaps SQLITE_MISUSE or SQLITE_AUTH, but it's hard to craft
a story for them.  I can just let them lie until someone expresses confusion
over a problem they're finding.

+  static bool IsExpectedSqliteCompileError(int error);
 
   // Collect various diagnostic information and post a crash dump to aid
   // debugging.  Dump rate per database is limited to prevent overwhelming the
@@ -493,8 +494,8 @@ class SQL_EXPORT Connection {
   // For recovery module.
   friend class Recovery;
 
-  // Allow test-support code to set/reset error ignorer.
-  friend class ScopedErrorIgnorer;
+  // Allow test-support code to set/reset error expecter.
+  friend class test::ScopedErrorExpecter;
 
   // Statement accesses StatementRef which we don't want to expose to everybody
   // (they should go through Statement).
@@ -536,12 +537,12 @@ class SQL_EXPORT Connection {
   // Internal helper for DoesTableExist and DoesIndexExist.
   bool DoesTableOrIndexExist(const char* name, const char* type) const;
 
-  // Accessors for global error-ignorer, for injecting behavior during tests.
-  // See test/scoped_error_ignorer.h.
-  typedef base::Callback<bool(int)> ErrorIgnorerCallback;
-  static ErrorIgnorerCallback* current_ignorer_cb_;
-  static void SetErrorIgnorer(ErrorIgnorerCallback* ignorer);
-  static void ResetErrorIgnorer();
+  // Accessors for global error-expecter, for injecting behavior during tests.
+  // See test/scoped_error_expecter.h.
+  typedef base::Callback<bool(int)> ErrorExpecterCallback;
+  static ErrorExpecterCallback* current_expecter_cb_;
+  static void SetErrorExpecter(ErrorExpecterCallback* expecter);
+  static void ResetErrorExpecter();
 
   // A StatementRef is a refcounted wrapper around a sqlite statement pointer.
   // Refcounting allows us to give these statements out to sql::Statement