Misc. cleanup found while mucking with search engines code:

Misc. cleanup found while mucking with search engines code:

* Don't handle DCHECK failure
* Remove NOTREACHED()/LOG(ERROR) from cases that look like they could
  legitimately happen
* All lines of args should begin at the same position
* WebDataService::SetDefaultSearchProvider() doesn't actually need a full
  TemplateURL, just an ID
* Use GetCachedStatement correctly, and in more places
* Make KeywordTableTest a friend of KeywordTable to reduce FRIEND_TEST
  declarations and in preparation for making Add/Update/RemoveKeyword private
* Data members should be private, not protected
* Function declarations: all args on first line or one arg per line
* Fix misspelling

BUG=none
TEST=none
R=shess@chromium.org

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

[Scott Hess - ex-Googler, 2014-03-31 18:09:07]

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
File chrome/browser/webdata/keyword_table.cc (right):

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
chrome/browser/webdata/keyword_table.cc:249: sql::Statement
s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
I hate constructed queries with a vengence.  That said, AFAICT,
GetKeywordColumns() will be static for a given run of Chrome, so I think this
works.

In changing these from unique to cached, do you actually expect them to be
called frequently enough that retaining the prepared statement will win
anything?  These look like pretty simple statements.  I'd say if they aren't
expected to be called a few times every time Chromium is executed, they might
not be worthwhile to cache.

https://codereview.chromium.org/217613002/diff/1/sql/transaction.cc
File sql/transaction.cc (right):

https://codereview.chromium.org/217613002/diff/1/sql/transaction.cc#newcode23
sql/transaction.cc:23: DCHECK(!is_open_) << "Beginning a transaction twice!";
These changes may cause actual incorrect changes in production which might be
wrong.  For instance, previously Begin() wouldn't stack another transaction in
the connection, now it will, and previously it could fail where now it would
succeed so the caller may take further actions based on that.  Rollback() and
Commit() previously wouldn't forward an incorrect request to the connection, now
they will.  Due to how these things interact implicitly in nested fashion, the
affected parties may not actually realize there's another transaction around
that could break them.  Since the destructor only calls rollback if a
transaction is open, a double-Begin() would not result in everything being
unwound unless the caller is making all the correct explicit unwinding calls
(though it probably goes without saying that the caller is doing something wrong
at this point).  I'm not sure it's a good idea to make this change.

On the other hand, I cannot think of anyplace where I've seen Transaction being
used in a way which would hit any of these issues.  My argument is more that the
Transaction is not constructed in a way which avoids all of these issues.

[Peter Kasting, 2014-03-31 19:53:56]

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
File chrome/browser/webdata/keyword_table.cc (right):

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
chrome/browser/webdata/keyword_table.cc:249: sql::Statement
s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
On 2014/03/31 18:09:08, shess wrote:
> I hate constructed queries with a vengence.  That said, AFAICT,
> GetKeywordColumns() will be static for a given run of Chrome, so I think this
> works.
> 
> In changing these from unique to cached, do you actually expect them to be
> called frequently enough that retaining the prepared statement will win
> anything?  These look like pretty simple statements.  I'd say if they aren't
> expected to be called a few times every time Chromium is executed, they might
> not be worthwhile to cache.

They may not be executed multiple times on every Chrome invocation, but when
they _are_ executed, they may be executed dozens or even hundreds of times. 
This is the case for e.g. syncing an existing set of search engines to a new
profile/installation.

https://codereview.chromium.org/217613002/diff/1/sql/transaction.cc
File sql/transaction.cc (right):

https://codereview.chromium.org/217613002/diff/1/sql/transaction.cc#newcode23
sql/transaction.cc:23: DCHECK(!is_open_) << "Beginning a transaction twice!";
On 2014/03/31 18:09:08, shess wrote:
> These changes may cause actual incorrect changes in production which might be
> wrong.  For instance, previously Begin() wouldn't stack another transaction in
> the connection, now it will, and previously it could fail where now it would
> succeed so the caller may take further actions based on that.  Rollback() and
> Commit() previously wouldn't forward an incorrect request to the connection,
now
> they will.  Due to how these things interact implicitly in nested fashion, the
> affected parties may not actually realize there's another transaction around
> that could break them.  Since the destructor only calls rollback if a
> transaction is open, a double-Begin() would not result in everything being
> unwound unless the caller is making all the correct explicit unwinding calls
> (though it probably goes without saying that the caller is doing something
wrong
> at this point).  I'm not sure it's a good idea to make this change.

