Move ErrorDelegate to its own file and add static utility functions to ErrorDelegate

Move ErrorDelegate to its own file and add static utility functions to ErrorDelegate

BUG=151841
Test=None
R=shess, erikwright
TBR=cpu

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162647

[pkotwicz, 2012-10-13 21:43:11]

In this CL, I moved ErrorDelegate to its own file (sql/error_delegate.h)
I also added sql::ErrorDelegate::IsErrorCatastrophic(). I think that this is the
correct place for this.

I also implemented an alternative to https://codereview.chromium.org/11112020 (I
am unsure if it is better). In my implementation KillDatabaseErrorDelegate is
the only error delegate for the cookies database.
This has the nice side effect of removing the necessity for
GetErrorHandlerForCookieDb(). As per the comment in chrome/browser/history/DEPS
we want to remove dependency on this file at least for history related code.

[pkotwicz, 2012-10-17 13:54:52]

Scott can you please take a look?

[Scott Hess - ex-Googler, 2012-10-17 21:28:48]

https://codereview.chromium.org/11141012/diff/5001/chrome/browser/diagnostics...
File chrome/browser/diagnostics/sqlite_diagnostics.cc (right):

https://codereview.chromium.org/11141012/diff/5001/chrome/browser/diagnostics...
chrome/browser/diagnostics/sqlite_diagnostics.cc:109: sql::ErrorDelegate*
GetErrorHandlerForHistoryDb() {
This seems error-prone!  But, AFAICT, it's right.

I have a change I'm poking to see if I can simplify this, but templates are
hard.

https://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:290:
LogAndRecordErrorInHistogram(error_type, connection, HistogramUniquifier());
I don't know that I'd actually have any issues with simply inlining the LOG()
and histogram here.  If you keep the histogram name the same then the stats will
maintain continuity with old records.

https://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:297:
attempted_to_kill_database_ = true;
Would it be reasonable to just set backend_ = NULL after posting?

https://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h
File sql/error_delegate.h (right):

https://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newco...
sql/error_delegate.h:27: // sqlite\preprocessed\sqlite3.h
Use forward slashes.  Or even just "sqlite3.h" would be fine, since this path is
no longer right.

https://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newco...
sql/error_delegate.h:37: // that you return the original |error| or the
appropiae error code.
appropriate.

https://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newco...
sql/error_delegate.h:44: static bool IsErrorCatastrophic(int error_type);
This probably belongs on sql::Connection, or perhaps just in sql:: namespace.

https://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newco...
sql/error_delegate.h:51: const UniqueT& uniquet_instance) {
I don't think uniquet_instance should be necessary.

Why is this in error_delegate.h rather than diagnostic_error_delegate.h?  I can
see how it's easier to reference from a subclass of ErrorDelegate, but since
it's static you can just do
DiagnosticErrorDelegate<Uniquifier>::LogAndRecord(...).  Or you could just
subclass DiagnosticErrorDelegate instead of ErrorDelegate.

https://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newco...
sql/error_delegate.h:55:
HistogramErrorRecorder<UniqueT>::RecordErrorInHistogram(error_type);
I think the HistogramErrorRecorder<> class is redundant.  Just inline that
function right here.  It will still be unique because this function will be
unique.

[pkotwicz, 2012-10-18 00:02:17]

Changes mostly as requested. I ended up leaving ErrorDelegate in
sql/connection.h

http://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:290:
LogAndRecordErrorInHistogram(error_type, connection, HistogramUniquifier());
I moved LogAndRecordErrorInHistogram into error_delegate_util.h. We could inline
the logging and histogram here but then I should expose
sql::GetMaxErrorCodeForHistogram()?

http://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:297:
attempted_to_kill_database_ = true;
I removed checking |backend_| as |backend_| should not ever be NULL anymore now
that error delegate is not ref counted anymore and I removed reset_backend().
Setting |backend_| to NULL would have the desired effect but I think it's kind
of hacky.

http://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h
File sql/error_delegate.h (right):

http://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newcode27
sql/error_delegate.h:27: // sqlite\preprocessed\sqlite3.h
On 2012/10/17 21:28:49, shess wrote:
> Use forward slashes.  Or even just "sqlite3.h" would be fine, since this path
is
> no longer right.

Done.

http://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newcode37
sql/error_delegate.h:37: // that you return the original |error| or the
appropiae error code.
On 2012/10/17 21:28:49, shess wrote:
> appropriate.

Done.

http://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newcode44
sql/error_delegate.h:44: static bool IsErrorCatastrophic(int error_type);
Moved to error_delegate_util.h Now the function's in the sql:: namespace.

[Scott Hess - ex-Googler, 2012-10-18 00:11:57]

lgtm

http://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:290:
LogAndRecordErrorInHistogram(error_type, connection, HistogramUniquifier());
On 2012/10/18 00:02:17, pkotwicz wrote:
> I moved LogAndRecordErrorInHistogram into error_delegate_util.h. We could
inline
> the logging and histogram here but then I should expose
> sql::GetMaxErrorCodeForHistogram()?

I think your current solution is pretty clean.

http://codereview.chromium.org/11141012/diff/5001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:297:
attempted_to_kill_database_ = true;
On 2012/10/18 00:02:17, pkotwicz wrote:
> I removed checking |backend_| as |backend_| should not ever be NULL anymore
now
> that error delegate is not ref counted anymore and I removed reset_backend().
> Setting |backend_| to NULL would have the desired effect but I think it's kind
> of hacky.

That's fair, backend_ isn't a flag.

http://codereview.chromium.org/11141012/diff/13002/sql/connection.h
File sql/connection.h (right):

http://codereview.chromium.org/11141012/diff/13002/sql/connection.h#newcode86
sql/connection.h:86: // |error| is an sqlite result code as seen in sqlite3.h
|connection| is the
Needs a comma after sqlite3.h - but now that this file is just cleanup, your
call as to whether you want to cleanup or just revert to what it was before.

http://codereview.chromium.org/11141012/diff/13002/sql/diagnostic_error_deleg...
File sql/diagnostic_error_delegate.h (right):

http://codereview.chromium.org/11141012/diff/13002/sql/diagnostic_error_deleg...
sql/diagnostic_error_delegate.h:28: LogAndRecordErrorInHistogram<UniqueT>(error,
connection);
I think this looks cleaner.

[erikwright (departed), 2012-10-18 00:13:29]

sqlite_persistent_cookie_store LGTM

http://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h
File sql/error_delegate.h (right):

http://codereview.chromium.org/11141012/diff/5001/sql/error_delegate.h#newcode51
sql/error_delegate.h:51: const UniqueT& uniquet_instance) {
Why an instance? Why not call this as

LogAndRecordErrorInHistogram<SomeType>(error, connection);

[commit-bot: I haz the power, 2012-10-18 01:07:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11141012/22011

[commit-bot: I haz the power, 2012-10-18 01:49:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-18 02:39:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11141012/30001

[commit-bot: I haz the power, 2012-10-18 04:31:54]

Change committed as 162647