Database usage adjustment for .../history

Database usage adjustment for .../history

R=shess@chromium.org
BUG=None
TEST=.../history tests


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

[Scott Hess - ex-Googler, 2012-01-03 23:41:57]

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
chrome/browser/history/download_database.cc:235: // TODO(rdsmith): Remove when
http://crbug.com/96627 is resolved.
The target of this TODO seems ambiguous, now.  Maybe should loop in rdsmith.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_database.cc:193: DCHECK(db_.GetLastChangeCount()
== 1);
I'm having troubles seeing how this line could possibly be making the assertions
it seems to think it is asserting.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_database.cc:207: return s.ColumnInt64(0);
This line would give 0 if the column type is NULL.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/in_memor...
File chrome/browser/history/in_memory_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_database.cc:78: NOTREACHED() <<
GetDB().GetErrorMessage();
If there was an error, shouldn't it already have been logged and NOTREACHED?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:269: return false;
Is this likely enough to happen to be worth bothering with?  Logically it seems
like statement should be under if(encoded) anyhow.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:281:
static_cast<int>(jpeg_data.size()));
Sigh, the parameter should have been a size_t in the first place...

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:330: DCHECK(score);
If you're going to add this, push it above the if() so that you're testing the
API's static usage, which presumably shouldn't change based on use_top_sites_.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:367: "UPDATE favicons SET
image_data=NULL, last_updated=? WHERE id=?"));
This would probably be cleaner with a single statement using BindNull(0). 
Please put a TODO(shess): to that end, if you don't care to go there.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:540: return false;
Is this still necessary?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:648: DLOG(FATAL) <<
db_.GetErrorMessage();
I can't tell if the Execute() cases would already be fatal, but wouldn't this
and the above already be fatal?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
File chrome/browser/history/top_sites_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:201: if (!statement.is_valid())
Is this still necessary?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:218: NOTREACHED() <<
db_->GetErrorMessage();
This line should be redundant?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:233: return;
Still necessary?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:250: NOTREACHED() <<
db_->GetErrorMessage();
The error message is redundant?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:332: return
select_statement.ColumnInt(0);
AFAICT, having this return anything other than a single row with a single
integer would be an error, as COUNT(...) can't return NULL or an empty result.

OTOH, I guess it's more consistent this way.  OK.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
File chrome/browser/history/url_database.cc (left):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
chrome/browser/history/url_database.cc:108: return false;
This can now return true in case of empty results.  Is that appropriate?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
chrome/browser/history/url_database.cc:257: }
I'm not entirely convinced that removing these is appropriate (well, the error
message is, but the return value maybe not).  But I need to move on for the day,
so I'll need to circle back to the rest of this file.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
chrome/browser/history/visit_database.cc:164: update_chain.Run();
I don't know if the previous short-circuit behavior was essential or not, but it
does seem like you should test Run() to maintain consistency.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
chrome/browser/history/visit_database.cc:367: return false;
Is this here because you don't trust FillVisitVector() to chain right, or
because it's more self-documenting?  The earlier cases seem to trust it.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
chrome/browser/history/visit_database.cc:459: if (!statement.Step()) {
It seems like this statement and the is_valid() above really are trying to
distinguish empty results from errors.  Maybe it would be better to explicitly
use Succeeded() in here.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
File chrome/browser/history/visitsegment_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:180: return false;
Ick.  It seems unlikely that |false| is the right return value in this case.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:201: return false;
Can be removed.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:212: return false;
Can be removed.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:344: return false;
Would you be willing to rewrite this to a pair of simpler statements?  I mean
like:

DELETE FROM segment_usage WHERE segment_id IN (SELECT id FROM segments WHERE
url_id = ?);
  and
DELETE FROM segments WHERE url_id = ?;

Performance-wise, they should be identical, but brittle-code-wise the two
simpler statements should be full of win.  Otherwise maybe just drop a
TODO(shess) in here?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:234: LOG(ERROR) << "No default search
provider with backup id.";
Drop the is_valid(), and use LOG_IF(s.Succeeded(), ERROR) to distinguish the
case there there was no error but the results were empty.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:245: LOG(ERROR) << "Statement has not
succeeded.";
Actually, I don't understand what this statement intends.  s.Step() being true
implies that s.Succeeded() should be true, doesn't it?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:413: if (!GetTableContents("keywords",
&keywords))
Looks like failing this one would have caused many layers of log spew :-).

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:420: if (signature.empty())
Is the NOTREACHED() in this case WRT sql/ API, or some other API?

