Refactoring, moving and renaming the NetworkActionPredictor.

Moving and renaming the NetworkActionPredictor into chrome/browser/predictors

BUG=
TEST=Same as before since this is just a move and renaming.


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

[dominich, 2012-03-06 16:42:13]

first pass thoughts.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_edit.cc:242:
UMA_HISTOGRAM_ENUMERATION("AutocompleteActionPredictor.Action",
add these to histograms.xml.

maybe mark the old ones as obsolete, but that might disappear them from the uma
dashboard, so check before you do.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:127:
AutocompleteActionPredictor::RecommendAction(
i think this should be indented?

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:180: void
AutocompleteActionPredictor::Shutdown() {
note for dominic: check OnPredictorDestroyed is being honoured.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:255: if
(initialized_) {
before this is called there's a DCHECK that initialized_ is true. Either that
constraint has been broken (which would be bad) or you don't need this check.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:310: rows_to_add,
this will cause a full copy of the vectors to be made. i wonder if it's better
to create them on the heap and pass the pointers in such a way that they're
destroyed after the task is completed - then only a pointer copy is required.
Maybe.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:509:
db_->autocomplete_table(),
is it worth storing the reference to the table instead of dereferencing it all
the time from db_?

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:22: FILE_PATH_LITERAL("Network
Action Predictor");
Consider renaming this. Two options: Delete the old one and let a new one build
over time, or convert the old one to the new database then delete the old one.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:31:
UMA_HISTOGRAM_MEMORY_KB("Predictor.DatabaseSizeKB",
histogram name should be AutocompleteActionPredictor.xxxx

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:159: const std::vector<Row>&
rows_to_add,
consider a typedef for std::vector<Row>

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:297:
autocomplete_table_->SetCancelled();
why do you need a method for this? cancelled_ is already protected so accessible
directly.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database.h (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:9: #include <ostream>
why is ostream needed?

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:27: class PredictorTableBase
Could be TableBase as we're in a namespace

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:32: virtual void
CreateTableIfNonExistant() = 0;
nit: Existent

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:41: void SetCancelled();
note for dominic: it's a bit weird to have the flag protected and the setter
private.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:63: class
AutocompleteActionPredictorTable : public PredictorTableBase {
could drop 'Predictor' from the class name

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:107: void
CreateTableIfNonExistant() OVERRIDE;
nit: Existent

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:119: class PredictorDatabase :
public RefcountedProfileKeyedService {
could drop 'Predictor' from name

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database_factory.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database_factory.cc:38:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
indented too far.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database_unittest.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database_unittest.cc:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database_unittest.cc:25: struct
AutocompleteActionPredictorTable::Row test_db[] = {
consider splitting this file into one per table type, or per predictor type.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
File chrome/browser/resources/predictors/OWNERS (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
chrome/browser/resources/predictors/OWNERS:1: dominich@chromium.org
given how familiar you are with this code now, you should add yourself.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
File chrome/browser/resources/predictors/predictors.html (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
chrome/browser/resources/predictors/predictors.html:34: <p>Predictors are
disabled.</p>
this will need updating to allow per-predictor disabling.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
File chrome/browser/resources/predictors/predictors.js (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
chrome/browser/resources/predictors/predictors.js:77:
document.addEventListener('DOMContentLoaded',
requestAutocompleteActionPredictorDb);
wrap line

[willchan no longer on Chromium, 2012-03-13 09:58:38]

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
File chrome/browser/predictors/predictor_database_factory.cc (right):

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database_factory.cc:40: return db;
I think your bug is here. You create a refcounted object (PredictorDatabase).
Its refcount is 0, and then you post it to the DB thread, which AddRef()s it to
1. There's a race between this variable getting stored into a scoped_refptr
somewhere and Initialize() finishing on the DB thread, leading to a Release() to
refcount 0, and a delete of this object.

[dominich, 2012-03-13 14:55:21]

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
File chrome/browser/predictors/predictor_database.cc (right):

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database.cc:267: autocomplete_table_(new
AutocompleteActionPredictorTable()) {
could the tables be created on the DB thread in the Initialize function instead?
Would that allow them to not be RefCounted?

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database.cc:276:
db_.set_exclusive_locking();
Does this add a mutex that blocks concurrent access, or wil attempted access
spin until the db_ is available?

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
File chrome/browser/predictors/predictor_database.h (right):

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database.h:26: class PredictorTableBase
this should be in its own file.

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database.h:33: sql::Connection* db_;
if each table has its own pointer to the same database, and the tables are
calling db functions on that connection, is there any guarantee that there won't
be concurrent access to the connection?

I realise they should all be on the DB thread and thus implicitly
single-threaded, but maybe it's worth enforcing that by making this private and
adding a protected accessor that checks the current thread.

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database.h:61: class
AutocompleteActionPredictorTable : public PredictorTableBase {
this should be in its own file.

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
File chrome/browser/predictors/predictor_database_factory.cc (right):

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/predictors/pr...
chrome/browser/predictors/predictor_database_factory.cc:40: return db;
On 2012/03/13 09:58:39, willchan wrote:
> I think your bug is here. You create a refcounted object (PredictorDatabase).
> Its refcount is 0, and then you post it to the DB thread, which AddRef()s it
to
> 1. There's a race between this variable getting stored into a scoped_refptr
> somewhere and Initialize() finishing on the DB thread, leading to a Release()
to
> refcount 0, and a delete of this object.

I agree.

One option is to take what is currently the NetworkActionPredictor and strip it
down to a PredictorDatabaseUI class that is ProfileKeyed and owns a pointer to
the PredictorDatabase. The initialization of the Database is then asynchronous.
Ie, it can do the PostTaskAndReply thing as the current code does and control
access to the PredictorDatabase pointer until initialization is complete.

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/resources/pre...
File chrome/browser/resources/predictors/predictors.css (right):

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/resources/pre...
chrome/browser/resources/predictors/predictors.css:52: 
you'll have to merge in a change here.

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/resources/pre...
File chrome/browser/resources/predictors/predictors.js (right):

http://codereview.chromium.org/9610006/diff/4003/chrome/browser/resources/pre...
chrome/browser/resources/predictors/predictors.js:60: 
you'll have to merge in a change here.

[Shishir, 2012-03-14 21:14:36]

jam - FYI creating new directory - chrome/browser/predictors
pkasting - FYI moving some autocomplete code.
willchan - Refcounted stuff in predictor_database.*
shess - Database stuff in  predictor_database.* and
autocomplete_action_predictor_table.*
dominich - All changes.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_edit.cc:242:
UMA_HISTOGRAM_ENUMERATION("AutocompleteActionPredictor.Action",
On 2012/03/06 16:42:13, dominich wrote:
> add these to histograms.xml.
> 
> maybe mark the old ones as obsolete, but that might disappear them from the
uma
> dashboard, so check before you do.

Done. Will send out a separate CL and check that in along with this.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:127:
AutocompleteActionPredictor::RecommendAction(
On 2012/03/06 16:42:13, dominich wrote:
> i think this should be indented?

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:255: if
(initialized_) {
On 2012/03/06 16:42:13, dominich wrote:
> before this is called there's a DCHECK that initialized_ is true. Either that
> constraint has been broken (which would be bad) or you don't need this check.

Removed.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:310: rows_to_add,
On 2012/03/06 16:42:13, dominich wrote:
> this will cause a full copy of the vectors to be made. i wonder if it's better
> to create them on the heap and pass the pointers in such a way that they're
> destroyed after the task is completed - then only a pointer copy is required.
> Maybe.

Do these vectors get very large? If not it might be worth the overhead to avoid
the complexity.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/autocomplete_action_predictor.cc:509:
db_->autocomplete_table(),
On 2012/03/06 16:42:13, dominich wrote:
> is it worth storing the reference to the table instead of dereferencing it all
> the time from db_?

We didnt want to do that earlier since the table was initially not refcounted.
Changed since we can do that now.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:22: FILE_PATH_LITERAL("Network
Action Predictor");
On 2012/03/06 16:42:13, dominich wrote:
> Consider renaming this. Two options: Delete the old one and let a new one
build
> over time, or convert the old one to the new database then delete the old one.

Adding a todo for this. Once more ppl are ok with the overall change, I will do
this.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:31:
UMA_HISTOGRAM_MEMORY_KB("Predictor.DatabaseSizeKB",
On 2012/03/06 16:42:13, dominich wrote:
> histogram name should be AutocompleteActionPredictor.xxxx

This is the histogram for the overall database so it makes sense to name it as
the predictor database.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:159: const std::vector<Row>&
rows_to_add,
On 2012/03/06 16:42:13, dominich wrote:
> consider a typedef for std::vector<Row>

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.cc:297:
autocomplete_table_->SetCancelled();
On 2012/03/06 16:42:13, dominich wrote:
> why do you need a method for this? cancelled_ is already protected so
accessible
> directly.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database.h (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:9: #include <ostream>
On 2012/03/06 16:42:13, dominich wrote:
> why is ostream needed?

From the earlier file. Removed.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:27: class PredictorTableBase
On 2012/03/06 16:42:13, dominich wrote:
> Could be TableBase as we're in a namespace

As we discussed, leaving as is. We want the names to be meaningful without the
namespaces.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:32: virtual void
CreateTableIfNonExistant() = 0;
On 2012/03/06 16:42:13, dominich wrote:
> nit: Existent

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:41: void SetCancelled();
On 2012/03/06 16:42:13, dominich wrote:
> note for dominic: it's a bit weird to have the flag protected and the setter
> private.

Yes, a little wierd, maybe move to protected?

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:63: class
AutocompleteActionPredictorTable : public PredictorTableBase {
On 2012/03/06 16:42:13, dominich wrote:
> could drop 'Predictor' from the class name

As discussed we want to keep this.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:107: void
CreateTableIfNonExistant() OVERRIDE;
On 2012/03/06 16:42:13, dominich wrote:
> nit: Existent

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database.h:119: class PredictorDatabase :
public RefcountedProfileKeyedService {
On 2012/03/06 16:42:13, dominich wrote:
> could drop 'Predictor' from name

As we discussed, leaving as is. We want the names to be meaningful without the
namespaces.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database_factory.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database_factory.cc:38:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
On 2012/03/06 16:42:13, dominich wrote:
> indented too far.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
File chrome/browser/predictors/predictor_database_unittest.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database_unittest.cc:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/03/06 16:42:13, dominich wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/predicto...
chrome/browser/predictors/predictor_database_unittest.cc:25: struct
AutocompleteActionPredictorTable::Row test_db[] = {
On 2012/03/06 16:42:13, dominich wrote:
> consider splitting this file into one per table type, or per predictor type.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
File chrome/browser/resources/predictors/OWNERS (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
chrome/browser/resources/predictors/OWNERS:1: dominich@chromium.org
On 2012/03/06 16:42:13, dominich wrote:
> given how familiar you are with this code now, you should add yourself.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
File chrome/browser/resources/predictors/predictors.html (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
chrome/browser/resources/predictors/predictors.html:34: <p>Predictors are
disabled.</p>
On 2012/03/06 16:42:13, dominich wrote:
> this will need updating to allow per-predictor disabling.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
File chrome/browser/resources/predictors/predictors.js (right):

https://chromiumcodereview.appspot.com/9610006/diff/1/chrome/browser/resource...
chrome/browser/resources/predictors/predictors.js:77:
document.addEventListener('DOMContentLoaded',
requestAutocompleteActionPredictorDb);
On 2012/03/06 16:42:13, dominich wrote:
> wrap line

Done.

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
File chrome/browser/predictors/predictor_database.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
chrome/browser/predictors/predictor_database.cc:267: autocomplete_table_(new
AutocompleteActionPredictorTable()) {
On 2012/03/13 14:55:21, dominich wrote:
> could the tables be created on the DB thread in the Initialize function
instead?
> Would that allow them to not be RefCounted?

I need to be able to bind to them in the UI thread so I need the pointers to be
non-null.

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
chrome/browser/predictors/predictor_database.cc:276:
db_.set_exclusive_locking();
On 2012/03/13 14:55:21, dominich wrote:
> Does this add a mutex that blocks concurrent access, or wil attempted access
> spin until the db_ is available?

Does it matter? Isnt all access to the db_ on the same thread now?

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
File chrome/browser/predictors/predictor_database.h (right):

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
chrome/browser/predictors/predictor_database.h:26: class PredictorTableBase
On 2012/03/13 14:55:21, dominich wrote:
> this should be in its own file.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
chrome/browser/predictors/predictor_database.h:33: sql::Connection* db_;
On 2012/03/13 14:55:21, dominich wrote:
> if each table has its own pointer to the same database, and the tables are
> calling db functions on that connection, is there any guarantee that there
won't
> be concurrent access to the connection?
> 
> I realise they should all be on the DB thread and thus implicitly
> single-threaded, but maybe it's worth enforcing that by making this private
and
> adding a protected accessor that checks the current thread.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
chrome/browser/predictors/predictor_database.h:61: class
AutocompleteActionPredictorTable : public PredictorTableBase {
On 2012/03/13 14:55:21, dominich wrote:
> this should be in its own file.

Done.

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
File chrome/browser/predictors/predictor_database_factory.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/4003/chrome/browser/predi...
chrome/browser/predictors/predictor_database_factory.cc:40: return db;
On 2012/03/13 14:55:21, dominich wrote:
> On 2012/03/13 09:58:39, willchan wrote:
> > I think your bug is here. You create a refcounted object
(PredictorDatabase).
> > Its refcount is 0, and then you post it to the DB thread, which AddRef()s it
> to
> > 1. There's a race between this variable getting stored into a scoped_refptr
> > somewhere and Initialize() finishing on the DB thread, leading to a
Release()
> to
> > refcount 0, and a delete of this object.
> 
> I agree.
> 
> One option is to take what is currently the NetworkActionPredictor and strip
it
> down to a PredictorDatabaseUI class that is ProfileKeyed and owns a pointer to
> the PredictorDatabase. The initialization of the Database is then
asynchronous.
> Ie, it can do the PostTaskAndReply thing as the current code does and control
> access to the PredictorDatabase pointer until initialization is complete.

Changed from a RefcountedProfileKeyedService to ProfileKeyedService along with
other changes to fix this. Asan seems to like the changes now.

[Scott Hess - ex-Googler, 2012-03-15 00:13:33]

I'm losing the thread due to interrupts, so here's what I got.

One overall thing I'm concerned about is splitting out the db access too finely
into different classes.  Since the schema often unifies them behind the scenes,
it can make things confusing when you're debugging a problem which is in the
database or threading, because you have to bounce between multiple points of
implementation.  Especially do you have a versioning story?

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.cc:469: void
AutocompleteActionPredictor::UpdateRow(
Having an iterator argument seems to implicitly assume the old structure of
iterating to call UpdateRow() and AddRow() one by one.  But now you collect
things and call AddAndUpdateRows() once.  Can these two functions go away
entirely?

[If so, then can the called UpdateRow() and AddRow() also be removed?]

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:18: const char
kAutocompletePredictorTableName[] = "network_action_predictor";
If kAutocompletePredictorTableName were a #define instead of a global, you could
say:
  "SELECT * FROM " kAutocompletePredictorTableName " WHERE id = ?"
instead of StringPrintf().c_str().

Admittedly, StringPrintf().c_str() isn't that horrible.

Also, I find myself a little nervous about breaking up the SQLite usage into
multiple classes, and then having things like this rename suggestion.  I think
that this would make versioning more challenging.

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:99:
row_buffer->clear();
I'm inclined to skip the CHECK for something like this which will crash either
way, but regardless of what you do, consider whether this shouldn't be above the
cancelled check, so that the API is consistent.

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:125: <<
kAutocompletePredictorTableName;
I think that there will already be a DCHECK failure in this case, so this one
will never get a chance to fail.

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:147: const
Rows& rows_to_update) {
Rather than having this call AddRow() and UpdateRow() which individually do a
bunch of duplicate setup, would it be reasonable for this function to inline
things and pull up the prepared statements, and then have those functions be
degenerate wrappers around this function?

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:161:
DB()->CommitTransaction();
You have a transaction around this, but you don't check actual success values
within, so your transaction doesn't really guarantee that the adds and updates
happened.  That's part of why I pondered unifying things above, because then it
would be easier to have errors rollback (use a sql::Transaction and don't commit
it).

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:227: << "
table.";
I think Execute() will have DCHECK'ed before you get here.

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor_table.h (right):

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor_table.h:71: void
DeleteRow(const Row::Id& id);
AFAICT, DeleteRow() is only called from tests.

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/p...
File chrome/browser/predictors/predictor_database.cc (right):

http://codereview.chromium.org/9610006/diff/20001/chrome/browser/predictors/p...
chrome/browser/predictors/predictor_database.cc:73: << db_path_.AsUTF8Unsafe();
Is this likely to be more helpful than the default debug-mode messages for
sql::Connection?

[Shishir, 2012-03-15 07:55:48]

Hi Scott,

We plan to have separate predictors all use different tables and as such we
should not have overlap between them. This should also help with the versioning
since each table can deal with versioning its own internal table on schema
change. Is there any other versioning that we need to worry about?

Thanks
Shishir

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor.cc:469: void
AutocompleteActionPredictor::UpdateRow(
On 2012/03/15 00:13:33, shess wrote:
> Having an iterator argument seems to implicitly assume the old structure of
> iterating to call UpdateRow() and AddRow() one by one.  But now you collect
> things and call AddAndUpdateRows() once.  Can these two functions go away
> entirely?
> 
> [If so, then can the called UpdateRow() and AddRow() also be removed?]

These two calls are needed only for unittests to be able to manipulate the cache
and the DB in a consistent fashion. I have combined them into a single function.
The only assumption in the function is the rows that are being updates or added
need to have unique keys, which seems to be the case.. Maybe I should check that
explicitly.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:18: const char
kAutocompletePredictorTableName[] = "network_action_predictor";
On 2012/03/15 00:13:33, shess wrote:
> If kAutocompletePredictorTableName were a #define instead of a global, you
could
> say:
>   "SELECT * FROM " kAutocompletePredictorTableName " WHERE id = ?"
> instead of StringPrintf().c_str().
> 
> Admittedly, StringPrintf().c_str() isn't that horrible.
> 
> Also, I find myself a little nervous about breaking up the SQLite usage into
> multiple classes, and then having things like this rename suggestion.  I think
> that this would make versioning more challenging.

The renaming is unfortunate since the current name is a bit confusing. But the
table naming and versioning changes will be limited to this class only and
possibly only to this file, so I am not sure why it would be more challenging.
Any specific concerns.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:99:
row_buffer->clear();
On 2012/03/15 00:13:33, shess wrote:
> I'm inclined to skip the CHECK for something like this which will crash either
> way, but regardless of what you do, consider whether this shouldn't be above
the
> cancelled check, so that the API is consistent.

Removed.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:125: <<
kAutocompletePredictorTableName;
On 2012/03/15 00:13:33, shess wrote:
> I think that there will already be a DCHECK failure in this case, so this one
> will never get a chance to fail.

Verified and Removed.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:147: const
Rows& rows_to_update) {
On 2012/03/15 00:13:33, shess wrote:
> Rather than having this call AddRow() and UpdateRow() which individually do a
> bunch of duplicate setup, would it be reasonable for this function to inline
> things and pull up the prepared statements, and then have those functions be
> degenerate wrappers around this function?

Done.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:161:
DB()->CommitTransaction();
On 2012/03/15 00:13:33, shess wrote:
> You have a transaction around this, but you don't check actual success values
> within, so your transaction doesn't really guarantee that the adds and updates
> happened.  That's part of why I pondered unifying things above, because then
it
> would be easier to have errors rollback (use a sql::Transaction and don't
commit
> it).

Done.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:227: << "
table.";
On 2012/03/15 00:13:33, shess wrote:
> I think Execute() will have DCHECK'ed before you get here.

Verified and Removed.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
File chrome/browser/predictors/autocomplete_action_predictor_table.h (right):

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor_table.h:71: void
DeleteRow(const Row::Id& id);
On 2012/03/15 00:13:33, shess wrote:
> AFAICT, DeleteRow() is only called from tests.

Removed.

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
File chrome/browser/predictors/predictor_database.cc (right):

https://chromiumcodereview.appspot.com/9610006/diff/20001/chrome/browser/pred...
chrome/browser/predictors/predictor_database.cc:73: << db_path_.AsUTF8Unsafe();
On 2012/03/15 00:13:33, shess wrote:
> Is this likely to be more helpful than the default debug-mode messages for
> sql::Connection?

Only helpful in the release version. Removed.

[Shishir, 2012-03-21 17:55:52]

Ping.

[cbentzel, 2012-03-26 17:52:14]

What sort of feedback are you looking for?

The CL description indicates that you are just looking for high-level comments,
since it hasn't been fully tested. Is this still the case?

http://codereview.chromium.org/9610006/diff/26002/chrome/browser/predictors/O...
File chrome/browser/predictors/OWNERS (right):

http://codereview.chromium.org/9610006/diff/26002/chrome/browser/predictors/O...
chrome/browser/predictors/OWNERS:2: shishir@chromium.org
You have to have full commit access to be an OWNER

http://dev.chromium.org/developers/owners-files-1

[Shishir, 2012-03-26 17:57:51]

Updated the description since this CL is now ready for review. Please review the
CL.

[cbentzel, 2012-03-26 18:01:23]

On 2012/03/26 17:57:51, Shishir wrote:
> Updated the description since this CL is now ready for review. Please review
the
> CL.

Did you talk to someone in chrome/OWNERS to see if chrome/browser/predictors is
an OK directory to add?

[cbentzel, 2012-03-26 18:04:00]

On 2012/03/26 18:01:23, cbentzel wrote:
> On 2012/03/26 17:57:51, Shishir wrote:
> > Updated the description since this CL is now ready for review. Please review
> the
> > CL.
> 
> Did you talk to someone in chrome/OWNERS to see if chrome/browser/predictors
is
> an OK directory to add?

Also - it would be much easier to review this CL if you had one which simply
moved the files from chrome/browser/autocomplete to chrome/browser/predictors,
and then do separate CLs which introduce the bigger refactoring changes.

[Shishir, 2012-03-26 21:46:35]

I had added jam for approval on the directory name, but have started another
thread for approval. I will update this CL with just the moved code and break
out the refactoring into another CL once the directory name is approved.

Thanks
Shishir

[cbentzel, 2012-03-27 00:03:42]

Thanks, Shishir - I think it will make this much easier to review!

On Mon, Mar 26, 2012 at 5:46 PM, <shishir@chromium.org> wrote:

> I had added jam for approval on the directory name, but have started
> another
> thread for approval. I will update this CL with just the moved code and
> break
> out the refactoring into another CL once the directory name is approved.
>
> Thanks
> Shishir
>
>
http://codereview.chromium.**org/9610006/<http://codereview.chromium.org/9610...
>

[Shishir, 2012-03-28 18:12:19]

As per Chris's suggestion, this CL now only has the NetworkActionPredictor moved
to chrome/browser/predictors directory and renamed to
AutocompleteActionPredictor since it was causing confusion.

Removing willchan, pkasting and shess since there are no changes for them to
review. 

Please take a look.

[cbentzel, 2012-03-29 00:34:28]

This seems fine now that you've simplified it. It would be nice to see if you
can get Dominic's stamp of approval, but not sure when he will be available to
look at it.

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.cc:370: for
(std::vector<AutocompleteActionPredictorDatabase::Row>::const_iterator
Perhaps a typedef to eliminate the line wrap?

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor.h (right):

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.h:33: // a
AutocompleteActionPredictorDatabase accessed asynchronously on the DB
Nit: an Autocomplete

[Shishir, 2012-03-29 00:43:55]

Hi Chris,

Doiminic was ok with the new names and the renaming. So I think this CL is safe
enough to submit without his review.

Thanks
Shishir

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.cc:370: for
(std::vector<AutocompleteActionPredictorDatabase::Row>::const_iterator
On 2012/03/29 00:34:28, cbentzel wrote:
> Perhaps a typedef to eliminate the line wrap?

This will change in the refactoring, so doesnt seem worth it.

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor.h (right):

http://codereview.chromium.org/9610006/diff/39001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.h:33: // a
AutocompleteActionPredictorDatabase accessed asynchronously on the DB
On 2012/03/29 00:34:28, cbentzel wrote:
> Nit: an Autocomplete

Done.

[cbentzel, 2012-03-30 14:33:34]

LGTM

You'll need to get appropriate OWNERS approval as well.

I did not go through the file moves with a fine-toothed comb.

http://codereview.chromium.org/9610006/diff/50001/chrome/browser/predictors/a...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/9610006/diff/50001/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.cc:370: for
(std::vector<AutocompleteActionPredictorDatabase::Row>::const_iterator
Nit: perhaps typedef so this doesn't need to wrap?

[dominich, 2012-04-26 20:56:02]

Rubberstamp LGTM

On 2012/03/30 14:33:34, cbentzel wrote:
> LGTM
> 
> You'll need to get appropriate OWNERS approval as well.
> 
> I did not go through the file moves with a fine-toothed comb.
> 
>
http://codereview.chromium.org/9610006/diff/50001/chrome/browser/predictors/a...
> File chrome/browser/predictors/autocomplete_action_predictor.cc (right):
> 
>
http://codereview.chromium.org/9610006/diff/50001/chrome/browser/predictors/a...
> chrome/browser/predictors/autocomplete_action_predictor.cc:370: for
> (std::vector<AutocompleteActionPredictorDatabase::Row>::const_iterator
> Nit: perhaps typedef so this doesn't need to wrap?

[Shishir, 2012-04-27 20:01:31]

Added Reviewers:

jhawkins for chrome/browser/profiles and new directory chrome/browser/predictors

erg for chrome/browser/ui/webui and chrome/browser/resources changes.

The new directory "predictors" was approved by darin.

[James Hawkins, 2012-04-27 20:26:07]

rubberstamp LGTM

[Elliot Glaysher, 2012-05-01 18:08:16]

I'm not a webui owners.

[Shishir, 2012-05-01 18:10:18]

On 2012/05/01 18:08:16, Elliot Glaysher wrote:
> I'm not a webui owners.

@erg: Need your approval for the chrome/browser/profiles changes. Sorry for the
confusion.

[Elliot Glaysher, 2012-05-01 18:12:50]

oh.

profiles lgtm

[commit-bot: I haz the power, 2012-05-01 22:28:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/9610006/68002

[commit-bot: I haz the power, 2012-05-01 22:29:50]

Presubmit check for 9610006-68002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/predictors/autocomplete_action_predictor_database_unittest.cc,
line 87, 82 chars \
  chrome/browser/predictors/autocomplete_action_predictor_database_unittest.cc,
line 92, 85 chars \
 
chrome/browser/ui/webui/predictors/autocomplete_action_predictor_dom_handler.cc,
line 20, 81 chars \
 
chrome/browser/ui/webui/predictors/autocomplete_action_predictor_dom_handler.cc,
line 26, 87 chars

Presubmit checks took 9.0s to calculate.

[commit-bot: I haz the power, 2012-05-01 23:25:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/9610006/78003

[commit-bot: I haz the power, 2012-05-02 00:57:43]

Can't apply patch for file chrome/browser/autocomplete/autocomplete_edit.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/autocomplete/autocomplete_edit.cc
Hunk #2 FAILED at 25.
Hunk #3 succeeded at 225 (offset 3 lines).
Hunk #4 succeeded at 413 (offset 3 lines).
Hunk #5 succeeded at 1122 (offset 5 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/autocomplete/autocomplete_edit.cc.rej

[commit-bot: I haz the power, 2012-05-02 01:34:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/9610006/85002

[commit-bot: I haz the power, 2012-05-02 03:06:07]

Change committed as 134865