Database usage adjustment for .../webkit

Database usage adjustment for .../webkit

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


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

[Scott Hess - ex-Googler, 2012-01-18 21:24:03]

Overall, looks like the right direction.

Warning: I'm going to disappear sometime on Friday for almost three weeks.  I'll
try to turn around re-reviews expediently.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/appcache/appcach...
File webkit/appcache/appcache_database.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/appcache/appcach...
webkit/appcache/appcache_database.cc:310: if (!statement.Step() ||
!statement.Succeeded())
I think that statement.Succeeded() implies !statement.Step(), so this can be
reduced to just check statement.Step().  Please hit the many other locations in
this file while there.

[I agree that this isn't directly your problem :-).  But your changes imply
inspecting the flow-of-control to return values, so it's probably an opportune
time to do it.]

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/appcache/appcach...
webkit/appcache/appcache_database.cc:629: return statement.Run();
Aside: These are the cases which make me ponder revising the API to allow
something like:
  return db_->Prepare(kSql).BindInt64(cache_id).Run();

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/databas...
File webkit/database/databases_table.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/databas...
webkit/database/databases_table.cc:45: select_statement.BindString(1,
UTF16ToUTF8(database_name));
Also convert:
-  BindString(0, UTF16ToUTF8(origin_identifier));
+ BindString16(0, origin_identifier);
while you're changing those lines.  Likewise in the other spots you've changed,
but your call if there are additional spots in the file which you didn't change
but would benefit.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/databas...
webkit/database/databases_table.cc:109:
origins->push_back(UTF8ToUTF16(statement.ColumnString(0)));
ColumnString16(0), here.  And in the following code which calls UTF8ToUTF16().

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/quota_t...
File webkit/database/quota_table.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/quota_t...
webkit/database/quota_table.cc:27: statement.BindString(0,
UTF16ToUTF8(origin_identifier));
BindString16() again for this file.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/quota/quota_data...
File webkit/quota/quota_database.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/quota/quota_data...
webkit/quota/quota_database.cc:166: if (!statement.Step() ||
!statement.Succeeded())
Also just statement.Step() for this file.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/quota/quota_data...
webkit/quota/quota_database.cc:209:
statement.Assign(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
Ooh, this idiom gives me shivers.  But let it stand.

[Greg Billock, 2012-01-18 21:54:33]

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/appcache/appcach...
File webkit/appcache/appcache_database.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/appcache/appcach...
webkit/appcache/appcache_database.cc:310: if (!statement.Step() ||
!statement.Succeeded())
On 2012/01/18 21:24:03, shess wrote:
> I think that statement.Succeeded() implies !statement.Step(), so this can be
> reduced to just check statement.Step().  Please hit the many other locations
in
> this file while there.
> 
> [I agree that this isn't directly your problem :-).  But your changes imply
> inspecting the flow-of-control to return values, so it's probably an opportune
> time to do it.]

No, that's fair. One goal is to regularize usage so that it hopefully stays that
way when code gets copy-pasted down into new areas.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/databas...
File webkit/database/databases_table.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/databas...
webkit/database/databases_table.cc:45: select_statement.BindString(1,
UTF16ToUTF8(database_name));
Done. Also UTF8ToUTF16(ColumnString()) -> ColumnString16()

On 2012/01/18 21:24:03, shess wrote:
> Also convert:
> -  BindString(0, UTF16ToUTF8(origin_identifier));
> + BindString16(0, origin_identifier);
> while you're changing those lines.  Likewise in the other spots you've
changed,
> but your call if there are additional spots in the file which you didn't
change
> but would benefit.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/databas...
webkit/database/databases_table.cc:109:
origins->push_back(UTF8ToUTF16(statement.ColumnString(0)));
On 2012/01/18 21:24:03, shess wrote:
> ColumnString16(0), here.  And in the following code which calls UTF8ToUTF16().

Done.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/quota_t...
File webkit/database/quota_table.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/database/quota_t...
webkit/database/quota_table.cc:27: statement.BindString(0,
UTF16ToUTF8(origin_identifier));
On 2012/01/18 21:24:03, shess wrote:
> BindString16() again for this file.

Done.

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/quota/quota_data...
File webkit/quota/quota_database.cc (right):

https://chromiumcodereview.appspot.com/9249025/diff/1/webkit/quota/quota_data...
webkit/quota/quota_database.cc:166: if (!statement.Step() ||
!statement.Succeeded())
On 2012/01/18 21:24:03, shess wrote:
> Also just statement.Step() for this file.

Done.

[Scott Hess - ex-Googler, 2012-01-18 23:37:11]

LGTM!

[commit-bot: I haz the power, 2012-01-31 18:21:30]

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

[commit-bot: I haz the power, 2012-01-31 18:21:34]

Presubmit check for 9249025-11001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/database/databases_table.cc,webkit/appcache/appcache_database.h,webkit/appcache/appcache_database.cc,webkit/database/database_tracker.cc,webkit/quota/quota_database.cc,webkit/database/quota_table.cc

Presubmit checks took 1.2s to calculate.

[Greg Billock, 2012-01-31 19:07:56]

Can I get an owners lgtm, Darin?

On 2012/01/31 18:21:34, I haz the power (commit-bot) wrote:
> Presubmit check for 9249025-11001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
webkit/database/databases_table.cc,webkit/appcache/appcache_database.h,webkit/appcache/appcache_database.cc,webkit/database/database_tracker.cc,webkit/quota/quota_database.cc,webkit/database/quota_table.cc
> 
> Presubmit checks took 1.2s to calculate.

[michaeln, 2012-02-01 19:07:13]

webkit/* lgtm

[commit-bot: I haz the power, 2012-02-02 23:30:27]

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

[commit-bot: I haz the power, 2012-02-03 01:50:13]

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

[commit-bot: I haz the power, 2012-02-03 05:20:11]

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

[commit-bot: I haz the power, 2012-02-03 07:15:17]

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

[commit-bot: I haz the power, 2012-02-07 00:47:43]

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

[commit-bot: I haz the power, 2012-02-07 04:28:12]

Change committed as 120709