[Scott Hess - ex-Googler, 2012-01-04 00:06:39]

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
chrome/browser/history/download_database.h:61: bool
RemoveDownloadsBetween(base::Time remove_begin, base::Time remove_end);
BTW: Where you've converted void->bool, consider WARN_UNUSED_RESULT to make
checking the return value part of the contract.  I'd like to use that in more of
the persistence code, but for most cases it involves tracking callers down and
figuring out how to deal with things, whereas in this case you're already
on-site...

[Greg Billock, 2012-01-04 19:20:19]

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
chrome/browser/history/download_database.cc:235: // TODO(rdsmith): Remove when
http://crbug.com/96627 is resolved.
On 2012/01/03 23:41:57, shess wrote:
> The target of this TODO seems ambiguous, now.  Maybe should loop in rdsmith.

I think I misplaced it. Should be by the erase I think.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
chrome/browser/history/download_database.h:61: bool
RemoveDownloadsBetween(base::Time remove_begin, base::Time remove_end);
Yeah. This particular case is used only in a ScheduleAndForget call.

On 2012/01/04 00:06:40, shess wrote:
> BTW: Where you've converted void->bool, consider WARN_UNUSED_RESULT to make
> checking the return value part of the contract.  I'd like to use that in more
of
> the persistence code, but for most cases it involves tracking callers down and
> figuring out how to deal with things, whereas in this case you're already
> on-site...

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_database.cc:193: DCHECK(db_.GetLastChangeCount()
== 1);
I read it as saying "ASSERT(number of rows changed by the previous UPDATE
statement was exactly 1)"

Meaning that the caller must ensure that visit_id is present and unique in the
visits table.

Of course, by that time it's too late and the DB is corrupted... :-/

On 2012/01/03 23:41:57, shess wrote:
> I'm having troubles seeing how this line could possibly be making the
assertions
> it seems to think it is asserting.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_database.cc:207: return s.ColumnInt64(0);
On 2012/01/03 23:41:57, shess wrote:
> This line would give 0 if the column type is NULL.

So we can just return s.ColumnInt64(0)?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/in_memor...
File chrome/browser/history/in_memory_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/in_memor...
chrome/browser/history/in_memory_database.cc:78: NOTREACHED() <<
GetDB().GetErrorMessage();
On 2012/01/03 23:41:57, shess wrote:
> If there was an error, shouldn't it already have been logged and NOTREACHED?

Yes. Fixed. This may have merged in on me, or I just missed it.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:269: return false;
On 2012/01/03 23:41:57, shess wrote:
> Is this likely enough to happen to be worth bothering with?  Logically it
seems
> like statement should be under if(encoded) anyhow.

Agreed.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:281:
static_cast<int>(jpeg_data.size()));
On 2012/01/03 23:41:57, shess wrote:
> Sigh, the parameter should have been a size_t in the first place...

Shall I add a todo?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:330: DCHECK(score);
On 2012/01/03 23:41:57, shess wrote:
> If you're going to add this, push it above the if() so that you're testing the
> API's static usage, which presumably shouldn't change based on use_top_sites_.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:367: "UPDATE favicons SET
image_data=NULL, last_updated=? WHERE id=?"));
On 2012/01/03 23:41:57, shess wrote:
> This would probably be cleaner with a single statement using BindNull(0). 
> Please put a TODO(shess): to that end, if you don't care to go there.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:540: return false;
Removed.

On 2012/01/03 23:41:57, shess wrote:
> Is this still necessary?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:648: DLOG(FATAL) <<
db_.GetErrorMessage();
On 2012/01/03 23:41:57, shess wrote:
> I can't tell if the Execute() cases would already be fatal, but wouldn't this
> and the above already be fatal?

