Clean up entries left by crashes in the DownloadDB.

Clean up entries left by crashes in the DownloadDB.

BUG=224385
R=benjhayden@chromium.org



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

[Randy Smith (Not in Mondays), 2013-03-27 19:31:54]

Ben, Scott: WDYT?

Ben: Whole CL., primary reviewer.

Scott: I'd like your eyes on the download_database.cc and history_unittest.cc
changes, mostly because I've gotten a healthy respect for the ways in which one
can do things wrong when one touches the DB :-}.  Please also scan for idioms
that don't belong at this level of the code.  I'm specifically curious about
your thoughts on the MessageLoop::Current()->PostTask() in QueryDownloads.

[benjhayden, 2013-03-27 19:38:14]

https://codereview.chromium.org/13044019/diff/1/chrome/browser/history/downlo...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/13044019/diff/1/chrome/browser/history/downlo...
chrome/browser/history/download_database.cc:455: statement.Run();
Do you want to call HistoryBackend::ScheduleCommit?

[Randy Smith (Not in Mondays), 2013-03-27 19:39:59]

https://codereview.chromium.org/13044019/diff/1/chrome/browser/history/downlo...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/13044019/diff/1/chrome/browser/history/downlo...
chrome/browser/history/download_database.cc:455: statement.Run();
On 2013/03/27 19:38:14, benjhayden_chromium wrote:
> Do you want to call HistoryBackend::ScheduleCommit?

I can't call that from inside DownloadDatabase.  Are you suggesting that I move
the spawn-off point up into HistoryBackend?  That would require me to scan the
list of returned downloads, instead of being able to set a bool "for free" when
I need to cleanup the entry.

[Randy Smith (Not in Mondays), 2013-03-27 19:45:41]

Guys, I'm sorry, please do *not* review this CL--the basic approach is flawed. 
My apologies for the spam; I'll post again when I have something reasonable.

(Flaw: Calling QueryDownloads() on a running database nukes all the IN_PROGRESS
entries.)

[Randy Smith (Not in Mondays), 2013-03-28 18:59:10]

Ok, I'm ready for review.  I've taken Scott off the reviewer list because IMO
this new code is simple enough not to require a DB expert's eye.  

Ben: WDYT?

[benjhayden, 2013-03-28 19:18:55]

lgtm

[benjhayden, 2013-03-28 19:21:03]

LGTM, one suggestion for the test that you can either take or leave as you wish.

https://codereview.chromium.org/13044019/diff/14002/chrome/browser/history/hi...
File chrome/browser/history/history_unittest.cc (right):

https://codereview.chromium.org/13044019/diff/14002/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:500: results[0].interrupt_reason);
Do you want to also test that adding an in_progress download and calling
QueryDownloads() doesn't break the in_progress download?

[Randy Smith (Not in Mondays), 2013-03-28 19:40:12]

Thanks!

https://codereview.chromium.org/13044019/diff/14002/chrome/browser/history/hi...
File chrome/browser/history/history_unittest.cc (right):

https://codereview.chromium.org/13044019/diff/14002/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:500: results[0].interrupt_reason);
On 2013/03/28 19:21:03, benjhayden_chromium wrote:
> Do you want to also test that adding an in_progress download and calling
> QueryDownloads() doesn't break the in_progress download?

I think chrome/browser/download/download_browsertest.cc DownloadHistoryCheck
does that; let me know if you disagree.

[commit-bot: I haz the power, 2013-04-01 14:04:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/13044019/27001

[commit-bot: I haz the power, 2013-04-01 14:05:06]

Presubmit check for 13044019-27001 failed and returned exit status 1.

INFO:root:Found 9 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/history/history_unittest.cc
    chrome/browser/history/download_database.cc
    chrome/browser/history/history_backend.cc
    chrome/browser/history/history_service.h
    chrome/browser/history/history_backend.h
    chrome/browser/history/download_database.h
    chrome/browser/history/history_service.cc

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Randy Smith (Not in Mondays), 2013-04-01 14:41:30]

Scott: Rubbertstamp?

[Randy Smith (Not in Mondays), 2013-04-01 14:41:43]

On 2013/04/01 14:41:30, rdsmith wrote:
> Scott: Rubbertstamp?

(for chrome/browser/history/*)

[sky, 2013-04-01 15:04:44]

sky->brettw

[brettw, 2013-04-01 23:52:51]

lgtm

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:436: void
DownloadDatabase::CleanUpInProgressEntries() {
I sort of prefer the scheme where the function is
EnsureInProgressEntriesCleanedUp() and it checks the flag and returns early,
which saves you the bool check in every caller. You can keep it like this if you
prefer, though.

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
File chrome/browser/history/download_database.h (right):

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
chrome/browser/history/download_database.h:25: // Visible outside of
download_database.cc only for testing.
Can this be private and the test made a friend? Also, I think these two sections
need a comment about what they mean.

[Randy Smith (Not in Mondays), 2013-04-02 18:58:52]

Brett: One more quick look, specifically at PS4->5 for the reasons given below?

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
File chrome/browser/history/download_database.cc (right):

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:436: void
DownloadDatabase::CleanUpInProgressEntries() {
On 2013/04/01 23:52:51, brettw wrote:
> I sort of prefer the scheme where the function is
> EnsureInProgressEntriesCleanedUp() and it checks the flag and returns early,
> which saves you the bool check in every caller. You can keep it like this if
you
> prefer, though.

Done.

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
File chrome/browser/history/download_database.h (right):

https://codereview.chromium.org/13044019/diff/27001/chrome/browser/history/do...
chrome/browser/history/download_database.h:25: // Visible outside of
download_database.cc only for testing.
On 2013/04/01 23:52:51, brettw wrote:
> Can this be private and the test made a friend? 

Done, but I'm not certain I think it was worth it, as it resulted in extra
include files in .h files (because of moving static functions into the class so
they had access to the constants and that requiring including a content include
file in the .h file).  I've isolated the changes for this comment to a single
patch set; willing to take a quick look and decide if you think it was worth it?
 If so, I'm good--I could just imagine that generic goodness would not equal
specific goodness in your mind in this case.

> Also, I think these two sections need a comment about what they mean.

Done.

[brettw, 2013-04-05 19:50:49]

Overall the new one seems slightly better to me so it seems fine.

Note: Randy pinged me separately on this so I noticed it. Normally when I say
LGTM I mark the thread with a label and never look at it again (to avoid
subsequent discussion and buildbot emails), so no amount of pinging on the code
review will make me notice it again. You'll have to do so separately.

[commit-bot: I haz the power, 2013-04-08 12:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/13044019/51001

[commit-bot: I haz the power, 2013-04-08 12:15:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-08 18:44:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/13044019/51001

[commit-bot: I haz the power, 2013-04-08 20:23:21]

Change committed as 192879