Remove last client of sql::DiagnosticErrorDelegate.

Remove last client of sql::DiagnosticErrorDelegate.

In the first pass, there was a problem with histograms.  I do not
recall what the problem was, so removing things.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201895

[Scott Hess - ex-Googler, 2013-05-21 23:31:09]

This case of the ErrorDelegate didn't get converted to the new tagged system
because of a unit-testing issue.  I'm planning to shift everything to use
callbacks, so I figure this one might as well go first.

I do have some CLs which manage to let tests hook error-handling, but I'm still
waiting to see if something prettier comes to mind.

[erikwright (departed), 2013-05-22 00:02:32]

I'm really confused about set_error_callback. It says it allows clients to
modify the error code, but the Callback signature doesn't seem to allow that.

And on the other hand, it seems like there is some additional side-effect
because just setting a no-op callback (which, on the surface and according to
connection.h, would have no effect) appears to prevent the error from being
reported somehow.

So I feel like there's some missing documentation or something else out of
alignment here.

[Scott Hess - ex-Googler, 2013-05-22 00:28:01]

On 2013/05/22 00:02:32, erikwright wrote:
> I'm really confused about set_error_callback. It says it allows clients to
> modify the error code, but the Callback signature doesn't seem to allow that.

Wait, what?  I thought I indicated it differed from ErrorDelegate by not
allowing the error code to be modified.  I'll go clean that up.  [Tomorrow,
looks like, my machine here looks like it's badly out of date.]

Hmm ... I guess I'm not sure what you're referring to.  It looks like it isn't
documented to return an error code.

> And on the other hand, it seems like there is some additional side-effect
> because just setting a no-op callback (which, on the surface and according to
> connection.h, would have no effect) appears to prevent the error from being
> reported somehow.

At some point, the FATAL-crash thing in connection.c which is supposed to
prevent new errors from being introduced was moved _after_ the ErrorDelegate. 
This meant that if you set an ErrorDelegate and then just ignored errors (which
was the default impl in most places), nobody would ever notice if you did
something which caused an error.  This was the case where someone was depending
on that.

> So I feel like there's some missing documentation or something else out of
> alignment here.

The correct thing to have would be something like sql::ScopedErrorAllower, where
a test could say "I am writing code which will handle SQLITE_CORRUPT and
SQLITE_READONLY, so make those non-fatal in this scope".  Then this code
wouldn't need a callback to allow tests to work, the tests could make those
assertions directly.  But we aren't there yet.

This is just a more-straightforward replacement for the previous mess of
template function/class.

[erikwright (departed), 2013-05-22 01:17:26]

I re-read the comment in connection.h.

It's quite long and has several line breaks, so I mis-interpreted the TODO at
the end as being the whole comment.

Also, the comment about "ErrorDelegate allowed..." is meant to be in contrast to
the method in question but I mis-read it.

LGTM. I gather you are already gradually cleaning this up so my suggestion that
you just benevolently dictate what the API should be and remove fence-sitting
comments will probably be preaching to the choir.

[erikwright (departed), 2013-05-22 01:17:27]

I re-read the comment in connection.h.

It's quite long and has several line breaks, so I mis-interpreted the TODO at
the end as being the whole comment.

Also, the comment about "ErrorDelegate allowed..." is meant to be in contrast to
the method in question but I mis-read it.

LGTM. I gather you are already gradually cleaning this up so my suggestion that
you just benevolently dictate what the API should be and remove fence-sitting
comments will probably be preaching to the choir.

[Scott Hess - ex-Googler, 2013-05-22 17:41:59]

On 2013/05/22 01:17:27, erikwright wrote:
> I re-read the comment in connection.h.
> 
> It's quite long and has several line breaks, so I mis-interpreted the TODO at
> the end as being the whole comment.
> 
> Also, the comment about "ErrorDelegate allowed..." is meant to be in contrast
to
> the method in question but I mis-read it.
> 
> LGTM. I gather you are already gradually cleaning this up so my suggestion
that
> you just benevolently dictate what the API should be and remove fence-sitting
> comments will probably be preaching to the choir.

Yeah, I probably gave the existing implementation too much benefit of the doubt.
 Which is silly, since the only person who will ever read the TODO is me.  I'll
put in some time today and see if I can just knock off the rest of the cases
quickly.

[commit-bot: I haz the power, 2013-05-22 17:42:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/15104003/6001

[commit-bot: I haz the power, 2013-05-22 17:53:01]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Scott Hess - ex-Googler, 2013-05-22 17:55:19]

Oops, marja for webkit/dom_storage/OWNERS

[marja, 2013-05-23 05:32:29]

lgtm; OWNER rubberstamp since webkit/dom_storage/dom_storage_database.cc is
adapting to API changes.

https://chromiumcodereview.appspot.com/15104003/diff/6001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_database.cc (right):

https://chromiumcodereview.appspot.com/15104003/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_database.cc:165:
db_->set_histogram_tag("DomStorageDatabase");
Will we get the same error histogram information via this than what we used to
get with the GetErrorHandlerForDomStorageDatabase? (I don't know what it does,
but looks like it gathers some histograms about... error counts??)

[Scott Hess - ex-Googler, 2013-05-23 05:45:37]

thanks.

https://chromiumcodereview.appspot.com/15104003/diff/6001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_database.cc (right):

https://chromiumcodereview.appspot.com/15104003/diff/6001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_database.cc:165:
db_->set_histogram_tag("DomStorageDatabase");
On 2013/05/23 05:32:29, marja wrote:
> Will we get the same error histogram information via this than what we used to
> get with the GetErrorHandlerForDomStorageDatabase? (I don't know what it does,
> but looks like it gathers some histograms about... error counts??)

Old School: Complicated function template calling class template, blah blah
blah.  Very cargo-cult.

New School: Tell sql::Connection a tag to identify you, it takes care of things.

Recently I have changed the histograms from Sqlite.<tag>.Error to
Sqlite.Error.<tag>, but in return they include extended error codes, and it's
now also tracking Sqlite.Version.<tag> (if you use MetaTable versioning) and
Sqlite.SizeKB.<tag> (size at startup), and I'm hopeful other ideas for
across-the-board additions will come to mind.

[commit-bot: I haz the power, 2013-05-23 18:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/15104003/6001

[commit-bot: I haz the power, 2013-05-23 21:45:56]

Change committed as 201895