I think you are right. We have a TODO to DLOG(FATAL) once any noncompliant
clients are converted to ExecuteAndReturnErrorCode. Shall I rely on that and
remove these? It is DLOG(ERROR) now...

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
File chrome/browser/history/top_sites_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:201: if (!statement.is_valid())
On 2012/01/03 23:41:57, shess wrote:
> Is this still necessary?

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:218: NOTREACHED() <<
db_->GetErrorMessage();
On 2012/01/03 23:41:57, shess wrote:
> This line should be redundant?

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:233: return;
On 2012/01/03 23:41:57, shess wrote:
> Still necessary?

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:250: NOTREACHED() <<
db_->GetErrorMessage();
On 2012/01/03 23:41:57, shess wrote:
> The error message is redundant?

Done. (Missed a bunch in this file, whether later adds or merges or whatever.
Once these are all in I'll do another sweep for laggards.)

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:332: return
select_statement.ColumnInt(0);
This is returning 0 on error, which is a bit of an error-swallowing misfeature,
but in this case I'm guessing it ends up not mattering?

On 2012/01/03 23:41:57, shess wrote:
> AFAICT, having this return anything other than a single row with a single
> integer would be an error, as COUNT(...) can't return NULL or an empty result.
> 
> OTOH, I guess it's more consistent this way.  OK.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
File chrome/browser/history/url_database.cc (left):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
chrome/browser/history/url_database.cc:108: return false;
On 2012/01/03 23:41:57, shess wrote:
> This can now return true in case of empty results.  Is that appropriate?

I think that's what this did before. The above should only return false in case
of invalid statement, and the .h file looks like the docs are right about that.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
chrome/browser/history/url_database.cc:257: }
On 2012/01/03 23:41:57, shess wrote:
> I'm not entirely convinced that removing these is appropriate (well, the error
> message is, but the return value maybe not).  But I need to move on for the
day,
> so I'll need to circle back to the rest of this file.

OK. Is the above build-up of a query manually kosher? Shouldn't we be using
BindInt there instead of manually  strappending? The macro is used thrice. Is
saving 40 bytes or whatever worth it?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
chrome/browser/history/visit_database.cc:164: update_chain.Run();
On 2012/01/03 23:41:57, shess wrote:
> I don't know if the previous short-circuit behavior was essential or not, but
it
> does seem like you should test Run() to maintain consistency.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
chrome/browser/history/visit_database.cc:367: return false;
Another error. I can only plead that there were dozens of these and the code's
merged like ten times since I wrote it. :-)

On 2012/01/03 23:41:57, shess wrote:
> Is this here because you don't trust FillVisitVector() to chain right, or
> because it's more self-documenting?  The earlier cases seem to trust it.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visit_da...
chrome/browser/history/visit_database.cc:459: if (!statement.Step()) {
On 2012/01/03 23:41:57, shess wrote:
> It seems like this statement and the is_valid() above really are trying to
> distinguish empty results from errors.  Maybe it would be better to explicitly
> use Succeeded() in here.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
File chrome/browser/history/visitsegment_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:180: return false;
Yes. Ouch. The .h file says returns 0 on failure, so changing.

On 2012/01/03 23:41:57, shess wrote:
> Ick.  It seems unlikely that |false| is the right return value in this case.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:201: return false;
On 2012/01/03 23:41:57, shess wrote:
> Can be removed.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:212: return false;
On 2012/01/03 23:41:57, shess wrote:
> Can be removed.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/visitseg...
chrome/browser/history/visitsegment_database.cc:344: return false;
Done. Yes, it cleans up real nice. :-)

On 2012/01/03 23:41:57, shess wrote:
> Would you be willing to rewrite this to a pair of simpler statements?  I mean
> like:
> 
> DELETE FROM segment_usage WHERE segment_id IN (SELECT id FROM segments WHERE
> url_id = ?);
>   and
> DELETE FROM segments WHERE url_id = ?;
> 
> Performance-wise, they should be identical, but brittle-code-wise the two
> simpler statements should be full of win.  Otherwise maybe just drop a
> TODO(shess) in here?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:234: LOG(ERROR) << "No default search
provider with backup id.";
On 2012/01/03 23:41:57, shess wrote:
> Drop the is_valid(), and use LOG_IF(s.Succeeded(), ERROR) to distinguish the
> case there there was no error but the results were empty.

