Refactoring AutocompleteActionPredictor Database.

Refactoring AutocompleteActionPredictor Database.


BUG=
TEST=Same as before


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

[Shishir, 2012-05-07 22:54:15]

Please review this.

[dominich, 2012-05-08 20:35:27]

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

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:45:
COMPILE_ASSERT(arraysize(kConfidenceCutoff) ==
could you move this to 42 so it's next to the array it is checking?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:83:
profile)->autocomplete_table()),
is this the right indent? it probably is but it is difficult to read. i'd expect
profile to be indented another couple, just because it wrapped.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:192: void
AutocompleteActionPredictor::Shutdown() {
you can remove this method if it's not required any more.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:440:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
change of behaviour here: we used to write to the cache before posting the task.
Is there a reason why you switched the order?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.h:184:
scoped_refptr<AutocompleteActionPredictorTable> db_;
db_ -> table_

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_factory.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_factory.h:12: 
nit remove extra line

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:17: //
TODO(shishir): Rename the table for consistency.
Why not do this? we'll lose two weeks of data but that will only take two weeks
to build up again.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:80: if
(cancelled_.IsSet() || !DB())
where does cancelled_ get set?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:80: if
(cancelled_.IsSet() || !DB())
can this check be combined into a single check? TableBase::IsCancelled() maybe?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:145:
DB()->RollbackTransaction();
change of behaviour: in the old code, we never rolled back the transaction but
instead DCHECKed on every statement. Now we would fail silently.

Please put the DCHECK back.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:158:
DB()->RollbackTransaction();
same as above: DCHECK.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:182:
DB()->RollbackTransaction();
ditto: DCHECK

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table_unittest.cc:131:
/*  EXPECT_EQ(0U, CountRecords());
remove this test.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table_unittest.cc:199:
void AutocompleteActionPredictorTableTest::TestUpdateRow() {
remove this test.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_unittest.cc:64:
ASCIIToUTF16(""), 5, 0,
bad merge? this should be string16()

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

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:21: // FIXME: This should move
to a more generic name.
TODO(shishir) instead?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:48: 
remove extra line

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:101:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
what stops users getting access to the table before this task runs?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/predictor_database_factory.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database_factory.h:17: // Singleton that
owns the PredictorDatabases and associates them with
I don't remember how this works - what is the story for incognito with this
setup?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/predictor_table_base.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_table_base.cc:21:
CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
consider 'using content::BrowserThread'

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/predictor_table_base.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_table_base.h:22: class PredictorTableBase
consider just PredictorTable. It's cleaner.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_dependency_manager.cc:193:
predictors::PredictorDatabaseFactory::GetInstance();
should there be an explicit dependency between these two?

[Shishir, 2012-05-08 21:49:03]

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

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:45:
COMPILE_ASSERT(arraysize(kConfidenceCutoff) ==
On 2012/05/08 20:35:28, dominich wrote:
> could you move this to 42 so it's next to the array it is checking?

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:83:
profile)->autocomplete_table()),
On 2012/05/08 20:35:28, dominich wrote:
> is this the right indent? it probably is but it is difficult to read. i'd
expect
> profile to be indented another couple, just because it wrapped.

looks more readable with db_ -> table_

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:192: void
AutocompleteActionPredictor::Shutdown() {
On 2012/05/08 20:35:28, dominich wrote:
> you can remove this method if it's not required any more.

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.cc:440:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
On 2012/05/08 20:35:28, dominich wrote:
> change of behaviour here: we used to write to the cache before posting the
task.
> Is there a reason why you switched the order?

Changed back. No reason. I assumed it would still behave similarly.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor.h:184:
scoped_refptr<AutocompleteActionPredictorTable> db_;
On 2012/05/08 20:35:28, dominich wrote:
> db_ -> table_

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_factory.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_factory.h:12: 
On 2012/05/08 20:35:28, dominich wrote:
> nit remove extra line

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:17: //
TODO(shishir): Rename the table for consistency.
On 2012/05/08 20:35:28, dominich wrote:
> Why not do this? we'll lose two weeks of data but that will only take two
weeks
> to build up again.

We talked about this last time. The idea is that we will also migrate the data
and delete the old table. Which is why I am not doing this now.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:80: if
(cancelled_.IsSet() || !DB())
On 2012/05/08 20:35:28, dominich wrote:
> where does cancelled_ get set?

The predictor_database sets it when it is asked to shutdown.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:80: if
(cancelled_.IsSet() || !DB())
On 2012/05/08 20:35:28, dominich wrote:
> can this check be combined into a single check? TableBase::IsCancelled()
maybe?

The semantics might be unclear. The DB is set to null when the database is
unusable (say the create table failed) or when it is not ready.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:145:
DB()->RollbackTransaction();
On 2012/05/08 20:35:28, dominich wrote:
> change of behaviour: in the old code, we never rolled back the transaction but
> instead DCHECKed on every statement. Now we would fail silently.
> 
> Please put the DCHECK back.

statement.Run() will DCHECK.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:158:
DB()->RollbackTransaction();
On 2012/05/08 20:35:28, dominich wrote:
> same as above: DCHECK.

statement.Run() will DCHECK.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:182:
DB()->RollbackTransaction();
On 2012/05/08 20:35:28, dominich wrote:
> ditto: DCHECK

statement.Run() will DCHECK.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table_unittest.cc:131:
/*  EXPECT_EQ(0U, CountRecords());
On 2012/05/08 20:35:28, dominich wrote:
> remove this test.

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table_unittest.cc:199:
void AutocompleteActionPredictorTableTest::TestUpdateRow() {
On 2012/05/08 20:35:28, dominich wrote:
> remove this test.

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_unittest.cc:64:
ASCIIToUTF16(""), 5, 0,
On 2012/05/08 20:35:28, dominich wrote:
> bad merge? this should be string16()

Done.

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

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:21: // FIXME: This should move
to a more generic name.
On 2012/05/08 20:35:28, dominich wrote:
> TODO(shishir) instead?

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:48: 
On 2012/05/08 20:35:28, dominich wrote:
> remove extra line

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:101:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
On 2012/05/08 20:35:28, dominich wrote:
> what stops users getting access to the table before this task runs?

Even if they have access to the table, the underlying db_ pointer will be null
so all database ops will be NoOp. The db_ pointers get set on line 75 when this
function comes back. Sounds reasonable?

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/predictor_database_factory.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database_factory.h:17: // Singleton that
owns the PredictorDatabases and associates them with
On 2012/05/08 20:35:28, dominich wrote:
> I don't remember how this works - what is the story for incognito with this
> setup?

By default in incognito the factory will return NULL when asked for the service.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/predictor_table_base.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_table_base.cc:21:
CHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::DB));
On 2012/05/08 20:35:28, dominich wrote:
> consider 'using content::BrowserThread'

Done.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/predictor_table_base.h (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_table_base.h:22: class PredictorTableBase
On 2012/05/08 20:35:28, dominich wrote:
> consider just PredictorTable. It's cleaner.

There is precedence for both kind of naming in the codebase. I like this because
it seems more readable. If you feel strongly about this, I will change it.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/profile...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/profile...
chrome/browser/profiles/profile_dependency_manager.cc:193:
predictors::PredictorDatabaseFactory::GetInstance();
On 2012/05/08 20:35:28, dominich wrote:
> should there be an explicit dependency between these two?

Added.

[dominich, 2012-05-08 22:49:39]

couple of nits.

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:80: if
(cancelled_.IsSet() || !DB())
On 2012/05/08 21:49:03, Shishir wrote:
> On 2012/05/08 20:35:28, dominich wrote:
> > can this check be combined into a single check? TableBase::IsCancelled()
> maybe?
> 
> The semantics might be unclear. The DB is set to null when the database is
> unusable (say the create table failed) or when it is not ready.

I understand. But throughout this class you have 

if (cancelled_.IsSet() || !DB())

I'm suggesting

if (IsCancelled())

which is defined in TableBase and is defined as 

{ return (cancelled_.IsSet() || !DB()); }

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

https://chromiumcodereview.appspot.com/10380041/diff/1/chrome/browser/predict...
chrome/browser/predictors/predictor_database.cc:101:
content::BrowserThread::PostTask(content::BrowserThread::DB, FROM_HERE,
On 2012/05/08 21:49:03, Shishir wrote:
> On 2012/05/08 20:35:28, dominich wrote:
> > what stops users getting access to the table before this task runs?
> 
> Even if they have access to the table, the underlying db_ pointer will be null
> so all database ops will be NoOp. The db_ pointers get set on line 75 when
this
> function comes back. Sounds reasonable?

Fine.

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

https://chromiumcodereview.appspot.com/10380041/diff/9002/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor.cc:41:
COMPILE_ASSERT(arraysize(kConfidenceCutoff) ==
nit: add a blank line before this line.

https://chromiumcodereview.appspot.com/10380041/diff/9002/chrome/browser/pred...
File chrome/browser/predictors/predictor_table_base.cc (right):

https://chromiumcodereview.appspot.com/10380041/diff/9002/chrome/browser/pred...
chrome/browser/predictors/predictor_table_base.cc:23: 
nit: remove blank line.

https://chromiumcodereview.appspot.com/10380041/diff/9002/chrome/browser/pred...
chrome/browser/predictors/predictor_table_base.cc:30: 
nit: remove blank line.

https://chromiumcodereview.appspot.com/10380041/diff/9002/chrome/browser/pred...
chrome/browser/predictors/predictor_table_base.cc:36: 
nit: remove blank line.

[Shishir, 2012-05-09 17:40:10]

http://codereview.chromium.org/10380041/diff/1/chrome/browser/predictors/auto...
File chrome/browser/predictors/autocomplete_action_predictor_table.cc (right):

http://codereview.chromium.org/10380041/diff/1/chrome/browser/predictors/auto...
chrome/browser/predictors/autocomplete_action_predictor_table.cc:80: if
(cancelled_.IsSet() || !DB())
On 2012/05/08 22:49:39, dominich wrote:
> On 2012/05/08 21:49:03, Shishir wrote:
> > On 2012/05/08 20:35:28, dominich wrote:
> > > can this check be combined into a single check? TableBase::IsCancelled()
> > maybe?
> > 
> > The semantics might be unclear. The DB is set to null when the database is
> > unusable (say the create table failed) or when it is not ready.
> 
> I understand. But throughout this class you have 
> 
> if (cancelled_.IsSet() || !DB())
> 
> I'm suggesting
> 
> if (IsCancelled())
> 
> which is defined in TableBase and is defined as 
> 
> { return (cancelled_.IsSet() || !DB()); }

Done.

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

http://codereview.chromium.org/10380041/diff/9002/chrome/browser/predictors/a...
chrome/browser/predictors/autocomplete_action_predictor.cc:41:
COMPILE_ASSERT(arraysize(kConfidenceCutoff) ==
On 2012/05/08 22:49:40, dominich wrote:
> nit: add a blank line before this line.

Done.

http://codereview.chromium.org/10380041/diff/9002/chrome/browser/predictors/p...
File chrome/browser/predictors/predictor_table_base.cc (right):

http://codereview.chromium.org/10380041/diff/9002/chrome/browser/predictors/p...
chrome/browser/predictors/predictor_table_base.cc:23: 
On 2012/05/08 22:49:40, dominich wrote:
> nit: remove blank line.

Done.

http://codereview.chromium.org/10380041/diff/9002/chrome/browser/predictors/p...
chrome/browser/predictors/predictor_table_base.cc:30: 
On 2012/05/08 22:49:40, dominich wrote:
> nit: remove blank line.

Done.

http://codereview.chromium.org/10380041/diff/9002/chrome/browser/predictors/p...
chrome/browser/predictors/predictor_table_base.cc:36: 
On 2012/05/08 22:49:40, dominich wrote:
> nit: remove blank line.

Done.

[dominich, 2012-05-09 17:53:13]

LGTM modulo comment tweak and trybots passing

http://codereview.chromium.org/10380041/diff/16001/chrome/browser/predictors/...
File chrome/browser/predictors/predictor_database.h (right):

http://codereview.chromium.org/10380041/diff/16001/chrome/browser/predictors/...
chrome/browser/predictors/predictor_database.h:30: // Useful for testing.
It's not just useful, it's actually used. Tweak the comment to reflect this
please.

[Shishir, 2012-05-09 18:07:29]

Added owners for reviews:

estade for chrome/browser/ui/webui/
sail for chrome/browser/profiles/

http://codereview.chromium.org/10380041/diff/16001/chrome/browser/predictors/...
File chrome/browser/predictors/predictor_database.h (right):

http://codereview.chromium.org/10380041/diff/16001/chrome/browser/predictors/...
chrome/browser/predictors/predictor_database.h:30: // Useful for testing.
On 2012/05/09 17:53:13, dominich wrote:
> It's not just useful, it's actually used. Tweak the comment to reflect this
> please.

Done.

[Evan Stade, 2012-05-09 21:34:30]

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/predictors/predictors_dom_handler.cc (right):

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/predictors/predictors_dom_handler.cc:36: for
(predictors::AutocompleteActionPredictor::DBCacheMap::const_iterator it
I would add a using predictors::AutocompleteActionPredictor because this line is
absurdly long and forces the = into the wrong place.

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/predictors/predictors_dom_handler.h (right):

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/predictors/predictors_dom_handler.h:23: class
PredictorsDOMHandler : public content::WebUIMessageHandler {
nit: remove DOM from the name

[sail, 2012-05-09 21:41:16]

profiles/* LGTM!

[Shishir, 2012-05-09 21:47:21]

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/predictors/predictors_dom_handler.cc (right):

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/predictors/predictors_dom_handler.cc:36: for
(predictors::AutocompleteActionPredictor::DBCacheMap::const_iterator it
On 2012/05/09 21:34:30, Evan Stade wrote:
> I would add a using predictors::AutocompleteActionPredictor because this line
is
> absurdly long and forces the = into the wrong place.

Done.

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/predictors/predictors_dom_handler.h (right):

http://codereview.chromium.org/10380041/diff/16003/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/predictors/predictors_dom_handler.h:23: class
PredictorsDOMHandler : public content::WebUIMessageHandler {
On 2012/05/09 21:34:30, Evan Stade wrote:
> nit: remove DOM from the name

Done.

[Evan Stade, 2012-05-10 18:36:08]

lgtm

[commit-bot: I haz the power, 2012-05-10 19:51:50]

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

[commit-bot: I haz the power, 2012-05-10 23:20:26]

Change committed as 136439