Persist download interrupt reason, both target and current paths, and url_chain.

Persist download interrupt reason, both target and current paths, url_chain, and danger type.

Primarily preparation for downloads resumption, but also relevant for 
unifying the CANCELLED and INTERRUPTED download states.

BUG=159501
BUG=7648
BUG=134513
BUG=172672
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180312

[Randy Smith (Not in Mondays), 2012-11-13 23:10:37]

Ben, Scott: PTAL?  

* Scott: chrome/browser/history, 

* Ben: Everything including chrome/browser/history; I want the
downloads-specific eyes as well as the database-knowledgeable eyes. 

Thanks!

[sky, 2012-11-14 00:34:31]

What do you plan on using download_url_chains for?

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

https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
chrome/browser/history/download_database.cc:265: statement.BindInt64(3,
info.start_time.ToTimeT());
Since you're writing migration code, could you migrate this to use
ToInternalValue like we do every where else for times?

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

https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
chrome/browser/history/download_database.cc:47: "id INTEGER NOT NULL,"          
// SQLite-generated primary key.
Your comment is a bit misleading here. Don't you mean that its 'downloads.id' ?

https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
chrome/browser/history/download_database.cc:169: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
I get nervous when we start persisting enums from different parts of the code
base, especially when the enum does not mention this at all.

[Randy Smith (Not in Mondays), 2012-11-14 13:53:37]

On 2012/11/14 00:34:31, sky wrote:
> What do you plan on using download_url_chains for?

So that's a bit of a "tug on a string" situation:
* Safe Browsing wants the full chain of URL redirects to evaluate the safety of
the download.
* In some cases, safe browsing does an evaluation at the end of the download
(after all content has been downloaded) that includes as one of its inputs a
hash of the full download as well as the full url_chain redirect.  This requires
us to have the url_chain present for the entire download.
* We're implementing Downloads resumption so that downloads that encounter an
error can be resumed at a later time.  We'd like "a later time" to not be
relevant to whether the browser has been restarted.  Thus we need to persist all
information associated with a download that's required for doing the entire
download.

https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
> chrome/browser/history/download_database.cc:265: statement.BindInt64(3,
> info.start_time.ToTimeT());
> Since you're writing migration code, could you migrate this to use
> ToInternalValue like we do every where else for times?

Certainly.  Implementation guidance questions: I presume that means I'll need to
transfer data between the old table and the new one using a statement.Step()
loop, and do the transformation from TimeT() to IntervalValue() in Chrome?  Can
I have two statements (select & insert) active and stepping at the same time, or
do I need to read everything into memory and then write it back out?

[sky, 2012-11-14 16:00:00]

Thanks for the description of downloads_url_chains. I don't see you deleting
from that table any where. Is that intentional?

See HistoryDatabase::MigrateTimeEpoch for time migration.

[Randy Smith (Not in Mondays), 2012-11-14 16:54:21]

On 2012/11/14 16:00:00, sky wrote:
> Thanks for the description of downloads_url_chains. I don't see you deleting
> from that table any where. Is that intentional?

Yikes!  No, that's what I'll euphemistically call an "oversight".  I'll fix
that; thanks for the catch.

> See HistoryDatabase::MigrateTimeEpoch for time migration.

Thanks much!

[benjhayden, 2012-11-14 19:43:22]

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

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:169: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
I agree with Scott. This is how we ran into trouble with the state.
A mapping layer like we have with state would be ideal.
There are currently about 20 interrupt_reasons, which seems feasible. If there
were >100, I'd start thinking about testing the immutability of the
interrupt_reason set.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:192: if
(GetDB().DoesTableExist("downloads")) {
&& DoesTableExist("downloads_url_chains")?

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:223: int column = 0;
I've also been very tempted to do this, and it is clever, but it also makes it
very easy to accidentally mismatch columns, and hard to notice when that
happens, so it's very fragile code.
Do you want to define kConstants for the column indices instead? They'd be used
both here and in the INSERT statement.

Bonus points if you want to split the DPSI reading/writing out to co-located
static functions alongside these kConstantColumnIndices and a string "id,
target_path, ... opened".

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:284: continue;
Why not overwrite?

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:330: BindFilePath(statement,
current_path, 0);
Check the column index here.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:386: return 0;
When you merge with my DownloadHistory, please be sure that this and the other
'return 0' change to returning kUninitializedHandle.

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

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/history...
chrome/browser/history/history_database.cc:346: if (!MigrateReasonAndPaths()) {
Put 'Downloads' in this method name?

[Randy Smith (Not in Mondays), 2012-11-14 22:47:34]

PTAL?  

I have one more test I want to write (to catch the problem Ben found in
UpdateDownloadPath()) but there's a few things that I'm pushing back on and seem
worth resolving while I write the test.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
File chrome/browser/history/download_database.cc (left):

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:265: statement.BindInt64(3,
info.start_time.ToTimeT());
On 2012/11/14 00:34:31, sky wrote:
> Since you're writing migration code, could you migrate this to use
> ToInternalValue like we do every where else for times?

Done.

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

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:47: "id INTEGER NOT NULL,"          
// SQLite-generated primary key.
On 2012/11/14 00:34:31, sky wrote:
> Your comment is a bit misleading here. Don't you mean that its 'downloads.id'
?

Right.  Copy and paste error.  Done.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:169: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
On 2012/11/14 00:34:31, sky wrote:
> I get nervous when we start persisting enums from different parts of the code
> base, especially when the enum does not mention this at all.

Fair enough.  There are enough errors in the enum that I'm reluctant to
construct a mapping function in this file, but I did put a note in
download_interrupt_reason_values.h indicating that the values may be persisted
and shouldn't be changed.  WDYT?

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:169: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> I agree with Scott. This is how we ran into trouble with the state.
> A mapping layer like we have with state would be ideal.
> There are currently about 20 interrupt_reasons, which seems feasible. If there
> were >100, I'd start thinking about testing the immutability of the
> interrupt_reason set.

Still pushing back, Milo--keeping 20 interrupt reasons in sync with the original
location strikes me as a fairly high maintenance burden.  I have marked the
original location as "don't change; you can add, but reserve values once they're
in place".  Willing to let me get away with that?

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:192: if
(GetDB().DoesTableExist("downloads")) {
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> && DoesTableExist("downloads_url_chains")?

Done, though I'm a little uncomfortable that I don't understand the "threat
model" that this function is supposed to be protecting against.  The obvious one
is database version skew, but we're handling that elsewhere. 

Scott, care to comment?  What kinds of gotchas am I looking to remedy here; only
a clean DB?  If so, why the EnsureColumnExists() statements?

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:223: int column = 0;
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> I've also been very tempted to do this, and it is clever, but it also makes it
> very easy to accidentally mismatch columns, and hard to notice when that
> happens, so it's very fragile code.

Does that make it different than what it replaces?  I see this as being very
slightly easier to figure out if you've got a match (the algorithm is to compare
against the previous column in lines of code and in the SQL, versus counting up
a number that's an absolute location), and noticeably easier to modify if you
need to.  Doesn't mean it's not fragile, but I think it's still an improvement. 

> Do you want to define kConstants for the column indices instead? 

That doesn't make the code easy to modify the way this does.

> They'd be used both here and in the INSERT statement.

You say it like it's a good thing :-}.  More specifically: The column numbers
are very much SQL statement dependent.  Sharing constants between different SQL
statements doesn't strike me as a win; then you've got a dependency between
those constants and the contents of the SQL statements.  

> Bonus points if you want to split the DPSI reading/writing out to co-located
> static functions alongside these kConstantColumnIndices and a string "id,
> target_path, ... opened".

That would work, but seems like real overkill for this problem.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:284: continue;
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> Why not overwrite?

I flipped a coin :-}.  Do you have some reason to think that overwrite might be
better?  I don't have a dog in the race.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:330: BindFilePath(statement,
current_path, 0);
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> Check the column index here.

Whoops.  Good catch.  Done.  I'll see if I can write a test for this, as well;
it should have been caught by testing.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:386: return 0;
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> When you merge with my DownloadHistory, please be sure that this and the other
> 'return 0' change to returning kUninitializedHandle.

Will do.

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

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/history...
chrome/browser/history/history_database.cc:346: if (!MigrateReasonAndPaths()) {
On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> Put 'Downloads' in this method name?

Done.

[benjhayden, 2012-11-15 15:57:41]

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

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:169: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
On 2012/11/14 22:47:34, rdsmith wrote:
> On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> > I agree with Scott. This is how we ran into trouble with the state.
> > A mapping layer like we have with state would be ideal.
> > There are currently about 20 interrupt_reasons, which seems feasible. If
there
> > were >100, I'd start thinking about testing the immutability of the
> > interrupt_reason set.
> 
> Still pushing back, Milo--keeping 20 interrupt reasons in sync with the
original
> location strikes me as a fairly high maintenance burden.  I have marked the
> original location as "don't change; you can add, but reserve values once
they're
> in place".  Willing to let me get away with that?

Comments don't do anything for me or the compiler.
I don't know about Scott, but I'd settle for a test that makes it extremely
clear that it's not the kind of test that one simply walks into and updates to
reflect changes to the xmacros file.

TEST(ProtectDownloadDatabaseFromCarelessInterruptReasonSchemaChange) {
  static const char kMessage[] = "\n\nYOU BROKE THE DownloadDatabase
SCHEMA!\n\n"
    "If interrupt reason values are changed, then many users' downloads
databases will be corrupted.\n"
    "New values may be assigned safely.\n"
    "If a value is removed from download_interrupt_reason_values.h, it MUST
NEVER be removed from kHistoricalValues, and it MUST NEVER be reassigned to a
new reason.\n";

  static const struct {int value; const char* name;} kHistoricalValues[] = {{1,
"FILE_FAILED"}, ...};

#define INTERRUPT_REASON(name, value) EXPECT_STREQ(name,
lookup(kHistoricalValues, value)) << kMessage;
#include "download_interrupt_reason_values.h"
#undef INTERRUPT_REASON
}

The goal is not to increase maintenance burden any more than necessary, but to
provide just enough of a speed bump at just the right place to prevent a
maintainer from accidentally speeding through the stop sign comment.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:192: if
(GetDB().DoesTableExist("downloads")) {
On 2012/11/14 22:47:34, rdsmith wrote:
> On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> > && DoesTableExist("downloads_url_chains")?
> 
> Done, though I'm a little uncomfortable that I don't understand the "threat
> model" that this function is supposed to be protecting against.  The obvious
one
> is database version skew, but we're handling that elsewhere. 
> 
> Scott, care to comment?  What kinds of gotchas am I looking to remedy here;
only
> a clean DB?  If so, why the EnsureColumnExists() statements?

The schema change represented by these EnsureColumnExists calls did not
correspond with a database version bump. The primary threat model guarded by
this method is when the table[s] don't exist at all. The EnsureColumnExists
parts guard against the secondary threat of schema skew because that wasn't
handled with a version bump.

Since you are bumping the version, you might be able to rewrite this function as
return ((DoesTableExist("downloads") || Execute(kSchema)) &&
        (DoesTableExist("downloads_url_chains") || Execute(kUrlChainSchema));

Scott might have a better idea.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:223: int column = 0;
On 2012/11/14 22:47:34, rdsmith wrote:
> On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> > I've also been very tempted to do this, and it is clever, but it also makes
it
> > very easy to accidentally mismatch columns, and hard to notice when that
> > happens, so it's very fragile code.
> 
> Does that make it different than what it replaces?  I see this as being very
> slightly easier to figure out if you've got a match (the algorithm is to
compare
> against the previous column in lines of code and in the SQL, versus counting
up
> a number that's an absolute location), and noticeably easier to modify if you
> need to.  Doesn't mean it's not fragile, but I think it's still an
improvement. 
> 
> > Do you want to define kConstants for the column indices instead? 
> 
> That doesn't make the code easy to modify the way this does.
> 
> > They'd be used both here and in the INSERT statement.
> 
> You say it like it's a good thing :-}.  More specifically: The column numbers
> are very much SQL statement dependent.  Sharing constants between different
SQL
> statements doesn't strike me as a win; then you've got a dependency between
> those constants and the contents of the SQL statements.  
> 
> > Bonus points if you want to split the DPSI reading/writing out to co-located
> > static functions alongside these kConstantColumnIndices and a string "id,
> > target_path, ... opened".
> 
> That would work, but seems like real overkill for this problem.

I think there's a philosophical subtlety here. Let's chat offline.

http://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downloa...
chrome/browser/history/download_database.cc:284: continue;
On 2012/11/14 22:47:34, rdsmith wrote:
> On 2012/11/14 19:43:22, benjhayden_chromium wrote:
> > Why not overwrite?
> 
> I flipped a coin :-}.  Do you have some reason to think that overwrite might
be
> better?  I don't have a dog in the race.

Ah, I missed the ORDER BY clause and thought that you might see urls out of
order.
Nevermind. :-)

[Randy Smith (Not in Mondays), 2012-11-15 20:10:37]

I've added the extra test, and modified InitDownloadTable() based on my
understanding of the threat model.  Ben and I have talked offline, and he's ok
with my column++ hack and my current approach to InitDownloadTable() (I
think--he should review :-}).  I'm holding off on implementing the test he
recommends to keep the database and DownloadInterruptReason in sync until I hear
back from Scott on his thoughts on that issue.