Done.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:245: LOG(ERROR) << "Statement has not
succeeded.";
Yes, this is amiss. It is dead code. It could be there's a logic error here, as
this implies an incorrect reading of the Statement API.

On 2012/01/03 23:41:57, shess wrote:
> Actually, I don't understand what this statement intends.  s.Step() being true
> implies that s.Succeeded() should be true, doesn't it?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:413: if (!GetTableContents("keywords",
&keywords))
:-) 
On 2012/01/03 23:41:57, shess wrote:
> Looks like failing this one would have caused many layers of log spew :-).

http://codereview.chromium.org/9071014/diff/1/chrome/browser/webdata/keyword_...
chrome/browser/webdata/keyword_table.cc:420: if (signature.empty())
On 2012/01/03 23:41:57, shess wrote:
> Is the NOTREACHED() in this case WRT sql/ API, or some other API?

Misfire. I should have kept NOTREACHED and done in the return false.

[Randy Smith (Not in Mondays), 2012-01-04 19:34:18]

LGTM given the move of the below mentioned comment back to the erase.

CCing Ben Hayden FHI (no need to review).  Ben: You might want to think about if
the problems we're seeing with the duplicate rowids might be caused for Just The
Right failures of sql statements, and if we should tweak returned_ids_
appropriately if those failures occur.

For my curiosity: What's the context for this change?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
chrome/browser/history/download_database.cc:235: // TODO(rdsmith): Remove when
http://crbug.com/96627 is resolved.
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > The target of this TODO seems ambiguous, now.  Maybe should loop in rdsmith.
> 
> I think I misplaced it. Should be by the erase I think.

Yes, it should.

[Randy Smith (Not in Mondays), 2012-01-04 19:36:05]

Whoops; really ccing Ben Hayden FHI this time.  

(Copy from last comment)  Ben: You might want to think about if the problems
we're seeing with the duplicate rowids might be caused for Just The Right
failures of sql statements, and if we should tweak returned_ids_ appropriately
if those failures occur.

[Greg Billock, 2012-01-04 21:08:06]

On Wed, Jan 4, 2012 at 11:34 AM,  <rdsmith@chromium.org> wrote:
> LGTM given the move of the below mentioned comment back to the erase.
>
> CCing Ben Hayden FHI (no need to review).  Ben: You might want to think
> about if
> the problems we're seeing with the duplicate rowids might be caused for Just
> The
> Right failures of sql statements, and if we should tweak returned_ids_
> appropriately if those failures occur.
>
> For my curiosity: What's the context for this change?

Long story, but it originating from a desire to consolidate NOTREACHED
lower in the database layer. On the plus side, we're close to being
able to provide yak hair wigs for the entire chrome team. :-)

>
>
>
>
http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
> File chrome/browser/history/download_database.cc (right):
>
>
http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/download...
> chrome/browser/history/download_database.cc:235: // TODO(rdsmith):
> Remove when http://crbug.com/96627 is resolved.
> On 2012/01/04 19:20:20, Greg Billock wrote:
>>
>> On 2012/01/03 23:41:57, shess wrote:
>> > The target of this TODO seems ambiguous, now.  Maybe should loop in
>
> rdsmith.
>
>> I think I misplaced it. Should be by the erase I think.
>
>
> Yes, it should.
>
> http://codereview.chromium.org/9071014/

[Randy Smith (Not in Mondays), 2012-01-04 21:22:15]

On 2012/01/04 21:08:06, Greg Billock wrote:
> Long story, but it originating from a desire to consolidate NOTREACHED
> lower in the database layer. On the plus side, we're close to being
> able to provide yak hair wigs for the entire chrome team. :-)

Unfortunately, I suspect most of the team has such wigs from their own shaving
expeditions.  And you know how people prefer to use things they've made
themselves :-}.