We have to make _some_ change, because the old code is wrong -- it handles
DCHECK failure, which is banned.  The old code claims that our code never
misuses Transactions in these ways, meaning that it's guaranteed safe to make
the change I'm making.  If that guarantee is violated, we already have bugs in
our code.  I'd rather not bandaid them.

Because failing these checks is purely a matter of actual coding errors
(misusing transaction semantics), it seems reasonable to demand that people use
transactions properly.  (By contrast, we couldn't do this with e.g. the return
values of attempts to execute SQL statements, since error cases like disk
corruption could always mean we get a real-world failure.)

> On the other hand, I cannot think of anyplace where I've seen Transaction
being
> used in a way which would hit any of these issues.  My argument is more that
the
> Transaction is not constructed in a way which avoids all of these issues.

All classes have two choices: they can be defensive and ensure they handle all
possible misuse, or they can pass that responsibility to their callers, and
assert that they are used properly.  Normally, the second seems preferable,
since it is generally both simpler and more conducive to all callers actually
being correct (we won't have people writing grossly wrong code and having the
callee "handle it", then having that code get copied elsewhere).

All this is a long way of saying that, while I'm willing to instead remove the
NOTREACHED()s and leave the return statements, I think the change I'm making
here is significantly better.

And I've never seen code that would hit this, either.

[Scott Hess - ex-Googler, 2014-03-31 23:52:08]

OK, while I don't fully agree with you on the desirability of the Transaction
change, I think that the issues I have are more due to it being a broken API
than due to your change (it shouldn't overload "transaction is open" with "API
is being used correctly" in the first place).  I'll think on whether there are
independent options to make that better.  

LGTM.

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
File chrome/browser/webdata/keyword_table.cc (right):

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
chrome/browser/webdata/keyword_table.cc:249: sql::Statement
s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
On 2014/03/31 19:53:56, Peter Kasting wrote:
> On 2014/03/31 18:09:08, shess wrote:
> > I hate constructed queries with a vengence.  That said, AFAICT,
> > GetKeywordColumns() will be static for a given run of Chrome, so I think
this
> > works.
> > 
> > In changing these from unique to cached, do you actually expect them to be
> > called frequently enough that retaining the prepared statement will win
> > anything?  These look like pretty simple statements.  I'd say if they aren't
> > expected to be called a few times every time Chromium is executed, they
might
> > not be worthwhile to cache.
> 
> They may not be executed multiple times on every Chrome invocation, but when
> they _are_ executed, they may be executed dozens or even hundreds of times. 
> This is the case for e.g. syncing an existing set of search engines to a new
> profile/installation.

OK.  I sometimes/often feel like we need a more adaptive statement cache which
actually works like a _cache_.

[Peter Kasting, 2014-04-01 00:18:55]

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
File chrome/browser/webdata/keyword_table.cc (right):

https://codereview.chromium.org/217613002/diff/1/chrome/browser/webdata/keywo...
chrome/browser/webdata/keyword_table.cc:249: sql::Statement
s(db_->GetCachedStatement(SQL_FROM_HERE, query.c_str()));
On 2014/03/31 23:52:08, shess wrote:
> On 2014/03/31 19:53:56, Peter Kasting wrote:
> > On 2014/03/31 18:09:08, shess wrote:
> > > I hate constructed queries with a vengence.  That said, AFAICT,
> > > GetKeywordColumns() will be static for a given run of Chrome, so I think
> this
> > > works.
> > > 
> > > In changing these from unique to cached, do you actually expect them to be
> > > called frequently enough that retaining the prepared statement will win
> > > anything?  These look like pretty simple statements.  I'd say if they
aren't
> > > expected to be called a few times every time Chromium is executed, they
> might
> > > not be worthwhile to cache.
> > 
> > They may not be executed multiple times on every Chrome invocation, but when
> > they _are_ executed, they may be executed dozens or even hundreds of times. 
> > This is the case for e.g. syncing an existing set of search engines to a new
> > profile/installation.
> 
> OK.  I sometimes/often feel like we need a more adaptive statement cache which
> actually works like a _cache_.

Yeah, it would be nice if there was a simple LRU cache for each table, or
something like that, and the caller didn't have to make any distinction between
statements, it was all just done automatically.

[Peter Kasting, 2014-04-01 20:18:19]

Committed patchset #1 manually as r260933 (presubmit successful).