PTAL.

[benjhayden, 2012-11-15 21:57:54]

http://codereview.chromium.org/11363222/diff/15001/chrome/browser/history/dow...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/11363222/diff/15001/chrome/browser/history/dow...
chrome/browser/history/download_database.cc:209: return
(!GetDB().DoesTableExist("downloads") &&
I don't think this is quite right.
I think a more exact translation of what we discussed offline might be something
like this:

bool DownloadDatabase::InitDownloadTable() {
  GetMetaTable().GetValue(kNextDownloadId, &next_id_);
  bool downloads_exists = GetDB().DoesTableExist("downloads");
  if (!downloads_exists &&
      GetDB().DoesTableExist("downloads_url_chain"))
    return false;  // WTF, downloads_url_chain but no downloads?
  if (downloads_exists)
    return true;
  return GetDB().Execute(kSchema) &&
         GetDB().Execute(kUrlChainSchema);
}

[Randy Smith (Not in Mondays), 2012-11-15 22:03:33]

On 2012/11/15 21:57:54, benjhayden_chromium wrote:
>
http://codereview.chromium.org/11363222/diff/15001/chrome/browser/history/dow...
> File chrome/browser/history/download_database.cc (right):
> 
>
http://codereview.chromium.org/11363222/diff/15001/chrome/browser/history/dow...
> chrome/browser/history/download_database.cc:209: return
> (!GetDB().DoesTableExist("downloads") &&
> I don't think this is quite right.
> I think a more exact translation of what we discussed offline might be
something
> like this:
> 
> bool DownloadDatabase::InitDownloadTable() {
>   GetMetaTable().GetValue(kNextDownloadId, &next_id_);
>   bool downloads_exists = GetDB().DoesTableExist("downloads");
>   if (!downloads_exists &&
>       GetDB().DoesTableExist("downloads_url_chain"))
>     return false;  // WTF, downloads_url_chain but no downloads?
>   if (downloads_exists)
>     return true;
>   return GetDB().Execute(kSchema) &&
>          GetDB().Execute(kUrlChainSchema);
> }

I glitched in that the second DoesTableExist() was intended to check
downloads_url_chain; I'll fixed that.  Beyond that, I think what I have is
identical to what you have (modulo EnsureColumnExists).  Do you disagree?

[Randy Smith (Not in Mondays), 2012-11-17 01:26:31]

Ok, I think we're in final shape.  PTAL.  The try jobs are gonna fail because
one of the tests depends on a binary file, so I'm going to need to land this via
dcommit.

+ Ben: Everything.
+ Scott: chrome/browser/history/* and a question below.  Your LGTM will also be
used as a rubberstamp for chrome/test/data, so you can look at those directories
or leave them to Elliot as you prefer.
+ Elliot:  chrome/test/data/*.  This is mostly the README file.
+ Evan: chrome/browser/ui/webui Rubberstamp (Ben primary reviewer).

Question for Scott: I upgraded the test History DBs in chrome/test/data via
executing the upgrade commands in download_database.cc by hand on them, rather
than trying to setup a Windows machine to run the generate_profile executable as
described in the README.txt file.  I think you're the expert on that executable;
is doing the upgrade by hand ok?

[sky, 2012-11-19 14:53:08]

On Fri, Nov 16, 2012 at 5:26 PM,  <rdsmith@chromium.org> wrote:
> Ok, I think we're in final shape.  PTAL.  The try jobs are gonna fail
> because
> one of the tests depends on a binary file, so I'm going to need to land this
> via
> dcommit.
>
> + Ben: Everything.
> + Scott: chrome/browser/history/* and a question below.  Your LGTM will also
> be
> used as a rubberstamp for chrome/test/data, so you can look at those
> directories
> or leave them to Elliot as you prefer.
> + Elliot:  chrome/test/data/*.  This is mostly the README file.
> + Evan: chrome/browser/ui/webui Rubberstamp (Ben primary reviewer).
>
> Question for Scott: I upgraded the test History DBs in chrome/test/data via
> executing the upgrade commands in download_database.cc by hand on them,
> rather
> than trying to setup a Windows machine to run the generate_profile
> executable as
> described in the README.txt file.  I think you're the expert on that
> executable;
> is doing the upgrade by hand ok?

I'm not sure if there are subtle differences if you upgrade in that
manner. I would prefer you run the executable.

  -Scott

>
> https://codereview.chromium.org/11363222/

[benjhayden, 2012-11-19 15:22:07]

http://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/his...
File chrome/browser/history/history_unittest.cc (right):

http://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/his...
chrome/browser/history/history_unittest.cc:548: const InterruptReasonAssociation
historical_reasons[] = {
Newline before this line and a comment forbidding removing entries, please.

http://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/his...
chrome/browser/history/history_unittest.cc:573: TEST_F(HistoryBackendDBTest,
ConfirmDownloadInterruptReasonOk) {
"Confirm...Ok" is too vague. All tests confirm that something is ok.

http://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/his...
chrome/browser/history/history_unittest.cc:574: // Are there any casesin which a
historical number has been repurposed
"cases in"

http://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/his...
chrome/browser/history/history_unittest.cc:604: << "or to update this test with
approriate commenting if you "
When might this be appropriate?

[Elliot Glaysher, 2012-11-19 20:02:07]

On 2012/11/17 01:26:31, rdsmith wrote:
> Ok, I think we're in final shape.  PTAL.  The try jobs are gonna fail because
> one of the tests depends on a binary file, so I'm going to need to land this
via
> dcommit.
> 
> + Ben: Everything.
> + Scott: chrome/browser/history/* and a question below.  Your LGTM will also
be
> used as a rubberstamp for chrome/test/data, so you can look at those
directories
> or leave them to Elliot as you prefer.
> + Elliot:  chrome/test/data/*.  This is mostly the README file.
> + Evan: chrome/browser/ui/webui Rubberstamp (Ben primary reviewer).
> 
> Question for Scott: I upgraded the test History DBs in chrome/test/data via
> executing the upgrade commands in download_database.cc by hand on them, rather
> than trying to setup a Windows machine to run the generate_profile executable
as
> described in the README.txt file.  I think you're the expert on that
executable;
> is doing the upgrade by hand ok?

lgtm i guess. (I'm still not a fan of revving the history database, but I guess
you have to do it.)

[Randy Smith (Not in Mondays), 2012-11-26 17:55:40]

On 2012/11/19 14:53:08, sky wrote:
> On Fri, Nov 16, 2012 at 5:26 PM,  <mailto:rdsmith@chromium.org> wrote:
> > Ok, I think we're in final shape.  PTAL.  The try jobs are gonna fail
> > because
> > one of the tests depends on a binary file, so I'm going to need to land this
> > via
> > dcommit.
> >
> > + Ben: Everything.
> > + Scott: chrome/browser/history/* and a question below.  Your LGTM will also
> > be
> > used as a rubberstamp for chrome/test/data, so you can look at those
> > directories
> > or leave them to Elliot as you prefer.
> > + Elliot:  chrome/test/data/*.  This is mostly the README file.
> > + Evan: chrome/browser/ui/webui Rubberstamp (Ben primary reviewer).
> >
> > Question for Scott: I upgraded the test History DBs in chrome/test/data via
> > executing the upgrade commands in download_database.cc by hand on them,
> > rather
> > than trying to setup a Windows machine to run the generate_profile
> > executable as
> > described in the README.txt file.  I think you're the expert on that
> > executable;
> > is doing the upgrade by hand ok?
> 
> I'm not sure if there are subtle differences if you upgrade in that
> manner. I would prefer you run the executable.

Which'll require me to get my windows machine in shape, as that executable isn't
built off Windows.  This yak is going to be *so* bald!  :-}

(Will do.)

> 
>   -Scott
> 
> >
> > https://codereview.chromium.org/11363222/

[Randy Smith (Not in Mondays), 2012-12-08 21:50:59]

On 2012/11/19 14:53:08, sky wrote:
> I'm not sure if there are subtle differences if you upgrade in that
> manner. I would prefer you run the executable.
(tl;dr: I think generate_profile.exe has bit rot and would like to go back to
doing the upgrade by hand.)

Scott: I got my windows box setup, built chrome and run it (to validate the
setup) and built generate_profile.exe and ran it.  I'm getting an access
violation with this stack dump:

ntdll32!NtRaiseException+0x12
ntdll32!KiUserExceptionDispatcher+0x29
generate_profile!std::_Hash<std::_Hmap_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,PrefService::Preference,stdext::hash_compare<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char> >
>
>,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> const ,PrefService::Preference> >,0> >::_Check_size+0x39
generate_profile!PrefsTabHelper::RegisterUserPrefs+0x299
generate_profile!chrome::RegisterUserPrefs+0x120
generate_profile!TestingProfile::CreateTestingPrefService+0x1a7
generate_profile!TestingProfile::GetPrefs+0x25
generate_profile!ProfileKeyedBaseFactory::RegisterUserPrefsOnProfile+0x19c
generate_profile!ProfileDependencyManager::CreateProfileServices+0x107
generate_profile!TestingProfile::Init+0x76
generate_profile!TestingProfile::TestingProfile+0x200
generate_profile!main+0x4e8
generate_profile!__tmainCRTStartup+0x1bf
generate_profile!mainCRTStartup+0xf
kernel32!BaseThreadInitThunk+0xe
ntdll32!__RtlUserThreadStart+0x70
ntdll32!_RtlUserThreadStart+0x1b

The line in RegisterUserPrefs on which the failure is occurring is:

  UScriptCode browser_script = GetScriptOfBrowserLocale();

And I believe (from an earlier debugger session, though not my current one) that
the failure is occuring in that function on the line:

  std::string locale = g_browser_process->GetApplicationLocale();

The debugger says that g_browser_process is null:

0:000:x86> ?? g_browser_process
class BrowserProcess * 0x00000000

I think this application has rotted.  I supposed I could learn what it does and
try to bring it into the 21st century, but my razor is getting a little clogged
with yak fur.  I can't imagine what the differences would be from executing the
upgrade commands by hand, and I believe it's happened once in the past already. 
Given the apparent problematic aspects of doing the "right" thing, ok to just
upgrade by hand?

[Randy Smith (Not in Mondays), 2012-12-09 15:14:45]

A fair number of changes described in the lines for PS 8-10.  All comments
addressed with the exception of Scott's and my discussion about
generate_profile, above.

Ben: I'm afraid I think you should do a full re-review; the changes from merging
to your history observer CL are large, and I've also added some tests.

Elliot: If I do end up generating the profile in some different way, I'll want
you to review that methodology, but that's dependent on Scott.  Otherwise,
you're off the hook

Scott: In addition to my question above, you might want to look at the changes
to download_database.cc in PS 8 & 9 (though I don't think they're major).

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

https://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:548: const InterruptReasonAssociation
historical_reasons[] = {
On 2012/11/19 15:22:07, benjhayden_chromium wrote:
> Newline before this line and a comment forbidding removing entries, please.

Done.

https://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:573: TEST_F(HistoryBackendDBTest,
ConfirmDownloadInterruptReasonOk) {
On 2012/11/19 15:22:07, benjhayden_chromium wrote:
> "Confirm...Ok" is too vague. All tests confirm that something is ok.

Done.

https://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:574: // Are there any casesin which a
historical number has been repurposed
On 2012/11/19 15:22:07, benjhayden_chromium wrote:
> "cases in"

Done.

https://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:604: << "or to update this test with
approriate commenting if you "
On 2012/11/19 15:22:07, benjhayden_chromium wrote:
> When might this be appropriate?

I couldn't think of a time, but it won't Seriously Mess Things Up the way using
the same value for different errors would.  So I don't want to say "Don't Do
That!" so much as "Are You Really Sure You Want to Do That?" 

I'll rewrite it a bit with what I think the should do, but leave the advisory
language in place.

[sky, 2012-12-10 15:28:25]

Brett might know here. Either way, someone should fix generate_profile.

  -Scott

On Sat, Dec 8, 2012 at 1:50 PM,  <rdsmith@chromium.org> wrote:
> On 2012/11/19 14:53:08, sky wrote:
>>
>> I'm not sure if there are subtle differences if you upgrade in that
>> manner. I would prefer you run the executable.
>
> (tl;dr: I think generate_profile.exe has bit rot and would like to go back
> to
> doing the upgrade by hand.)
>
> Scott: I got my windows box setup, built chrome and run it (to validate the
> setup) and built generate_profile.exe and ran it.  I'm getting an access
> violation with this stack dump:
>
> ntdll32!NtRaiseException+0x12
> ntdll32!KiUserExceptionDispatcher+0x29
>
generate_profile!std::_Hash<std::_Hmap_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>>
>>
>>
,PrefService::Preference,stdext::hash_compare<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>>
>>
,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>> >
>
>
>>
>>
,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>> const ,PrefService::Preference> >,0> >::_Check_size+0x39
>
> generate_profile!PrefsTabHelper::RegisterUserPrefs+0x299
> generate_profile!chrome::RegisterUserPrefs+0x120
> generate_profile!TestingProfile::CreateTestingPrefService+0x1a7
> generate_profile!TestingProfile::GetPrefs+0x25
> generate_profile!ProfileKeyedBaseFactory::RegisterUserPrefsOnProfile+0x19c
> generate_profile!ProfileDependencyManager::CreateProfileServices+0x107
> generate_profile!TestingProfile::Init+0x76
> generate_profile!TestingProfile::TestingProfile+0x200
> generate_profile!main+0x4e8
> generate_profile!__tmainCRTStartup+0x1bf
> generate_profile!mainCRTStartup+0xf
> kernel32!BaseThreadInitThunk+0xe
> ntdll32!__RtlUserThreadStart+0x70
> ntdll32!_RtlUserThreadStart+0x1b
>
> The line in RegisterUserPrefs on which the failure is occurring is:
>
>   UScriptCode browser_script = GetScriptOfBrowserLocale();
>
> And I believe (from an earlier debugger session, though not my current one)
> that
> the failure is occuring in that function on the line:
>
>   std::string locale = g_browser_process->GetApplicationLocale();
>
> The debugger says that g_browser_process is null:
>
> 0:000:x86> ?? g_browser_process
> class BrowserProcess * 0x00000000
>
> I think this application has rotted.  I supposed I could learn what it does
> and
> try to bring it into the 21st century, but my razor is getting a little
> clogged
> with yak fur.  I can't imagine what the differences would be from executing
> the
> upgrade commands by hand, and I believe it's happened once in the past
> already.
> Given the apparent problematic aspects of doing the "right" thing, ok to
> just
> upgrade by hand?
>
>
> https://codereview.chromium.org/11363222/

[benjhayden, 2012-12-10 16:38:41]

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

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:159: // since the Windows Epoch). 
Note that this is dependent in the
dependent on

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:160: // implementation of base::Time
and needs to change if that changes.
What parts of base::Time specifically?

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:253: continue;
Does this leak info?

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:265: int64 db_handle =
statement_chain.ColumnInt64(0);
Why not use the column++ trick here?

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:297:
results->push_back(*it->second);
Took me a second to see that this copies the info. Comment?

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:371:
statement_insert.BindInt64(column++, info.end_time.ToInternalValue());
I'm going to trust that you've talked to the appropriate people about using
ToInternalValue, because I don't know it.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:375: return 0;
return kUninitializedHandle;

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:390: return 0;
return kUninitializedHandle;

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

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:404: // Confirm donwloads_url_chains
table is valid.
downloads_url_chains

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:488: { "FILE_FAILED",  1 },
I thought that spaces inside braces was unstylish.

[Randy Smith (Not in Mondays), 2012-12-10 20:13:53]

On 2012/12/10 15:28:25, sky wrote:
> Brett might know here. Either way, someone should fix generate_profile.

Filed http://crbug.com/165225.


> 
>   -Scott
> 
> On Sat, Dec 8, 2012 at 1:50 PM,  <mailto:rdsmith@chromium.org> wrote:
> > On 2012/11/19 14:53:08, sky wrote:
> >>
> >> I'm not sure if there are subtle differences if you upgrade in that
> >> manner. I would prefer you run the executable.
> >
> > (tl;dr: I think generate_profile.exe has bit rot and would like to go back
> > to
> > doing the upgrade by hand.)
> >
> > Scott: I got my windows box setup, built chrome and run it (to validate the
> > setup) and built generate_profile.exe and ran it.  I'm getting an access
> > violation with this stack dump:
> >
> > ntdll32!NtRaiseException+0x12
> > ntdll32!KiUserExceptionDispatcher+0x29
> >
>
generate_profile!std::_Hash<std::_Hmap_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >>
> >>
> >>
>
,PrefService::Preference,stdext::hash_compare<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >>
> >>
> ,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >> >
> >
> >
> >>
> >>
>
,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >> const ,PrefService::Preference> >,0> >::_Check_size+0x39
> >
> > generate_profile!PrefsTabHelper::RegisterUserPrefs+0x299
> > generate_profile!chrome::RegisterUserPrefs+0x120
> > generate_profile!TestingProfile::CreateTestingPrefService+0x1a7
> > generate_profile!TestingProfile::GetPrefs+0x25
> > generate_profile!ProfileKeyedBaseFactory::RegisterUserPrefsOnProfile+0x19c
> > generate_profile!ProfileDependencyManager::CreateProfileServices+0x107
> > generate_profile!TestingProfile::Init+0x76
> > generate_profile!TestingProfile::TestingProfile+0x200
> > generate_profile!main+0x4e8
> > generate_profile!__tmainCRTStartup+0x1bf
> > generate_profile!mainCRTStartup+0xf
> > kernel32!BaseThreadInitThunk+0xe
> > ntdll32!__RtlUserThreadStart+0x70
> > ntdll32!_RtlUserThreadStart+0x1b
> >
> > The line in RegisterUserPrefs on which the failure is occurring is:
> >
> >   UScriptCode browser_script = GetScriptOfBrowserLocale();
> >
> > And I believe (from an earlier debugger session, though not my current one)
> > that
> > the failure is occuring in that function on the line:
> >
> >   std::string locale = g_browser_process->GetApplicationLocale();
> >
> > The debugger says that g_browser_process is null:
> >
> > 0:000:x86> ?? g_browser_process
> > class BrowserProcess * 0x00000000
> >
> > I think this application has rotted.  I supposed I could learn what it does
> > and
> > try to bring it into the 21st century, but my razor is getting a little
> > clogged
> > with yak fur.  I can't imagine what the differences would be from executing
> > the
> > upgrade commands by hand, and I believe it's happened once in the past
> > already.
> > Given the apparent problematic aspects of doing the "right" thing, ok to
> > just
> > upgrade by hand?
> >
> >
> > https://codereview.chromium.org/11363222/

[Randy Smith (Not in Mondays), 2012-12-11 18:17:30]

Ben: Incorporated your comments.  PTAL.

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

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:159: // since the Windows Epoch). 
Note that this is dependent in the
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> dependent on

Done.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:160: // implementation of base::Time
and needs to change if that changes.
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> What parts of base::Time specifically?

Updated comment.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:253: continue;
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> Does this leak info?

D'oh!  Thanks.  Switched to scoped_ptr<>.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:265: int64 db_handle =
statement_chain.ColumnInt64(0);
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> Why not use the column++ trick here?

Didn't seem as necessary as there were only two items, so the "Is this item at
this index in the above select statement" check was pretty much equivalent to
the "Is this item after this other item in the above select statement" check. 
However, I certainly don't mind using it, and it has the advantage of
consistency.  Done.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:297:
results->push_back(*it->second);
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> Took me a second to see that this copies the info. Comment?

Done.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:371:
statement_insert.BindInt64(column++, info.end_time.ToInternalValue());
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> I'm going to trust that you've talked to the appropriate people about using
> ToInternalValue, because I don't know it.

It was Scott in c#2 and c#4 above, and I believe he reviewed this file, so we
should be ok.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:375: return 0;
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> return kUninitializedHandle;

Done.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:390: return 0;
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> return kUninitializedHandle;

Done.

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

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:404: // Confirm donwloads_url_chains
table is valid.
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> downloads_url_chains

Done.

https://codereview.chromium.org/11363222/diff/35001/chrome/browser/history/hi...
chrome/browser/history/history_unittest.cc:488: { "FILE_FAILED",  1 },
On 2012/12/10 16:38:42, benjhayden_chromium wrote:
> I thought that spaces inside braces was unstylish.

Switched to the style I see in history_query_unittest.cc:test_entries[].

[Randy Smith (Not in Mondays), 2012-12-11 18:37:12]

On 2012/12/10 15:28:25, sky wrote:
> Brett might know here. Either way, someone should fix generate_profile.

Scott: Brett says that he doesn't have any idea.  Would you be willing to give
me more of a sense as to what problems you're worried about in the by-hand
upgrade?  It seems like I'd be doing exactly the same thing that chrome does
when it encounters a back-rev DB in the wild, so if there's a problem with it,
there's a problem with that process as well.

With regard to generate_profile: Agreed (though Elliott gave me an earful about
wanting to get rid of those tests, which might mean getting rid of
generate_profile) but I am weak in all of Windows expertise, GYP
understanding/chrome tools building and linking (to make it build on Linux; I
doubt there's any particular reason why it's windows-only), and profile
generation.  So I continue to think that I'm not the right person.  Does it have
an OWNER?

> 
>   -Scott
> 
> On Sat, Dec 8, 2012 at 1:50 PM,  <mailto:rdsmith@chromium.org> wrote:
> > On 2012/11/19 14:53:08, sky wrote:
> >>
> >> I'm not sure if there are subtle differences if you upgrade in that
> >> manner. I would prefer you run the executable.
> >
> > (tl;dr: I think generate_profile.exe has bit rot and would like to go back
> > to
> > doing the upgrade by hand.)
> >
> > Scott: I got my windows box setup, built chrome and run it (to validate the
> > setup) and built generate_profile.exe and ran it.  I'm getting an access
> > violation with this stack dump:
> >
> > ntdll32!NtRaiseException+0x12
> > ntdll32!KiUserExceptionDispatcher+0x29
> >
>
generate_profile!std::_Hash<std::_Hmap_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >>
> >>
> >>
>
,PrefService::Preference,stdext::hash_compare<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >>
> >>
> ,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >> >
> >
> >
> >>
> >>
>
,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char>
> >> const ,PrefService::Preference> >,0> >::_Check_size+0x39
> >
> > generate_profile!PrefsTabHelper::RegisterUserPrefs+0x299
> > generate_profile!chrome::RegisterUserPrefs+0x120
> > generate_profile!TestingProfile::CreateTestingPrefService+0x1a7
> > generate_profile!TestingProfile::GetPrefs+0x25
> > generate_profile!ProfileKeyedBaseFactory::RegisterUserPrefsOnProfile+0x19c
> > generate_profile!ProfileDependencyManager::CreateProfileServices+0x107
> > generate_profile!TestingProfile::Init+0x76
> > generate_profile!TestingProfile::TestingProfile+0x200
> > generate_profile!main+0x4e8
> > generate_profile!__tmainCRTStartup+0x1bf
> > generate_profile!mainCRTStartup+0xf
> > kernel32!BaseThreadInitThunk+0xe
> > ntdll32!__RtlUserThreadStart+0x70
> > ntdll32!_RtlUserThreadStart+0x1b
> >
> > The line in RegisterUserPrefs on which the failure is occurring is:
> >
> >   UScriptCode browser_script = GetScriptOfBrowserLocale();
> >
> > And I believe (from an earlier debugger session, though not my current one)
> > that
> > the failure is occuring in that function on the line:
> >
> >   std::string locale = g_browser_process->GetApplicationLocale();
> >
> > The debugger says that g_browser_process is null:
> >
> > 0:000:x86> ?? g_browser_process
> > class BrowserProcess * 0x00000000
> >
> > I think this application has rotted.  I supposed I could learn what it does
> > and
> > try to bring it into the 21st century, but my razor is getting a little
> > clogged
> > with yak fur.  I can't imagine what the differences would be from executing
> > the
> > upgrade commands by hand, and I believe it's happened once in the past
> > already.
> > Given the apparent problematic aspects of doing the "right" thing, ok to
> > just
> > upgrade by hand?
> >
> >
> > https://codereview.chromium.org/11363222/

[Evan Stade, 2012-12-11 18:58:43]

rslgtm on webui

[sky, 2012-12-11 19:20:21]

On Tue, Dec 11, 2012 at 10:37 AM,  <rdsmith@chromium.org> wrote:
> On 2012/12/10 15:28:25, sky wrote:
>>
>> Brett might know here. Either way, someone should fix generate_profile.
>
>
> Scott: Brett says that he doesn't have any idea.  Would you be willing to
> give
> me more of a sense as to what problems you're worried about in the by-hand
> upgrade?  It seems like I'd be doing exactly the same thing that chrome does
> when it encounters a back-rev DB in the wild, so if there's a problem with
> it,
> there's a problem with that process as well.

generate_profile is suppose to generate an ideal fresh profile. I
don't know enough about the sql side to say if the migration code
would end up with a slightly different db (maybe more allocated size
or even less allocated size). Running migration code runs the risk of
someone later trying to run generate_profile and getting something
different. We should either fix generate_profile or nuke it.

  -Scott

> With regard to generate_profile: Agreed (though Elliott gave me an earful
> about
> wanting to get rid of those tests, which might mean getting rid of
> generate_profile) but I am weak in all of Windows expertise, GYP
> understanding/chrome tools building and linking (to make it build on Linux;
> I
> doubt there's any particular reason why it's windows-only), and profile
> generation.  So I continue to think that I'm not the right person.  Does it
> have
> an OWNER?
>
>
>
>>    -Scott
>
>
>> On Sat, Dec 8, 2012 at 1:50 PM,  <mailto:rdsmith@chromium.org> wrote:
>> > On 2012/11/19 14:53:08, sky wrote:
>> >>
>> >> I'm not sure if there are subtle differences if you upgrade in that
>> >> manner. I would prefer you run the executable.
>> >
>> > (tl;dr: I think generate_profile.exe has bit rot and would like to go
>> > back
>> > to
>> > doing the upgrade by hand.)
>> >
>> > Scott: I got my windows box setup, built chrome and run it (to validate
>> > the
>> > setup) and built generate_profile.exe and ran it.  I'm getting an access
>> > violation with this stack dump:
>> >
>> > ntdll32!NtRaiseException+0x12
>> > ntdll32!KiUserExceptionDispatcher+0x29
>> >
>
>
>
generate_profile!std::_Hash<std::_Hmap_traits<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>>
>> >>
>> >>
>> >>
>
>
>
,PrefService::Preference,stdext::hash_compare<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>>
>> >>
>> >>
>>
>>
,std::less<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>> >> >
>> >
>> >
>> >>
>> >>
>
>
>
,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char>
>>
>> >> const ,PrefService::Preference> >,0> >::_Check_size+0x39
>> >
>> > generate_profile!PrefsTabHelper::RegisterUserPrefs+0x299
>> > generate_profile!chrome::RegisterUserPrefs+0x120
>> > generate_profile!TestingProfile::CreateTestingPrefService+0x1a7
>> > generate_profile!TestingProfile::GetPrefs+0x25
>> >
>> > generate_profile!ProfileKeyedBaseFactory::RegisterUserPrefsOnProfile+0x19c
>> > generate_profile!ProfileDependencyManager::CreateProfileServices+0x107
>> > generate_profile!TestingProfile::Init+0x76
>> > generate_profile!TestingProfile::TestingProfile+0x200
>> > generate_profile!main+0x4e8
>> > generate_profile!__tmainCRTStartup+0x1bf
>> > generate_profile!mainCRTStartup+0xf
>> > kernel32!BaseThreadInitThunk+0xe
>> > ntdll32!__RtlUserThreadStart+0x70
>> > ntdll32!_RtlUserThreadStart+0x1b
>> >
>> > The line in RegisterUserPrefs on which the failure is occurring is:
>> >
>> >   UScriptCode browser_script = GetScriptOfBrowserLocale();
>> >
>> > And I believe (from an earlier debugger session, though not my current
>> > one)
>> > that
>> > the failure is occuring in that function on the line:
>> >
>> >   std::string locale = g_browser_process->GetApplicationLocale();
>> >
>> > The debugger says that g_browser_process is null:
>> >
>> > 0:000:x86> ?? g_browser_process
>> > class BrowserProcess * 0x00000000
>> >
>> > I think this application has rotted.  I supposed I could learn what it
>> > does
>> > and
>> > try to bring it into the 21st century, but my razor is getting a little
>> > clogged
>> > with yak fur.  I can't imagine what the differences would be from
>> > executing
>> > the
>> > upgrade commands by hand, and I believe it's happened once in the past
>> > already.
>> > Given the apparent problematic aspects of doing the "right" thing, ok to
>> > just
>> > upgrade by hand?
>> >
>> >
>> > https://codereview.chromium.org/11363222/
>
>
>
>
> https://codereview.chromium.org/11363222/

[benjhayden, 2012-12-11 19:48:13]

Just a couple of nits and the trybots left.

https://codereview.chromium.org/11363222/diff/42001/chrome/browser/download/d...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/11363222/diff/42001/chrome/browser/download/d...
chrome/browser/download/download_browsertest.cc:112: *results = results_;
Any reason not to use scoped_ptr<>.Pass() instead of copying the vector twice?

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

https://codereview.chromium.org/11363222/diff/42001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:214: DCHECK(results);
Is this necessary? Doesn't the dereference in the next line do basically the
same thing?

[Randy Smith (Not in Mondays), 2012-12-12 19:19:17]

Ben: PTAL.  Note that the try bots are going to fail because of the issues
raised in c#12; I'll make sure that those are the only failures before
committing.

https://codereview.chromium.org/11363222/diff/42001/chrome/browser/download/d...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/11363222/diff/42001/chrome/browser/download/d...
chrome/browser/download/download_browsertest.cc:112: *results = results_;
On 2012/12/11 19:48:13, benjhayden_chromium wrote:
> Any reason not to use scoped_ptr<>.Pass() instead of copying the vector twice?

Done.

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

https://codereview.chromium.org/11363222/diff/42001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:214: DCHECK(results);
On 2012/12/11 19:48:13, benjhayden_chromium wrote:
> Is this necessary? Doesn't the dereference in the next line do basically the
> same thing?

Done.

[benjhayden, 2012-12-12 19:49:51]

lgtm

[Randy Smith (Not in Mondays), 2012-12-20 19:50:46]

[Subtitle: Randy demonstrates he's a glutton for punishment :-{.]

Scott: After the fix in http://codereview.chromium.org/11573018, I went to
regenerate the profile in profile_with_default_theme/Default.  I wasn't sure if
I had left junk in that directory, so I deleted it before running
generate_profile.  After I did that, git status shows me the following (current
directory chrome/test/data/profiles/profile_with_default_theme):

# Changes not staged for commit:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	deleted:    Default/Cookies
#	modified:   Default/Favicons
#	deleted:    Default/Full Text Index
#	modified:   Default/History
#	deleted:    Default/Preferences
#	modified:   Default/Top Sites
#	deleted:    Default/Visited Links

I'm concerned because the "deleted" entries suggest that generate_profile is
*not* a complete source for generating the contents of that directory; either it
wasn't really what was used originally, or changes in the code have resulted in
not creating all the files that it originally did.  This makes me a little
reluctant to either run generate_profile on top of what's already there (which
is what I did for testing in the fix CL) or nuke the directory and replace it
with the output of generate_profile.  WDYT?

[sky, 2012-12-20 20:24:35]

AFAIK generate_profile is used for everything in there. Brett may know more.

  -Scott

On Thu, Dec 20, 2012 at 11:50 AM,  <rdsmith@chromium.org> wrote:
> [Subtitle: Randy demonstrates he's a glutton for punishment :-{.]
>
> Scott: After the fix in http://codereview.chromium.org/11573018, I went to
> regenerate the profile in profile_with_default_theme/Default.  I wasn't sure
> if
> I had left junk in that directory, so I deleted it before running
> generate_profile.  After I did that, git status shows me the following
> (current
> directory chrome/test/data/profiles/profile_with_default_theme):
>
> # Changes not staged for commit:
> #   (use "git add/rm <file>..." to update what will be committed)
> #   (use "git checkout -- <file>..." to discard changes in working
> directory)
> #
> #       deleted:    Default/Cookies
> #       modified:   Default/Favicons
> #       deleted:    Default/Full Text Index
> #       modified:   Default/History
> #       deleted:    Default/Preferences
> #       modified:   Default/Top Sites
> #       deleted:    Default/Visited Links
>
> I'm concerned because the "deleted" entries suggest that generate_profile is
> *not* a complete source for generating the contents of that directory;
> either it
> wasn't really what was used originally, or changes in the code have resulted
> in
> not creating all the files that it originally did.  This makes me a little
> reluctant to either run generate_profile on top of what's already there
> (which
> is what I did for testing in the fix CL) or nuke the directory and replace
> it
> with the output of generate_profile.  WDYT?
>
>
> https://codereview.chromium.org/11363222/

[brettw, 2012-12-20 21:35:19]

I guess I'd nuke the directory and make it fresh. Who knows what it will break,
though.

[Randy Smith (Not in Mondays), 2012-12-20 22:17:52]

On 2012/12/20 21:35:19, brettw wrote:
> I guess I'd nuke the directory and make it fresh. Who knows what it will
break,
> though.

So the tea-leaves I'm reading suggest that the last two times the history
database was revved before the downloads team got active on it
(https://chromiumcodereview.appspot.com/10014007,
https://chromiumcodereview.appspot.com/10067030) those directories were not
regenerated with generate_profile (I believe that because the History file was
the only one changed).  My guess is that the History file in each of those CLs
was upgraded by hand.  So at the moment I'm feeling a pull towards doing the
upgrade by hand and removing generate_profile.  

I've sent emails to the people who did each of those CLs asking how they did the
version bump; georgey@ doesn't appear to be reachable anymore, and I haven't
heard back from michaelbai@.  But unless he says "Oh, I definitely used
generate_profile", I'm probably going to go the nuke-and-upgrade-by-hand route,
so if that sounds like a Really Bad Idea, please let me know.

[sky, 2012-12-20 23:27:01]

I feel like if we're going to going to go that route we should nuke
generate_profile entirely.

On Thu, Dec 20, 2012 at 2:17 PM,  <rdsmith@chromium.org> wrote:
> On 2012/12/20 21:35:19, brettw wrote:
>>
>> I guess I'd nuke the directory and make it fresh. Who knows what it will
>
> break,
>>
>> though.
>
>
> So the tea-leaves I'm reading suggest that the last two times the history
> database was revved before the downloads team got active on it
> (https://chromiumcodereview.appspot.com/10014007,
> https://chromiumcodereview.appspot.com/10067030) those directories were not
> regenerated with generate_profile (I believe that because the History file
> was
> the only one changed).  My guess is that the History file in each of those
> CLs
> was upgraded by hand.  So at the moment I'm feeling a pull towards doing the
> upgrade by hand and removing generate_profile.
>
> I've sent emails to the people who did each of those CLs asking how they did
> the
> version bump; georgey@ doesn't appear to be reachable anymore, and I haven't
> heard back from michaelbai@.  But unless he says "Oh, I definitely used
> generate_profile", I'm probably going to go the nuke-and-upgrade-by-hand
> route,
> so if that sounds like a Really Bad Idea, please let me know.
>
>
> https://codereview.chromium.org/11363222/

[Randy Smith (Not in Mondays), 2012-12-20 23:28:34]

On 2012/12/20 23:27:01, sky wrote:
> I feel like if we're going to going to go that route we should nuke
> generate_profile entirely.

That's what I'm proposing, yeah.  It has the obvious minuses (our new test data
will always be dependent on being created from our old test data) but it's
looking like that's the situation we're in, so let's acknowledge it and move on.
 I'll also document this in all of its glory in the README.txt file in
profile_with_default_theme. 

> 
> On Thu, Dec 20, 2012 at 2:17 PM,  <mailto:rdsmith@chromium.org> wrote:
> > On 2012/12/20 21:35:19, brettw wrote:
> >>
> >> I guess I'd nuke the directory and make it fresh. Who knows what it will
> >
> > break,
> >>
> >> though.
> >
> >
> > So the tea-leaves I'm reading suggest that the last two times the history
> > database was revved before the downloads team got active on it
> > (https://chromiumcodereview.appspot.com/10014007,
> > https://chromiumcodereview.appspot.com/10067030) those directories were not
> > regenerated with generate_profile (I believe that because the History file
> > was
> > the only one changed).  My guess is that the History file in each of those
> > CLs
> > was upgraded by hand.  So at the moment I'm feeling a pull towards doing the
> > upgrade by hand and removing generate_profile.
> >
> > I've sent emails to the people who did each of those CLs asking how they did
> > the
> > version bump; georgey@ doesn't appear to be reachable anymore, and I haven't
> > heard back from michaelbai@.  But unless he says "Oh, I definitely used
> > generate_profile", I'm probably going to go the nuke-and-upgrade-by-hand
> > route,
> > so if that sounds like a Really Bad Idea, please let me know.
> >
> >
> > https://codereview.chromium.org/11363222/

[sky, 2012-12-20 23:35:45]

+brett again

On Thu, Dec 20, 2012 at 3:28 PM,  <rdsmith@chromium.org> wrote:
> On 2012/12/20 23:27:01, sky wrote:
>>
>> I feel like if we're going to going to go that route we should nuke
>> generate_profile entirely.
>
>
> That's what I'm proposing, yeah.  It has the obvious minuses (our new test
> data
> will always be dependent on being created from our old test data) but it's
> looking like that's the situation we're in, so let's acknowledge it and move
> on.
>  I'll also document this in all of its glory in the README.txt file in
> profile_with_default_theme.
>
>
>
>> On Thu, Dec 20, 2012 at 2:17 PM,  <mailto:rdsmith@chromium.org> wrote:
>> > On 2012/12/20 21:35:19, brettw wrote:
>> >>
>> >> I guess I'd nuke the directory and make it fresh. Who knows what it
>> >> will
>> >
>> > break,
>> >>
>> >> though.
>> >
>> >
>> > So the tea-leaves I'm reading suggest that the last two times the
>> > history
>> > database was revved before the downloads team got active on it
>> > (https://chromiumcodereview.appspot.com/10014007,
>> > https://chromiumcodereview.appspot.com/10067030) those directories were
>> > not
>> > regenerated with generate_profile (I believe that because the History
>> > file
>> > was
>> > the only one changed).  My guess is that the History file in each of
>> > those
>> > CLs
>> > was upgraded by hand.  So at the moment I'm feeling a pull towards doing
>> > the
>> > upgrade by hand and removing generate_profile.
>> >
>> > I've sent emails to the people who did each of those CLs asking how they
>> > did
>> > the
>> > version bump; georgey@ doesn't appear to be reachable anymore, and I
>> > haven't
>> > heard back from michaelbai@.  But unless he says "Oh, I definitely used
>> > generate_profile", I'm probably going to go the nuke-and-upgrade-by-hand
>> > route,
>> > so if that sounds like a Really Bad Idea, please let me know.
>> >
>> >
>> > https://codereview.chromium.org/11363222/
>
>
>
>
> https://codereview.chromium.org/11363222/

[Randy Smith (Not in Mondays), 2012-12-24 14:25:20]

Scott, you keep faking me out by saying "+brett" and not actually adding him! 
:-}

Forwarded message:

+brett again

On Thu, Dec 20, 2012 at 3:28 PM,  <rdsmith@chromium.org> wrote:
> On 2012/12/20 23:27:01, sky wrote:
>>
>> I feel like if we're going to going to go that route we should nuke
>> generate_profile entirely.
>
>
> That's what I'm proposing, yeah.  It has the obvious minuses (our new test
> data
> will always be dependent on being created from our old test data) but it's
> looking like that's the situation we're in, so let's acknowledge it and move
> on.
>  I'll also document this in all of its glory in the README.txt file in
> profile_with_default_theme.
>

[Randy Smith (Not in Mondays), 2012-12-28 19:13:52]

Update on this CL: I chatted with Brett offline, and we agreed that I'd try
regenerating the profile with generate_profile, and if the relevant tests pass
consider that good enough.  I did that, and they do.  So I'm going to go with
regenerating the profiles rather than killing generate_profile.

I'm going to hold off for a couple more days on asking for final stamps on this
CL, as there may be a few more fields I want to stuff into the history while I'm
here, but I'm hopeful that this resolves the big blockages on this CL.  Scott,
please let me know if you disagree.

Copious amount of detail as to what I actually did :-}:

* Test suite creation:
** Created a list of all tests that use these profiles (unit_tests:
HistoryProfileTest.TypicalProfileVersion, performance_ui_tests: StartupTest.*,
NewTabUIStartupTest.*).  
** Decided to skip running NewTabUIStartupTest.* as they're disabled at the
moment (http://crbug.com/69940) and I didn't think I could tell the difference
between my breaking them and flakiness.

* Profile regeneration:
** Deleted profile_with_default_theme/Default and ran generate_profile as
described in that directory.  Removed all files that were shown as deleted in
"git status" and added all files that were shown as added.  
** Copied all history related files from profile_with_default_theme/Default to
profile_with_complex_theme/Default; "git add" to include files that weren't
previously there. 

* Test running (done on both Linux and Windows):
** Built unit_tests and performance_ui_tests in Release build configuration.
** Ran all tests listed above; confirmed they pass.

[Elliot Glaysher, 2013-01-02 18:40:29]

I just want to verify that you're using the exact same files in
profile_with_{default,complex}_theme/. (For git binary reasons).


On Fri, Dec 28, 2012 at 11:13 AM, <rdsmith@chromium.org> wrote:

> Update on this CL: I chatted with Brett offline, and we agreed that I'd try
> regenerating the profile with generate_profile, and if the relevant tests
> pass
> consider that good enough.  I did that, and they do.  So I'm going to go
> with
> regenerating the profiles rather than killing generate_profile.
>
> I'm going to hold off for a couple more days on asking for final stamps on
> this
> CL, as there may be a few more fields I want to stuff into the history
> while I'm
> here, but I'm hopeful that this resolves the big blockages on this CL.
>  Scott,
> please let me know if you disagree.
>
> Copious amount of detail as to what I actually did :-}:
>
> * Test suite creation:
> ** Created a list of all tests that use these profiles (unit_tests:
> HistoryProfileTest.**TypicalProfileVersion, performance_ui_tests:
> StartupTest.*,
> NewTabUIStartupTest.*).
> ** Decided to skip running NewTabUIStartupTest.* as they're disabled at the
> moment (http://crbug.com/69940) and I didn't think I could tell the
> difference
> between my breaking them and flakiness.
>
> * Profile regeneration:
> ** Deleted profile_with_default_theme/**Default and ran generate_profile
> as
> described in that directory.  Removed all files that were shown as deleted
> in
> "git status" and added all files that were shown as added.
> ** Copied all history related files from profile_with_default_theme/**Default
> to
> profile_with_complex_theme/**Default; "git add" to include files that
> weren't
> previously there.
>
> * Test running (done on both Linux and Windows):
> ** Built unit_tests and performance_ui_tests in Release build
> configuration.
> ** Ran all tests listed above; confirmed they pass.
>
>
>
https://codereview.chromium.**org/11363222/<https://codereview.chromium.org/1...
>

[Randy Smith (Not in Mondays), 2013-01-02 19:04:22]

On 2013/01/02 18:40:29, Elliot Glaysher wrote:
> I just want to verify that you're using the exact same files in
> profile_with_{default,complex}_theme/. (For git binary reasons).

I'm using the exact same files for the files I copied (History*); everything
that's new in *complex* is copied exactly from *default*.


> 
> 
> On Fri, Dec 28, 2012 at 11:13 AM, <mailto:rdsmith@chromium.org> wrote:
> 
> > Update on this CL: I chatted with Brett offline, and we agreed that I'd try
> > regenerating the profile with generate_profile, and if the relevant tests
> > pass
> > consider that good enough.  I did that, and they do.  So I'm going to go
> > with
> > regenerating the profiles rather than killing generate_profile.
> >
> > I'm going to hold off for a couple more days on asking for final stamps on
> > this
> > CL, as there may be a few more fields I want to stuff into the history
> > while I'm
> > here, but I'm hopeful that this resolves the big blockages on this CL.
> >  Scott,
> > please let me know if you disagree.
> >
> > Copious amount of detail as to what I actually did :-}:
> >
> > * Test suite creation:
> > ** Created a list of all tests that use these profiles (unit_tests:
> > HistoryProfileTest.**TypicalProfileVersion, performance_ui_tests:
> > StartupTest.*,
> > NewTabUIStartupTest.*).
> > ** Decided to skip running NewTabUIStartupTest.* as they're disabled at the
> > moment (http://crbug.com/69940) and I didn't think I could tell the
> > difference
> > between my breaking them and flakiness.
> >
> > * Profile regeneration:
> > ** Deleted profile_with_default_theme/**Default and ran generate_profile
> > as
> > described in that directory.  Removed all files that were shown as deleted
> > in
> > "git status" and added all files that were shown as added.
> > ** Copied all history related files from
profile_with_default_theme/**Default
> > to
> > profile_with_complex_theme/**Default; "git add" to include files that
> > weren't
> > previously there.
> >
> > * Test running (done on both Linux and Windows):
> > ** Built unit_tests and performance_ui_tests in Release build
> > configuration.
> > ** Ran all tests listed above; confirmed they pass.
> >
> >
> >
>
https://codereview.chromium.**org/11363222/%3Chttps://codereview.chromium.org...>
> >

[Randy Smith (Not in Mondays), 2013-01-23 17:55:28]

Ok.  I think this is ready for final review from Ben & Scott.

Ben: Could you review the persistence of danger type in the PS20->21 change?

Scott: Could you review the method used for adding the binary files (PS21->22,
which is obviously unreviewable, so I'm really talking about the method
described in c#37).

Thanks!

[sky, 2013-01-23 19:39:50]

Glad you good generate_profile working. LGTM

[benjhayden, 2013-01-23 21:02:06]

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:246: EXPECT_CALL(manager(),
MockCreateDownloadItem1(
Did you consider my idea of passing a single struct to MockCreateDownloadItem
and overloading its operator==? I think it might actually be less confusing and
error-prone than splitting it into two.

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

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:87: case
DownloadItem::MAX_DOWNLOAD_STATE: return kStateInvalid;
Do we want NOTREACHED() here and after the next line, like in DangerTypeToInt?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:205: if (!GetDB().Execute("ALTER
TABLE downloads RENAME TO downloads_tmp"))
Is this method called in a transaction?
If not, would it be possible to rename downloads_tmp back to downloads and drop
the url chain table if the migration fails?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:229: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
Is there any code anywhere that assumes that, if the state is INTERRUPTED, then
the reason can't be NONE?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:244: if (!GetDB().Execute("DROP
TABLE downloads_tmp"))
Is this something that we want to do regardless of whether any previous step in
the migration fails?
Would renaming from downloads to downloads_tmp fail if downloads_tmp already
exists?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:315: (info->state ==
DownloadItem::MAX_DOWNLOAD_STATE ?
Sometimes I wonder if you're just making fun of how my code sometimes looks.
Then I think C++ just does this to everybody.

if (state == MAX_STATE) {
  UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_STATE, DROPPED_MAX);
  continue;
}
if (danger == DANGER_MAX) {
  UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_DANGER_TYPE,
DROPPED_MAX);
  continue;
}

Also the DROPPED_ enum can be in an anonymous namespace in this file, right?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:322:
"Download.DatabaseRecordDropped", reason, DROPPED_REASON_MAX + 1);
histograms.xml CL?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:362: continue;
Why discard the url instead of assigning it to its place in the chain?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:394: statement.BindInt(column++,
DangerTypeToInt(data.danger_type));
Do you want to move the DangerTypeToInt up to join the StateToInt?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:451:
statement_insert.BindInt(column++, content::DOWNLOAD_INTERRUPT_REASON_NONE);
Why not copy the interrupt_reason from info here?

[Randy Smith (Not in Mondays), 2013-01-23 23:20:39]

Thanks, Scott! 

Ben: Incorporated your comments; PTAL.

I'm apparently far enough behind LKGR that patches from this CL won't apply; I'm
going to put off syncing until I have all LGTMs.  Don't worry, I won't submit
without try jobs passing to the extent that they're going to (<- binary files +
change in DB version == specific tests will fail until landing).

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/download/d...
File chrome/browser/download/download_history_unittest.cc (right):

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/download/d...
chrome/browser/download/download_history_unittest.cc:246: EXPECT_CALL(manager(),
MockCreateDownloadItem1(
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Did you consider my idea of passing a single struct to MockCreateDownloadItem
> and overloading its operator==? I think it might actually be less confusing
and
> error-prone than splitting it into two.

I hadn't because its seemed more complex than I wanted, but your comment here
made me decide to throw it together and see how it looked, and you're quite
right; that's better.  Done.

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

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:87: case
DownloadItem::MAX_DOWNLOAD_STATE: return kStateInvalid;
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Do we want NOTREACHED() here and after the next line, like in DangerTypeToInt?

Done.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:205: if (!GetDB().Execute("ALTER
TABLE downloads RENAME TO downloads_tmp"))
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Is this method called in a transaction?
> If not, would it be possible to rename downloads_tmp back to downloads and
drop
> the url chain table if the migration fails?

It is wrapped in a transaction.  See HistoryDatabase::Init().

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:229: statement_populate.BindInt(0,
content::DOWNLOAD_INTERRUPT_REASON_NONE);
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Is there any code anywhere that assumes that, if the state is INTERRUPTED,
then
> the reason can't be NONE?

I would argue from first principles that there can't be, or at least that if
there is, it's never run :-}.  This line doesn't change the behavior of download
items created from the history; if you check the appropriate DII constructor
before this change, you'll find that we initialize interrupt_reason to NONE on
history readin.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:244: if (!GetDB().Execute("DROP
TABLE downloads_tmp"))
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Is this something that we want to do regardless of whether any previous step
in
> the migration fails?
> Would renaming from downloads to downloads_tmp fail if downloads_tmp already
> exists?

I'm slightly confused.  If any previous step in the migration fails, we won't
get here; we'll return false instead.  So I don't think we need to worry about
that case?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:315: (info->state ==
DownloadItem::MAX_DOWNLOAD_STATE ?
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Sometimes I wonder if you're just making fun of how my code sometimes looks.
> Then I think C++ just does this to everybody.

I try not to be subtle when I make fun of people--I'd personally hate the
uncertainty, so I don't want to inflict it :-}.

> if (state == MAX_STATE) {
>   UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_STATE,
DROPPED_MAX);
>   continue;
> }
> if (danger == DANGER_MAX) {
>   UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_DANGER_TYPE,
> DROPPED_MAX);
>   continue;
> }

I thought about doing that, but I didn't want to, because I only want one entry
per dropped record.  It's a lot more important to me to have a count of the
records dropped due to corruption than to get the reason the record was
corrupted--I only save that because I felt bad about creating a new histogram
for one value, and couldn't get to RecordDownloadCounts() because of dependency
violations.

> Also the DROPPED_ enum can be in an anonymous namespace in this file, right?

Good point; I should have thought of that.  Done.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:322:
"Download.DatabaseRecordDropped", reason, DROPPED_REASON_MAX + 1);
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> histograms.xml CL?

It's in progress; I was going to upload it before I landed this CL.  I'll take
this note as a request for you to be included on the review for that CL :-}.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:362: continue;
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Why discard the url instead of assigning it to its place in the chain?

I think we covered this in
https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
(don't you hate reviewing CLs that don't land for *months*?? :-}).  If the
current chain size is > chain_index, that means we're dealing with some form of
DB corruption (note the ORDER BY clause in the original SELECT statement). 
Maybe that means I should drop the entire record, do you have an opinion?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:394: statement.BindInt(column++,
DangerTypeToInt(data.danger_type));
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Do you want to move the DangerTypeToInt up to join the StateToInt?

Yeah.  Any reason for me not to switch them both to a DCHECK?

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:451:
statement_insert.BindInt(column++, content::DOWNLOAD_INTERRUPT_REASON_NONE);
On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> Why not copy the interrupt_reason from info here?

Huh.  I have no idea.  Copy paste error, best guess.  Done.

[benjhayden, 2013-01-25 20:03:46]

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

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:87: case
DownloadItem::MAX_DOWNLOAD_STATE: return kStateInvalid;
On 2013/01/23 23:20:39, rdsmith wrote:
> On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > Do we want NOTREACHED() here and after the next line, like in
DangerTypeToInt?
> 
> Done.

Looks like you added them to IntToState but not StateToInt.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:244: if (!GetDB().Execute("DROP
TABLE downloads_tmp"))
On 2013/01/23 23:20:39, rdsmith wrote:
> On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > Is this something that we want to do regardless of whether any previous step
> in
> > the migration fails?
> > Would renaming from downloads to downloads_tmp fail if downloads_tmp already
> > exists?
> 
> I'm slightly confused.  If any previous step in the migration fails, we won't
> get here; we'll return false instead.  So I don't think we need to worry about
> that case?

Sorry. I saw that this might not be run and was wondering if we want to
restructure this method so that this *would* be run if something went wrong.
If the transaction in HistoryDatabase would revert this method if something goes
wrong, then that should be fine.
I'm just wondering if there could possibly be a situation where we rename
downloads to downloads_tmp, then bail without deleting downloads_tmp, or bail
without re-creating downloads. That would seem to crap up the History database.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:315: (info->state ==
DownloadItem::MAX_DOWNLOAD_STATE ?
On 2013/01/23 23:20:39, rdsmith wrote:
> On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > Sometimes I wonder if you're just making fun of how my code sometimes looks.
> > Then I think C++ just does this to everybody.
> 
> I try not to be subtle when I make fun of people--I'd personally hate the
> uncertainty, so I don't want to inflict it :-}.
> 
> > if (state == MAX_STATE) {
> >   UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_STATE,
> DROPPED_MAX);
> >   continue;
> > }
> > if (danger == DANGER_MAX) {
> >   UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_DANGER_TYPE,
> > DROPPED_MAX);
> >   continue;
> > }
> 
> I thought about doing that, but I didn't want to, because I only want one
entry
> per dropped record.  It's a lot more important to me to have a count of the
> records dropped due to corruption than to get the reason the record was
> corrupted--I only save that because I felt bad about creating a new histogram
> for one value, and couldn't get to RecordDownloadCounts() because of
dependency
> violations.

What about this?

if (state...) {
} else if (danger...) {
}

> 
> > Also the DROPPED_ enum can be in an anonymous namespace in this file, right?
> 
> Good point; I should have thought of that.  Done.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:362: continue;
On 2013/01/23 23:20:39, rdsmith wrote:
> On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > Why discard the url instead of assigning it to its place in the chain?
> 
> I think we covered this in
>
https://codereview.chromium.org/11363222/diff/1/chrome/browser/history/downlo...
> (don't you hate reviewing CLs that don't land for *months*?? :-}).  If the
> current chain size is > chain_index, that means we're dealing with some form
of
> DB corruption (note the ORDER BY clause in the original SELECT statement). 
> Maybe that means I should drop the entire record, do you have an opinion?

Ah, nope, lgtm.

[Randy Smith (Not in Mondays), 2013-01-25 20:37:21]

Wasn't sure whether the LGTM in your last message was meant to apply to just
that code snippet, or the entire CL.  Rietveld thinks it applies to the entire
CL :-}.

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

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:87: case
DownloadItem::MAX_DOWNLOAD_STATE: return kStateInvalid;
On 2013/01/25 20:03:47, benjhayden_chromium wrote:
> On 2013/01/23 23:20:39, rdsmith wrote:
> > On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > > Do we want NOTREACHED() here and after the next line, like in
> DangerTypeToInt?
> > 
> > Done.
> 
> Looks like you added them to IntToState but not StateToInt.

Ooops; right.  Should have been the other way around.  Braino.  Done.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:244: if (!GetDB().Execute("DROP
TABLE downloads_tmp"))
On 2013/01/25 20:03:47, benjhayden_chromium wrote:
> On 2013/01/23 23:20:39, rdsmith wrote:
> > On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > > Is this something that we want to do regardless of whether any previous
step
> > in
> > > the migration fails?
> > > Would renaming from downloads to downloads_tmp fail if downloads_tmp
already
> > > exists?
> > 
> > I'm slightly confused.  If any previous step in the migration fails, we
won't
> > get here; we'll return false instead.  So I don't think we need to worry
about
> > that case?
> 
> Sorry. I saw that this might not be run and was wondering if we want to
> restructure this method so that this *would* be run if something went wrong.
> If the transaction in HistoryDatabase would revert this method if something
goes
> wrong, then that should be fine.
> I'm just wondering if there could possibly be a situation where we rename
> downloads to downloads_tmp, then bail without deleting downloads_tmp, or bail
> without re-creating downloads. That would seem to crap up the History
database.

Fair enough.  I think that's taken care of by the transaction, though.

https://codereview.chromium.org/11363222/diff/91001/chrome/browser/history/do...
chrome/browser/history/download_database.cc:315: (info->state ==
DownloadItem::MAX_DOWNLOAD_STATE ?
On 2013/01/25 20:03:47, benjhayden_chromium wrote:
> On 2013/01/23 23:20:39, rdsmith wrote:
> > On 2013/01/23 21:02:06, benjhayden_chromium wrote:
> > > Sometimes I wonder if you're just making fun of how my code sometimes
looks.
> > > Then I think C++ just does this to everybody.
> > 
> > I try not to be subtle when I make fun of people--I'd personally hate the
> > uncertainty, so I don't want to inflict it :-}.
> > 
> > > if (state == MAX_STATE) {
> > >   UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_STATE,
> > DROPPED_MAX);
> > >   continue;
> > > }
> > > if (danger == DANGER_MAX) {
> > >   UMA_HISTOGRAM_ENUMERATION("...RecordDropped", DROPPED_BAD_DANGER_TYPE,
> > > DROPPED_MAX);
> > >   continue;
> > > }
> > 
> > I thought about doing that, but I didn't want to, because I only want one
> entry
> > per dropped record.  It's a lot more important to me to have a count of the
> > records dropped due to corruption than to get the reason the record was
> > corrupted--I only save that because I felt bad about creating a new
histogram
> > for one value, and couldn't get to RecordDownloadCounts() because of
> dependency
> > violations.
> 
> What about this?
> 
> if (state...) {
> } else if (danger...) {
> }

I'm willing if you want, but I'd prefer the current style; I think it
communicates the "want one logged element per dropped record" with less
fragility.  I did play around a bit (there is a lot of boilerplate code here
currently) but I'd like to have only one instance of the histogram name string,
and creating a character constant for that is its own useless boilerplate.  I
did switch the ?: cascade over to ifs, as I think that's easier to read.  Let me
know if you'd like more changes.

> 
> > 
> > > Also the DROPPED_ enum can be in an anonymous namespace in this file,
right?
> > 
> > Good point; I should have thought of that.  Done.
>

[benjhayden, 2013-01-25 20:56:23]

Whole CL LGTM, thanks! :-)

[Randy Smith (Not in Mondays), 2013-01-29 16:53:14]

Another round on the guitar.  Note that I've fixed the problems called out in
http://crbug.com/172672 for the download tests, but only patched around them for
the failing android test.

Please review the PS25->PS26 change:
* benhayden@: Everything.
* sky@: history_unittest.cc, history.22.sql (specifically 172672 stuff).
* michaelbai@: history.21.sql.

[benjhayden, 2013-01-29 17:53:43]

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/download/...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/download/...
chrome/browser/download/download_browsertest.cc:1523:
content::PluginService::GetInstance()->SetPluginListForTesting(&plugin_list);
Why?

[Randy Smith (Not in Mondays), 2013-01-29 18:06:30]

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/download/...
File chrome/browser/download/download_browsertest.cc (right):

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/download/...
chrome/browser/download/download_browsertest.cc:1523:
content::PluginService::GetInstance()->SetPluginListForTesting(&plugin_list);
On 2013/01/29 17:53:43, benjhayden_chromium wrote:
> Why?

The test was failing because there was some form of plugin enabled for flash
files, so the file wasn't being downloaded.  I've added a comment in my sandbox,
but won't upload until I get other review comments to minimize review spam.

Having said that, I remain confused as to why such a plugin is being loaded in
in my sandbox.  I may dig into this further.

[Randy Smith (Not in Mondays), 2013-01-30 20:51:00]

sky, michaelbai, benjhayden: Ping?

[Randy Smith (Not in Mondays), 2013-01-30 20:52:03]

On 2013/01/29 18:06:30, rdsmith wrote:
>
https://codereview.chromium.org/11363222/diff/111001/chrome/browser/download/...
> File chrome/browser/download/download_browsertest.cc (right):
> 
>
https://codereview.chromium.org/11363222/diff/111001/chrome/browser/download/...
> chrome/browser/download/download_browsertest.cc:1523:
> content::PluginService::GetInstance()->SetPluginListForTesting(&plugin_list);
> On 2013/01/29 17:53:43, benjhayden_chromium wrote:
> > Why?
> 
> The test was failing because there was some form of plugin enabled for flash
> files, so the file wasn't being downloaded.  I've added a comment in my
sandbox,
> but won't upload until I get other review comments to minimize review spam.
> 
> Having said that, I remain confused as to why such a plugin is being loaded in
> in my sandbox.  I may dig into this further.

Apparently, my linux box has a builtin flash plugin.  I'm leaving this in unless
I get other pushback.

[benjhayden, 2013-01-30 20:59:56]

lgtm

[sky, 2013-01-30 23:03:00]

I approved this a while back.
What specifically do you need me to review?

[Randy Smith (Not in Mondays), 2013-01-30 23:29:07]

On 2013/01/30 23:03:00, sky wrote:
> I approved this a while back.
> What specifically do you need me to review?

Basically the stuff that came out of our offline conversation on the right way
to do DB migration upgrades.  That's PS25->PS26, history_unittest.cc,
history.22.sql.  (Noted in c#47, but I know you get crazy many review requests.)

[sky, 2013-01-31 01:28:48]

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/history/h...
File chrome/browser/history/history_unittest.cc (right):

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/history/h...
chrome/browser/history/history_unittest.cc:126: void CreateDBVersion(int
version) {
Wrap all calls to this in ASSERT_NO_FATAL_FAILURE.

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/history/h...
chrome/browser/history/history_unittest.cc:315: int64 db_handle = 0;
THis code isn't particularly readable. Can you create a dump of this and load it
too?

[Randy Smith (Not in Mondays), 2013-01-31 20:11:28]

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/history/h...
File chrome/browser/history/history_unittest.cc (right):

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/history/h...
chrome/browser/history/history_unittest.cc:126: void CreateDBVersion(int
version) {
On 2013/01/31 01:28:49, sky wrote:
> Wrap all calls to this in ASSERT_NO_FATAL_FAILURE.

Done, but could you clue me as to what the goal is?  I don't see any way that a
fatal failure could occur outside of the internal ASSERT_NO_FATAL_FAILURE.

https://codereview.chromium.org/11363222/diff/111001/chrome/browser/history/h...
chrome/browser/history/history_unittest.cc:315: int64 db_handle = 0;
On 2013/01/31 01:28:49, sky wrote:
> THis code isn't particularly readable. Can you create a dump of this and load
it
> too?

I can, and if it's important to you I will.  But I find the idiom fairly
readable (it's parallel to, e.g. DownloadDatabase::UpdateDownload()) and if I
turn it into a script I'm going to need to manage the relationship between a
time value specified inside the script and a different time constant outside the
script, which is fairly simple now, but wouldn't be then.  So I'm going to ask
you to tell me twice :-}.

(If I do do the script I'll want to execute a string straight from this file
rather than create a separate file, but I doubt you care about that.)

[sky, 2013-01-31 21:16:21]

Fair enough, LGTM