[Greg Billock, 2012-01-09 23:12:53]

Any other comments?

On 2012/01/04 21:22:15, rdsmith wrote:
> On 2012/01/04 21:08:06, Greg Billock wrote:
> > Long story, but it originating from a desire to consolidate NOTREACHED
> > lower in the database layer. On the plus side, we're close to being
> > able to provide yak hair wigs for the entire chrome team. :-)
> 
> Unfortunately, I suspect most of the team has such wigs from their own shaving
> expeditions.  And you know how people prefer to use things they've made
> themselves :-}.

[Scott Hess - ex-Googler, 2012-01-10 22:04:55]

Here's a pass responding to earlier comments.  I'll make another pass looking
for new comments :-).

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
File chrome/browser/history/history_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_database.cc:193: DCHECK(db_.GetLastChangeCount()
== 1);
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > I'm having troubles seeing how this line could possibly be making the
> assertions
> > it seems to think it is asserting.
> 
> I read it as saying "ASSERT(number of rows changed by the previous UPDATE
> statement was exactly 1)"

Do you mean an update statement which ran sometime prior to the call to
SetSegmentID()?  Because the UPDATE statement a couple lines above here has not
run, yet.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_database.cc:207: return s.ColumnInt64(0);
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > This line would give 0 if the column type is NULL.
> 
> So we can just return s.ColumnInt64(0)?

Yes, NULL is interpreted as zero for integer requests.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:281:
static_cast<int>(jpeg_data.size()));
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > Sigh, the parameter should have been a size_t in the first place...
> 
> Shall I add a todo?

Over in BindBlob()?  I think that makes sense.  SQLite doesn't seem to make use
of size_t, but I think it would make sense for sql/ to provide that interface,
rather than exporting it to the caller.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/thumbnai...
chrome/browser/history/thumbnail_database.cc:648: DLOG(FATAL) <<
db_.GetErrorMessage();
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > I can't tell if the Execute() cases would already be fatal, but wouldn't
this
> > and the above already be fatal?
> 
> I think you are right. We have a TODO to DLOG(FATAL) once any noncompliant
> clients are converted to ExecuteAndReturnErrorCode. Shall I rely on that and
> remove these? It is DLOG(ERROR) now...

If Execute() is currently not doing FATAL, then leave those for now and they can
be dealt with if/when someone gets up speed to deal with that case.  But
redundant fatals on Run and Step are redundant.

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
File chrome/browser/history/top_sites_database.cc (right):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/top_site...
chrome/browser/history/top_sites_database.cc:332: return
select_statement.ColumnInt(0);
Yeah, I think it doesn't matter (or, at least, it's probably one of dozens of
cases like this which we haven't noticed, yet).

On 2012/01/04 19:20:20, Greg Billock wrote:
> This is returning 0 on error, which is a bit of an error-swallowing
misfeature,
> but in this case I'm guessing it ends up not mattering?
> 
> On 2012/01/03 23:41:57, shess wrote:
> > AFAICT, having this return anything other than a single row with a single
> > integer would be an error, as COUNT(...) can't return NULL or an empty
result.
> > 
> > OTOH, I guess it's more consistent this way.  OK.
>

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
File chrome/browser/history/url_database.cc (left):

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
chrome/browser/history/url_database.cc:108: return false;
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > This can now return true in case of empty results.  Is that appropriate?
> 
> I think that's what this did before. The above should only return false in
case
> of invalid statement, and the .h file looks like the docs are right about
that.

As it stands, it's always returning true, so if it is appropriate to succeed in
case of error, then it could have a void return.  Or maybe it should return
Succeeded()?

http://codereview.chromium.org/9071014/diff/1/chrome/browser/history/url_data...
chrome/browser/history/url_database.cc:257: }
On 2012/01/04 19:20:20, Greg Billock wrote:
> On 2012/01/03 23:41:57, shess wrote:
> > I'm not entirely convinced that removing these is appropriate (well, the
error
> > message is, but the return value maybe not).  But I need to move on for the
> day,
> > so I'll need to circle back to the rest of this file.
> 
> OK. Is the above build-up of a query manually kosher? Shouldn't we be using
> BindInt there instead of manually  strappending? The macro is used thrice. Is
> saving 40 bytes or whatever worth it?

