Remove ref counting on sql::ErrorDelegate

Remove ref counting on sql::ErrorDelegate

BUG=151841
Test=None

R=shess
TBR=jamesr,erikwright

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

[pkotwicz, 2012-10-12 17:57:30]

Scott and Erik, can you please take a look

[Scott Hess - ex-Googler, 2012-10-12 20:04:51]

http://codereview.chromium.org/11111021/diff/3001/sql/sqlite_features_unittes...
File sql/sqlite_features_unittest.cc (right):

http://codereview.chromium.org/11111021/diff/3001/sql/sqlite_features_unittes...
sql/sqlite_features_unittest.cc:51: SQLiteFeaturesTest() : error_handler_(new
StatementErrorHandler) {}
I'm nervous about this, because it's an owning pointer until SetUp(), and then
ownership is passed to the connection.  The new should be moved to SetUp().

Even then, you have no way to assert that the connection actually kept the
handler around.  Better might be to refactor the handler so that the error_ and
sql_text_ items are in SQLiteFeaturesTest and the handler takes pointers to
those.

That said, I'd be satisfied with a TODO(shess): or something to clean that up. 
I'm thinking of converting the handler object to a callback, which would have
the same basic issue.

http://codereview.chromium.org/11111021/diff/3001/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

http://codereview.chromium.org/11111021/diff/3001/sql/statement_unittest.cc#n...
sql/statement_unittest.cc:46: SQLStatementTest() : error_handler_(new
StatementErrorHandler) {}
Same thing in this file as the other file.

[Scott Hess - ex-Googler, 2012-10-12 20:04:59]

One question I have is whether this will be failsafe in the case that anyone is
sharing a single handler between multiple connection instances?  I think it
would be, because you've removed the ref-counting so it can't happen.  But,
well, doesn't mean it can't.

[pkotwicz, 2012-10-12 22:24:57]

Changes as requested

[pkotwicz, 2012-10-12 22:26:30]

I do not think it is possible to share a single handler between multiple
connection instances any more given that the connection takes ownership of the
error handler.

[pkotwicz, 2012-10-16 21:04:17]

Ping!

[Scott Hess - ex-Googler, 2012-10-16 22:32:25]

lgtm, with noted changes.

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc...
sql/statement_unittest.cc:58: int sqlite_error() const { return error_; }
Either sqlite_error() or get_sqlite_error() both here and
sqlite_features_unittest.cc.

Also, seeing as the same classes are present in both places ... can this file be
shifted into an anonymous namespace?

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc...
sql/statement_unittest.cc:125: ResetError();
This shouldn't be necessary unless SQLStatementTest can be re-used between
tests.  But even if that were the case, the reset needs to happen in SetUp(). 
So I think you can just delete this line and the function it calls.

[pkotwicz, 2012-10-17 13:39:04]

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc...
sql/statement_unittest.cc:125: ResetError();
This is actually still necessary for EXPECT_EQ(SQLITE_OK, error_) in TearDown().

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

If we want to get rid of ResetError() in statement_unittest, I will fix this in
a followup patch. (http://codereview.chromium.org/11141012/ I believe)

[commit-bot: I haz the power, 2012-10-17 13:52:40]

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

[commit-bot: I haz the power, 2012-10-17 13:52:46]

Presubmit check for 11111021-21001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/database
    webkit/appcache
    chrome/browser/net

Presubmit checks took 1.4s to calculate.

[Scott Hess - ex-Googler, 2012-10-17 14:53:49]

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/11111021/diff/11003/sql/statement_unittest.cc...
sql/statement_unittest.cc:125: ResetError();
On 2012/10/17 13:39:04, pkotwicz wrote:
> This is actually still necessary for EXPECT_EQ(SQLITE_OK, error_) in
TearDown().

Doh - makes sense.  If all the sql:: calls are checking their return values it's
not really necessary, but I'm not sure it's worth making such changes for this
refactor.

[commit-bot: I haz the power, 2012-10-17 15:41:52]

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

[commit-bot: I haz the power, 2012-10-17 17:41:21]

Change committed as 162443