More SQL statement usage regularization. Back-touch some

More SQL statement usage regularization. Back-touch some
already-edited files to standardize usage.

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


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

[wtc, 2012-02-10 18:30:53]

Drive-by review comments on Patch Set 1:

I only review the changes in chrome/browser/net.  They
look good except for the issue with log messages below.

mattm: could you take a quick look at
chrome/browser/net/sqlite_origin_bound_cert_store.cc?

https://chromiumcodereview.appspot.com/9365030/diff/1/chrome/browser/net/sqli...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/1/chrome/browser/net/sqli...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:263: << "version 3.";

All the warning log messages should in this function should
say the same thing:
  LOG(WARNING) << "Unable to update origin bound cert database to "
               << "version 4.";

This also applies to the warning message on line 278 below.

[Greg Billock, 2012-02-10 23:54:54]

https://chromiumcodereview.appspot.com/9365030/diff/1/chrome/browser/net/sqli...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/1/chrome/browser/net/sqli...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:263: << "version 3.";
On 2012/02/10 18:30:53, wtc wrote:
> 
> All the warning log messages should in this function should
> say the same thing:
>   LOG(WARNING) << "Unable to update origin bound cert database to "
>                << "version 4.";
> 
> This also applies to the warning message on line 278 below.

Done.

[Scott Hess - ex-Googler, 2012-02-13 22:04:03]

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/net/s...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/net/s...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:222: << "version 4.";
Nope, this stanza is upgrading to version 2, not version 4.

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/net/s...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:261: if (!smt.is_valid()) {
I think this would be reasonable to leave as !stmt.is_valid() ||
!other_stmt.is_valid(), etc.  No real gain to short-circuiting.

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/passw...
File chrome/browser/password_manager/login_database.cc (left):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/passw...
chrome/browser/password_manager/login_database.cc:141: if (!s) {
I'd leave the test, or guard the final histogram with s.Succeeded().

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/safe_...
File chrome/browser/safe_browsing/filter_false_positive_perftest.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/safe_...
chrome/browser/safe_browsing/filter_false_positive_perftest.cc:135:
sql::Statement count_statement(db.GetCachedStatement(SQL_FROM_HERE, query));
Lovely.  LGTM, but feel free to drop this particular file, as this format is no
longer supported.  I'll work on removing the code entirely. 
http://crbug.com/114086

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/data...
File webkit/database/databases_table.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/data...
webkit/database/databases_table.cc:83: 
Egregious whitespace changes are egregious.  I'd remove this file unless you
originated the formatting you're changing.

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/quot...
File webkit/database/quota_table.cc (left):

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/quot...
webkit/database/quota_table.cc:28: 
Likewise with these whitespace changes.

[Scott Hess - ex-Googler, 2012-02-13 22:07:15]

https://chromiumcodereview.appspot.com/9365030/diff/1/chrome/browser/net/sqli...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/1/chrome/browser/net/sqli...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:263: << "version 3.";
On 2012/02/10 23:54:54, Greg Billock wrote:
> On 2012/02/10 18:30:53, wtc wrote:
> > 
> > All the warning log messages should in this function should
> > say the same thing:
> >   LOG(WARNING) << "Unable to update origin bound cert database to "
> >                << "version 4.";
> > 
> > This also applies to the warning message on line 278 below.
> 
> Done.

Hmm, just saw this WRT my review.  Note that _this_ code is in the if
(cur_version <= 3) case, whereas the above "version 2" change which I referenced
was in the (cur_version == 1) case.  The log line should refer to the version
the database will be in after the local code in the if() statement runs, not the
version the overall function will leave things.  So if a new version is added,
the old update code will be left alone.

[Greg Billock, 2012-02-13 22:48:59]

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/net/s...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/net/s...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:222: << "version 4.";
On 2012/02/13 22:04:04, shess wrote:
> Nope, this stanza is upgrading to version 2, not version 4.

I think you're right. I was reacting to wtc's comment that they should all be
the same, but I think that was premature.

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/net/s...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:261: if (!smt.is_valid()) {
On 2012/02/13 22:04:04, shess wrote:
> I think this would be reasonable to leave as !stmt.is_valid() ||
> !other_stmt.is_valid(), etc.  No real gain to short-circuiting.

Done.

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/passw...
File chrome/browser/password_manager/login_database.cc (left):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/passw...
chrome/browser/password_manager/login_database.cc:141: if (!s) {
On 2012/02/13 22:04:04, shess wrote:
> I'd leave the test, or guard the final histogram with s.Succeeded().

Sounds good. I was thinking in terms of return values, but here the side effects
are UMA counts.

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/safe_...
File chrome/browser/safe_browsing/filter_false_positive_perftest.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/safe_...
chrome/browser/safe_browsing/filter_false_positive_perftest.cc:135:
sql::Statement count_statement(db.GetCachedStatement(SQL_FROM_HERE, query));
On 2012/02/13 22:04:04, shess wrote:
> Lovely.  LGTM, but feel free to drop this particular file, as this format is
no
> longer supported.  I'll work on removing the code entirely. 
> http://crbug.com/114086

If it's dead, I'll leave it to eliminate pressure on the operator! of Statement,
so it'll decouple 114086 from this whole refactoring.

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/data...
File webkit/database/databases_table.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/data...
webkit/database/databases_table.cc:83: 
On 2012/02/13 22:04:04, shess wrote:
> Egregious whitespace changes are egregious.  I'd remove this file unless you
> originated the formatting you're changing.

I've been using this formatting everywhere else. I just missed this spot. Part
of my intent is to make all the usages of the API look stylistically very
uniform. Part of that is emphasizing the "paragraph" of creating the statement
and binding to it. That'll be more concrete once we have the bind order
enforcement, too.

[Scott Hess - ex-Googler, 2012-02-13 22:52:58]

LGTM, except the afore-mentioned LOG-version-change.

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/safe_...
File chrome/browser/safe_browsing/filter_false_positive_perftest.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/chrome/browser/safe_...
chrome/browser/safe_browsing/filter_false_positive_perftest.cc:135:
sql::Statement count_statement(db.GetCachedStatement(SQL_FROM_HERE, query));
On 2012/02/13 22:48:59, Greg Billock wrote:
> On 2012/02/13 22:04:04, shess wrote:
> > Lovely.  LGTM, but feel free to drop this particular file, as this format is
> no
> > longer supported.  I'll work on removing the code entirely. 
> > http://crbug.com/114086
> 
> If it's dead, I'll leave it to eliminate pressure on the operator! of
Statement,
> so it'll decouple 114086 from this whole refactoring.

OK.

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/data...
File webkit/database/databases_table.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/7001/webkit/database/data...
webkit/database/databases_table.cc:83: 
On 2012/02/13 22:48:59, Greg Billock wrote:
> On 2012/02/13 22:04:04, shess wrote:
> > Egregious whitespace changes are egregious.  I'd remove this file unless you
> > originated the formatting you're changing.
> 
> I've been using this formatting everywhere else. I just missed this spot. Part
> of my intent is to make all the usages of the API look stylistically very
> uniform. Part of that is emphasizing the "paragraph" of creating the statement
> and binding to it. That'll be more concrete once we have the bind order
> enforcement, too.

OK.  I do kind of think that's tilting against windmills, though...

[Greg Billock, 2012-02-13 23:45:09]

On 2012/02/13 22:52:58, shess wrote:
> LGTM, except the afore-mentioned LOG-version-change.

Which one was that? I think I got them all in sqlite_origin_bound_cert_store.

[Scott Hess - ex-Googler, 2012-02-14 00:28:18]

LGTM, with these changes.  Else we can argue it :-).

I think the v3/v4 confusion is really down to that upgrade having been combined
when it probably shouldn't have.  My preference is generally to treat upgrade
code like historical version-control data - you never fix it in-place, you only
add things to the end.  But that ship has sailed in this case, so I'd just aim
for keeping changes minimal.

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:222: << "version 2.";
This was the one I was mentioning.  Now it's right again!

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:247: << "version 3.";
But now this is back to wtc's earlier comment that it should be version 4
(because the if() which gets to here upgrades to version 4).

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:290: << "version 3.";
And this one to version 4.

[Greg Billock, 2012-02-14 01:13:44]

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:247: << "version 3.";
My head hurts on this -- it looks like we're upgrading from 2->3, and then the
ALTER TABLE below from 3->4. I don't really understand the archaeology, though,
so I'm happy to put it to what it was.

On 2012/02/14 00:28:18, shess wrote:
> But now this is back to wtc's earlier comment that it should be version 4
> (because the if() which gets to here upgrades to version 4).

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:290: << "version 3.";
On 2012/02/14 00:28:18, shess wrote:
> And this one to version 4.

Done.

[Scott Hess - ex-Googler, 2012-02-14 01:28:36]

lgtm

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
File chrome/browser/net/sqlite_origin_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/9365030/diff/10004/chrome/browser/net/...
chrome/browser/net/sqlite_origin_bound_cert_store.cc:247: << "version 3.";
My theory is that at some point someone added the 3-to-4 upgrade by modifying
the 2-to-3 upgrade, so now it's (2-or-3)-to-4.  But I think the relevant version
is the version things get set to on commit. (4).

On 2012/02/14 01:13:44, Greg Billock wrote:
> My head hurts on this -- it looks like we're upgrading from 2->3, and then the
> ALTER TABLE below from 3->4. I don't really understand the archaeology,
though,
> so I'm happy to put it to what it was.
> 
> On 2012/02/14 00:28:18, shess wrote:
> > But now this is back to wtc's earlier comment that it should be version 4
> > (because the if() which gets to here upgrades to version 4).

[commit-bot: I haz the power, 2012-02-16 18:43:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9365030/16001

[commit-bot: I haz the power, 2012-02-16 18:43:49]

Can't apply patch for file chrome/browser/net/sqlite_origin_bound_cert_store.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/net/sqlite_origin_bound_cert_store.cc
Hunk #3 FAILED at 384.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/net/sqlite_origin_bound_cert_store.cc.rej

[commit-bot: I haz the power, 2012-02-16 19:37:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9365030/20001

[commit-bot: I haz the power, 2012-02-16 19:37:34]

Presubmit check for 9365030-20001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/history/visit_database.cc,chrome/browser/history/download_database.cc,webkit/database/databases_table.cc,chrome/browser/net/sqlite_persistent_cookie_store.cc,chrome/browser/history/visitsegment_database.cc,chrome/browser/net/sqlite_origin_bound_cert_store.cc,webkit/database/quota_table.cc

Presubmit checks took 1.7s to calculate.

[wtc, 2012-02-16 20:00:40]

Patch Set 5 LGTM.  I only reviewed chrome/browser/net/*.

[Scott Hess - ex-Googler, 2012-02-16 20:18:29]

Greg, would you be willing to hold off for a few hours and accept another merge
conflict?  I have a slight change in sqlite_persistent_cookie_store.cc which is
very likely to conflict with your change, but if it bakes well on canary I'm
going to nominate it for merging up the stack.

[I'm just trying to validate that it fixes the crash for realz and doesn't just
crash things three calls later.]

[Greg Billock, 2012-02-16 20:58:51]

sure

On 2012/02/16 20:18:29, shess wrote:
> Greg, would you be willing to hold off for a few hours and accept another
merge
> conflict?  I have a slight change in sqlite_persistent_cookie_store.cc which
is
> very likely to conflict with your change, but if it bakes well on canary I'm
> going to nominate it for merging up the stack.
> 
> [I'm just trying to validate that it fixes the crash for realz and doesn't
just
> crash things three calls later.]

[Scott Hess - ex-Googler, 2012-02-17 17:48:17]

BTW, in case you didn't notice, my other change is in.  You might see a
slight-but-obvious merge conflict, but it shouldn't affect earlier lgtm.

On 2012/02/16 20:58:51, Greg Billock wrote:
> sure
> 
> On 2012/02/16 20:18:29, shess wrote:
> > Greg, would you be willing to hold off for a few hours and accept another
> merge
> > conflict?  I have a slight change in sqlite_persistent_cookie_store.cc which
> is
> > very likely to conflict with your change, but if it bakes well on canary I'm
> > going to nominate it for merging up the stack.
> > 
> > [I'm just trying to validate that it fixes the crash for realz and doesn't
> just
> > crash things three calls later.]

[commit-bot: I haz the power, 2012-02-17 20:46:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9365030/26001

[commit-bot: I haz the power, 2012-02-17 20:46:19]

Presubmit check for 9365030-26001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/database/quota_table.cc,webkit/database/databases_table.cc,chrome/browser/history/visitsegment_database.cc,chrome/browser/history/visit_database.cc,chrome/browser/history/download_database.cc

Presubmit checks took 3.1s to calculate.

[Greg Billock, 2012-02-17 20:49:08]

Need lgtm for webkit/database from owners.

On 2012/02/17 20:46:19, I haz the power (commit-bot) wrote:
> Presubmit check for 9365030-26001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
webkit/database/quota_table.cc,webkit/database/databases_table.cc,chrome/browser/history/visitsegment_database.cc,chrome/browser/history/visit_database.cc,chrome/browser/history/download_database.cc
> 
> Presubmit checks took 3.1s to calculate.

[michaeln, 2012-02-17 21:03:26]

webkit/database lgtm

[commit-bot: I haz the power, 2012-02-17 21:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9365030/26001

[commit-bot: I haz the power, 2012-02-17 21:19:47]

Presubmit check for 9365030-26001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/history/visit_database.cc,chrome/browser/history/visitsegment_database.cc,chrome/browser/history/download_database.cc

Presubmit checks took 2.0s to calculate.

[Greg Billock, 2012-02-17 22:28:22]

Need owners lgtm for chrome/browser/history

On 2012/02/17 21:19:47, I haz the power (commit-bot) wrote:
> Presubmit check for 9365030-26001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
chrome/browser/history/visit_database.cc,chrome/browser/history/visitsegment_database.cc,chrome/browser/history/download_database.cc
> 
> Presubmit checks took 2.0s to calculate.

[sky, 2012-02-17 23:00:29]

LGTM

[commit-bot: I haz the power, 2012-02-17 23:34:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9365030/26001

[commit-bot: I haz the power, 2012-02-18 01:38:14]

Try job failure for 9365030-26001 (retry) on win_rel for steps "check_deps,
browser_tests".
It's a second try, previously, steps "check_deps, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-18 01:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/9365030/26001

[commit-bot: I haz the power, 2012-02-18 08:26:12]

Change committed as 122692