In general I hate dynamically-generated queries, persistence is not the area to
save a few bytes.  But, by the same token, for a clean-up pass like you're
doing, I'm a little nervous about making substantive changes.  In this case, it
seems like there are a couple levels of dynamism going on, so nervous on top of
nervous.

[Scott Hess - ex-Googler, 2012-01-10 22:29:49]

OK, a few more comments.  Thanks!

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/thumb...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/thumb...
chrome/browser/history/thumbnail_database.cc:363: statement.BindInt64(2,
icon_id);
+100!

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
File chrome/browser/history/url_database.cc (left):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:257: }
In HistoryBackend::IterateURLs(), returning false leads to
iterator->OnComplete(false), while returning true eventually leads to
iterator->OnComplete(true) (and the rows get passed to the iterator).  I don't
know what OnComplete() means, but I'm willing to assume that someone meant to do
that.  I think you need to put back the is_valid() test.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:272: }
In this case, true/false controls whether InMemoryURLIndex::ReloadFromHistory()
builds and saves an index file.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:289: }
And likewise for ThumbnailDatabase::MigrateIconMappingData().

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
File chrome/browser/history/url_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:409: return true;  // Term already
exists, no need to add it.
I think you need a call to if (!Succeeded()) return false, here.  This case
tests that there was at least one row.  An error means failure, not that there
weren't any rows.

I'm willing to grant that the INSERT would probably fail in that case, but I
don't like parsing things that fine.

Hmm.  It might also make sense to drop a TODO here to think about using an index
to enforce this rather than code.  Then it could just be a single INSERT OR
IGNORE statement.  I'd need to research more on how often this happens to see if
it would be worth the space.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:117: return true;
WDYT about removing the is_valid() check and returning Succeeded()?

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:383: return false;
!Step() below should cover this, though the comment might need a tweak.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:473: if (!statement.Step() ||
statement.ColumnInt64(0) == 0) {
Odd.  I wonder how this code came to be written that way.  I suspect that the ==
0 case happens when there are no visits and it gets a NULL.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
File chrome/browser/history/visitsegment_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visitsegment_database.cc:328: "(SELECT id FROM segments
WHERE url_id = ?)"));
I suspect you actually want to put this one after the other one.  If the tests
pass, it's possible that this isn't being tested!

And this one doesn't need the sub-select.

[Greg Billock, 2012-01-11 23:42:12]

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/thumb...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/thumb...
chrome/browser/history/thumbnail_database.cc:363: statement.BindInt64(2,
icon_id);
On 2012/01/10 22:29:49, shess wrote:
> +100!

winning! :-)

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
File chrome/browser/history/url_database.cc (left):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:257: }
On 2012/01/10 22:29:49, shess wrote:
> In HistoryBackend::IterateURLs(), returning false leads to
> iterator->OnComplete(false), while returning true eventually leads to
> iterator->OnComplete(true) (and the rows get passed to the iterator).  I don't
> know what OnComplete() means, but I'm willing to assume that someone meant to
do
> that.  I think you need to put back the is_valid() test.

Set initialized_ to the statement validity now. I think you're right; the
NOTREACHED makes me think the developer doesn't think this will ever happen,
though, which may be an error, but agreed we shouldn't change the logic.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:272: }
On 2012/01/10 22:29:49, shess wrote:
> In this case, true/false controls whether
InMemoryURLIndex::ReloadFromHistory()
> builds and saves an index file.

Used the idiom from above.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:289: }
On 2012/01/10 22:29:49, shess wrote:
> And likewise for ThumbnailDatabase::MigrateIconMappingData().

ditto

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
File chrome/browser/history/url_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/url_d...
chrome/browser/history/url_database.cc:409: return true;  // Term already
exists, no need to add it.
On 2012/01/10 22:29:49, shess wrote:
> I think you need a call to if (!Succeeded()) return false, here.  This case
> tests that there was at least one row.  An error means failure, not that there
> weren't any rows.
> 
> I'm willing to grant that the INSERT would probably fail in that case, but I
> don't like parsing things that fine.
> 
> Hmm.  It might also make sense to drop a TODO here to think about using an
index
> to enforce this rather than code.  Then it could just be a single INSERT OR
> IGNORE statement.  I'd need to research more on how often this happens to see
if
> it would be worth the space.

