Add the LoggedIn Predictor, to detect which websites a user is likely

Add the LoggedIn Predictor, to detect which websites a user is likely
logged into (or has been logged into).
R=shishir@chromium.org, bauerb@chromium.org

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

[tburkard, 2013-04-13 00:10:53]

Thanks for reviewing.

bauerb: please review browsing_data/*
shishir: everything else.

Thanks.

[Bernhard Bauer, 2013-04-13 06:46:23]

browsing_data/ LGTM with a nit:

https://codereview.chromium.org/13903018/diff/1/chrome/browser/browsing_data/...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/browsing_data/...
chrome/browser/browsing_data/browsing_data_remover.h:251: // Invoked on the IO
thread to clear the LoggedIn Predictor.
This method isn't actually called on the IO thread.

[Shishir, 2013-04-16 21:22:04]

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
File chrome/browser/predictors/logged_in_predictor_table.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:41: if
(!effective_domain.empty() && effective_domain[0] == '.')
Comment on this code.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:68: Statement
statement(DB()->GetUniqueStatement(
GetCachedStatement.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:102: "CREATE TABLE %s ( "
Will probably fit in one line?

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:108: bool success =
Since you have only one table, put

if (db->DoesTableExist(kTableName))
  return;

at the top and here:

if (!db->execute...)
  ResetDB();

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:122:
base::StringPrintf("SELECT count(*) FROM %s",
one line?

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
File chrome/browser/predictors/logged_in_predictor_table.h (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.h:25: //  - UrlResourceTable
- resources per Urls.
These comments are invalid. Please update.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.h:29: class
LoggedInPredictorTable : public PredictorTableBase {
LoggedInPredictor seems a bit generic - Would LoggedInPrerenderPredictor be
better?

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.h:31: void Add(const GURL&
url);
Please explain the table schema.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:236:
logged_in_predictor_trial->AppendGroup(kLoggedInPredictorEnabledGroup, 100);
Not using finch?

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
You should also respect the "predict network action settings".

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tab_helper.cc:244: MaybeGetLoggedInTable()
const {
Breakup after the return param

[tburkard, 2013-04-16 21:47:19]

https://codereview.chromium.org/13903018/diff/1/chrome/browser/browsing_data/...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/browsing_data/...
chrome/browser/browsing_data/browsing_data_remover.h:251: // Invoked on the IO
thread to clear the LoggedIn Predictor.
On 2013/04/13 06:46:24, Bernhard Bauer wrote:
> This method isn't actually called on the IO thread.

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
File chrome/browser/predictors/logged_in_predictor_table.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:41: if
(!effective_domain.empty() && effective_domain[0] == '.')
On 2013/04/16 21:22:04, Shishir wrote:
> Comment on this code.

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:68: Statement
statement(DB()->GetUniqueStatement(
On 2013/04/16 21:22:04, Shishir wrote:
> GetCachedStatement.

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:102: "CREATE TABLE %s ( "
On 2013/04/16 21:22:04, Shishir wrote:
> Will probably fit in one line?

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:108: bool success =
On 2013/04/16 21:22:04, Shishir wrote:
> Since you have only one table, put
> 
> if (db->DoesTableExist(kTableName))
>   return;
> 
> at the top and here:
> 
> if (!db->execute...)
>   ResetDB();

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.cc:122:
base::StringPrintf("SELECT count(*) FROM %s",
On 2013/04/16 21:22:04, Shishir wrote:
> one line?

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
File chrome/browser/predictors/logged_in_predictor_table.h (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.h:25: //  - UrlResourceTable
- resources per Urls.
On 2013/04/16 21:22:04, Shishir wrote:
> These comments are invalid. Please update.

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.h:29: class
LoggedInPredictorTable : public PredictorTableBase {
As explained, this will eventually be used for purposes other than Prerender.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/predictors/log...
chrome/browser/predictors/logged_in_predictor_table.h:31: void Add(const GURL&
url);
On 2013/04/16 21:22:04, Shishir wrote:
> Please explain the table schema.

Done.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:236:
logged_in_predictor_trial->AppendGroup(kLoggedInPredictorEnabledGroup, 100);
Not for canary/dev at this point.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
Since we are not taking any actual action (but just observing), this should not
be necessary.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
Since we are not taking any actual action (but just observing), this should not
be necessary.

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tab_helper.cc:244: MaybeGetLoggedInTable()
const {
On 2013/04/16 21:22:04, Shishir wrote:
> Breakup after the return param

Done.

[Shishir, 2013-04-16 22:25:13]

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
On 2013/04/16 21:47:19, tburkard wrote:
> Since we are not taking any actual action (but just observing), this should
not
> be necessary.

Add a todo to ensure that you dont forget this.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:666:
predictors::PredictorDatabaseFactory::GetForProfile(profile_)->
You want to check that the database exists here too as it might be null for some
reason.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
File chrome/browser/predictors/logged_in_predictor_table.h (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
chrome/browser/predictors/logged_in_predictor_table.h:23: // thread.
Could you add a comment here along the lines of - manages one table {domain
(primary key), added_timestamp).

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
chrome/browser/predictors/logged_in_predictor_table.h:31: void Exists(const
GURL& url, bool* is_present, bool* lookup_succeeded);
Consider renaming to HasUserLoggedIn

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
chrome/browser/predictors/logged_in_predictor_table.h:45: std::string
GetKey(const GURL& url);
const

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
Should this not return false for Incognito.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:279:
predictors::LoggedInPredictorTable* logged_in_predictor_table() {
Consider returning the scoped_refptr

[tburkard, 2013-04-16 22:45:33]

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/13903018/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
The predictor itself is never taking any action - only anyone *using* the
loggedinpredictor.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/browsing_d...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/browsing_d...
chrome/browser/browsing_data/browsing_data_remover.cc:666:
predictors::PredictorDatabaseFactory::GetForProfile(profile_)->
On 2013/04/16 22:25:14, Shishir wrote:
> You want to check that the database exists here too as it might be null for
some
> reason.

Done.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
File chrome/browser/predictors/logged_in_predictor_table.h (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
chrome/browser/predictors/logged_in_predictor_table.h:23: // thread.
On 2013/04/16 22:25:14, Shishir wrote:
> Could you add a comment here along the lines of - manages one table {domain
> (primary key), added_timestamp).

Done.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
chrome/browser/predictors/logged_in_predictor_table.h:31: void Exists(const
GURL& url, bool* is_present, bool* lookup_succeeded);
On 2013/04/16 22:25:14, Shishir wrote:
> Consider renaming to HasUserLoggedIn

Done.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/predictors...
chrome/browser/predictors/logged_in_predictor_table.h:45: std::string
GetKey(const GURL& url);
On 2013/04/16 22:25:14, Shishir wrote:
> const

Done.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
chrome/browser/prerender/prerender_field_trial.cc:273: return
base::FieldTrialList::FindFullName(kLoggedInPredictorTrialName) ==
No notion of a profile here. I changed prerender_manager to not instantiate when
we are in an OTR profile.

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/13903018/diff/32018/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:279:
predictors::LoggedInPredictorTable* logged_in_predictor_table() {
Not necessary since the idea is that people should not hold on to the pointer
themselves, only via PostTask, which will take care of it.

[Shishir, 2013-04-16 23:17:56]

lgtm

[tburkard, 2013-04-16 23:33:32]

Committed patchset #9 manually as r194484.