I see what you mean. If the select statement is broken, we should not attempt
the insert.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
File chrome/browser/history/visit_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:117: return true;
On 2012/01/10 22:29:49, shess wrote:
> WDYT about removing the is_valid() check and returning Succeeded()?

Done.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visit_database.cc:383: return false;
On 2012/01/10 22:29:49, shess wrote:
> !Step() below should cover this, though the comment might need a tweak.

Done.

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
File chrome/browser/history/visitsegment_database.cc (right):

http://codereview.chromium.org/9071014/diff/9002/chrome/browser/history/visit...
chrome/browser/history/visitsegment_database.cc:328: "(SELECT id FROM segments
WHERE url_id = ?)"));
On 2012/01/10 22:29:49, shess wrote:
> I suspect you actually want to put this one after the other one.  If the tests
> pass, it's possible that this isn't being tested!
> 
> And this one doesn't need the sub-select.

Oy, good point. I see what you're saying. I think I ran the tests. I'll have a
look... Nope, if this is tested, it is way, way up the call stack, not directly
at all.

[Ivan Korotkov, 2012-01-13 09:53:41]

LGTM for KeywordTable with nits:

http://codereview.chromium.org/9071014/diff/23005/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/9071014/diff/23005/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.cc:237: scoped_ptr<TemplateURL>
template_url(new TemplateURL());
Looks like there is no more need in scoped_ptr here.

http://codereview.chromium.org/9071014/diff/23005/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.cc:581: LOG(WARNING) << "No keyword with
id: " << id << ", ignoring.";
Shouldn't this be a LOG_IF(..., s.Succeeded(), ...) ?

[Greg Billock, 2012-01-13 20:23:19]

http://codereview.chromium.org/9071014/diff/23005/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/9071014/diff/23005/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.cc:237: scoped_ptr<TemplateURL>
template_url(new TemplateURL());
On 2012/01/13 09:53:41, Ivan Korotkov wrote:
> Looks like there is no more need in scoped_ptr here.

Done.

http://codereview.chromium.org/9071014/diff/23005/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.cc:581: LOG(WARNING) << "No keyword with
id: " << id << ", ignoring.";
On 2012/01/13 09:53:41, Ivan Korotkov wrote:
> Shouldn't this be a LOG_IF(..., s.Succeeded(), ...) ?

Yeah, that's probably better to maintain semantics, although Step() will crash
in debug if there is an sql error.

[Scott Hess - ex-Googler, 2012-01-13 21:41:33]

LGTM, with the indicated change.

http://codereview.chromium.org/9071014/diff/30001/chrome/browser/history/visi...
File chrome/browser/history/visitsegment_database.cc (right):

http://codereview.chromium.org/9071014/diff/30001/chrome/browser/history/visi...
chrome/browser/history/visitsegment_database.cc:336: "(SELECT id FROM segments
WHERE url_id = ?)"));
DELETE and SELECT refer to the same table, coalesce:
   DELETE FROM segments WHERE url_id = ?;

[Greg Billock, 2012-01-18 19:32:21]

http://codereview.chromium.org/9071014/diff/30001/chrome/browser/history/visi...
File chrome/browser/history/visitsegment_database.cc (right):

http://codereview.chromium.org/9071014/diff/30001/chrome/browser/history/visi...
chrome/browser/history/visitsegment_database.cc:336: "(SELECT id FROM segments
WHERE url_id = ?)"));
On 2012/01/13 21:41:33, shess wrote:
> DELETE and SELECT refer to the same table, coalesce:
>    DELETE FROM segments WHERE url_id = ?;

Done.

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

STILL LGTM!

[commit-bot: I haz the power, 2012-01-18 21:56:22]

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

[commit-bot: I haz the power, 2012-01-18 23:43:29]

Change committed as 118184